Skip to content

refactor: normalize ModelPack config to Docker format in API responses#875

Open
ilopezluna wants to merge 1 commit intomainfrom
fix/cncf-model-ls-empty-columns
Open

refactor: normalize ModelPack config to Docker format in API responses#875
ilopezluna wants to merge 1 commit intomainfrom
fix/cncf-model-ls-empty-columns

Conversation

@ilopezluna
Copy link
Copy Markdown
Contributor

Populate empty columns and created date for CNCF models in model ls

Models packaged with --format cncf showed empty PARAMETERS, QUANTIZATION, ARCHITECTURE, and SIZE columns, plus '56 years ago' for CREATED in docker model ls.

This PR fixes that.

Copy link
Copy Markdown
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've found 1 issue, and left some high level feedback:

  • normalizeConfig only special-cases configs whose concrete type is *modelpack.Model, but in real usage the config is likely a *modelpack.ModelConfig; consider detecting ModelPack configs via the same mechanism as Descriptor (e.g., IsModelPackConfig or type-asserting to the config type) so CNCF configs are reliably normalized outside of the tests.
  • Descriptor now uses modelpack.IsModelPackConfig while normalizeConfig relies on a concrete type-assertion; aligning these code paths (e.g., by sharing a helper or using the same detection strategy) would reduce the risk of config-format drift and subtle inconsistencies between descriptor and config normalization.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- normalizeConfig only special-cases configs whose concrete type is *modelpack.Model, but in real usage the config is likely a *modelpack.ModelConfig; consider detecting ModelPack configs via the same mechanism as Descriptor (e.g., IsModelPackConfig or type-asserting to the config type) so CNCF configs are reliably normalized outside of the tests.
- Descriptor now uses modelpack.IsModelPackConfig while normalizeConfig relies on a concrete type-assertion; aligning these code paths (e.g., by sharing a helper or using the same detection strategy) would reduce the risk of config-format drift and subtle inconsistencies between descriptor and config normalization.

## Individual Comments

### Comment 1
<location path="pkg/distribution/internal/partial/partial.go" line_range="75" />
<code_context>
+		if err := json.Unmarshal(raw, &mp); err != nil {
+			return types.Descriptor{}, fmt.Errorf("unmarshal modelpack config: %w", err)
+		}
+		return types.Descriptor{Created: mp.Descriptor.CreatedAt}, nil
+	}
+
</code_context>
<issue_to_address>
**question (bug_risk):** Only propagating the Created timestamp for ModelPack descriptors may drop other descriptor metadata.

For ModelPack configs this returns a descriptor with only `Created` set; all other fields stay at their zero values. Callers of `Descriptor` may expect metadata like media type, digest, and size (as they get from the Docker path via `cf.Descriptor`). If ModelPack can provide any of these fields, consider mapping them to avoid returning a partially populated descriptor for this format.
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment thread pkg/distribution/internal/partial/partial.go
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces support for the CNCF ModelPack format alongside the existing Docker format. Key changes include updating the Descriptor function to handle both formats and adding a normalizeConfig utility to ensure consistent API responses by converting ModelPack configurations to the standard Docker format. Feedback was provided regarding the normalizeConfig function, specifically suggesting the inclusion of the ContextSize field to ensure the normalized object fully represents the source interface and avoids potential data loss.

Comment on lines +18 to +24
return &types.Config{
Format: cfg.GetFormat(),
Parameters: cfg.GetParameters(),
Quantization: cfg.GetQuantization(),
Architecture: cfg.GetArchitecture(),
Size: cfg.GetSize(),
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

The normalizeConfig function omits the ContextSize field when converting from modelpack.Model to types.Config. While the current ModelPack implementation returns nil for context size, it is better to include it for completeness and to ensure the normalized types.Config fully represents the ModelConfig interface. This also makes the normalization logic more robust if ModelPack adds support for this field in the future.

return &types.Config{
			Format:       cfg.GetFormat(),
			Parameters:   cfg.GetParameters(),
			Quantization: cfg.GetQuantization(),
			Architecture: cfg.GetArchitecture(),
			Size:         cfg.GetSize(),
			ContextSize:  cfg.GetContextSize(),
		}
References
  1. Pragmatism and correctness: ensure the normalized object fully represents the source interface to avoid silent data loss or inconsistent API responses. (link)

@ilopezluna ilopezluna requested a review from a team April 20, 2026 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant