[6/?] feat(python-setup): setup orchestrator (first CLI-client caller) - #2046
[6/?] feat(python-setup): setup orchestrator (first CLI-client caller)#2046rugpanov wants to merge 1 commit into
Conversation
|
🤖 Integration tests triggered for |
3508259 to
b48d2ae
Compare
|
🤖 Integration tests triggered for |
b48d2ae to
fa992f3
Compare
|
🤖 Integration tests ❌ 1 of 35 test jobs failed for |
| * This is the first real caller of {@link PythonSetupCliClient}. The visibility | ||
| * gate and serverless-version selection are injected seams (see | ||
| * {@link PythonSetupSetupDeps}) that the extension currently wires to stubs, so | ||
| * shipping this alongside the client shows the client's usage without depending | ||
| * on tickets that are not built yet. |
There was a problem hiding this comment.
nit: I don't think this is useful as code comments
| * This is the first real caller of {@link PythonSetupCliClient}. The visibility | |
| * gate and serverless-version selection are injected seams (see | |
| * {@link PythonSetupSetupDeps}) that the extension currently wires to stubs, so | |
| * shipping this alongside the client shows the client's usage without depending | |
| * on tickets that are not built yet. |
There was a problem hiding this comment.
Agreed — trimmed the class doc down to just the flow summary and dropped the PR-narrative prose. Also did a pass over the rest of the file removing comments that only restated self-explanatory names. Done in f84e6ff.
| // A real-run success must carry everything the success path consumes: | ||
| // the provisioned venv to adopt, and the target/resolved fields we | ||
| // persist as the drift-detection baseline. If any is missing (a dry-run | ||
| // shape, or CLI/schema drift), we can't deliver a usable, tracked | ||
| // environment — treat it as a failure rather than reporting a hollow | ||
| // "ready" that either skips interpreter adoption or leaves drift | ||
| // detection with no baseline. | ||
| if (!result.venvPath || !result.target || !result.resolved) { |
There was a problem hiding this comment.
Can we merge this into isPythonSetupSuccess since it's an error case?
There was a problem hiding this comment.
I kept isPythonSetupSuccess as the CLI-contract predicate (ok === true) because it is legitimately true for a successful --dry-run (which has no venvPath), and folding the stricter check in would break that contract and its existing consumers/test. Instead I extracted the check into a named orchestrator-level type guard, isLocalEnvironmentReady(), so the "this is an error case" intent is explicit without changing the shared predicate. f84e6ff.
| return; | ||
| } | ||
|
|
||
| await this.deps.adoptInterpreter(result.venvPath); |
There was a problem hiding this comment.
Do we intentionally not show an error message when this fails? What is the consequence of the Python extension not being pointed at the provisioned venv interpreter?
There was a problem hiding this comment.
Good catch — that was an oversight, not intentional. Consequence: the venv is provisioned on disk but the Python extension is not pointed at it, so imports/dbconnect will not resolve — and previously it failed silently. Fixed by wrapping adoptInterpreter(): a failure now surfaces the error and keeps the flow not-ready. Added a test (surfaces an error and stays not-ready when interpreter adoption fails). f84e6ff.
fa992f3 to
1c238f1
Compare
|
🤖 Integration tests triggered for |
1c238f1 to
5f6604a
Compare
5f6604a to
8353bcb
Compare
|
🤖 Integration tests triggered for |
8353bcb to
cade982
Compare
*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
cade982 to
f84e6ff
Compare
|
🤖 Integration tests ✅ all 35 test jobs passed for |
2 participants
Why
#2039 adds
PythonSetupCliClientwith no call site — as raised in review, the API is hard to judge without seeing it used. This PR adds the orchestrator that actually drives the client end-to-end, so the client's usage ships alongside it.What
PythonSetupEnvironmentSetup(python-setup/controllers/) — the first real caller ofPythonSetupCliClient:cli.run(invocation, {cwd, onLog})under a progress indicator,{envKey, pythonVersion}for drift detection,getPythonSetupErrorMessagecopy,PythonSetupCancelledError→ treated as a silent user action (no error toast).It consumes the real client surface (
run(invocation{compute}, {cwd, onLog}),isPythonSetupSuccess,getPythonSetupErrorMessage), and depends on the client structurally (CliRunner) so the flow is unit-testable without spawn/vscode.The two not-yet-built pieces are injected seams, not hard deps
Per the agreed approach, the gate and serverless-version selection are stubbed behind injected functions with explicit TODOs, so this PR stacks only on the CLI-client change and doesn't pull in unbuilt tickets:
isVisible()— TODO(DECO-27781 / [4/?] feat(python-setup): package-manager gating & live detection #2044):shouldShowPythonSetup(...). The wiring stub returnsfalse, so the feature is inert end-to-end for now.resolveCompute()— TODO(DECO-27782): serverless-version picker (scoreServerlessVersions+pickServerlessVersion). The wiring stub returnsundefined, so a serverless setup is a no-op until that ticket; the cluster case can already resolve.The real gate and picker drop in through these seams in their own tickets (and the extension.ts wiring lands in DECO-27786). Nothing is wired into the extension in this PR.
Verification
yarn --cwd packages/databricks-vscode test:unit— 344 passing (6 new orchestrator cases: success→ready+adopt+saveState, gate-closed→no run, no-compute→no run, CLI failure→mapped error, cancel→silent,onDidChangeStatefires).test:lint— clean ·build— clean.This pull request and its description were written by Isaac.