feat(pack): add base digest pinning for adapters (best-effort)#1126
feat(pack): add base digest pinning for adapters (best-effort)#1126rishi-jat wants to merge 4 commits intokitops-ml:mainfrom
Conversation
Signed-off-by: Rishi Jat <rishijat098@gmail.com>
There was a problem hiding this comment.
Pull request overview
Adds best-effort base digest pinning when packing adapter-style kits so the resulting Kitfile can record the immutable OCI manifest digest of the referenced base model.
Changes:
- Persist a new
model.baseDigestfield onartifact.Model(Kitfile schema) to store the resolved base manifest digest. - During
kit pack, attempt to resolve the base model reference to a manifest digest (local-first, remote fallback) and write it intoKitfile.Model.BaseDigestwhen available.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pkg/cmd/pack/pack.go | Adds base digest resolution during pack and a helper to resolve local-first with remote fallback. |
| pkg/artifact/kitfile.go | Extends Kitfile Model schema with a baseDigest field. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
amisevsk
left a comment
amisevsk
left a comment
There was a problem hiding this comment.
See comments from copilot
Signed-off-by: Rishi Jat <rishijat098@gmail.com>
Done, have a look. |
There was a problem hiding this comment.
Pull request overview
Adds best-effort base manifest digest pinning when packing adapters, so the adapter Kitfile can record an immutable base identifier for later linkage/reproducibility.
Changes:
- Add
Model.BaseDigestto the Kitfile schema (baseDigestin YAML/JSON). - During
kit pack, attempt to resolvemodel.path(base model ref) to an OCI manifest digest using local-first resolution with remote fallback. - Persist the resolved digest into
kitfile.Model.BaseDigestwhen available.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| pkg/cmd/pack/pack.go | Adds base digest resolution logic during packing and stores it into the Kitfile model metadata. |
| pkg/artifact/kitfile.go | Extends Kitfile model schema with baseDigest field. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
gorkem
left a comment
gorkem
left a comment
There was a problem hiding this comment.
I think we should think a bit more about the logic that would not mislead the user silently
|
@gorkem Addressed all comments. |
amisevsk
left a comment
amisevsk
left a comment
There was a problem hiding this comment.
Not sure I see the benefit of this change. All it effectively does is add a warning log during pack time.
| output.Logf(output.LogLevelWarn, "overriding user-specified baseDigest %s with resolved %s", | ||
| kitfile.Model.BaseDigest, baseDigest) | ||
| } | ||
| kitfile.Model.BaseDigest = baseDigest |
There was a problem hiding this comment.
What's the point of being able to specify a base digest if it just gets overridden, pushing the digest mismatch into a single warning log line at build time.
Imagine running this build in a CI -- will you notice that the digest has changed? If not, what was the point in including it?
| // Expected cases: no base digest resolved locally (skip pinning, no remote fallback) | ||
| if errors.Is(err, util.ErrNoKitfile) || errors.Is(err, util.ErrNotAModelKit) { | ||
| return "", nil | ||
| } | ||
| if errors.Is(err, errdef.ErrNotFound) || errors.Is(err, errdef.ErrMissingReference) { | ||
| // Reference not found locally, try remote | ||
| } else { | ||
| // Unexpected local error (IO issues, corruption, etc.) | ||
| return "", fmt.Errorf("failed to resolve base digest locally: %w", err) | ||
| } |
There was a problem hiding this comment.
The comment here contradicts the code that follows.
Description
Adds base digest pinning for adapters during
kit pack.Resolves the base model reference to its OCI manifest digest and stores it in
Model.BaseDigest.Uses local-first resolution with remote fallback.
This is best-effort only — pack never fails if resolution is not possible.
Partially addresses #899