[8/?] feat(python-setup): merge serverless-version selection into the compute picker - #2053
Open
rugpanov wants to merge 9 commits into
Open
[8/?] feat(python-setup): merge serverless-version selection into the compute picker#2053rugpanov wants to merge 9 commits into
rugpanov wants to merge 9 commits into
Conversation
Contributor
Author
|
🤖 Integration tests ❌ 1 of 35 test jobs failed for |
rugpanov
force-pushed
the
rugpanov/python-setup-compute-picker
branch
from
July 27, 2026 11:30
81c66cb to
81a2338
Compare
Contributor
Author
|
🤖 Integration tests triggered for |
rugpanov
force-pushed
the
rugpanov/python-setup-compute-picker
branch
from
July 27, 2026 11:45
81a2338 to
52ac5a3
Compare
Contributor
Author
|
🤖 Integration tests ❌ 1 of 35 test jobs failed for |
rugpanov
force-pushed
the
rugpanov/python-setup-compute-picker
branch
from
July 27, 2026 13:30
52ac5a3 to
30adfe6
Compare
Contributor
Author
|
🤖 Integration tests triggered for |
Contributor
Author
|
🤖 Integration tests ✅ all 35 test jobs passed for |
Contributor
Author
|
🤖 Integration tests ✅ all 35 test jobs passed for |
rugpanov
added a commit
that referenced
this pull request
Jul 27, 2026
*Why*
The CLI client (PythonSetupCliClient) shipped with no call site, which makes its
API hard to review in isolation. This adds the orchestrator that actually
drives it end-to-end, so the client's usage lands alongside the client.
*What*
- Add PythonSetupEnvironmentSetup (python-setup/controllers): decide whether to
run, resolve the compute target, invoke cli.run() under a progress indicator,
then on success adopt the provisioned venv interpreter and persist
{envKey, pythonVersion} for drift detection; on failure surface the mapped
getPythonSetupErrorMessage copy; treat PythonSetupCancelledError as a silent
user action.
- Consumes the real client surface: run(invocation{compute}, {cwd, onLog, token}),
isPythonSetupSuccess, getPythonSetupErrorMessage. The client is depended on
structurally (CliRunner) so the flow is unit-testable without spawn/vscode.
- The two not-yet-built pieces are injected seams with explicit TODOs, not hard
dependencies:
* isVisible() -> TODO(#2044): shouldShowPythonSetup(...). Wiring stub
returns false, so the feature is inert end-to-end for now.
* resolveCompute() -> TODO(#2052 / #2053): serverless-version picker. Wiring
stub returns undefined, so a serverless setup is a no-op until then.
Review fixes (Claude + Codex + @rclarey):
- Cancellation is no longer dead code: ProgressTask now exposes a
CancellationLike token alongside the log sink, and setup() threads it into
cli.run({cwd, onLog, token}). The production withProgress passes through the
progress notification's own token, so a user "Cancel" tears down the
project-mutating CLI instead of being a silent no-op.
- Re-entrancy guard: overlapping setup() calls coalesce onto a single in-flight
run (cleared in finally, incl. on rejection) so two setup-local processes
can't race the same cwd's writes/backup.
- Strict real-run success via an isLocalEnvironmentReady() type guard: an ok:true
result must carry venvPath AND target/resolved. Missing any (dry-run shape /
CLI-schema drift) is treated as a failure. Kept separate from
isPythonSetupSuccess (ok===true), which stays true for a --dry-run.
- Interpreter adoption is no longer a silent failure: adoptInterpreter() is now
wrapped so a failure surfaces the error and stays not-ready, instead of
rejecting with no user-facing message and a provisioned-but-unadopted venv.
- Trimmed doc/inline comments to the non-obvious "why"; dropped prose that only
restated self-explanatory names.
*Verification*
yarn --cwd packages/databricks-vscode test:unit (352 passing; 14 orchestrator
cases, incl. success-without-venvPath, success-missing-target/resolved,
token threaded to cli.run, re-entrancy coalescing, in-flight guard cleared on
rejection, and adoption-failure surfaces error + stays not-ready). test:lint and
build (tsc --build) clean.
Co-authored-by: Isaac
misha-db
reviewed
Jul 28, 2026
misha-db
reviewed
Jul 28, 2026
misha-db
approved these changes
Jul 28, 2026
misha-db
approved these changes
Jul 28, 2026
Contributor
Author
|
🤖 Integration tests ❌ 1 of 35 test jobs failed for |
*Why* Merging serverless-version selection into the compute picker means the chosen version must be remembered with the compute selection, so setup never re-prompts for it. This adds the persistence field. *What* - Add optional serverlessVersion to OverrideableConfigState (+ isOverrideableConfigKey) -- the CLI's bare-integer --serverless-version value (e.g. "5"). It routes through the existing ConfigModel get/set overrideable path automatically. Backward compatibility: the field is optional and independent of `serverless`. Existing configs that predate it have `serverless: true` with no version, which stays valid -- a version-less serverless selection falls back to the scored default. Writing an unrelated key never fabricates a version (covered by test). *Verification* yarn --cwd packages/databricks-vscode test:unit (344 passing; 4 new: key guard, round-trip persist, clear-to-undefined, legacy version-less config untouched). Co-authored-by: Isaac
*Why* The compute picker needs to save the chosen serverless version with the serverless selection, and setup needs to read it back, so ConnectionManager -- the source of truth for the attached compute -- must carry it. *What* - enableServerless(version?) persists serverlessVersion when supplied (also when serverless is already on, so re-picking the version alone is saved); a bare enableServerless() keeps working unchanged. - disableServerless() clears the version (it only has meaning while serverless is the selected compute). - updateServerless() loads the persisted version on reload, so it survives a restart without re-prompting. - Add get serverlessVersion(): the persisted value, or undefined when serverless is off or no version was chosen (consumers fall back to the scored default). Backward compatibility: the no-arg enableServerless() path and version-less persisted configs are preserved; undefined simply means "use the default". *Verification* yarn --cwd packages/databricks-vscode build (clean; type-checks the new overrideable-key access and getter). Existing unit suite unaffected. Co-authored-by: Isaac
*Why*
The serverless-version picker ranks observations; without a real source it
degrades to a lone fallback candidate. A project's bundle already declares its
serverless environment version(s), which is the strongest provenance signal, so
we harvest them to seed the picker.
*What*
- collectBundleServerlessVersions(bundle): defensively walks a parsed bundle and
emits every serverless `environment_version` as a {version, source:"bundleYaml"}
observation for scoreServerlessVersions. environment_version is specific to
serverless environment specs and is a bare integer ("5") -- the same form
--serverless-version takes. The walk tolerates unresolved ${var...} string
nodes and schema shape changes without throwing, coerces numeric YAML values
to bare strings, and de-duplicates preserving first-seen order.
*Verification*
yarn --cwd packages/databricks-vscode test:unit (350 passing; 6 new: single find,
multi-resource dedup, numeric coercion, string-node tolerance, empty/version-less,
non-scalar skip).
Co-authored-by: Isaac
*Why* Selecting "Serverless" in the compute picker only recorded a boolean; the serverless environment version was then either an invisible setting (legacy) or would be asked on every setup run. Merging the version choice into the compute picker lets the user confirm it once, at compute-selection time, persisted with the selection so setup never re-prompts. *What* - ConnectionCommands.attachClusterQuickPickCommand: when "Serverless" is chosen, route through a new selectServerless() step. - selectServerless(): with the python-setup feature opted into, rank the serverless versions from the project's bundle (collectBundleServerlessVersions -> scoreServerlessVersions), show the picker (top candidate pre-selected), and persist via enableServerless(version); dismissing the version picker makes no compute change. With the feature off it is the plain, unchanged enableServerless() -- the picker is byte-for-byte today's. Constraint 1 (backward compat): flag-off behaviour is identical to today, and bundle read failures fall back to scoring with no observations rather than blocking compute selection. Constraint 2 (feature flag): the version sub-step is gated on isPythonSetupEnabled() (the PR #2045 opt-in), so only opted-in users see it. *Verification* yarn --cwd packages/databricks-vscode build / test:lint / test:unit (350 passing; build type-checks the validateConfig access and picker wiring). Co-authored-by: Isaac
…e picker *Why* selectServerless reimplemented the collect->score->pick pipeline inline, duplicating resolveServerlessVersion (which had no caller as a result). Now that the resolver no longer owns the feature-flag gate, the compute picker can call it directly. *What* - ConnectionCommands.pickServerlessVersion delegates to resolveServerlessVersion, supplying only the evidence source (bundle via collectBundleServerlessVersions, degrading to [] on read failure) and the confirm UI (pickServerlessVersion). The scoring/pipeline lives in one place, with one home to grow evidence sources (notebooks, workspace default). - selectServerless still owns the flag branch: off -> plain enableServerless(); on -> resolve a version, undefined = dismissed -> no compute change. *Verification* yarn --cwd packages/databricks-vscode build / test:lint / test:unit (352 passing). Co-authored-by: Isaac
*Why*
DECO-27782 lists .ipynb notebook metadata as a serverless-version evidence
source (weight `notebook`, between bundle and workspace-default). The scorer
already ranks it, but nothing produced it -- so it never reached the picker.
This adds the collector.
*What*
- collectNotebookServerlessVersions(notebooks): pure collector over already-parsed
.ipynb JSON. Reads environment_version from the Databricks notebook metadata
block (metadata["application/vnd.databricks.v1+notebook"].environmentMetadata
.environment_version), emitting {version, source:"notebook"} observations.
Bare-integer form, matching --serverless-version. Only inspects the Databricks
metadata key (plain Jupyter notebooks contribute nothing), defensively walks it
for the field, tolerates missing/string/null nodes, coerces numeric values, and
de-duplicates preserving first-seen order.
*Verification*
yarn --cwd packages/databricks-vscode test:unit (359 passing; 7 new: metadata-key
read, multi-notebook dedup, numeric coercion, non-Databricks notebook ignored,
missing-metadata tolerance, non-scalar skip, empty list).
Co-authored-by: Isaac
*Why* The notebook collector existed but nothing invoked it. The compute picker's evidence gathering only read the bundle, so notebook-declared versions never reached the ranking. *What* - Add collectProjectNotebookVersions(projectRoot): thin I/O wrapper that globs the project's .ipynb files (excluding node_modules), JSON-parses each (skipping unreadable/malformed ones), and delegates to the pure collectNotebookServerlessVersions. - ConnectionCommands: gather bundle + notebook observations in parallel via a new collectServerlessObservations(), each source independently guarded so one failure never blocks the other or compute selection; the scorer merges/de-dupes across sources. Add WorkspaceFolderManager to the constructor for the project root (guarded: activeProjectUri throws when there's no project). - extension.ts: pass workspaceFolderManager to the single ConnectionCommands call site. This closes 3 of DECO-27782's 4 evidence sources (bundle + notebook + fallback). The workspace-default API source remains deferred: the pinned SDK 0.18.0 does not expose Environments.GetDefaultWorkspaceBaseEnvironment. *Verification* yarn --cwd packages/databricks-vscode build / test:lint / test:unit (359 passing). Co-authored-by: Isaac
Why: Code review of the compute-picker serverless-version work surfaced two real gaps in the version-harvesting path (plus a false-positive on the notebook field name worth pinning down so it isn't "fixed" later). What: - projectNotebookVersions: exclude `.venv`/`venv`/`site-packages` (the python-setup feature provisions its venv *inside* the project root, and dependency notebooks would otherwise inject spurious notebook-weighted version signals) plus `.git`/`.databricks`/`dist`/`build`; cap the scan at 200 notebooks and log when truncated, so the compute picker can't be stalled by a huge repo. New test covers the exclusions. - ConnectionManager.updateServerless: re-validate the persisted `serverlessVersion` with `isSupportedVersion` on load; a hand-edited or out-of-range value is dropped to undefined (scored default) rather than exposed as if the `--serverless-version` flag would accept it. - notebookServerlessVersions: document that the mixed casing (camelCase `environmentMetadata` / snake_case `environment_version`) is the real exported `.ipynb` shape (confirmed against Databricks KB), so the leaf is intentionally not camelCase. Verification: - yarn test:unit — 362 passing (3 new: project-notebook scan + exclusions). - test:lint clean · build clean (type-checks the ConnectionManager change). Co-authored-by: Isaac
Why: Review asked about the UX cost of scanning up to MAX_NOTEBOOKS_SCANNED (200) notebooks on the interactive compute-picker path. `.ipynb` files embed cell outputs (plots/images) and can be tens of MB, so the prior `Promise.all` that read + fully `JSON.parse`d every match risked a latency and peak-memory spike before the version sub-picker appears. What: - Cheap substring pre-filter: skip `JSON.parse` (and its large-object allocation) for any notebook whose raw text doesn't even contain `environment_version` -- the common case, since most notebooks declare no serverless environment. - Bounded concurrency (SCAN_CONCURRENCY = 20) instead of reading all matches at once, so peak footprint stays flat regardless of repo size. - Tests: parse is skipped for a version-less notebook (proven by writing it as invalid JSON that still doesn't throw), and collection still works past the concurrency batch boundary (>20 notebooks). Verification: - yarn test:unit — 364 passing (2 new: pre-filter skip + batch boundary). - test:lint clean · build clean. Co-authored-by: Isaac
rugpanov
force-pushed
the
rugpanov/python-setup-compute-picker
branch
from
July 28, 2026 13:30
d86a3cb to
8e57467
Compare
rugpanov
changed the base branch from
rugpanov/python-setup-serverless-picker
to
main
July 28, 2026 13:31
Contributor
Author
|
🤖 Integration tests triggered for |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Selecting Serverless in the compute picker only ever recorded a boolean — the serverless environment version was then either an invisible setting (legacy pip flow reads
serverlessDbconnectVersion) or would have to be asked on every setup run. This merges the version choice into the compute picker: the user confirms it once, at compute-selection time, and it's persisted with the selection so setup never re-prompts.Part of DECO-27786 (see the split note below).
What
OverrideableConfigState.serverlessVersion?: string(routes through the existingConfigModeloverrideable path automatically).enableServerless(version?)persists it (also when serverless is already on, so re-picking the version alone saves);disableServerless()clears it;updateServerless()loads it on reload and re-validates it (a hand-edited / out-of-range value is dropped to the scored default rather than exposed as a valid--serverless-version); newget serverlessVersion().collectBundleServerlessVersions(bundle): defensively walks the parsed bundle and harvests every serverlessenvironment_version(a bare integer, same form--serverless-versiontakes) as abundleYamlscoring observation, so the picker is seeded with real candidates rather than a lone fallback.collectNotebookServerlessVersions(notebooks)+collectProjectNotebookVersions(projectRoot): harvest the serverless version recorded in.ipynbmetadata (metadata["application/vnd.databricks.v1+notebook"].environmentMetadata.environment_version) as anotebookscoring observation. The project scan excludes dependency/venv/build trees (.venv,site-packages,node_modules,.git,.databricks,dist,build) so a package's sample notebook can't inject a spurious signal, is capped (200 notebooks) and runs bounded-concurrency batches with a cheap substring pre-filter to skip the JSON parse of notebooks that don't declare a version — so it can't stall the compute picker on a large repo.ConnectionCommands: choosing Serverless routes throughselectServerless(), which (when opted in) collects the bundle + notebook observations, ranks them, shows [7/?] feat(python-setup): serverless version resolution & picker #2052's picker (top candidate pre-selected), and persists viaenableServerless(version). Each source is collected independently and guarded, so one failing source never blocks the other or compute selection.Constraint 1 — backward compatibility
serverless. Existing configs withserverless: trueand no version stay valid — a version-less selection falls back to the scored default (5). No migration writes; writing an unrelated key never fabricates a version (tested). A persisted version is re-validated on load, so a downgrade/upgrade that leaves an out-of-range value can't leak it to the CLI.enableServerless()with no version). The new field is just an unused optional.databricks.connect.serverlessDbconnectVersionsetting (a DBR/dbconnect version like17.3, used by the legacy pip rail) is a separate namespace — this code never reads or writes it.Constraint 2 — feature-flag guard (#2045)
The version sub-step is gated on
isPythonSetupEnabled()(thePYTHON_SETUP_FEATURE_IDopt-in from #2045). Only opted-in users ever see the version picker; everyone else gets the unchanged serverless selection.Verification
yarn --cwd packages/databricks-vscode test:unit— 364 passing (config persistence incl. legacy-config-untouched, bundle collector, notebook collector, and project-notebook scan incl. exclusions / parse-skip / batch boundary). ConnectionManager + picker wiring type-checked bybuild.test:lint— clean ·build— clean.Note on the ticket split
DECO-27786 (extension wiring & config-view dispatch) is being split into two diffs. This is Diff A — the compute-picker merge (shared compute code, releasable on its own). Diff B (construct detector/client/orchestrator in
activate(),EnvironmentComponentdispatch, readserverlessVersionfromConnectionManagerin the setup flow) will follow once the orchestrator (#2046) and client (#2039) merge, so it can go cleanly offmain. Nothing in this diff consumesserverlessVersionyet — the picker persists it for Diff B to read.This pull request and its description were written by Isaac.