AIR CLI Integration: Scaffold experimental AIR CLI command package#5564
AIR CLI Integration: Scaffold experimental AIR CLI command package#5564riddhibhagwat-db wants to merge 2 commits into
Conversation
subprocess's text= keyword was added in 3.7; universal_newlines is the exact equivalent and also works on older interpreters. Unblocks running the acceptance suite on a Python 3.6 host. Co-authored-by: Isaac
Add the experimental `air` command group as the Go port surface for the Python `air` CLI. Every subcommand (run, status, list, logs, cancel, register-image) is registered as a stub that returns a not-implemented error; the real implementations land in later milestones. Includes unit tests for the command-tree wiring and the not-implemented stubs, plus an acceptance test exercising the stubs end-to-end. Co-authored-by: Isaac
Waiting for approvalBased on git history, these people are best suited to review:
Eligible reviewers: Suggestions based on git history. See OWNERS for ownership rules. |
riddhibhagwat-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
riddhibhagwat-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
|
Commit: 0474207
22 interesting tests: 15 SKIP, 7 KNOWN
Top 28 slowest tests (at least 2 minutes):
|
simonfaltum
left a comment
simonfaltum
left a comment
There was a problem hiding this comment.
Thanks for the scaffold. The Go code itself is in good shape and the acceptance setup follows the existing patterns (the EnvMatrix override and the committed out.test.toml are exactly right). Requesting changes for three things:
- The
tools/list_embeds.pychange needs to come out, see the inline comment. It's unrelated to this PR and the stated rationale doesn't match the repo. - Package location should follow the established experimental layout (
experimental/air/cmd), see the inline comment. - The description needs fixing: it says 7 stub subcommands but lists and implements 6 (your own Tests section says six). Also drop the
list_embeds.pybullet together with that change.
Two questions:
- Which Python
airCLI source/version were the flags mapped from? Please link it in the description; flag parity isn't verifiable from this repo. - Was a seventh subcommand intentionally dropped, or is "7" a typo?
The remaining inline comments (test assertion, help case, --retry help text, cancel args, test.toml defaults) are all quick fixes.
| out = subprocess.check_output( | ||
| ["git", "grep", "--no-color", "-E", "^" + EMBED, "--", "*.go"], | ||
| text=True, | ||
| universal_newlines=True, |
There was a problem hiding this comment.
This change doesn't belong in this PR and the rationale doesn't hold up. This script isn't part of the acceptance harness, it's invoked by the Taskfile (EMBED_SOURCES var). The repo's Python floor is also way above 3.6: the tools/ scripts declare requires-python = ">=3.12", CI runs 3.13, and tools/check_deadcode.py plus several acceptance/bin/ scripts still use text=True, so this change alone wouldn't buy 3.6 compatibility anyway. universal_newlines is the legacy alias that text replaced in 3.7.
Please drop this file from the PR (one PR = one change). If ./task fails on your machine because of an old python3, the fix is upgrading your local Python.
| package experimental | ||
|
|
||
| import ( | ||
| "github.com/databricks/cli/cmd/experimental/air" |
There was a problem hiding this comment.
The established layout for experimental features is experimental/<name>/cmd: that's where aitools, genie and postgres live, and cmd/experimental/ only owns the dispatcher plus the single-file workspace_open command. AIR is going to grow real implementation code over the next milestones, so please move this package to experimental/air/cmd (imported as aircmd here). Moving later only gets more expensive as milestones stack on top.
One catch to take care of in the same PR: TEST_PACKAGES in Taskfile.yml covers ./cmd/... but not ./experimental/... (only ssh), so add ./experimental/air/... to it, otherwise the unit tests silently stop running after the move. Update the go test path in the description's Tests section too.
| t.Run(name, func(t *testing.T) { | ||
| require.NotNil(t, cmd.RunE, "command should define RunE") | ||
| err := cmd.RunE(cmd, nil) | ||
| assert.ErrorContains(t, err, "not implemented") |
There was a problem hiding this comment.
This passes even if a stub returns the wrong name, i.e. if cancel.go copy-pasted notImplemented("list") this test wouldn't notice. You have the name as the map key, so assert the full message:
assert.EqualError(t, err, fmt.Sprintf("`air %s` is not implemented yet", name))| errcode trace $CLI experimental air cancel 123 | ||
|
|
||
| title "register-image" | ||
| errcode trace $CLI experimental air register-image my-image:latest |
There was a problem hiding this comment.
The PR's stated goal is making the command tree reviewable, so let's pin it in the acceptance output. Add a help case and the tree, short descriptions and flags all land in output.txt, then any future change shows up as a diff:
title "help"
trace $CLI experimental air --help
|
|
||
| cmd.Flags().IntVar(&node, "node", 0, "Fetch logs from this node") | ||
| cmd.Flags().IntVar(&lines, "lines", 10000, "For completed runs, print the last N lines") | ||
| cmd.Flags().IntVar(&retry, "retry", -1, "View logs from a specific retry attempt (default: latest)") |
There was a problem hiding this comment.
pflag appends its own marker for non-zero defaults, so the help renders a double default (checked against a local build):
--retry int View logs from a specific retry attempt (default: latest) (default -1)
Reword to fold the sentinel in, something like "View logs from a specific retry attempt; -1 means latest".
|
|
||
| cmd := &cobra.Command{ | ||
| Use: "cancel [RUN_ID...]", | ||
| Args: cobra.ArbitraryArgs, |
There was a problem hiding this comment.
cobra.ArbitraryArgs accepts air cancel with no IDs and no --all, and also --all combined with explicit IDs. It doesn't matter while this is a stub, but the arg contract is part of what this PR puts up for review. Either add a small Args validator now (return root.InvalidArgsError like the helpers in cmd/root/args.go, so the usage string gets printed) or leave an explicit TODO for the implementation PR.
| @@ -0,0 +1,6 @@ | |||
| Local = true | |||
There was a problem hiding this comment.
Local = true and Cloud = false are already the defaults from the root acceptance/test.toml, and config is inherited (the genie test next door omits them). Keep just the [EnvMatrix] override and its comment.
| "github.com/stretchr/testify/assert" | ||
| ) | ||
|
|
||
| // TestNewRegistersAllSubcommands asserts the `ai` command wires up every |
4 participants
Changes
cmd/experimental/air/package containing an air parent command plus 7 stub subcommands: run, status, list, logs, cancel, register-imageWhy
The AI runtime CLI ships today as a separately installed Python wheel with its own auth, output, and packaging. Folding it into the main Go CLI gives users one databricks install with consistent profiles, authentication, and -o json output, and removes a parallel toolchain to maintain. Landing the package scaffold first lets the individual commands be ported in small, reviewable PRs (status is next) instead of one large drop. Every stub is wired and navigable, so the command tree and registration are reviewable now without functional code.
Tests
test with:
go test ./cmd/experimental/air/...go test ./acceptance -run 'TestAccept/experimental/air'