Add support for Modelfile: parse Modelfile and apply directives#628
Add support for Modelfile: parse Modelfile and apply directives#628Pnkcaht wants to merge 3 commits intodocker:mainfrom
Conversation
…tives Signed-off-by: pnkcaht <samzoovsk19@gmail.com>
Summary of ChangesHello @Pnkcaht, 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 significantly enhances the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for Modelfile to configure model packaging, which is a great feature for improving user experience and centralizing configuration. The implementation correctly parses the Modelfile, handles various directives and their aliases, and prioritizes CLI flags over file-based options. The code includes good error handling with informative messages.
I've identified a few areas for improvement:
- The path normalization logic is overly restrictive, preventing the use of valid absolute or relative paths that point outside the
Modelfile's directory. - The validation for
DIR_TARpaths can incorrectly reject valid paths. - A long conditional statement for path type validation could be refactored for better readability.
My detailed comments provide specific suggestions to address these points. Overall, this is a solid contribution that will significantly enhance the package command.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The new
applyModelfilehelper doesn’t appear to be invoked anywhere in the command flow, so Modelfile directives may never actually be applied; consider wiring it intoRunE(or equivalent) before the options are validated/used. - The
normalizePathlogic currently rejects any path that resolves outside the Modelfile’s directory (via thefilepath.Relcheck), which conflicts with the stated support for absolute paths and may unnecessarily block legitimate absolute locations; consider either relaxing this constraint or scoping the traversal check to only disallow..in relative paths.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `applyModelfile` helper doesn’t appear to be invoked anywhere in the command flow, so Modelfile directives may never actually be applied; consider wiring it into `RunE` (or equivalent) before the options are validated/used.
- The `normalizePath` logic currently rejects any path that resolves outside the Modelfile’s directory (via the `filepath.Rel` check), which conflicts with the stated support for absolute paths and may unnecessarily block legitimate absolute locations; consider either relaxing this constraint or scoping the traversal check to only disallow `..` in relative paths.
## Individual Comments
### Comment 1
<location> `cmd/cli/commands/package.go:673-682` </location>
<code_context>
+
+// normalizePath resolves relative paths relative to the Modelfile's directory and cleans them.
+// Rejects path traversal attempts.
+func normalizePath(path, baseDir string) (string, error) {
+ if !filepath.IsAbs(path) {
+ path = filepath.Join(baseDir, path)
+ }
+
+ abs, err := filepath.Abs(path)
+ if err != nil {
+ return "", err
+ }
+
+ cleanPath := filepath.Clean(abs)
+ // Extra security: reject any '..' segments after normalization
+ rel, err := filepath.Rel(baseDir, cleanPath)
+ if err != nil {
+ return "", err
</code_context>
<issue_to_address>
**🚨 suggestion (security):** Consider normalizing `baseDir` to an absolute, cleaned path before calling `filepath.Rel`.
`normalizePath` converts `path` to an absolute path, but `baseDir` may still be relative (e.g., if `opts.modelfile` was relative). Using a relative `baseDir` in `filepath.Join` and later `filepath.Rel` makes the traversal check dependent on the current working directory and can yield unexpected `Rel` results or errors. Normalizing `baseDir` with `filepath.Abs`/`filepath.Clean` first would make the traversal check more reliable and independent of CWD.
Suggested implementation:
```golang
// normalizePath resolves relative paths relative to the Modelfile's directory and cleans them.
// Rejects path traversal attempts.
func normalizePath(path, baseDir string) (string, error) {
// Normalize baseDir to an absolute, cleaned path so checks are independent of CWD.
baseAbs, err := filepath.Abs(baseDir)
if err != nil {
return "", err
}
baseClean := filepath.Clean(baseAbs)
if !filepath.IsAbs(path) {
path = filepath.Join(baseClean, path)
}
abs, err := filepath.Abs(path)
if err != nil {
return "", err
}
cleanPath := filepath.Clean(abs)
// Extra security: reject any '..' segments after normalization
rel, err := filepath.Rel(baseClean, cleanPath)
if err != nil {
return "", err
}
```
If there are any other callers that construct `baseDir` assuming it is relative to the current working directory, those assumptions are now explicitly enforced by `filepath.Abs(baseDir)` inside `normalizePath`. No further code changes should be required, but it may be worth double‑checking any tests that rely on the previous, CWD-dependent behavior.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Since I'm working on other issues, I haven't added the optional elements I mentioned in the description yet. However, I will add them and submit a new PR or update this one if it's not merged. The code needs to be perfect, so if you find any errors, even the smallest ones, please let me know. Thank you. |
Signed-off-by: pnkcaht <samzoovsk19@gmail.com>
There was a problem hiding this comment.
Code Review
This pull request adds excellent support for using a Modelfile to configure the docker model package command. This greatly improves the user experience by allowing for centralized, reusable model packaging configurations. The implementation is robust, correctly handling path resolution, prioritizing CLI flags, and including important security checks for path traversal. The code is well-structured, with clear separation of concerns into helper functions. My review focuses on a few minor areas for improvement to align with common Go idioms and enhance maintainability, such as defining static data at the package level and improving an error message for better user feedback. Overall, this is a well-executed and valuable addition.
|
Since this has heavy influence from Ollama, I propose docker model create. About the point on working on many issues. If you want to pursue this, you need to actively own it. That means every bug, issue, concern etc. must be promptly addressed by you (and you have to strive to keep up with Ollama syntax). If you need this feature and have the capacity to own this indefinitely, please continue. |
|
understand the ownership concern and I’m comfortable actively maintaining this feature — addressing bugs, feedback, and keeping the behavior aligned where appropriate. I’m already a regular contributor across other CNCF/Docker-related projects (like MCP and Cagent), and I intend to apply the same level of care here. I’m happy to iterate on naming (docker model create vs the current approach) and scope to better fit the project direction. If at any point this feature no longer aligns with the roadmap, I’m open to adjusting or narrowing it accordingly. Let me know what level of scope and ownership you’d be comfortable with for merging this. |
Signed-off-by: pnkcaht <samzoovsk19@gmail.com>
2 participants
What I Did
Modelfilein thepackagecommand.packageOptions.SAFETENSORS-DIR,CHAT-TEMPLATE,MM-PROJ,CONTEXT, etc.Releated Issue
Closes #613
Before / After
Before
After
Modelfile example
Diagram
flowchart TD A[Start: CLI command] --> B{Modelfile provided?} B -- No --> C[Use CLI flags only] B -- Yes --> D[Parse Modelfile line by line] D --> E{Instruction type} E -- Source (GGUF/DDUF/SAFE) --> F[Set opts.<field> if empty] E -- License/Chat/MMProj --> G[Append or set path] E -- Context/CTX --> H[Set context size if unset] E -- DIR-TAR --> I[Append relative paths] F & G & H & I --> J[Finalized options] J --> K[Initialize Builder] K --> L[Package Model] L --> M{Push?} M -- Yes --> N[Push to registry] M -- No --> O[Load into Model Runner] N & O --> P[End: Success]Missing / Optional Considerations
The following optional fields may cause issues if used incorrectly but are not required:
If omitted, the builder will still work:
Users can specify these either via Modelfile or CLI flags.