feat(python-setup): open the compute picker inline when no compute is attached - #2114
Merged
Conversation
rugpanov
had a problem deploying
to
test-trigger-is
August 13, 2026 15:18 — with 
GitHub Actions
Error
GitHub Actions
Error
Contributor
Author
|
🤖 Integration tests triggered for |
rugpanov
had a problem deploying
to
test-trigger-is
August 13, 2026 15:36 — with
GitHub Actions
Error
GitHub Actions
Error
Contributor
Author
|
🤖 Integration tests ✅ all 35 test jobs passed for |
Contributor
Author
|
🤖 Integration tests ✅ all 35 test jobs passed for |
rugpanov
had a problem deploying
to
test-trigger-is
August 13, 2026 21:53 — with
GitHub Actions
Error
GitHub Actions
Error
rugpanov
had a problem deploying
to
test-trigger-is
August 14, 2026 05:49 — with
GitHub Actions
Error
GitHub Actions
Error
Contributor
Author
|
🤖 Integration tests triggered for |
rugpanov
had a problem deploying
to
test-trigger-is
August 14, 2026 05:59 — with
GitHub Actions
Error
GitHub Actions
Error
Contributor
Author
|
🤖 Integration tests ✅ all 35 test jobs passed for |
rugpanov
force-pushed
the
python-setup-select-compute
branch
from
August 14, 2026 12:22
c801ac6 to
f26d817
Compare
rugpanov
had a problem deploying
to
test-trigger-is
August 14, 2026 12:23 — with
GitHub Actions
Error
GitHub Actions
Error
Contributor
Author
|
🤖 Integration tests ✅ all 35 test jobs passed for |
misha-db
approved these changes
Aug 14, 2026
rugpanov
enabled auto-merge (squash)
August 14, 2026 13:29
Why: Clicking "Set up Python environment" with no compute attached dead-ended on a "select a cluster or serverless compute first" warning, forcing the user to leave the flow, attach compute, and come back. A serverless session missing only its version already prompts inline and continues; the no-compute case should be just as direct. What: - resolveCompute (pythonSetupDeps): when nothing is attached, open the existing compute picker inline, then re-read the attachment and continue setup against whatever the user selected. Dismissing the picker leaves the attachment unchanged, so the flow still falls through to the existing guidance message + no-compute telemetry -- nothing regresses, the picker is only an added opportunity. - attachClusterQuickPick command (ConnectionCommands): its returned promise now resolves only once the picker has fully closed (after any attach/enable it triggers, or immediately on dismissal), so a caller can await the outcome and re-read state. Existing fire-and-forget callers ignore the result unchanged. - extension.ts: wire promptSelectCompute to the compute-picker command. Verification: - yarn build; unit suite green (462 passing) incl. new resolveCompute cases: picker opened when none attached, runs against the attached compute, not opened when a cluster is already attached, version prompted when the picker leaves serverless version-less. - yarn fix / test:lint clean. Co-authored-by: Isaac
Why: Code review (Codex + a Claude reviewer, converging independently) caught that re-reading the attached compute right after the picker closed is deterministically stale for a cluster: ConnectionManager.attachCluster only writes the `clusterId` config, and the cluster object is rebuilt by a separate, network-gated, fire-and-forget config listener that the attach never awaits. So the immediate re-read saw `cluster: undefined` and setup dead-ended on the very message this feature removes. (The serverless path only worked by luck -- enableServerless sets its state synchronously.) The first cut's passing test masked this by mutating the mock synchronously. What: - attachClusterQuickPick command now RETURNS the chosen compute (SelectedCompute: cluster or version-complete serverless) or undefined on dismissal, so callers use the selection directly. selectServerless returns the confirmed version. Existing fire-and-forget callers ignore the new return value, unchanged. - resolveCompute (pythonSetupDeps) consumes that return value instead of re-reading connectionManager state, eliminating the race. - Harden the accept handler while here (review nits): guard a repeated Enter / empty selection, and catch+log so a failing attach can't leak an unhandled rejection or leave the picker open. - Tests rewritten to model the real contract: the picker returns the compute while attachedCompute stays `none`, proving the flow no longer depends on a re-read. Verification: - yarn build; full unit suite green (736 passing). yarn fix / test:lint clean. Co-authored-by: Isaac
…te type Why: Round-1 multi-source review (Codex + a Claude reviewer) surfaced three points. Codex's "attach failure returns as success" was verified a false positive: setup-local receives the clusterId/version descriptor directly (setupLocalArgs), so it is valid regardless of whether the config write persisted, and @onerror already surfaces any failure to the user. The two actionable notes were a test-coverage gap on the picker's producer logic and an unchecked type link. What: - Add unit tests for attachClusterQuickPickCommand's return contract via a scriptable QuickPick fake: resolves to the attached cluster (attaching once), undefined on dismissal, undefined on empty accept, and the `settled` re-entry guard (a second Enter neither re-attaches nor changes the result). This is the riskiest new code and had no producer-side test. - Alias SelectedCompute to SetupCompute (the setup-local compute shape) instead of re-declaring it, so drift is a compile error rather than a silent mismatch through executeCommand's unchecked generic. - Use Loggers.Extension (the enum) instead of the "Extension" string literal, matching the codebase convention. Verification: - yarn build; full unit suite green (740 passing, +4 new). yarn fix / test:lint clean. Co-authored-by: Isaac
… + tests Why: Iterative review re-raised that the picker returns the chosen compute even when attachCluster/enableServerless silently fail (they are @onerror with throw:false, so a config-write failure shows a popup but does not throw). Decision: keep this best-effort -- the returned descriptor is the compute the user picked and is all setup-local needs (it takes the id/version directly), and the failure is already surfaced to the user. The prior code comment overclaimed that the catch nulls the selection on attach failure, which is dead for those swallowed operations. What: - Correct the catch comment: it is defense-in-depth for an UNEXPECTED throw (version sub-picker, openExternal, a future non-decorated path), not a success/failure discriminator for the @onError-swallowed attach/enable. - Document at the cluster branch why returning the descriptor is intentional even when the attach's persistence fails. - Add contract tests: the chosen cluster is still returned when the attach silently fails (best-effort), and an unexpected throw settles the Promise to undefined (no hang, no selection). Verification: - yarn build; full unit suite green (742 passing, +2). yarn fix / test:lint clean. Co-authored-by: Isaac
Why: The explanatory comments added across the feature were far longer than the code warranted. What: - Condense the doc/inline comments in ConnectionCommands.ts (picker command, settle guard, best-effort attach, catch, SelectedCompute, selectServerless), pythonSetupDeps.ts (promptSelectCompute, resolveCompute), extension.ts, and the two test files. No code or behavior changes. Verification: - yarn build; full unit suite green (742 passing). yarn fix / test:lint clean. Co-authored-by: Isaac
rugpanov
force-pushed
the
python-setup-select-compute
branch
from
August 14, 2026 13:29
f26d817 to
001396e
Compare
Contributor
|
If integration tests don't run automatically, an authorized user can run them manually by following the instructions below: Trigger: Inputs:
Checks will be approved automatically on success. |
rugpanov
had a problem deploying
to
test-trigger-is
August 14, 2026 13:31 — with
GitHub Actions
Error
GitHub Actions
Error
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
Clicking Set up Python environment with no compute attached dead-ended on a "Select a cluster or serverless compute before setting up the environment" warning, forcing the user to leave the flow, attach compute, and come back. A serverless session missing only its version already prompts inline and continues — the no-compute case should be just as direct.
What
resolveCompute(pythonSetupDeps.ts) — when nothing is attached, open the existing compute picker inline, then re-read the attachment and continue setup against whatever the user selected.attachClusterQuickPickcommand (ConnectionCommands.ts) — its returned promise now resolves only once the picker has fully closed (after any attach/enable it triggers, or immediately on dismissal), so a caller can await the outcome and re-read state. Existing fire-and-forget callers ignore the result, unchanged.extension.ts— wirepromptSelectComputeto the compute-picker command.Behavior preserved
no-computetelemetry. The picker is purely an added opportunity — nothing regresses.Backward compatibility
attachClusterQuickPickis a public command; itsexecuteCommandpromise now settles after the picker closes instead of immediately. The three existing callers (cluster tree-item click,EnvironmentDependenciesVerifier, run-flowpromptForClusterAttach) are all fire-and-forget and ignore the result — verified no regression, only more-correct resolution timing.Verification
resolveComputecases: picker opened when none attached, runs against the attached compute, not opened when a cluster is already attached, version prompted when the picker leaves serverless version-less.yarn fix/test:lintclean.This pull request and its description were written by Isaac.