Fixes push progress #557
Fixes push progress #557
Conversation
Summary of ChangesHello @ilopezluna, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses an issue where progress for image push operations was not being displayed correctly. It introduces changes across the progress reporting and OCI distribution layers to accurately identify push operations, update progress messages to reflect 'uploading' status, and integrate incremental progress tracking for data being sent during a push. This significantly enhances the user experience by providing clear feedback during image uploads. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The magic string "uploading" is now a cross-package contract between progress reporting and the CLI; consider defining a shared constant (or a small typed enum) so both sides can refer to it without relying on a hard-coded literal.
- For push progress,
layerSizeis set tor.imageSizewhile the reader progress uses per-layer bytes; ifimageSizeis the total image size this may skew percentage/completion semantics—double-check whether this should be the current layer size instead of the whole image. - In
Write,progress.NewReaderWithOffsetwrapsrcbased oncompleted; ifcompletedis tracking image-level bytes rather than layer-level bytes, it may be worth clarifying this with a brief comment to avoid future confusion about what the offset represents.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The magic string "uploading" is now a cross-package contract between progress reporting and the CLI; consider defining a shared constant (or a small typed enum) so both sides can refer to it without relying on a hard-coded literal.
- For push progress, `layerSize` is set to `r.imageSize` while the reader progress uses per-layer bytes; if `imageSize` is the total image size this may skew percentage/completion semantics—double-check whether this should be the current layer size instead of the whole image.
- In `Write`, `progress.NewReaderWithOffset` wraps `rc` based on `completed`; if `completed` is tracking image-level bytes rather than layer-level bytes, it may be worth clarifying this with a brief comment to avoid future confusion about what the offset represents.
## Individual Comments
### Comment 1
<location> `pkg/distribution/internal/progress/reporter.go:100-103` </location>
<code_context>
layerSize = safeUint64(size)
- } else {
- layerSize = safeUint64(p.Total)
+ } else { // In case of Push there is no layer yet
+ // Use imageSize as layer is not known at this point
+ layerSize = r.imageSize
+ layerID = "uploading" // Fake ID for push operations to enable progress display
}
incrementalBytes := p.Complete - lastComplete
</code_context>
<issue_to_address>
**suggestion:** Avoid hardcoding the "uploading" sentinel string in multiple places; centralize it as a shared constant.
This fake layer ID is currently coupled to `writeDockerProgress`, which also checks `layerID == "uploading"`. Using the same magic string in multiple places is brittle and easy to break during refactors or via typos. Define a single shared constant (e.g., exported from a shared or `internal/progress` package) and reference it from both sides so the sentinel remains consistent and easy to find.
Suggested implementation:
```golang
package progress
const uploadingLayerID = "uploading"
```
```golang
} else { // In case of Push there is no layer yet
// Use imageSize as layer is not known at this point
layerSize = r.imageSize
layerID = uploadingLayerID // Fake ID for push operations to enable progress display
}
```
```golang
if layerID == uploadingLayerID {
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request fixes progress reporting for image push operations. It achieves this by introducing a mechanism to track upload progress in pkg/distribution/oci/remote/remote.go and sending progress updates. A special layer ID "uploading" is used in pkg/distribution/internal/progress/reporter.go to signal a push operation, which is then interpreted by cmd/cli/desktop/progress.go to display the correct status messages like "Uploading" and "Push complete". The changes look good and address the issue. My main feedback is to replace the hardcoded string "uploading" with a shared constant to improve maintainability.
doringeman
left a comment
doringeman
left a comment
There was a problem hiding this comment.
LGTM!
I would like the UX to be consistent though.
$ ldm pull smollm2
bf6f20a60305: Pull complete [==================================================>] 270.6MB/270.6MB
cfc7749b96f6: Pull complete [==================================================>] 11.36kB/11.36kB
Model pulled successfully
$ ldm push doringeman657/smollm2
uploading: Push complete [==================================================>] 270.6MB/270.6MB
Model pushed successfully
Note the layer ID instead of "uploading", per-layer update vs single line and no blank line before the "Model pulled successfully" message.
Recorded in #558. 🙌
To show progress: