Skip to content

Commit 2c53258

Browse files
Fix package command (#514)
* fix(package): update packageModel function signature to include context and client to make it easier to test * test(integration): add integration tests for packaging GGUF models * Do not normalize model name in the client * Move model name normalization to distribution * Move model name normalization into client to be able to resolve short model IDs * Add test to ensure short IDs are properly resolved * remove unneeded tests * update tagging to be a no op when using same tag as reference * update model filtering logic to match docker cli * No need to normalize in cli anymore * fix lint * remove unneeded tests * fix(distribution): normalize tags in WriteLightweightModel Signed-off-by: Dorin Geman <[email protected]> --------- Signed-off-by: Dorin Geman <[email protected]> Co-authored-by: Dorin Geman <[email protected]>
1 parent ec4b03e commit 2c53258

File tree

14 files changed

+715
-628
lines changed

14 files changed

+715
-628
lines changed

cmd/cli/commands/integration_test.go

Lines changed: 125 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -587,32 +587,6 @@ func TestIntegration_TagModel(t *testing.T) {
587587
})
588588
}
589589

590-
// Final verification: List the model and verify all tags are present
591-
t.Run("verify all tags in model inspect", func(t *testing.T) {
592-
inspectedModel, err := env.client.Inspect(modelID, false)
593-
require.NoError(t, err, "Failed to inspect model by ID")
594-
595-
t.Logf("Model has %d tags: %v", len(inspectedModel.Tags), inspectedModel.Tags)
596-
597-
// The model should have at least the original tag plus all created tags
598-
require.GreaterOrEqual(t, len(inspectedModel.Tags), len(createdTags)+1,
599-
"Model should have at least %d tags (original + created)", len(createdTags)+1)
600-
601-
// Verify each created tag is in the model's tag list
602-
for expectedTag := range createdTags {
603-
found := false
604-
for _, actualTag := range inspectedModel.Tags {
605-
if actualTag == expectedTag || actualTag == fmt.Sprintf("%s:latest", expectedTag) { // Handle implicit latest tag
606-
found = true
607-
break
608-
}
609-
}
610-
require.True(t, found, "Expected tag %s not found in model's tag list", expectedTag)
611-
}
612-
613-
t.Logf("✓ All %d created tags verified in model's tag list", len(createdTags))
614-
})
615-
616590
// Test error case: tagging non-existent model
617591
t.Run("error on non-existent model", func(t *testing.T) {
618592
err := tagModel(newTagCmd(), env.client, "non-existent-model:v1", "ai/should-fail:latest")
@@ -1016,6 +990,131 @@ func TestIntegration_RemoveModel(t *testing.T) {
1016990
})
1017991
}
1018992

