Fix model namespace normalization for package and tag commands#262
Fix model namespace normalization for package and tag commands#262ericcurtin merged 1 commit intomainfrom
Conversation
Reviewer's GuideThis PR enforces consistent normalization of model namespaces by applying NormalizeModelName in the package and tag commands and adding a comprehensive test to verify the expected normalization behavior. Sequence diagram for model name normalization in package and tag commandssequenceDiagram
participant User as actor User
participant CLI as CLI Command
participant Models as models.NormalizeModelName
participant Registry as Registry
User->>CLI: Run package/tag command with model name
CLI->>Models: NormalizeModelName(modelName)
Models-->>CLI: Normalized model name
CLI->>Registry: Use normalized model name for further processing
Class diagram for NormalizeModelName usage in CLI commandsclassDiagram
class CLICommand {
+string modelName
+string normalizedModelName
+run()
}
class Models {
+NormalizeModelName(name string) string
}
CLICommand --> Models : 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 standardizes the handling of model names within the CLI's 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
|
|
@mikesir87 I think this fixes your issue WDYT? |
There was a problem hiding this comment.
Pull Request Overview
Fixes inconsistent model name normalization across package and tag commands; adds tests to document and prevent regressions.
- Normalize target name in tag command to include default org/tag
- Normalize tag string in package command before parsing
- Add tests to verify NormalizeModelName consistency
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| cmd/cli/commands/tag.go | Ensures both source and target refs are normalized before tagging. |
| cmd/cli/commands/package.go | Normalizes the user-supplied tag/model ref prior to name.NewTag parsing. |
| cmd/cli/commands/utils_test.go | Adds table-driven tests asserting NormalizeModelName behavior. |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Code Review
This pull request effectively addresses the inconsistency in model name normalization across the package and tag commands by consistently applying models.NormalizeModelName. The changes are clear and the addition of TestNormalizeModelNameConsistency is a great step towards ensuring this behavior doesn't regress.
However, I've identified a critical issue that could lead to a panic. In the package command flow, if an empty string is provided as the model name (e.g., docker model package ""), newModelRunnerTarget is called with an empty tag. This results in target.tag being nil. Subsequently, the modelRunnerTarget.Write method will panic when it calls t.tag.String(). While the fix for this panic is outside the changed lines of this PR (it would involve adding a nil check for t.tag in modelRunnerTarget.Write before calling methods on it), this is a critical bug that can be triggered from the CLI and should be addressed.
I have also left one comment regarding test structure to improve maintainability.
ilopezluna
left a comment
There was a problem hiding this comment.
Trying to package a local model using the branch I get the following error:
docker model package --gguf /Users/ilopezluna/Desktop/models/gemma3n-4b-it-text-GGUF/model.gguf \
ignaciolopezluna020/embedding:dir-test
Adding GGUF file from "/Users/ilopezluna/Desktop/models/gemma3n-4b-it-text-GGUF/model.gguf"
Loading model to Model Runner...
Transferred: 13103.58 MB
Failed to package model
package model: failed to load packaged model: tag model: tagging failed with status 404 Not Found: tagging model: model not found
a1aefad to
57e7644
Compare
|
Thanks @ilopezluna removing this line fixes it, I think: please keep reporting these issues, it's appreciated. |
57e7644 to
10d2d97
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 3 out of 3 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 test to verify model namespace normalization consistency Signed-off-by: Eric Curtin <eric.curtin@docker.com>
771713b to
82db531
Compare
Add test to verify model namespace normalization consistency
Summary by Sourcery
Use models.NormalizeModelName to normalize model names in both package and tag commands, ensuring default namespace and tag are applied consistently, and add tests to verify this behavior.
Bug Fixes:
Enhancements:
Tests: