Repository navigation
Add air images push for pushing images to DAR - #6800
maggiewang-db wants to merge 6 commits into
Conversation
c7733c1 to
a69358f
Compare
44b1724 to
31019cb
Compare
31019cb to
53678be
Compare
| cancel Cancel one or more runs | ||
| convert-to-dabs Convert an AIR run YAML into a Databricks Asset Bundle | ||
| get Show status, configuration, and timing details for a specified run | ||
| image Manage container images for AI Runtime |
There was a problem hiding this comment.
couple questions here:
- is the plan to support this in air long term or is this a temporary solution? if the latter, do you know what the process for deprecating or if we should mark this as experimental somehow? I think we should figure that out before merging
- do you think we will extend image resource to additional commands, like
air image listto see list of available images or is that out of the scope for air? for consistency with databricks cli convention andpools, I think this should be pluralimagesthough I can see why image is slightly preferable for this specific setup command
There was a problem hiding this comment.
- I think we should treat it as long term. This command is only designed for AIR users. There is a chance that CPU users will find it useful too, and at that time it might become a native command under
databricks. But it's not on the roadmap right now. - @yolocs 's team is building CRUD commands for UC images in Databricks CLI. I think we should ask users to use those commands.
There was a problem hiding this comment.
got it, I think I still prefer using plural images for consistency with air commands (even though docker uses singular). I don't think it's that weird since we do have commands like air pools get, and another option for the naming would be publish. setup can sound a bit like a one-time configuration but publish shows the outcome (air images publish)
There was a problem hiding this comment.
Changed to databricks air images push.
| cmd.Flags().StringVarP(&opts.source, "source", "s", "", "Source container image to push") | ||
| cmd.Flags().StringVar(&opts.catalog, "catalog", "", "Unity Catalog catalog for the image; use lowercase letters, digits, and underscores") | ||
| cmd.Flags().StringVar(&opts.schema, "schema", "", "Unity Catalog schema for the image; use lowercase letters, digits, and underscores") | ||
| cmd.Flags().StringVar(&opts.image, "image", "", "Artifact name and optional tag in Databricks Artifact Registry; artifact names must be 255 characters or less and tags must be 128 characters or less") |
There was a problem hiding this comment.
--image feels a little unclear alongside --source: it does not indicate that this is the destination value, and “image” is already used by the command group. Something like --dest-image, --target-image, or --artifact (though I am not sure that is the correct terminology when the value includes an optional tag) might distinguish it more clearly. Whichever name we use, the help should make it explicit that this is the destination ARTIFACT[:TAG] and that it defaults to the source image name and tag.
There was a problem hiding this comment.
Changed to --artifact ARTIFACT[:TAG] and clarified the defaults.
| return fmt.Errorf("configure Docker authentication: %w", err) | ||
| } | ||
|
|
||
| target := fmt.Sprintf("%s/%s.%s.%s:%s", registryHost, resolved.catalog, resolved.schema, resolved.artifact, resolved.tag) |
There was a problem hiding this comment.
Can we print one concise summary before the Docker operations, for example: Publishing nvidia/cuda:13.4.1 as research.nvidia.cuda:13.4.1 using profile prod? It would also be helpful to say whether the command is using the local source image or pulling it. The local-first behavior makes sense, but that choice is currently silent.
There was a problem hiding this comment.
Done. Added the summary and local/pull messages.
| runCommand func(context.Context, io.Reader, io.Writer, io.Writer, string, ...string) error, | ||
| dockerPath, source string, | ||
| ) bool { | ||
| return runCommand(ctx, nil, io.Discard, io.Discard, dockerPath, "image", "inspect", source) == nil |
There was a problem hiding this comment.
Every docker image inspect error currently becomes “image missing,” even when it failed for another reason such as the Docker daemon being unavailable. Can we check that the daemon is available immediately before this phase, or otherwise distinguish an operational failure from a missing image before deciding to pull?
There was a problem hiding this comment.
Done. Docker failures now stop the command instead of triggering a pull.
| target := fmt.Sprintf("%s/%s.%s.%s:%s", registryHost, resolved.catalog, resolved.schema, resolved.artifact, resolved.tag) | ||
| if opts.pull || !imageExistsLocally(ctx, deps.runCommand, dockerPath, resolved.source) { | ||
| cmdio.LogProgress(ctx, "Pulling source image "+resolved.source) | ||
| if err := deps.runCommand(ctx, cmd.InOrStdin(), cmd.OutOrStdout(), cmd.ErrOrStderr(), dockerPath, "pull", resolved.source); err != nil { |
There was a problem hiding this comment.
This pulls the host’s default architecture, which may differ from the AIR workload platform. Is relying on the developer machine’s platform intentional, or could it result in publishing an incompatible image? The command does not currently expose a platform option. One approach would be to pass Docker’s --platform option using the supported workload platform as the default, and validate an existing local image against that platform.
There was a problem hiding this comment.
Done. Pulls use linux/amd64; local and pulled images are validated.
| if err != nil { | ||
| return resolvedImageSetupOptions{}, err | ||
| } | ||
| if !catalogSchemaPattern.MatchString(catalog) { |
There was a problem hiding this comment.
This validation rejects hyphenated catalog and schema names such as my-catalog, even though they are valid under the Docker repository grammar used by DAR. As written, users with an existing valid destination cannot publish to it through this command. The validation should use the actual DAR repository-component rules.
There was a problem hiding this comment.
Done. Catalog/schema validation now accepts valid hyphenated names.
| cmd.Flags().StringVar(&opts.catalog, "catalog", "", "Unity Catalog catalog for the image; use lowercase letters, digits, and underscores") | ||
| cmd.Flags().StringVar(&opts.schema, "schema", "", "Unity Catalog schema for the image; use lowercase letters, digits, and underscores") | ||
| cmd.Flags().StringVar(&opts.image, "image", "", "Artifact name and optional tag in Databricks Artifact Registry; artifact names must be 255 characters or less and tags must be 128 characters or less") | ||
| cmd.Flags().StringVar(&opts.region, "region", "", "Workspace region; detected automatically when omitted") |
There was a problem hiding this comment.
I think we can remove --region now that auth docker configure infers the registry region automatically and has deprecated the flag. Removing the flag, lookup, and fallback prompt would make the user experience simpler because region is one of the harder implementation details for users to discover and enter correctly.
There was a problem hiding this comment.
Removed --region; region detection is automatic.
|
|
||
| unityCatalogReference := fmt.Sprintf("%s.%s.%s:%s", resolved.catalog, resolved.schema, resolved.artifact, resolved.tag) | ||
| cmdio.LogString(ctx, "Pushed image.") | ||
| _, err = fmt.Fprintf( |
There was a problem hiding this comment.
This command inherits the global -o json option, but the success path always writes plain text and forwards Docker output to stdout. That means -o json does not produce parseable JSON. It should either return a structured result containing the Unity Catalog image reference or reject JSON output explicitly for this command.
There was a problem hiding this comment.
Done. -o json returns image references; Docker output goes to stderr.
air image setup for pushing images to DARair images push for pushing images to DAR
|
An authorized user can trigger integration tests manually by following the instructions below: Trigger: Inputs:
Checks will be approved automatically on success. |
| tag = after | ||
| } | ||
|
|
||
| if len(artifact) > maxArtifactNameLength { |
There was a problem hiding this comment.
Can we validate the full catalog.schema.artifact length instead? Docker’s 255-character limit applies to the repository name, so a 255-character artifact will fail later during docker tag.
There was a problem hiding this comment.
Done. The full catalog.schema.artifact is limited to 255 characters in #6869.
caroline-db
left a comment
There was a problem hiding this comment.
left one more comment on checking for character length but otherwise LGTM if tested/working. btw I think you will need to recreate this PR from a direct branch of the repo rather than from a fork in order for the CI tests to authenticate and run.
|
Recreated as #6869 from a branch in databricks/cli so CI can authenticate. |
## Changes Add the preview `databricks air images push` command to configure Docker authentication and push an image into a Unity Catalog catalog and schema. It supports prompts or flags, automatic region detection, and text or JSON results. Images are pulled for `linux/amd64` when needed and validated before upload. ```sh databricks air images push -p prod \ -s nvidia/cuda:13.4.1-cudnn-devel-ubuntu26.04 \ --catalog research --schema nvidia \ --artifact cuda:13.4.1-cudnn-devel-ubuntu26.04 ``` Validate DAR name/tag rules and the combined 255-character `catalog.schema.artifact` limit before Docker operations. The workspace must enable the required Preview features. ## Why Combine Docker authentication, image tagging, and upload so AIR users can prepare a custom image for a workload in one command. Replaces databricks#6800 with a branch in `databricks/cli` so CI can authenticate. The original discussion and review history remain on databricks#6800. ## Tests - `./task fmt`, `./task checks`, and `./task lint`. - `go test ./cmd/air` and `./task test` (full acceptance suite: 5,428 tests, 6 skipped). - Coverage includes region inference, local and pulled images, platform and Docker failures, clean JSON output, valid name separators, and the 255/256-character repository boundary. Earlier cloud validation, recorded before the command rename, successfully pushed an image and ran a GPU workload. Cloud validation was not rerun for these review fixes. _Command naming, review fixes, tests, and PR description updated with Codex._
Changes
Add the preview
databricks air images pushcommand to configure Docker authentication and push a container image to Databricks Artifact Registry under a Unity Catalog catalog and schema. It supports interactive prompts or flags, infers the registry region, and returns theenvironment.unity_catalog_imagevalue as text or JSON. It reuses locallinux/amd64images or pulls for that platform, and stops on Docker failures or incompatible images.Run
databricks air images pushwithout image flags to answer the prompts interactively. The workspace must enable the required Preview features.Why
Combine Docker authentication, image tagging, and upload so AIR users can prepare a custom image for a workload in one command.
Tests
./task fmt,./task checks, and./task lint../task test: all unit modules and the full acceptance suite (5,428 tests, 6 skipped).Validation used Go 1.26.8, Python 3.12 on PATH, jq 1.8.1, Ruff 0.15.17, and a clean temporary directory outside
/tmp(which contains an unrelated.gitdirectory in this environment).The acceptance test covers region inference, hyphenated destinations, local and pulled images, platform validation, Docker failures, removed flags, and clean JSON output with Docker progress on stderr. Earlier cloud validation, before the command rename, recorded a successful image push to
dar_uc.maggiewang.air_image_e2e:pr6800-20260922and a successful GPU workload with outputcustom image e2e: cuda=True devices=1.Command naming, review fixes, tests, and PR description updated with Codex.