refactor: normalize ModelPack config to Docker format in API responses#875
refactor: normalize ModelPack config to Docker format in API responses#875ilopezluna wants to merge 1 commit intomainfrom
Conversation
There was a problem hiding this comment.
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>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
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.
| return &types.Config{ | ||
| Format: cfg.GetFormat(), | ||
| Parameters: cfg.GetParameters(), | ||
| Quantization: cfg.GetQuantization(), | ||
| Architecture: cfg.GetArchitecture(), | ||
| Size: cfg.GetSize(), | ||
| } |
There was a problem hiding this comment.
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
- Pragmatism and correctness: ensure the normalized object fully represents the source interface to avoid silent data loss or inconsistent API responses. (link)
Populate empty columns and created date for CNCF models in
model lsModels packaged with
--format cncfshowed empty PARAMETERS, QUANTIZATION, ARCHITECTURE, and SIZE columns, plus '56 years ago' for CREATED indocker model ls.This PR fixes that.