Package context size #243
Merged
Package context size #243
Conversation
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
Contributor
Reviewer's GuideThis PR enables creating new model variants by repackaging an existing model with config-only changes (e.g., context size) via a new --from flag and a lightweight repackaging path that avoids duplicating layer data. Sequence diagram for lightweight model repackaging workflowsequenceDiagram
actor User
participant CLI
participant DistributionClient
participant LocalStore
User->>CLI: Run 'docker model package --from <model> --context-size <tokens> <new_tag>'
CLI->>DistributionClient: Initialize builder from existing model
CLI->>DistributionClient: Set new config (e.g., context size)
CLI->>DistributionClient: Detect config-only change
CLI->>DistributionClient: Call WriteLightweightModel()
DistributionClient->>LocalStore: WriteLightweight(model, tags)
LocalStore->>LocalStore: Verify layers exist
LocalStore->>LocalStore: Write manifest and config only
DistributionClient->>CLI: Confirm model variant creation
Class diagram for updated Builder and DistributionClient classesclassDiagram
class Builder {
- model: ModelArtifact
- originalLayers: v1.Layer[]
+ FromGGUF(path)
+ FromSafetensors(paths)
+ FromModel(model)
+ WithLicense(path)
+ WithContextSize(size)
+ WithMultimodalProjector(path)
+ WithChatTemplateFile(path)
+ WithConfigArchive(path)
+ WithDirTar(path)
+ Model()
+ Build(ctx, target, pw)
+ HasOnlyConfigChanges()
}
class DistributionClient {
+ WriteLightweightModel(model, tags)
}
Builder "1" -- "1" ModelArtifact
DistributionClient "1" -- "1" LocalStore
Class diagram for LocalStore with WriteLightweight methodclassDiagram
class LocalStore {
+ Write(model, tags, writer)
+ WriteLightweight(model, tags)
+ Read(reference)
+ hasBlob(digest)
+ writeConfigFile(model)
+ WriteManifest(digest, manifest)
+ AddTags(digest, tags)
}
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
There was a problem hiding this comment.
Pull Request Overview
Adds a repackaging workflow to create lightweight model variants from existing models, enabling config-only changes (e.g., context size) without re-uploading layers.
- Introduces --from to docker model package and validation for mutually exclusive sources.
- Adds builder.FromModel/Model, distribution client WriteLightweightModel, and LocalStore.WriteLightweight to write only config+manifest when layers already exist.
- Adds builder tests covering FromModel with config changes and additional layers.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/distribution/internal/store/store.go | Implements WriteLightweight to write only config and manifest, verifying layers exist. |
| pkg/distribution/distribution/client.go | Adds WriteLightweightModel that delegates to store. |
| pkg/distribution/builder/builder_test.go | Adds tests for creating a builder from an existing model and modifying or extending it. |
| pkg/distribution/builder/builder.go | Adds FromModel and Model getters to support repackaging. |
| cmd/cli/commands/package.go | Adds --from flag, source validation, repackaging path selection, and calls into distribution client for lightweight writes. |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Comment on lines
272
to
281
| diffID, err := layer.DiffID() | ||
| if err != nil { | ||
| return fmt.Errorf("getting layer diffID: %w", err) | ||
| } | ||
| hasBlob, err := s.hasBlob(diffID) | ||
| if err != nil { | ||
| return fmt.Errorf("checking if layer %s exists: %w", diffID, err) | ||
| } | ||
| if !hasBlob { | ||
| return fmt.Errorf("layer %s not found in store, cannot use lightweight write", diffID) |
There was a problem hiding this comment.
Checking for existing layers via DiffID can force decompression and rehash of each layer, which undermines the 'lightweight' path. Prefer using layer.Digest() (compressed hash) for existence checks to avoid recomputation and to match typical content store keys. Example change: use digest, err := layer.Digest(); has, err := s.hasBlob(digest).
Suggested change
| diffID, err := layer.DiffID() | |
| if err != nil { | |
| return fmt.Errorf("getting layer diffID: %w", err) | |
| } | |
| hasBlob, err := s.hasBlob(diffID) | |
| if err != nil { | |
| return fmt.Errorf("checking if layer %s exists: %w", diffID, err) | |
| } | |
| if !hasBlob { | |
| return fmt.Errorf("layer %s not found in store, cannot use lightweight write", diffID) | |
| digest, err := layer.Digest() | |
| if err != nil { | |
| return fmt.Errorf("getting layer digest: %w", err) | |
| } | |
| hasBlob, err := s.hasBlob(digest) | |
| if err != nil { | |
| return fmt.Errorf("checking if layer %s exists: %w", digest, err) | |
| } | |
| if !hasBlob { | |
| return fmt.Errorf("layer %s not found in store, cannot use lightweight write", digest) |
juangcarmona
approved these changes
Oct 15, 2025
juangcarmona
left a comment
juangcarmona
left a comment
There was a problem hiding this comment.
Great job! It will work for many of us. Thanks!
|
It will be good if the 'docker model ls' can output context_size directly to avoid having to carry out an inspection for this parameter by 'docker model inspect'. |
Contributor
Author
I think that's a great idea, I'll open an issue, in case anyone of you would like to contribute feel free to open a PR! |
Contributor
Author
|
@aleksmelnikov this is the issue: #246 |
# Conflicts: # cmd/cli/commands/package.go # cmd/cli/docs/reference/docker_model_package.yaml # cmd/cli/docs/reference/model.md # cmd/cli/docs/reference/model_package.md
Contributor
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <[email protected]>
Contributor
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 5 out of 5 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.
Contributor
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Contributor
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
cmd/cli/commands/package.go:1
- The HasOnlyConfigChanges check is performed before applying layer-affecting options like --dir-tar. If the user supplies --from and --dir-tar, HasOnlyConfigChanges will incorrectly evaluate to true and the lightweight branch will run, silently ignoring the requested dir-tar additions. Move the processing of all potential layer additions (e.g., --dir-tar, --license, --multimodal-projector, etc.) before computing useLightweight, or guard the lightweight path with len(opts.dirTarPaths) == 0 (and other layer-affecting flags) to ensure we don't skip requested layers.
package commands
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}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `pkg/distribution/builder/builder.go:154-163` </location>
<code_context>
+// HasOnlyConfigChanges returns true if the builder was created from an existing model
</code_context>
<issue_to_address>
**issue (bug_risk):** HasOnlyConfigChanges may return false negatives if Layers() returns layers in a different order.
If Layers() does not guarantee order, sort or normalize layers by digest before comparing to ensure accurate results.
</issue_to_address>
### Comment 2
<location> `pkg/distribution/internal/store/store.go:271-280` </location>
<code_context>
+ return fmt.Errorf("getting layers: %w", err)
+ }
+
+ for _, layer := range layers {
+ digest, err := layer.Digest()
+ if err != nil {
+ return fmt.Errorf("getting layer digest: %w", err)
+ }
+ hasBlob, err := s.hasBlob(digest)
+ if err != nil {
+ return fmt.Errorf("checking if layer %s exists: %w", digest, err)
+ }
+ if !hasBlob {
+ return fmt.Errorf("layer %s not found in store, cannot use lightweight write", digest)
+ }
</code_context>
<issue_to_address>
**suggestion:** The WriteLightweight method does not handle concurrent writes or partial failures.
Failures between writing config, manifest, or tags may leave the store inconsistent. Please implement rollback or atomic operations, or clearly document the risk of partial writes.
Suggested implementation:
```golang
/*
WriteLightweight writes only the manifest and config for a model, assuming layers already exist in the store.
This is used for config-only modifications where the layer data hasn't changed.
Note: This method attempts to rollback config/manifest/tag writes if a failure occurs, but does not guarantee atomicity across all files.
Concurrent writes may still result in inconsistent state. Use with caution if strong consistency is required.
*/
func (s *LocalStore) WriteLightweight(mdl v1.Image, tags []string) error {
```
```golang
// Write the config JSON file
var writtenFiles []string
configPath := s.configPath(mdl)
if err := s.writeConfig(mdl, configPath); err != nil {
return fmt.Errorf("writing config: %w", err)
}
writtenFiles = append(writtenFiles, configPath)
manifestPath := s.manifestPath(mdl)
if err := s.writeManifest(mdl, manifestPath); err != nil {
// Rollback config
_ = s.removeFile(configPath)
return fmt.Errorf("writing manifest: %w", err)
}
writtenFiles = append(writtenFiles, manifestPath)
for _, tag := range tags {
tagPath := s.tagPath(tag)
if err := s.writeTag(mdl, tagPath); err != nil {
// Rollback manifest and config
for _, f := range writtenFiles {
_ = s.removeFile(f)
}
return fmt.Errorf("writing tag %s: %w", tag, err)
}
writtenFiles = append(writtenFiles, tagPath)
}
return nil
```
You will need to implement the following helper methods in your `LocalStore` type if they do not already exist:
- `configPath(mdl v1.Image) string`
- `manifestPath(mdl v1.Image) string`
- `tagPath(tag string) string`
- `writeConfig(mdl v1.Image, path string) error`
- `writeManifest(mdl v1.Image, path string) error`
- `writeTag(mdl v1.Image, path string) error`
- `removeFile(path string) error` (should safely remove a file if it exists)
If your codebase already has similar helpers, adjust the calls accordingly.
</issue_to_address>
### Comment 3
<location> `pkg/distribution/internal/store/store.go:302` </location>
<code_context>
+ if err := s.WriteManifest(digest, rm); err != nil {
+ return fmt.Errorf("write manifest: %w", err)
+ }
+ if err := s.AddTags(digest.String(), tags); err != nil {
+ return fmt.Errorf("adding tags: %w", err)
+ }
</code_context>
<issue_to_address>
**question (bug_risk):** AddTags is called after writing the manifest, which could lead to untagged manifests on failure.
If AddTags fails, the manifest remains in the store without any tag reference. Please review if this orphaned state is acceptable or if additional cleanup should be implemented.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+271
to
+280
| for _, layer := range layers { | ||
| digest, err := layer.Digest() | ||
| if err != nil { | ||
| return fmt.Errorf("getting layer digest: %w", err) | ||
| } | ||
| hasBlob, err := s.hasBlob(digest) | ||
| if err != nil { | ||
| return fmt.Errorf("checking if layer %s exists: %w", digest, err) | ||
| } | ||
| if !hasBlob { |
Contributor
There was a problem hiding this comment.
suggestion: The WriteLightweight method does not handle concurrent writes or partial failures.
Failures between writing config, manifest, or tags may leave the store inconsistent. Please implement rollback or atomic operations, or clearly document the risk of partial writes.
Suggested implementation:
/*
WriteLightweight writes only the manifest and config for a model, assuming layers already exist in the store.
This is used for config-only modifications where the layer data hasn't changed.
Note: This method attempts to rollback config/manifest/tag writes if a failure occurs, but does not guarantee atomicity across all files.
Concurrent writes may still result in inconsistent state. Use with caution if strong consistency is required.
*/
func (s *LocalStore) WriteLightweight(mdl v1.Image, tags []string) error { // Write the config JSON file
var writtenFiles []string
configPath := s.configPath(mdl)
if err := s.writeConfig(mdl, configPath); err != nil {
return fmt.Errorf("writing config: %w", err)
}
writtenFiles = append(writtenFiles, configPath)
manifestPath := s.manifestPath(mdl)
if err := s.writeManifest(mdl, manifestPath); err != nil {
// Rollback config
_ = s.removeFile(configPath)
return fmt.Errorf("writing manifest: %w", err)
}
writtenFiles = append(writtenFiles, manifestPath)
for _, tag := range tags {
tagPath := s.tagPath(tag)
if err := s.writeTag(mdl, tagPath); err != nil {
// Rollback manifest and config
for _, f := range writtenFiles {
_ = s.removeFile(f)
}
return fmt.Errorf("writing tag %s: %w", tag, err)
}
writtenFiles = append(writtenFiles, tagPath)
}
return nilYou will need to implement the following helper methods in your LocalStore type if they do not already exist:
configPath(mdl v1.Image) stringmanifestPath(mdl v1.Image) stringtagPath(tag string) stringwriteConfig(mdl v1.Image, path string) errorwriteManifest(mdl v1.Image, path string) errorwriteTag(mdl v1.Image, path string) errorremoveFile(path string) error(should safely remove a file if it exists)
If your codebase already has similar helpers, adjust the calls accordingly.
| if err := s.WriteManifest(digest, rm); err != nil { | ||
| return fmt.Errorf("write manifest: %w", err) | ||
| } | ||
| if err := s.AddTags(digest.String(), tags); err != nil { |
Contributor
There was a problem hiding this comment.
question (bug_risk): AddTags is called after writing the manifest, which could lead to untagged manifests on failure.
If AddTags fails, the manifest remains in the store without any tag reference. Please review if this orphaned state is acceptable or if additional cleanup should be implemented.
Contributor
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
cmd/cli/commands/package.go:1
- The lightweight decision is made before processing --dir-tar paths, so using --from together with --dir-tar can incorrectly take the lightweight path and silently ignore the requested tar layers. Consider either including the presence of dirTarPaths in the decision (e.g.,
useLightweight := opts.fromModel != \"\" && len(opts.dirTarPaths) == 0 && pkg.HasOnlyConfigChanges()) or moving the dir-tar processing before evaluating HasOnlyConfigChanges.
package commands
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
ericcurtin
approved these changes
Oct 20, 2025
Contributor
ericcurtin
left a comment
ericcurtin
left a comment
There was a problem hiding this comment.
LGTM, worth reading through the bot comments to see if they are worth picking up.
Contributor
Author
@ericcurtin the comments related to leaving the store in an inconsistent state, while true, are not related to this PR, as the |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
fixes #223
How to use it:
This pull request adds support for creating new model variants by repackaging an existing model, allowing users to make config-only modifications (such as changing context size) without re-uploading or duplicating large model files. It introduces a new
--fromflag to thedocker model packagecommand, implements lightweight repackaging logic, and ensures that only config changes (with no new files) use this optimized path. The changes also include new tests for the repackaging workflow.New packaging source and CLI enhancements:
--from <model>flag to thedocker model packagecommand, allowing users to repackage an existing model as a new variant. The CLI now validates that only one of--gguf,--safetensors-dir, or--fromis provided.Builder and distribution logic:
FromModelfunction in thebuilderpackage to create a new builder from an existing model artifact, enabling modifications such as changing config or adding layers. Also added aModel()method to access the underlying model artifact.Lightweight repackaging implementation:
WriteLightweightModelin the distribution client andWriteLightweightin the local store, enabling efficient creation of new model variants by writing only the manifest and config when all layers already exist in the store.Testing and validation:
Dependency and import updates:
distributionpackage in the CLI to support the new distribution client functionality.Summary by Sourcery
Introduce lightweight repackaging of existing Docker model artifacts to enable config-only variants without duplicating layer data. Add a new --from flag to docker model package, implement builder and store support for writing only manifest and config, and update CLI logic and documentation accordingly.
New Features:
Enhancements:
Documentation:
Tests: