Skip to content

feat: show size of model when using docker model search#985

Open
KeeTraxx wants to merge 2 commits into
docker:mainfrom
KeeTraxx:feat/model-search-shows-size
Open

feat: show size of model when using docker model search#985
KeeTraxx wants to merge 2 commits into
docker:mainfrom
KeeTraxx:feat/model-search-shows-size

Conversation

@KeeTraxx

Copy link
Copy Markdown

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:

image

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Comment thread cmd/cli/search/backend_resolution.go

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment thread cmd/cli/search/backend_resolution.go
Comment thread cmd/cli/commands/search_test.go Outdated
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