-
Notifications
You must be signed in to change notification settings - Fork 656
Add pkg/model Resolver API for build encapsulation #2663
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
bbb432c to
0c88da8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds a new pkg/model package that centralizes Cog model build/inspect/pull behavior behind a Resolver API, and migrates CLI commands to use it. It also extends registry inspection to expose image labels (including for multi-platform images) and updates the Docker build interface to return an image ID.
Changes:
- Introduce
pkg/modelwithParsedRef,Source,Image,Model,Factory, andResolver(plus tests). - Migrate CLI commands to build/inspect/pull via the new
model.Resolver. - Plumb image IDs/labels through Docker build and registry inspect to support model metadata extraction.
Reviewed changes
Copilot reviewed 32 out of 32 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/registry/registry_client.go | Adds label extraction (including default-manifest selection for indexes) to support model inspection. |
| pkg/registry/manifest_result.go | Extends manifest result to include labels. |
| pkg/model/source.go | Adds Source abstraction for project dir + parsed config. |
| pkg/model/source_test.go | Tests Source constructors. |
| pkg/model/ref.go | Adds validated/parsed image reference type and parsing options. |
| pkg/model/ref_test.go | Tests reference parsing behavior. |
| pkg/model/ref_types.go | Adds declarative Ref types and Resolver.Resolve dispatcher. |
| pkg/model/ref_types_test.go | Tests Ref resolution behavior across tag/local/remote/build refs. |
| pkg/model/image.go | Adds Image with Cog label accessors and parsing helpers. |
| pkg/model/image_test.go | Tests image label accessors and parsing/to-model behavior. |
| pkg/model/model.go | Adds Model wrapper with convenience methods (GPU/fast/schema/image ref). |
| pkg/model/model_test.go | Tests Model convenience methods. |
| pkg/model/options.go | Adds build/base-build option structs and defaulting. |
| pkg/model/options_test.go | Tests build option defaulting behavior. |
| pkg/model/errors.go | Adds sentinel errors for “not found” and “not a Cog model”. |
| pkg/model/errors_test.go | Tests sentinel error wrapping semantics. |
| pkg/model/factory.go | Adds build backend interface + Dockerfile-backed default factory. |
| pkg/model/factory_test.go | Tests factory wiring and interface compliance. |
| pkg/model/resolver.go | Adds Resolver orchestration for local/remote inspect, pull, build, build-base. |
| pkg/model/resolver_test.go | Comprehensive tests for resolver behavior and fallback rules. |
| pkg/image/build.go | Changes build functions to return image IDs and propagates through label-adding step. |
| pkg/docker/command/command.go | Updates ImageBuild interface to return an image ID. |
| pkg/docker/docker.go | Implements ImageBuild returning the build-produced image digest. |
| pkg/docker/dockertest/mock_command.go | Updates mock to match new ImageBuild signature. |
| pkg/docker/dockertest/command_mocks.go | Updates generated mocks for new ImageBuild signature/return values. |
| pkg/cli/build.go | Migrates build command to model.Resolver and factors flag→BuildOptions helpers. |
| pkg/cli/push.go | Migrates push workflow to model.Resolver build output. |
| pkg/cli/run.go | Migrates run build/base-build selection to model.Resolver. |
| pkg/cli/predict.go | Migrates predict build/pull/inspect flow to model.Resolver. |
| pkg/cli/train.go | Migrates train build/pull/inspect flow to model.Resolver. |
| pkg/cli/serve.go | Migrates serve base-build flow to model.Resolver. |
| pkg/cli/baseimage.go | Adapts to new ImageBuild return signature. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 32 out of 32 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 32 out of 32 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
mfainberg-cf
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A lot going on here. This seems generally ok as long as the resulting images make sense.
|
Yep! Images should have no change, this is just a wrapper to abstract away the underlying docker/registry calls. The declarative weights work and new OCI format will be much simpler now! |
Introduces the model package as the foundation for the Build API Encapsulation epic (cog-fuo). This first phase adds: - ParsedRef: wrapper around go-containerregistry/pkg/name.Reference - ParseRef(): validates and parses image references with options - ParseOption functions: Insecure(), WithDefaultRegistry(), WithDefaultTag() - Methods: Registry(), Repository(), Tag(), Digest(), IsTag(), IsDigest(), IsReplicate() The lazy method design queries the underlying reference on-demand, ensuring IsReplicate() always reflects the current registry host setting.
Source wraps config + projectDir for build inputs. BuildOptions consolidates all build flags with WithDefaults().
Factory interface enables pluggable build backends. DockerfileFactory wraps existing image.Build() function.
- Resolver orchestrates building and loading Models - Load() with functional options: LocalOnly, RemoteOnly, PreferRemote, WithPlatform - LoadByID() for stable image ID-based lookups (short or full SHA) - Build() delegates to Factory and inspects result - Default PreferLocal behavior with smart fallback (only on not-found errors) - Comprehensive tests with mocked docker/registry
Implements the Ref pattern for declarative model resolution: - Ref interface with internal resolve() method - TagRef/FromTag: resolves via Load (local-first, fallback to remote) - LocalRef/FromLocal: resolves from local docker daemon only - RemoteRef/FromRemote: resolves from remote registry only - BuildRef/FromBuild: resolves by building from Source - Resolver.Resolve(ctx, ref) dispatches to appropriate resolution
- ImageBuild now returns (string, error) with manifest digest - image.Build() returns the final image ID from label-adding step - DockerfileFactory.Build() populates Image.Digest - Resolver.Build() uses digest for inspection, falls back to tag This fixes instability where tag resolution could pull from remote registry instead of using the locally-built image.
Refactor build.go to use the new model.Resolver.Build() API instead of directly calling image.Build() with 19 parameters. This encapsulates build complexity behind the Resolver abstraction. Changes: - Replace config.GetConfig() with model.NewSource() - Replace image.Build() call with resolver.Build() using BuildOptions - Use m.ImageRef() for output message instead of input imageName - Add defensive nil checks for src.Config.Build
…dels - Rename Load() to Inspect() for clearer semantics (metadata only, no pull) - Add Pull() method that ensures image is locally available - Add sentinel errors: ErrNotCogModel, ErrNotFound - Validate Cog model labels in both local and remote inspection - Update registry.Inspect() to fetch config blob for labels - Support multi-platform indexes by picking default image for labels - Migrate CLI commands (predict, train) to use resolver.Pull() - Add BuildBase support for dev mode base image builds
- Add Image.ParsedConfig() - parses cog config JSON from labels - Add Image.ParsedOpenAPISchema() - parses OpenAPI schema from labels - Add Image.ToModel() - converts Image to Model by parsing labels - Refactor resolver to use new methods, eliminating duplicate parsing code - Fix: schema parsing errors now propagate instead of being swallowed
- Remove redundant inspectLocal wrapper in Pull() - Drop unused Model fields (Weights, Runtime, GitCommit, GitTag) - Fix pickDefaultImage to log errors instead of silently swallowing - Deprecate Fast field in BuildOptions (read from config at build time) - Fix test mocks to return errors instead of panic
- Pass context to pickDefaultImage for cancellation support - Add name.Insecure option to NewDigest for HTTP registry compatibility - Validate ImageBuild returns non-empty image ID from buildkit - Fix comment: org.cogmodel.config -> run.cog.config - Add nil validation to Resolver.Build/BuildBase methods
- loadLocal/loadRemote now return accurate error messages: - "not found" only for actual not-found errors - "failed to inspect" for auth, network, permission errors - pickDefaultImage returns (v1.Image, error) instead of silently returning nil on failures - Multi-platform index label fetch now propagates errors - Updated tests to verify new error message format
b0b5159 to
2cbf6c8
Compare
Summary
This PR introduces a new
pkg/modelpackage that provides a unified API for building, inspecting, and pulling Cog models. This encapsulates the build process behind a clean interface.Key Changes
New
pkg/modelpackage with:ParsedReffor validated image reference parsingImagestruct with Cog label accessors and convenience methodsModelstruct representing a validated Cog model with config and schemaSourcestruct for model source (directory + config)BuildOptionsfor build configurationFactoryinterface for pluggable build backendsResolverfor orchestrating builds and image loadingAPI:
resolver.Build(ctx, source, opts)- Build a model from sourceresolver.Inspect(ctx, ref)- Inspect a local or remote imageresolver.InspectByID(ctx, id)- Inspect by image IDresolver.Pull(ctx, ref)- Ensure image is locally availableSentinel errors
ErrNotCogModelandErrNotFoundfor proper error handlingCLI migration - All CLI commands migrated to use the new Resolver:
build.gopush.gorun.gopredict.gotrain.goserve.goTesting