-
Notifications
You must be signed in to change notification settings - Fork 625
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
Conversation
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>
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.
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 |
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.
probably worth checking the len(manifests)
and returning a non-nil err
if it is empty rather than potentially panicking here
pkg/web/client.go
Outdated
@@ -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) |
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.
make sure to use fmt.Errorf
with %w
instead of %v
so that the error is wrapped instead of stringified
tools/uploader/s3.go
Outdated
// 64 GB default limit | ||
uploader.MaxPartUploads = util.GetEnvOrDefault(UPLOADER_MAX_PARTS_UPLOAD_KEY, 1024*4, parseInt32) |
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.
What is the 64 GB referencing here in the comment?
tools/uploader/s3.go
Outdated
func (s *s3Uploader) Initialize() { | ||
if !s.initialized { | ||
s.mpuBufferPool = sync.Pool{ | ||
New: func() interface{} { |
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.
prefer any
over interface{}
🎉
tools/uploader/s3.go
Outdated
// Initialize if we haven't already | ||
if !s.initialized { | ||
s.Initialize() | ||
} |
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.
IIUC this should be safe, right?
// 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>
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:
Insert your username in
test-integration/test_integration/fixtures/fast-build
so that the image line withimage: 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