Conversation
…l layer files from nested subdirectories
Contributor
Summary of ChangesHello @ilopezluna, 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 introduces a significant update to the model packaging system, primarily focusing on enhancing how Safetensors models are handled. It transitions to a more granular, layer-per-file approach for Safetensors, which improves flexibility and metadata extraction. Concurrently, it streamlines the packaging CLI by removing the generic 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}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a new V0.2 layer-per-file packaging mechanism and refactors the existing packaging logic. This involves removing the packaging package and the --dir-tar command-line flag. The documentation has been updated to reflect these changes. The overall changes simplify the packaging process, especially for Safetensors models.
I have a couple of suggestions for improvement:
- In
pkg/distribution/builder/from_directory.go, errors during format handler retrieval and config extraction are silently ignored. It would be better to log these errors to aid in debugging. - In
pkg/distribution/huggingface/model.go, the new logic for handling GGUF models from Hugging Face seems to have dropped support for packaging auxiliary configuration files (liketokenizer.json), which could be a regression.
Additionally, I noticed the vllm backend has been removed. I assume this is an intentional part of the refactoring.
…mand descriptions
ilopezluna
force-pushed
the
use-new-packaging
branch
from
cd3860f to
1f61305
Compare
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
findSafetensorsFileyou returnfilepath.SkipAllfrom the Walk callback, butfilepathdoesn’t defineSkipAll(onlySkipDir), so this won’t compile; use the appropriate sentinel (e.g.,filepath.SkipDir) or restructure the walk logic to terminate early in a supported way. - The refactor of
buildModelFromFiles/buildGGUFModelV01removes the previous config archive handling for GGUF models, so non-chat-template config files downloaded from HuggingFace are no longer added to the artifact; confirm this is intentional or reintroduce packaging of tokenizer/merges/etc. to avoid regressions when running GGUF models built from HF.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `findSafetensorsFile` you return `filepath.SkipAll` from the Walk callback, but `filepath` doesn’t define `SkipAll` (only `SkipDir`), so this won’t compile; use the appropriate sentinel (e.g., `filepath.SkipDir`) or restructure the walk logic to terminate early in a supported way.
- The refactor of `buildModelFromFiles`/`buildGGUFModelV01` removes the previous config archive handling for GGUF models, so non-chat-template config files downloaded from HuggingFace are no longer added to the artifact; confirm this is intentional or reintroduce packaging of tokenizer/merges/etc. to avoid regressions when running GGUF models built from HF.
## Individual Comments
### Comment 1
<location> `pkg/distribution/huggingface/model.go:118-113` </location>
<code_context>
+
+// buildGGUFModelV01 builds a GGUF model using V0.1 packaging (backward compatible).
+// GGUF uses FromPaths + config archive approach.
+func buildGGUFModelV01(localPaths map[string]string, weightFiles, configFiles []RepoFile, tempDir string) (types.ModelArtifact, error) {
// Collect weight file paths (sorted for reproducibility)
var weightPaths []string
</code_context>
<issue_to_address>
**issue (bug_risk):** GGUF V0.1 builder no longer uses configFiles despite comment claiming a config archive approach
The current `buildGGUFModelV01` implementation no longer uses `configFiles`: the previous `WithConfigArchive` flow is gone, but the comment still says `GGUF uses FromPaths + config archive approach.` Please either (a) update the comment and remove the unused `configFiles` parameter if config packaging is intentionally dropped, or (b) restore the config-archive handling to preserve the prior behavior.
</issue_to_address>
### Comment 2
<location> `pkg/distribution/internal/bundle/parse.go:115-124` </location>
<code_context>
+ // Search recursively for V0.2 models with nested directory structure
+ // (e.g., text_encoder/model.safetensors)
+ var firstFound string
+ walkErr := filepath.Walk(modelDir, func(path string, info os.FileInfo, err error) error {
+ if err != nil {
+ return nil // skip errors
+ }
+ if info.IsDir() {
+ return nil
+ }
+ if filepath.Ext(path) == ".safetensors" && !strings.HasPrefix(info.Name(), ".") {
+ rel, relErr := filepath.Rel(modelDir, path)
+ if relErr == nil {
+ firstFound = rel
+ return filepath.SkipAll // found one, stop walking
+ }
+ }
+ return nil
+ })
+ if walkErr != nil {
+ return "", fmt.Errorf("walk for safetensors files: %w", walkErr)
+ }
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Recursive safetensors search swallows Walk/Rel errors, which may hide real filesystem issues
In the recursive branch, the walk callback ignores all errors and also drops `filepath.Rel` failures, so permission/IO/path issues are indistinguishable from “no safetensors found” (returns `"", nil`). This can mask real problems with malformed or unreadable bundles. It would be better to propagate the first meaningful error (and return it) or at least log it so callers can tell the difference between absence of files and filesystem failures.
Suggested implementation:
```golang
walkErr := filepath.Walk(modelDir, func(path string, info os.FileInfo, err error) error {
if err != nil {
// Propagate filesystem errors so callers can distinguish them from
// the case where no safetensors files are present.
return err
}
```
```golang
if filepath.Ext(path) == ".safetensors" && !strings.HasPrefix(info.Name(), ".") {
rel, relErr := filepath.Rel(modelDir, path)
if relErr != nil {
// Treat a bad relative path as a real error instead of silently
// ignoring it, so malformed bundles surface to the caller.
return relErr
}
firstFound = rel
return filepath.SkipAll // found one, stop walking
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
# Conflicts: # pkg/distribution/huggingface/model.go
Contributor
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
findSafetensorsFile, returningfilepath.SkipAllfrom afilepath.Walkcallback will be treated as an error and wrapped aswalk for safetensors files: skip all; usefilepath.SkipDirwithWalkor switch toWalkDirand handleSkipAllexplicitly so early-exit doesn't surface as an error.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `findSafetensorsFile`, returning `filepath.SkipAll` from a `filepath.Walk` callback will be treated as an error and wrapped as `walk for safetensors files: skip all`; use `filepath.SkipDir` with `Walk` or switch to `WalkDir` and handle `SkipAll` explicitly so early-exit doesn't surface as an error.
## Individual Comments
### Comment 1
<location> `pkg/distribution/internal/bundle/parse.go:124` </location>
<code_context>
+ if info.IsDir() {
+ return nil
+ }
+ if filepath.Ext(path) == ".safetensors" && !strings.HasPrefix(info.Name(), ".") {
+ rel, relErr := filepath.Rel(modelDir, path)
+ if relErr != nil {
</code_context>
<issue_to_address>
**issue (bug_risk):** The use of `filepath.SkipAll` will not compile; use a supported sentinel (e.g., `filepath.SkipDir`) or a custom error to stop walking.
`filepath.Walk` can only short-circuit via `filepath.SkipDir` or a non-nil error; `filepath.SkipAll` does not exist in the package, so this won’t compile. To stop after the first match, either:
- Return a sentinel error (e.g. `errFoundSafetensors`) and treat it specially after `Walk`, or
- Carefully use `filepath.SkipDir` so it only affects the intended directory.
A sentinel error is typically the simplest and safest option.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
doringeman
approved these changes
Feb 13, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request simplifies and improves the
docker model packagecommand by removing the--dir-taroption and its related implementation, updating documentation for clarity and accuracy, and enhancing how configuration metadata is extracted for directory-based model packaging. The changes also improve the user experience with better help text and more robust handling of model formats.CLI and Documentation Improvements:
--dir-taroption and all related code, including flag definitions, validation, processing logic, and documentation references. This option is no longer supported.docker model packagehelp text, command documentation, and CLI reference files to provide clearer, more structured instructions and usage details. The new help text explicitly describes supported input types and packaging behaviors.Packaging Logic Enhancements:
FromDirectorybuilder method, simplifying the code and ensuring each file is packaged as a separate OCI layer.Compatibility Handling: