Add pkg/model Resolver API for build encapsulation#2663
Conversation
bbb432c to
0c88da8
Compare
There was a problem hiding this comment.
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.
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.
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.
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