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.
|
@amisevsk sorry for late - would look into the review asap and do the suggested changes.Thanks! |
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