Package Safetensors #194
Package Safetensors #194
Conversation
Reviewer's GuideThis PR adds support for packaging Safetensors-format models by extracting directory scanning and config archiving into a new packaging package, and updating both the CLI and mdltool to leverage this reusable logic with appropriate flags, validations, and builder integrations. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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 significantly enhances the model packaging capabilities by integrating native support for the Safetensors format. Users can now package Safetensors models directly from a directory, which will automatically discover 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.
Pull Request Overview
This pull request adds support for packaging Safetensors models alongside existing GGUF support. The implementation introduces a new code path for handling Safetensors directories, including validation, file discovery, and temporary archiving of configuration files.
Key changes:
- Refactored packaging logic into a reusable
packagingpackage containing directory scanning and tar archive creation - Updated CLI to support
--safetensors-dirflag as an alternative to--gguf - Modified
mdltoolto use the new shared packaging functionality
Reviewed Changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pkg/distribution/packaging/safetensors.go | New package providing directory scanning and config archive creation for Safetensors models |
| cmd/mdltool/main.go | Refactored to use shared packaging functions, removed duplicate code |
| cmd/cli/go.mod | Updated module dependencies and added local replacement |
| cmd/cli/commands/package.go | Added Safetensors support with new CLI flag and validation logic |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Use cobra’s MarkFlagsMutuallyExclusive for --gguf and --safetensors-dir to simplify mutual‐exclusion validation and improve CLI help messaging.
- Extract the repeated absolute‐path validation and cleaning logic into a shared helper to reduce duplication between GGUF and Safetensors flag handling.
- Consider refactoring PackageFromDirectory to return an io.Reader (or in‐memory buffer) for the config archive so callers don’t need to manage temporary file creation and cleanup.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Use cobra’s MarkFlagsMutuallyExclusive for --gguf and --safetensors-dir to simplify mutual‐exclusion validation and improve CLI help messaging.
- Extract the repeated absolute‐path validation and cleaning logic into a shared helper to reduce duplication between GGUF and Safetensors flag handling.
- Consider refactoring PackageFromDirectory to return an io.Reader (or in‐memory buffer) for the config archive so callers don’t need to manage temporary file creation and cleanup.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![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 effectively adds support for packaging Safetensors models. The changes are well-structured, introducing a new packaging package to share logic between the CLI and mdltool, which is a great refactoring. The command-line argument handling and validation for the new --safetensors-dir flag are thorough. I have a couple of suggestions to improve error handling and code maintainability.
| for _, filePath := range configFiles { | ||
| // Open the file | ||
| file, err := os.Open(filePath) | ||
| if err != nil { | ||
| tw.Close() | ||
| tmpFile.Close() | ||
| os.Remove(tmpPath) | ||
| return "", fmt.Errorf("open config file %s: %w", filePath, err) | ||
| } | ||
|
|
||
| // Get file info for tar header | ||
| fileInfo, err := file.Stat() | ||
| if err != nil { | ||
| file.Close() | ||
| tw.Close() | ||
| tmpFile.Close() | ||
| os.Remove(tmpPath) | ||
| return "", fmt.Errorf("stat config file %s: %w", filePath, err) | ||
| } | ||
|
|
||
| // Create tar header (use only basename, not full path) | ||
| header := &tar.Header{ | ||
| Name: filepath.Base(filePath), | ||
| Size: fileInfo.Size(), | ||
| Mode: int64(fileInfo.Mode()), | ||
| ModTime: fileInfo.ModTime(), | ||
| } | ||
|
|
||
| // Write header | ||
| if err := tw.WriteHeader(header); err != nil { | ||
| file.Close() | ||
| tw.Close() | ||
| tmpFile.Close() | ||
| os.Remove(tmpPath) | ||
| return "", fmt.Errorf("write tar header for %s: %w", filePath, err) | ||
| } | ||
|
|
||
| // Copy file contents | ||
| if _, err := io.Copy(tw, file); err != nil { | ||
| file.Close() | ||
| tw.Close() | ||
| tmpFile.Close() | ||
| os.Remove(tmpPath) | ||
| return "", fmt.Errorf("write tar content for %s: %w", filePath, err) | ||
| } | ||
|
|
||
| file.Close() | ||
| } |
There was a problem hiding this comment.
The error handling logic within this for loop is repeated in multiple places (e.g., lines 84-88, 93-98, etc.). The cleanup sequence tw.Close(); tmpFile.Close(); os.Remove(tmpPath) is duplicated, which makes the code verbose and harder to maintain. Consider refactoring to reduce this duplication. One approach would be to extract the logic for adding a single file to the tar archive into a helper function. Another is to wrap the loop in a closure to centralize the cleanup logic.
# Conflicts: # cmd/cli/commands/package.go # cmd/cli/go.mod # cmd/cli/go.sum
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
This pull request adds support for packaging models stored in Safetensors format in both the CLI (
docker model package) and themdltoolutility. The changes introduce a new code path for handling Safetensors directories, including validation, file discovery, and temporary archiving of configuration files. The packaging logic for Safetensors is refactored into a reusable function in a newpackagingpackage.Summary by Sourcery
Add support for packaging models stored in the Safetensors format across both the CLI and mdltool, extracting Safetensors directory scanning and config archiving into a reusable packaging package.
New Features:
Enhancements:
Build:
Documentation: