-
Notifications
You must be signed in to change notification settings - Fork 79
Add model name normalization and display stripping #240
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
Reviewer's GuideThis PR implements a uniform model name normalization strategy by introducing normalizeModelName (and stripDefaultsFromModelName) to prepend the default "ai/" org and ":latest" tag when missing, refactors existing client and server code to use these helpers, updates all CLI commands to normalize user inputs and strip defaults for display, and adds comprehensive unit tests for both functions. Sequence diagram for model name normalization in CLI commandssequenceDiagram
actor User
participant CLI
participant "normalizeModelName()"
participant Backend
User->>CLI: Enter model name (e.g., "gemma3")
CLI->>"normalizeModelName()": Normalize model name
"normalizeModelName()"-->>CLI: Returns normalized name (e.g., "ai/gemma3:latest")
CLI->>Backend: Send normalized model name
Class diagram for model name normalization utilitiesclassDiagram
class Utils {
+normalizeModelName(model string) string
+stripDefaultsFromModelName(model string) string
}
Utils <.. CLI : uses
Utils <.. Backend : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Summary of ChangesHello @ericcurtin, 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 enhances the CLI's model management by introducing robust model name normalization and display stripping. The changes ensure that model names are consistently interpreted internally by automatically applying default prefixes and tags, while simultaneously improving the user experience by presenting cleaner, less verbose model names in command outputs. This standardization simplifies user input and maintains clarity in the system's model handling. Highlights
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
|
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.
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.
Pull Request Overview
This PR adds comprehensive model name normalization functionality to standardize how model names are handled across the CLI and API. The changes ensure that model names are consistently formatted with default organization prefix ("ai/") and tag (":latest") when missing, while preserving special cases like HuggingFace models and external registries.
Key changes:
- Added
normalizeModelNamefunction that adds "ai/" prefix and ":latest" tag when missing - Added
stripDefaultsFromModelNamefunction to clean up display names by removing defaults - Applied normalization to all CLI commands (pull, push, run, configure, inspect, etc.) and API handlers
Reviewed Changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
pkg/inference/models/manager.go |
Added normalization to API handlers for model operations |
cmd/cli/desktop/desktop.go |
Updated client methods to use new normalization instead of HuggingFace-only normalization |
cmd/cli/commands/utils.go |
Added core normalization and display stripping functions |
cmd/cli/commands/utils_test.go |
Added comprehensive test coverage for normalization functions |
cmd/cli/commands/*.go |
Applied normalization to all CLI commands that handle model names |
cmd/cli/commands/list.go |
Updated filtering logic and added display name stripping |
cmd/cli/commands/ps.go |
Added display name stripping for running models |
Comments suppressed due to low confidence (1)
cmd/cli/commands/list.go:1
- The
slicesimport is removed but the package may still be needed elsewhere in the file. Ensure all uses of theslicespackage have been replaced with alternative implementations.
package commands
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
Backend and frontend changes to make "ai/" and ":latest" optional and defaults, we need both. Now it's just: docker model run gemma |
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.
Code Review
This pull request introduces model name normalization and display stripping, which is a valuable enhancement for user experience and consistency across the CLI. The changes are applied to numerous commands, which is great. However, I've identified a critical bug in the stripDefaultsFromModelName function that leads to incorrect model name displays. Additionally, the normalizeModelName function is duplicated across three packages with slight inconsistencies, which poses a maintainability risk. I've provided detailed feedback and suggestions to address these issues.
460a84a to
fc07567
Compare
fc07567 to
c9c4844
Compare
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.
Pull Request Overview
Copilot reviewed 15 out of 16 changed files in this pull request and generated 3 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
c9c4844 to
0bc6f3b
Compare
0bc6f3b to
3fef10f
Compare
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.
Pull Request Overview
Copilot reviewed 15 out of 16 changed files in this pull request and generated 3 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
3fef10f to
882db0e
Compare
882db0e to
92acddb
Compare
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.
Pull Request Overview
Copilot reviewed 15 out of 16 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
cmd/cli/commands/list.go:3
- The unused import
"slices"should be removed since it's no longer needed after the filtering logic refactoring.
import (
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
92acddb to
e79a94b
Compare
e79a94b to
1e1f37d
Compare
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.
Pull Request Overview
Copilot reviewed 15 out of 16 changed files in this pull request and generated 3 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
1e1f37d to
7c3e7c9
Compare
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.
Pull Request Overview
Copilot reviewed 15 out of 16 changed files in this pull request and generated no new comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Add normalization to configure, list, and unload commands Signed-off-by: Eric Curtin <[email protected]>
dfff6ac to
9d0898e
Compare
| // - "myorg/gemma3:latest" -> "myorg/gemma3" | ||
| // - "gemma3:latest" -> "gemma3" | ||
| // - "hf.co/bartowski/model:latest" -> "hf.co/bartowski/model" | ||
| func stripDefaultsFromModelName(model string) string { |
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.
nit: Maybe this should be implemented in the model manager along with NormalizeModelName? Could be called ExtractModelBaseName().
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.
Good consideration, I thought of that at one point, might do it in a follow on PR. The reason it's not is one has wider use than the other. But I get your point they are similar, one does the opposite of the other.
I actually think it's a pity we didn't go lowercase for our dmr quantization sometimes, the :tag . We actually have code here that converts everything to lowercase for huggingface and it's generally what ollama does also.
p1-0tr
left a comment
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.
One small nit, but otherwise LGTM.
Add normalization to configure, list, and unload commands
Summary by Sourcery
Introduce consistent model name normalization and display stripping across the CLI and manager, ensuring default organization and tag are applied or removed appropriately.
New Features:
Enhancements:
Tests: