Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: integrate with job attachment download cli as a openjd action run #476

Merged
merged 6 commits into from
Nov 22, 2024

Conversation

godobyte
Copy link
Contributor

@godobyte godobyte commented Nov 13, 2024

What was the problem/requirement? (What/Why)

Worker Agent runs sync_input and sync_output callbacks to interact with customer s3 as worker agent user. This brings the following problem

  1. job user is a more suitable user to run the sync process
  2. sync_input and sync_output handling in session polluted the purpose of the class

What was the solution? (How)

Worker Agent is in the process of changing Job Attachment sync input and sync output to use attachment and manifest commands running as job user.

What is the impact of this change?

This is still in development and is under a feature flag.

How was this change tested?

Locally set the feature flag, build and install whl files, run hatch deadline-worker-agent

Was this change documented?

It's currently only documented in the code docstring.

Is this a breaking change?

No


By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@godobyte godobyte changed the title wa integration initial draft feat!: attachment cli wa integration initial draft Nov 13, 2024
Comment on lines 262 to 271
# =========================== TO BE DELETED ===========================
path_mapping_file_path: str = os.path.join(
session._session.working_directory, "path_mapping"
)
for rule in job_attachment_path_mappings:
rule["source_path"] = rule["destination_path"]

with open(path_mapping_file_path, "w", encoding="utf8") as f:
f.write(json.dumps([rule for rule in job_attachment_path_mappings]))
# =========================== TO BE DELETED ===========================
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keeping it for now, pending changes and testing with attachment upload cli action integration.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually think writing out a path mapping file is super useful for users? They can easily cat this file and use jq Or are we depending on the environment variable for path mapping for customers?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Openjd session already writes the path mapping files, it's in a slightly different format tho, that's why I was thinking whether still keep this path mapping file or reuse the path written by openjd. Openjd would need to change to return the file path, and the attachment CLI would need slight change to read the following file.

{
    "version": "pathmapping-1.0",
    "path_mapping_rules": [{
        "source_path_format": "POSIX",
        "source_path": "/local/bea/bealine-clients/deadline-cloud-worker-agent/scripts/submit_jobs/asset_example",
        "destination_path": "/sessions/session-39d74575600f47eb80d31d440721a27b7mrqf9ip/assetroot-a7714e87e776d9f1c179"
    }]
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The path to the file can be provided by command-line argument by using the Session.PathMappingRulesFile string interpolation expression (docs) in the step script.

It does require that the job attachments CLI accept this command-line argument and understand the file format (docs)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's a good idea if it fits the data flow, need to make sure the Session.PathMappingRulesFile can be used for both attachment download and attachment upload. Let's chat more offline.

)
if "stepId" not in action_definition:
action_queue_entry = cast(

Check notice

Code scanning / CodeQL

Unused local variable Note

Variable action_queue_entry is not used.
job_attachment_details=job_attachment_details,
)
else:
action_queue_entry = cast(

Check notice

Code scanning / CodeQL

Unused local variable Note

Variable action_queue_entry is not used.
@godobyte godobyte changed the title feat!: attachment cli wa integration initial draft feat: integrate with job attachment download cli as a openjd action run Nov 14, 2024
@godobyte godobyte changed the title feat: integrate with job attachment download cli as a openjd action run feat!: integrate with job attachment download cli as a openjd action run Nov 14, 2024


class AttachmentDownloadAction(OpenjdAction):
"""Action to synchronize input job attachments for a AWS Deadline Cloud job
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit, for Deadline Cloud Session? Since a session has job and step-step dependency.

)
)

vfs_handled = self._vfs_handling(
Copy link
Contributor

@leongdl leongdl Nov 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit, is this starting VFS? If yes, maybe self._start_vfs(...

@godobyte godobyte added enhancement New feature or request security Pull requests that could impact security labels Nov 19, 2024
@godobyte godobyte changed the title feat!: integrate with job attachment download cli as a openjd action run feat: integrate with job attachment download cli as a openjd action run Nov 19, 2024
@godobyte godobyte force-pushed the sync-input branch 2 times, most recently from 2aa7cff to 83d67b7 Compare November 20, 2024 00:04
@godobyte godobyte marked this pull request as ready for review November 20, 2024 00:17
@godobyte godobyte requested a review from a team as a code owner November 20, 2024 00:17
if ASSET_SYNC_JOB_USER_FEATURE:
action_definition = cast(AttachmentDownloadActionApiModel, action_definition)
if "stepId" not in action_definition:
action_queue_entry = cast(

Check notice

Code scanning / CodeQL

Unused local variable Note

Variable action_queue_entry is not used.
job_attachment_details=job_attachment_details,
)
else:
action_queue_entry = cast(

Check notice

Code scanning / CodeQL

Unused local variable Note

Variable action_queue_entry is not used.
Copy link
Contributor

@jusiskin jusiskin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work here! I have a few suggested improvements. Happy to have a discussion on which ones we want to adopt.

src/deadline_worker_agent/api_models.py Show resolved Hide resolved
src/deadline_worker_agent/api_models.py Show resolved Hide resolved
src/deadline_worker_agent/boto/shim.py Show resolved Hide resolved
src/deadline_worker_agent/feature_flag.py Outdated Show resolved Hide resolved
Comment on lines 262 to 271
# =========================== TO BE DELETED ===========================
path_mapping_file_path: str = os.path.join(
session._session.working_directory, "path_mapping"
)
for rule in job_attachment_path_mappings:
rule["source_path"] = rule["destination_path"]

with open(path_mapping_file_path, "w", encoding="utf8") as f:
f.write(json.dumps([rule for rule in job_attachment_path_mappings]))
# =========================== TO BE DELETED ===========================
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The path to the file can be provided by command-line argument by using the Session.PathMappingRulesFile string interpolation expression (docs) in the step script.

It does require that the job attachments CLI accept this command-line argument and understand the file format (docs)

@godobyte godobyte force-pushed the sync-input branch 2 times, most recently from 2088943 to 7d2c4d6 Compare November 21, 2024 21:48
Signed-off-by: Godot Bian <[email protected]>
session._session._path_mapping_rules.sort(key=lambda rule: -len(rule.source_path.parts))

# =========================== TO BE DELETED ===========================
path_mapping_file_path: str = os.path.join(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit this will be deleted later right?

Copy link
Contributor Author

@godobyte godobyte Nov 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. In order to delete this, I'd need to do some additional changes, but that'd be part of this feature completion.

@godobyte godobyte merged commit 07abc76 into aws-deadline:mainline Nov 22, 2024
15 checks passed
@godobyte godobyte deleted the sync-input branch November 22, 2024 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request security Pull requests that could impact security
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants