feat: show size of model when using docker model search#985
Conversation
There was a problem hiding this comment.
Code Review
This pull request adds model size information to search results by updating the backend resolvers to calculate and return the total size of a model, extending the SearchResult struct, and displaying the formatted size in the CLI search output. A critical issue was identified in cmd/cli/search/backend_resolution.go where a potential nil pointer dereference on manifest could cause a runtime panic when calculating the total size.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The goroutine in resolveSearchResultBackends still captures the loop variable i directly, which can lead to all goroutines using the final index; consider copying i into a local variable inside the loop before calling group.Go.
- In TestPrettyPrintSearchResults, the "unknown size empty" check uses want == "", which is skipped by the
if c.want != ""guard and therefore never asserts anything; adjust this test so it actually verifies the expected behavior for unknown sizes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The goroutine in resolveSearchResultBackends still captures the loop variable i directly, which can lead to all goroutines using the final index; consider copying i into a local variable inside the loop before calling group.Go.
- In TestPrettyPrintSearchResults, the "unknown size empty" check uses want == "", which is skipped by the `if c.want != ""` guard and therefore never asserts anything; adjust this test so it actually verifies the expected behavior for unknown sizes.
## Individual Comments
### Comment 1
<location path="cmd/cli/search/backend_resolution.go" line_range="173-181" />
<code_context>
for i := range resolved {
group.Go(func() error {
- backend, err := resolve(workerCtx, resolved[i])
+ backend, size, err := resolve(workerCtx, resolved[i])
if err != nil || backend == "" {
resolved[i].Backend = backendUnknown
return nil
}
resolved[i].Backend = backend
+ resolved[i].Size = size
return nil
})
</code_context>
<issue_to_address>
**issue (bug_risk):** Loop variable `i` is captured by the goroutine, which can cause data races and incorrect indexing under concurrency.
Because the closure uses `i` from the outer scope, all goroutines share the same index as the loop advances. With `errgroup` running them concurrently, this can cause writes to the wrong `resolved` element or even out-of-bounds access. Capture `i` (and/or `resolved[i]`) as locals before starting the goroutine, e.g.
```go
for i := range resolved {
i := i
group.Go(func() error {
backend, size, err := resolve(workerCtx, resolved[i])
// ...
})
}
```
or pass `resolved[i]` into the closure by value. This ensures deterministic, race-free behavior.
</issue_to_address>
### Comment 2
<location path="cmd/cli/commands/search_test.go" line_range="90-93" />
<code_context>
+ {"docker hub model name", "ai/llama3.2"},
+ {"huggingface prefix added", "hf.co/meta-llama/Llama-3.2-1B"},
+ {"size formatted", "4.30GB"},
+ {"unknown size empty", ""},
+ {"downloads formatted", "1.2M"},
+ {"source docker hub", search.DockerHubSourceName},
+ {"source huggingface", search.HuggingFaceSourceName},
+ }
+
</code_context>
<issue_to_address>
**issue (testing):** The "unknown size empty" check does not actually verify that the size column is empty for unknown sizes
Because `want == ""` skips the containment check, the test never asserts the behavior for `Size == 0` and could miss regressions (e.g., rendering `"0.00B"`). Please make this case assertive by either:
- Verifying that the Hugging Face model row does not contain any size pattern (`"GB"`, `"MB"`, `"kB"`, or trailing `"B"`), or
- Parsing the table rows and asserting that the size column in the second row is an empty string.
This will ensure unknown sizes are guaranteed to render as an empty cell.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Add model sizes when searching for models.
When using model-runner on GPUs with limited VRAM, it is useful to know the size of a specific model. It also helps deciding whether you want to download a 30 GB model vs a smaller one, because you were too lazy to look it up elsewhere.
Sample output for
docker model search qwen: