Support packaging CNCF models

[ilopezluna]

This pull request adds support for building model artifacts in the CNCF ModelPack format in addition to the existing Docker format. It introduces a new --format CLI option, updates the packaging logic, and improves error handling for context size settings. Documentation and test cases are also updated to reflect the new functionality.

Packaging format support:

• Added a --format flag to the model package CLI command, allowing users to select between "docker" (default) and "cncf" (CNCF ModelPack spec) output artifact formats. The flag is validated, and the builder logic is updated to handle both formats. (cmd/cli/commands/package.go [1] [2] [3] [4] [5]
• The builder now supports a WithFormat option and a new BuildFormat type, enabling the creation of artifacts in either Docker or CNCF format. (pkg/distribution/builder/builder.go [1] [2] [3]

Test and documentation updates:

• Integration tests for the packaging command are updated to use the new format option. (cmd/cli/commands/integration_test.go [1] [2] [3]
• CLI documentation and markdown reference files are updated to describe the new --format option. (cmd/cli/docs/reference/docker_model_package.yaml [1] cmd/cli/docs/reference/model_package.md [2]

Error handling improvements:

• Setting context size now properly checks for errors and fails the operation if an invalid value is provided. (cmd/cli/commands/integration_test.go [1] cmd/cli/commands/package.go [2]

Dependency updates:

• Added github.com/modelpack/model-spec v0.0.7 to dependencies for CNCF ModelPack support. (go.mod go.modR25)

[gemini-code-assist]

Code Review

This pull request introduces support for the CNCF ModelPack artifact format, alongside the existing Docker format. Key changes include adding a --format flag to the package command, updating the builder logic to handle format conversions and layer media type remapping, and introducing a new CNCFModel type. The WithContextSize method now returns an error for CNCF format, as it's not supported by the spec. Review feedback highlights two critical issues: one regarding incomplete layer descriptors in remappedLayer.GetDescriptor that could break registry operations, and another about ClassifyLayer misclassifying existing CNCF layers if they lack filepath annotations, leading to incorrect artifact generation.

[ilopezluna]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces support for packaging models in the CNCF ModelPack format via a new --format flag in the package command. This involves significant changes to the model builder, including media type remapping and configuration conversion. However, several critical issues have been identified: the default layer classification in modelpack/convert.go is too broad and risks misclassifying weight layers as configuration. Additionally, the use of type aliases from github.com/modelpack/model-spec/specs-go/v1 (v0.0.7) introduces incorrect JSON tags, leading to non-compliant CNCF model artifacts. Finally, the DiffID fallback logic for remote layers in oci/remote.go is problematic for compressed layers, as it can return a compressed digest instead of the required uncompressed one, posing data integrity risks.

[sourcery-ai]

Hey - I've found 1 issue

Prompt for AI Agents

Please address the comments from this code review:

## Individual Comments

### Comment 1
<location path="pkg/distribution/modelpack/convert.go" line_range="118-120" />
<code_context>
+	desc types.Descriptor,
+	diffIDs []digest.Digest,
+) Model {
+	now := time.Now()
+	createdAt := desc.Created
+	if createdAt == nil {
+		createdAt = &now
+	}
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Defaulting createdAt to time.Now() in DockerConfigToModelPack can make conversions non-deterministic

By setting `createdAt` to `time.Now()` when `desc.Created` is nil, this function introduces non-deterministic data into the resulting config, which in turn makes digests unstable between runs. Since some callers (like `FromDirectory`) already provide a deterministic `Created` when needed, it would be better to leave `createdAt` nil if unset and let callers explicitly choose when to inject a timestamp, preserving deterministic behavior by default and avoiding accidental reproducibility regressions.

Suggested implementation:

```golang
func DockerConfigToModelPack(
	cfg types.Config,
	desc types.Descriptor,
	diffIDs []digest.Digest,
) Model {
	// Preserve determinism by propagating desc.Created directly.
	// Callers that require a concrete timestamp should set desc.Created explicitly.
	createdAt := desc.Created

```

1. If `time` is only imported to support the removed `time.Now()` usage, remove the unused `time` import from the import block of `pkg/distribution/modelpack/convert.go`.
2. Ensure that wherever `createdAt` is used later in this function (likely when constructing the `Model` struct), it still accepts `nil` (e.g., the field type should be `*time.Time` or a pointer type that allows `nil`).
3. If any tests assumed a non-nil `Created` after conversion, update them to either:
   - set `desc.Created` explicitly before calling `DockerConfigToModelPack`, or
   - assert the behavior when `Created` is `nil` if that is now the expected default.
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}