-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
experimental prefect-aws
bundle steps
#17201
Conversation
516919a
to
5e5e188
Compare
CodSpeed Performance ReportMerging #17201 will not alter performanceComparing Summary
|
ee9a341
to
aa08515
Compare
prefect-aws
bundle stepsprefect-aws
bundle steps
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implementation looks good! Could you add some tests for the CLI interface also? We'll want to ensure that behavior is maintained.
remove extra code
aa08515
to
b5420a9
Compare
test CLI interface
4f4a5ea
to
3040570
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool use of typer
- left some minor bookkeeping comments, otherwise this makes sense to me
bucket: S3 bucket name | ||
key: S3 object key | ||
output_dir: Local directory to save the bundle (if None, uses a temp directory) | ||
aws_credentials_block_name: Name of the AWS credentials block to use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
two notes:
- worth calling out if block is
None
, credentials inferred from environment - prob worth doc'ing the return value (what is generally in the dict?)
aws_credentials_block_name=aws_credentials_block_name, | ||
) | ||
|
||
bundle_data = from_json(Path(download_result["local_path"]).read_bytes()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
given the reliance on the local_path
key, could be worth elevating the return type of download_bundle
to a TypedDict
or dataclass
|
||
try: | ||
s3.upload_file(str(filepath), bucket, key) | ||
return {"bucket": bucket, "key": key, "url": f"s3://{bucket}/{key}"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same super minor point: could be worth a structured return object
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One small question on test assertions, but otherwise LGTM!
sys.argv = [ | ||
"upload.py", | ||
str(mock_bundle_file), | ||
"--bucket", | ||
"test-bucket", | ||
"--key", | ||
"test-key", | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we verify these values get passed into the function as expected?
this PR adds the code for transporting bundles to and from s3, allowing execution of the bundle directly from s3 with a
Runner
example
results in
Related to #17194