Skip to content

fix: use existing formatSize function, size regards model files only#988

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

fix: use existing formatSize function, size regards model files only#988
KeeTraxx wants to merge 3 commits into
docker:mainfrom
KeeTraxx:feat/model-search-size-followup

Conversation

@KeeTraxx

Copy link
Copy Markdown
Contributor

Follow up PR for #985

Fixes comments from @doringeman

Added additional tests.

@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 consolidates size formatting logic by exporting and reusing FormatSize from the format package, and updates the search backend resolution to filter model files before calculating total size. The review feedback correctly identifies that returning "0.00B" for non-positive sizes is misleading compared to the original "n/a" representation, and points out that an error returned by FilterModelFiles is currently being silently ignored.

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 pkg/distribution/format/format.go
Comment thread pkg/distribution/format/format_test.go Outdated
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 1 issue, and left some high level feedback:

  • Changing zero/negative size formatting from "n/a" to "0.00B" alters the semantics for existing callers; consider confirming that all consumers (especially CLI output) expect this new representation or mapping it at the call site if needed.
  • In backend_resolution.go the error from distributionhf.FilterModelFiles is ignored; if filtering can fail in meaningful ways, it may be safer to propagate or handle that error rather than silently falling back.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- Changing zero/negative size formatting from "n/a" to "0.00B" alters the semantics for existing callers; consider confirming that all consumers (especially CLI output) expect this new representation or mapping it at the call site if needed.
- In `backend_resolution.go` the error from `distributionhf.FilterModelFiles` is ignored; if filtering can fail in meaningful ways, it may be safer to propagate or handle that error rather than silently falling back.

## Individual Comments

### Comment 1
<location path="cmd/cli/search/backend_resolution.go" line_range="95-96" />
<code_context>
 		return backendUnknown, 0, err
 	}
-	return backendFromRepoFiles(repoFiles), distributionhf.TotalSize(repoFiles), nil
+	modelFiles, _ := distributionhf.FilterModelFiles(repoFiles)
+	return backendFromRepoFiles(repoFiles), distributionhf.TotalSize(modelFiles), nil
 }

</code_context>
<issue_to_address>
**issue (bug_risk):** Swallowing the error from `FilterModelFiles` can lead to incorrect size calculation or mask resolution issues.

The error from `FilterModelFiles` is ignored, yet `TotalSize` is still computed from `modelFiles`. If filtering fails or returns a partial result, `Resolve` may return `nil` with a zero or incomplete size, making it hard to distinguish between "no model files" and "failed to determine model files" and potentially under-reporting sizes. Please propagate the error, or at least fall back to `repoFiles` for size computation when filtering fails.
</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
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.

1 participant