Skip to content

Create multipart uploader for fast push #2135

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

Merged
merged 18 commits into from
Feb 11, 2025

Conversation

NikhilSinha1
Copy link
Contributor

@NikhilSinha1 NikhilSinha1 commented Feb 7, 2025

Summary

We are having loads of problems with the AWS managed uploader so let's replace it with a different managed uploader that we have more control over.

Test Plan

I tested this using the following script:

pushd test-integration/test_integration/fixtures/fast-build
dd if=/dev/urandom of=output.dat bs=1G count=1
../../../../cog push --debug --x-fast
rm output.dat
popd

Insert your username in test-integration/test_integration/fixtures/fast-build so that the image line with image: r8.im/<R8_USERNAME>/test says your username instead of <R8_USERNAME>

And also don't forget to create the model test in Replicate web for your username!

This set of commands should succeed

Signed-off-by: Nikhil Sinha <nikhil@replicate.com>
Signed-off-by: Nikhil Sinha <nikhil@replicate.com>
…ultipart-uploader

Signed-off-by: Nikhil Sinha <nikhil@replicate.com>
Signed-off-by: Nikhil Sinha <nikhil@replicate.com>
Signed-off-by: Nikhil Sinha <nikhil@replicate.com>
Signed-off-by: Nikhil Sinha <nikhil@replicate.com>
Signed-off-by: Nikhil Sinha <nikhil@replicate.com>
@NikhilSinha1 NikhilSinha1 marked this pull request as ready for review February 11, 2025 02:41
Copy link
Contributor

@meatballhat meatballhat left a comment

Choose a reason for hiding this comment

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

Nothing blocking pending the test fixes 👍🏼

Do I understand correctly that most of the uploader code was adapted from another codebase?

if err != nil {
return nil, err
}

return &manifest, nil
return &manifests[0], nil // Docker inspect returns us a list of manifests
Copy link
Contributor

Choose a reason for hiding this comment

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

probably worth checking the len(manifests) and returning a non-nil err if it is empty rather than potentially panicking here

@@ -102,19 +108,19 @@ func (c *Client) PostNewVersion(ctx context.Context, image string, weights []Fil
func (c *Client) versionFromManifest(image string, weights []File, files []File) (*Version, error) {
manifest, err := c.dockerCommand.Inspect(image)
if err != nil {
return nil, err
return nil, fmt.Errorf("failed to inspect docker image: %v", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

make sure to use fmt.Errorf with %w instead of %v so that the error is wrapped instead of stringified

Comment on lines 65 to 66
// 64 GB default limit
uploader.MaxPartUploads = util.GetEnvOrDefault(UPLOADER_MAX_PARTS_UPLOAD_KEY, 1024*4, parseInt32)
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the 64 GB referencing here in the comment?

func (s *s3Uploader) Initialize() {
if !s.initialized {
s.mpuBufferPool = sync.Pool{
New: func() interface{} {
Copy link
Contributor

Choose a reason for hiding this comment

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

prefer any over interface{} 🎉

Comment on lines 158 to 161
// Initialize if we haven't already
if !s.initialized {
s.Initialize()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

IIUC this should be safe, right?

Suggested change
// Initialize if we haven't already
if !s.initialized {
s.Initialize()
}
s.Initialize()

Signed-off-by: Nikhil Sinha <nikhil@replicate.com>
Signed-off-by: Nikhil Sinha <nikhil@replicate.com>
…ultipart-uploader

Signed-off-by: Nikhil Sinha <nikhil@replicate.com>
Signed-off-by: Nikhil Sinha <nikhil@replicate.com>
Signed-off-by: Nikhil Sinha <nikhil@replicate.com>
Signed-off-by: Nikhil Sinha <nikhil@replicate.com>
Signed-off-by: Nikhil Sinha <nikhil@replicate.com>
Signed-off-by: Nikhil Sinha <nikhil@replicate.com>
…ultipart-uploader

Signed-off-by: Nikhil Sinha <nikhil@replicate.com>
Signed-off-by: Nikhil Sinha <nikhil@replicate.com>
@NikhilSinha1 NikhilSinha1 enabled auto-merge (squash) February 11, 2025 22:37
@NikhilSinha1 NikhilSinha1 merged commit c95774f into main Feb 11, 2025
21 checks passed
@NikhilSinha1 NikhilSinha1 deleted the nikhil/replace-multipart-uploader branch February 11, 2025 22:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants