Skip to content

feat(datasets): allow multipart uploads for large datasets#384

Merged
cwetherill-ps merged 1 commit intomasterfrom
cwetherill/nb-917-bug-uploading-large-datasets-from-cli
Apr 8, 2022
Merged

feat(datasets): allow multipart uploads for large datasets#384
cwetherill-ps merged 1 commit intomasterfrom
cwetherill/nb-917-bug-uploading-large-datasets-from-cli

Conversation

@cwetherill-ps
Copy link
Contributor

@cwetherill-ps cwetherill-ps commented Apr 7, 2022

This attempts to fall back to a multipart upload strategy with presigned
URLs in the event that a dataset is larger than 500MB

@cwetherill-ps
Copy link
Contributor Author

cwetherill-ps commented Apr 7, 2022 •
edited
Loading

mostly not awful, just need to navigate some lingering query param errors:

<Error>
<Code>AuthorizationQueryParametersError</Code>
<Message>
Query-string authentication version 4 requires the X-Amz-Algorithm, X-Amz-Credential, X-Amz-Signature, X-Amz-Date, X-Amz-SignedHeaders, and X-Amz-Expires parameters.
</Message>
<Key>
te6x30gzr/datasets/dswrkyj0ymtibue/versions/glkpp4r/data/asdf.csv
</Key>
<BucketName>bucket</BucketName>
<Resource>
/bucket/te6x30gzr/datasets/dswrkyj0ymtibue/versions/glkpp4r/data/asdf.csv
</Resource>
<RequestId>16E3B91D7F4FEF72</RequestId>
<HostId>669e732c-41ff-4697-98ec-4a8644d1e8f5</HostId>
</Error>

ref. https://docs.aws.amazon.com/AmazonS3/latest/API/sigv4-query-string-auth.html

@e-r-i-k-a e-r-i-k-a requested a review from marquiswashere April 7, 2022 22:56
@cwetherill-ps cwetherill-ps force-pushed the cwetherill/nb-917-bug-uploading-large-datasets-from-cli branch from 610c066 to 5481ffe Compare April 8, 2022 16:28
@cwetherill-ps cwetherill-ps marked this pull request as ready for review April 8, 2022 16:28
@cwetherill-ps cwetherill-ps requested a review from lamroger April 8, 2022 16:29
cwetherill-ps
cwetherill-ps commented Apr 8, 2022
View reviewed changes
gradient/commands/datasets.py

@classmethod
def _put(cls, path, url, content_type):
# @classmethod
Copy link
Contributor Author

@cwetherill-ps cwetherill-ps Apr 8, 2022

Choose a reason for hiding this comment

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

I don't think this breaks anything; I haven't been able to at least

@cwetherill-ps cwetherill-ps force-pushed the cwetherill/nb-917-bug-uploading-large-datasets-from-cli branch from 5481ffe to 8de0ab6 Compare April 8, 2022 16:31
cwetherill-ps
cwetherill-ps commented Apr 8, 2022
View reviewed changes
gradient/commands/datasets.py
update_status()
pool.put(self._put, url=pre_signed.url,
path=result['path'], content_type=result['mimetype'])
pool.put(
Copy link
Contributor Author

@cwetherill-ps cwetherill-ps Apr 8, 2022

Choose a reason for hiding this comment

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

Granted, this isn't really ideal. We're single-threading all parts of an upload in a single worker rather than distributing all N parts among all M workers in the pool. This will result in longer upload times, but that's better than the broken upload we have today. Soooo baby steps.

cwetherill-ps
cwetherill-ps commented Apr 8, 2022
View reviewed changes
gradient/commands/datasets.py
# less than the part_minsize, AND we want to 1-index
# our range to match what AWS expects for part
# numbers
for part in range(1, (size // part_minsize) + 2):
Copy link
Contributor Author

@cwetherill-ps cwetherill-ps Apr 8, 2022

Choose a reason for hiding this comment

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

this'll also add an extra empty part if the upload is exactly divisible by 500MB, which will probably cause an error from AWS due to it being too small. But also 🤷

Copy link

@ghost ghost Aug 13, 2022 •
edited by ghost
Loading

Choose a reason for hiding this comment

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

This cost me my day. Use ceil...

@cwetherill-ps
feat(datasets): allow multipart uploads for large datasets
5b2a78c 
This attempts to fall back to a multipart upload strategy with presigned
URLs in the event that a dataset is larger than 500MB
@cwetherill-ps cwetherill-ps force-pushed the cwetherill/nb-917-bug-uploading-large-datasets-from-cli branch from 8de0ab6 to 5b2a78c Compare April 8, 2022 17:38
marquiswashere
marquiswashere approved these changes Apr 8, 2022
View reviewed changes
Copy link

@marquiswashere marquiswashere left a comment

Choose a reason for hiding this comment

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

LGTM, we can test multi-threading later haha.

@cwetherill-ps cwetherill-ps merged commit 53d4c87 into master Apr 8, 2022
@cwetherill-ps cwetherill-ps deleted the cwetherill/nb-917-bug-uploading-large-datasets-from-cli branch April 8, 2022 18:26
@PSBOT
Copy link

PSBOT commented Apr 8, 2022

🎉 This PR is included in version 1.11.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@PSBOT PSBOT added the released label Apr 8, 2022
This was referenced Aug 13, 2022
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lamroger lamroger Awaiting requested review from lamroger

2 more reviewers

@marquiswashere marquiswashere marquiswashere approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

released

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@cwetherill-ps @PSBOT @marquiswashere