-
Notifications
You must be signed in to change notification settings - Fork 0
Implement s3 Delegation & Multipart Upload for files. #504
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
base: main
Are you sure you want to change the base?
Conversation
e32e992 to
7609750
Compare
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
| message=msg, | ||
| error_code=ApiErrorCode.ASSET_INVALID_CONTENT_TYPE, | ||
| http_status_code=HTTPStatus.UNPROCESSABLE_ENTITY, | ||
| ) from None |
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.
is there a reason to not embed the except, and then raise ... from None?
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.
tbh I copied it from regular upload.
|
|
||
| @router.post("/{entity_route}/{entity_id}/assets/upload/initiate", include_in_schema=False) | ||
| @router.post("/{entity_route}/{entity_id}/assets/upload/initiate") | ||
| def initiate_entity_asset_upload( |
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.
I wonder if this should include the word multipart; other uploads are "initiated" elsewhere, so the terminology is non-specific
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.
So sth like:
"/{entity_route}/{entity_id}/assets/multipart-upload/initiate"
?
| ), | ||
| ] = None | ||
| label: AssetLabel | ||
| preferred_part_count: Annotated[int, Field(description="Hint of desired part count.")] = ( |
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.
is this hint needed? I'm not sure we need this flexibility.
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.
I assumed that a user client and a cloud service will have different needs for number of parts. I can remove it if it seems not useful.
|
|
||
|
|
||
| def clip(x: int, min_value: int, max_value: int) -> int: | ||
| """Clamp x to the inclusive range [min_value, max_value].""" |
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.
is it clip or clamp?
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.
It's clip in numpy :D
|
|
||
| S3_MULTIPART_UPLOAD_MAX_SIZE: int = 5 * 1024**3 # TODO: Set appropriate upper file size limit | ||
| S3_MULTIPART_UPLOAD_MIN_PART_SIZE: int = 5 * 1024**2 | ||
| S3_MULTIPART_UPLOAD_MAX_PART_SIZE: int = 5 * 1024**3 |
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.
this seems very high, no?
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.
Yeah, these parameters need some fine tuning. These are the s3 limits. We may want to stay way below that.
6e5031f to
39b437f
Compare
Multipart Asset Upload Flow
1. Initiate Upload
POST /{entity_type}/{entity_id}/assets/upload/initiateInitiates an S3 multipart upload and returns presigned URLs for each part.
A database asset is created with status = "uploading" and upload_meta containing the S3 upload_id and part metadata.
2. Upload Parts
The client uploads file parts directly to S3 using the provided presigned URLs.
3. Complete Upload
POST /{entity_type}/{entity_id}/assets/{asset_id}/upload/completeCompletes the multipart upload.
The asset is updated to status = "created" and upload_meta is cleared.
4. Delete / Abort Upload
DELETE /{entity_type}/{entity_id}/assets/{asset_id}Deletes the asset record triggering the asset deletion event. If the asset is in the uploading state, the corresponding S3 multipart upload is aborted.