993+
// TestIntegration_PackageModel tests packaging a GGUF model file
994+
// to ensure the model is properly loaded and tagged in the model store.
995+
// This test reproduces issue #461 where packaging fails with "model not found" during tagging.
996+
func TestIntegration_PackageModel(t *testing.T) {
997+
env := setupTestEnv(t)
998+
999+
// Ensure no models exist initially
1000+
models, err := listModels(false, env.client, true, false, "")
1001+
require.NoError(t, err)
1002+
if len(models) != 0 {
1003+
t.Fatal("Expected no initial models, but found some")
1004+
}
1005+
1006+
// Use the dummy GGUF file from assets
1007+
dummyGGUFPath := filepath.Join("../../../assets/dummy.gguf")
1008+
absPath, err := filepath.Abs(dummyGGUFPath)
1009+
require.NoError(t, err)
1010+
1011+
// Check if the file exists
1012+
_, err = os.Stat(absPath)
1013+
require.NoError(t, err, "dummy.gguf not found at %s", absPath)
1014+
1015+
// Test case 1: Package a GGUF file with a simple tag
1016+
t.Run("package GGUF with simple tag", func(t *testing.T) {
1017+
targetTag := "ai/packaged-test:latest"
1018+
1019+
// Create package options
1020+
opts := packageOptions{
1021+
ggufPath: absPath,
1022+
tag: targetTag,
1023+
}
1024+
1025+
// Execute the package command using the helper function with test client
1026+
t.Logf("Packaging GGUF file %s as %s", absPath, targetTag)
1027+
err := packageModel(env.ctx, newPackagedCmd(), env.client, opts)
1028+
require.NoError(t, err, "Failed to package GGUF model")
1029+
1030+
// Verify the model was loaded and tagged
1031+
t.Logf("Verifying model was loaded and tagged")
1032+
models, err := listModels(false, env.client, false, false, "")
1033+
require.NoError(t, err)
1034+
require.NotEmpty(t, models, "No models found after packaging")
1035+
1036+
// Verify we can inspect the model by tag
1037+
model, err := env.client.Inspect(targetTag, false)
1038+
require.NoError(t, err, "Failed to inspect packaged model by tag: %s", targetTag)
1039+
require.NotEmpty(t, model.ID, "Model ID should not be empty")
1040+
require.Contains(t, model.Tags, targetTag, "Model should have the expected tag")
1041+
1042+
t.Logf("✓ Successfully packaged and tagged model: %s (ID: %s)", targetTag, model.ID[7:19])
1043+
1044+
// Cleanup
1045+
err = removeModel(env.client, model.ID, true)
1046+
require.NoError(t, err, "Failed to remove model")
1047+
})
1048+
1049+
// Test case 2: Package with context size override
1050+
t.Run("package GGUF with context size", func(t *testing.T) {
1051+
targetTag := "ai/packaged-ctx:latest"
1052+
1053+
// Create package options with context size
1054+
opts := packageOptions{
1055+
ggufPath: absPath,
1056+
tag: targetTag,
1057+
contextSize: 4096,
1058+
}
1059+
1060+
// Create a command for context
1061+
cmd := newPackagedCmd()
1062+
// Set the flag as changed for context size
1063+
cmd.Flags().Set("context-size", "4096")
1064+
1065+
// Execute the package command using the helper function with test client
1066+
t.Logf("Packaging GGUF file with context size 4096 as %s", targetTag)
1067+
err := packageModel(env.ctx, cmd, env.client, opts)
1068+
require.NoError(t, err, "Failed to package GGUF model with context size")
1069+
1070+
// Verify the model was loaded and tagged
1071+
model, err := env.client.Inspect(targetTag, false)
1072+
require.NoError(t, err, "Failed to inspect packaged model")
1073+
require.Contains(t, model.Tags, targetTag, "Model should have the expected tag")
1074+
1075+
t.Logf("✓ Successfully packaged model with context size: %s", targetTag)
1076+
1077+
// Cleanup
1078+
err = removeModel(env.client, model.ID, true)
1079+
require.NoError(t, err, "Failed to remove model")
1080+
})
1081+
1082+
// Test case 3: Package with different org
1083+
t.Run("package GGUF with custom org", func(t *testing.T) {
1084+
targetTag := "myorg/packaged-test:v1"
1085+
1086+
// Create package options
1087+
opts := packageOptions{
1088+
ggufPath: absPath,
1089+
tag: targetTag,
1090+
}
1091+
1092+
// Create a command for context
1093+
cmd := newPackagedCmd()
1094+
1095+
// Execute the package command using the helper function with test client
1096+
t.Logf("Packaging GGUF file as %s", targetTag)
1097+
err := packageModel(env.ctx, cmd, env.client, opts)
1098+
require.NoError(t, err, "Failed to package GGUF model with custom org")
1099+
1100+
// Verify the model was loaded and tagged
1101+
model, err := env.client.Inspect(targetTag, false)
1102+
require.NoError(t, err, "Failed to inspect packaged model")
1103+
require.Contains(t, model.Tags, targetTag, "Model should have the expected tag")
1104+
1105+
t.Logf("✓ Successfully packaged model with custom org: %s", targetTag)
1106+
1107+
// Cleanup
1108+
err = removeModel(env.client, model.ID, true)
1109+
require.NoError(t, err, "Failed to remove model")
1110+
})
1111+
1112+
// Verify all models are cleaned up
1113+
models, err = listModels(false, env.client, true, false, "")
1114+
require.NoError(t, err)
1115+
require.Empty(t, strings.TrimSpace(models), "All models should be removed after cleanup")
1116+
}
1117+
10191118
func int32ptr(n int32) *int32 {
10201119
return &n
10211120
}

cmd/cli/commands/list.go

Lines changed: 28 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -72,27 +72,39 @@ func listModels(openai bool, desktopClient *desktop.Client, quiet bool, jsonForm
7272
}
7373

7474
if modelFilter != "" {
75-
// Normalize the filter to match stored model names (backend normalizes when storing)
76-
normalizedFilter := dmrm.NormalizeModelName(modelFilter)
75+
// If filter doesn't contain '/', prepend default namespace 'ai/'
76+
if !strings.Contains(modelFilter, "/") {
77+
modelFilter = "ai/" + modelFilter
78+
}
79+
7780
var filteredModels []dmrm.Model
81+
82+
// Check if filter has a colon (i.e., includes a tag)
83+
hasColon := strings.Contains(modelFilter, ":")
84+
7885
for _, m := range models {
79-
hasMatchingTag := false
86+
var matchingTags []string
8087
for _, tag := range m.Tags {
81-
// Tags are stored in normalized format by the backend
82-
if tag == normalizedFilter {
83-
hasMatchingTag = true
84-
break
85-
}
86-
// Also check without the tag part
87-
modelName, _, _ := strings.Cut(tag, ":")
88-
filterName, _, _ := strings.Cut(normalizedFilter, ":")
89-
if modelName == filterName {
90-
hasMatchingTag = true
91-
break
88+
if hasColon {
89+
// Filter includes a tag part - do exact match
90+
// Tags are stored in normalized format by the backend
91+
if tag == modelFilter {
92+
matchingTags = append(matchingTags, tag)
93+
}
94+
} else {
95+
// Filter has no colon - match repository name only (part before ':')
96+
repository, _, _ := strings.Cut(tag, ":")
97+
if repository == modelFilter {
98+
matchingTags = append(matchingTags, tag)
99+
}
92100
}
93101
}
94-
if hasMatchingTag {
95-
filteredModels = append(filteredModels, m)
102+
// Only include the model if at least one tag matched, and only include matching tags
103+
if len(matchingTags) > 0 {
104+
// Create a copy of the model with only the matching tags
105+
filteredModel := m
106+
filteredModel.Tags = matchingTags
107+
filteredModels = append(filteredModels, filteredModel)
96108
}
97109
}
98110
models = filteredModels

cmd/cli/commands/package.go

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -131,7 +131,7 @@ func newPackagedCmd() *cobra.Command {
131131
},
132132
RunE: func(cmd *cobra.Command, args []string) error {
133133
opts.tag = args[0]
134-
if err := packageModel(cmd, opts); err != nil {
134+
if err := packageModel(cmd.Context(), cmd, desktopClient, opts); err != nil {
135135
cmd.PrintErrln("Failed to package model")
136136
return fmt.Errorf("package model: %w", err)
137137
}
@@ -254,7 +254,7 @@ func initializeBuilder(cmd *cobra.Command, opts packageOptions) (*builderInitRes
254254
return result, nil
255255
}
256256

257-
func packageModel(cmd *cobra.Command, opts packageOptions) error {
257+
func packageModel(ctx context.Context, cmd *cobra.Command, client *desktop.Client, opts packageOptions) error {
258258
var (
259259
target builder.Target
260260
err error
@@ -264,7 +264,7 @@ func packageModel(cmd *cobra.Command, opts packageOptions) error {
264264
registry.WithUserAgent("docker-model-cli/" + desktop.Version),
265265
).NewTarget(opts.tag)
266266
} else {
267-
target, err = newModelRunnerTarget(desktopClient, opts.tag)
267+
target, err = newModelRunnerTarget(client, opts.tag)
268268
}
269269
if err != nil {
270270
return err
@@ -357,7 +357,7 @@ func packageModel(cmd *cobra.Command, opts packageOptions) error {
357357
done := make(chan error, 1)
358358
go func() {
359359
defer pw.Close()
360-
done <- pkg.Build(cmd.Context(), target, pw)
360+
done <- pkg.Build(ctx, target, pw)
361361
}()
362362

363363
scanner := bufio.NewScanner(pr)
@@ -443,7 +443,7 @@ func (t *modelRunnerTarget) Write(ctx context.Context, mdl types.ModelArtifact,
443443
return fmt.Errorf("get model ID: %w", err)
444444
}
445445
if t.tag.String() != "" {
446-
if err := desktopClient.Tag(id, parseRepo(t.tag), t.tag.TagStr()); err != nil {
446+
if err := t.client.Tag(id, parseRepo(t.tag), t.tag.TagStr()); err != nil {
447447
return fmt.Errorf("tag model: %w", err)
448448
}
449449
}

0 commit comments

Comments
 (0)