Skip to content

localenv: fall back to installed Python - #6457

Open
rugpanov wants to merge 6 commits into
mainfrom
python-install-fallback
Open

localenv: fall back to installed Python#6457
rugpanov wants to merge 6 commits into
mainfrom
python-install-fallback

Conversation

@rugpanov

@rugpanov rugpanov commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Changes

  • keep uv python install as the first attempt
  • after a download failure, ask uv python find for a compatible interpreter already on the machine — uv's own discovery excludes the active venv, prefers uv-managed installs, and returns the highest locally-installed patch — then provision with the exact executable path it returns
  • expose a categorical resolution in JSON and explain successful fallback in CLI output

Why

Python downloads can fail on restricted networks even when a compatible interpreter is already installed. Reusing that interpreter lets setup complete without retrying the failed download.

If discovery or provisioning still fails, the structured result preserves the fallback state so clients can offer manual interpreter selection.

Tests

  • GOTOOLCHAIN=go1.26.5 GOPROXY=direct go test ./libs/localenv/...
  • affected command tests
  • Go formatting, lint, whitespace, tidy, dead-code, link, changelog, and lockfile checks

@anton-107

Copy link
Copy Markdown
Contributor

Reviewed the full diff. go vet and go test ./libs/localenv/ ./cmd/environments/ both pass on the branch, so nothing here is a build or test break — the findings are behavioral and structural. The uv-behavior claims below are measured against a real uv 0.11.21 rather than inferred.

Two of these I'd treat as blocking; the rest are ordinary review comments.


🔴 Blocking

1. libs/localenv/uv.go:167 — the request built from requires-python is unparseable for some forms we accept, and uv fails silently

EnsurePython concatenates the raw requires-python onto ,==<minor>.*. For an idiomatic floor that's fine. But PythonMinorFromRequires also accepts operator-less and pinned forms, and those build a request uv cannot parse — and uv does not error on it. It prints [] and exits 0. So the code can't distinguish "your specifier is malformed" from "nothing is installed", and reports no compatible installed Python found while a perfectly good interpreter sits in /usr/bin.

Measured with uv python list --only-installed --all-versions --output-format json --no-managed-python <REQUEST>:

request result
cpython@>=3.12,==3.12.* /usr/bin/python3.12
cpython@>=3.12,<3.13,==3.12.* /usr/bin/python3.12
cpython@3.12,==3.12.* [], exit 0 ❌
cpython@==3.12,==3.12.* [], exit 0 ❌
cpython@===3.11,==3.11.* [], exit 0 ❌
cpython@==3.12.* (control) /usr/bin/python3.12

All three failing forms are ones TestPythonMinorFromRequires explicitly accepts (3.12, ===3.11, !=3.11,3.12, 3.12,!=3.12.4).

Suggested fix: drop constraint and request "==" + minor + ".*" on its own. It isn't buying anything — pyMinor is already the floor and validate only asserts the minor. It also removes a second problem for free: constraint is the untrimmed raw TOML value (parseConstraints trims only for its emptiness check), so requires-python = " >=3.12 " forwards cpython@ >=3.12 ,==3.12.* into argv.

2. Missing .nextchanges/ fragment

Per .agents/rules/changelog.md. This changes provisioning behavior, adds a line to text output, and adds a pythonResolution key to the --output json contract — user-visible on all three counts. #6445 added .nextchanges/cli/setup-local-multiline-toml.md for a smaller change.


Repo rules — worth settling before merge

3. uv.go:32runFn reimplements process.WithStub, and it short-circuits the path under test. process.WithStub(ctx) (libs/process/stub.go:16) already hooks runCmd inside process.Background with WithStdoutFor/WithFailureFor/WithCallback; it's used in cmd/labs/project/installer_test.go, bundle/config/mutator/python/python_mutator_test.go, and libs/aitools/installer/plugin_test.go. Beyond the duplication (CLAUDE.md: search for an existing helper first), runUvOutput returns at 151-153 before resolveIndexURL, so the argv-to-subprocess wiring for the new uv python list calls and the UV_INDEX_URL bridge have zero coverage — and that index-URL path is the whole restricted-network motivation for the feature.

4. No doc comments on any new symbol. selectInstalledPython (51), newerInstalledPython (81), runUvOutput (150), listInstalledPython (184), and the types uvRunFn (35), uvPython (37), installedPython (46) — in a file where runUv, venvPython, lineWithPrefix, pipConfPaths, and redactURLCredentials each carry several sentences. Relatedly, pythonListArgs encodes uv-specific grammar (--only-installed, --managed-python, cpython@<specifiers>) with no source link, while the same file links docs.astral.sh three times elsewhere — and finding 1 is exactly the assumption a link would have let a reader check.

5. uv.go:193 — deleted rationale in the Provision doc comment. The removed text named the concrete repro and tied it to the validate phase: "uv sync selects the newest installed interpreter satisfying requires-python (e.g. 3.13 for a >=3.12 floor), which then fails validation against the 3.12 target." The replacement ("can select a newer interpreter than the target") drops both, so the next reader can't tell the guard is load-bearing for pipeline.validate. Also syncArgs (311-315) still says "pinning the interpreter to pyMinor via --python" after the parameter was renamed and widened to accept a path.


Correctness / behavior

6. uv.go:93 — the managed-vs-system tie-break is dead code. selectInstalledPython always walks the managed group first, so candidate.managed && !current.managed is true && !true in the managed group and false && … in the system group. It always returns false. TestSelectInstalledPython/"managed wins an equal-version tie" passes purely on iteration order, so the documented preference is unenforced and will vanish silently if anyone reorders the two groups or merges them into one uv call.

7. uv.go:81 — patch is compared before managed, inverting uv's own default. uv defaults to python-preference = "managed": prefer uv-managed installs over system ones even when the system one is newer. Here a uv-managed 3.12.10 loses to a distro 3.12.11, so the venv gets built on a distro-patched interpreter uv would never have picked — while a self-contained managed one sat right there. Given the premise is "the managed download path is broken on this network", an already-downloaded managed interpreter is the more reliable pick. Suggest preferring managed unconditionally and using patch only to break ties within a group. (Happy to be argued out of this one.)

8. uv.go:67 — one empty path aborts the whole selection. The return "", errors.New(...) guard sits inside the scan loop, after the managed group has been walked, so a single entry with a null/empty path discards a viable candidate best is already holding. uv types these fields as nullable in its real JSON ("symlink":null, "url":null). continue is the right action here.

9. uv.go:163 — a cancelled context still shells out twice. Ctrl-C during a slow download makes installErr a signal failure; the code then runs uv python list twice against the dead ctx purely to fail, and the recorded error and phase Detail carry two spurious "list failed" causes for what the user experienced as a clean interrupt. An early if ctx.Err() != nil return keeps the diagnostic clean.

10. pipeline.go:504selection.Executable is forwarded unchecked. The third arg used to be pyMinor, validated upstream. A PythonSelection{} with a nil error now yields uv sync --python "", surfacing as an opaque E_PROVISION. The parameter is also now stringly overloaded ("either a minor request or an absolute path") while the caller already holds selection.Resolution and could disambiguate.


Diagnosability of the fallback path

This is the cluster I'd most like to see addressed together — for a feature whose entire reason to exist is "the network broke the download", none of the three is recoverable from a --debug transcript today.

11. uv.go:171 — the list command's stderr is structurally unreachable. uvFailure uses errors.AsType[*process.ProcessError], which walks the errors.Join(installErr, err) tree depth-first and so always matches installErr first; the list error is found second and never consulted. ProcessError.Error() is only Command: Err — it omits Stderr. So error: unexpected argument '--managed-python' found, or an EACCES on the uv binary, appears nowhere in the output or in --output json.

12. uv.go:181 — the chosen interpreter is never logged. The file logs uv: discovered binary at %s and uv: not found; running installer: %s for far less consequential events, but the fact that decides whether the resulting venv is trustworthy goes unrecorded. One log.Debugf before the return matches the existing convention.

13. cmd/environments/output.go:114 — the fallback is surfaced only on success. renderResult returns at the pipelineErr != nil branch long before renderSuccess, so a user whose uv sync then fails on a distro python (missing headers, patched ssl) sees only "✗ Setup failed while provisioning" with no hint that Python came from the system. pipeline.go:500-503 deliberately sets PythonResolution before Provision for exactly this; text output just doesn't read it.


Questions / discretionary

14. uv.go:325 — no minimum-uv-version check. --managed-python/--no-managed-python replaced --python-preference only-managed/only-system in uv 0.5.x–0.6, and --output-format json plus the positional REQUEST are similarly recent. On an older uv both list calls fail and the feature is silently inert (made worse by finding 11 hiding the reason). How much this matters depends on whether EnsureAvailable always lands a recent uv — if it does, feel free to wave this off. EnsureAvailable already captures the version string at uv.go:130-134 if a gate is wanted.

15. uv.go:177 — the comparator may be re-deriving what uv already gives you. uv python list emits entries best-first (verified: 3.13.7, 3.12.11, 3.11.13, 3.10.18 descending), so --all-versions plus newerInstalledPython, the installedPython wrapper type, the anonymous []struct{pythons; managed}, and the best pointer collapse to "first element of managed, else first of system". Separately, the same three-line return PythonSelection{}, uvFailure(ErrPythonInstall, errors.Join(installErr, err), …) is copy-pasted verbatim at 171, 175, and 179 — one named wrapper would also be the single place to fix finding 11.

16. On testing pythonResolution: it's emitted as "uv_install_succeeded" on every real run and omitted on every dry run — and all 10 acceptance goldens are --dry-run, which is why none needed regeneration. TestResultOmitsUnknownPythonResolution pins only the bare-NewResult case, so the value consumers will actually branch on is unpinned, and there's no PythonResolutionUnspecified constant giving them a documented zero. Minor: the comment at pkgmanager.go:37-39 justifies the field against telemetry, but buildSetupLocalEvent never reads it.

@anton-107 anton-107 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the detailed review. The following findings need to be addressed before this can merge:

  • #1uv.go:167, constraint concatenation silently disabling fallback. Real bug: the accepted bare/==/=== requires-python forms produce a request uv resolves to [] (exit 0), so a valid installed interpreter is missed. One caveat on the remedy — dropping the constraint and requesting only cpython@==<minor>.* is lossy: the validate phase compares major.minor only, so it will not catch an excluded/capped patch, and uv sync --python <path> would then reject it. Prefer normalizing requires-python into a valid specifier (trim whitespace + rewrite the bare/exact forms) and keep the ==<minor>.* pin.
  • #2 — missing .nextchanges/ fragment. Required by .agents/rules/changelog.md for this user-visible behavior + JSON-contract change.
  • #3runFn reimplements process.WithStub. Switch to the existing seam; it also gives coverage to the new uv python list argv wiring and the UV_INDEX_URL bridge, which are currently untested (and that bridge is the whole restricted-network motivation).
  • #6 — dead managed-vs-system tie-break. Because the managed group is always iterated first, the candidate.managed && !current.managed branch never fires. Remove the always-false expression and document that managed-first ordering provides the preference, or make the preference actually reachable.
  • #8 — one empty path aborts the entire selection. continue past unusable entries and fail only if none remain, or keep the strict check with a one-line comment documenting the --only-installed invariant.

Requesting changes on these.

@rugpanov
rugpanov force-pushed the python-install-fallback branch 6 times, most recently from 8dbb696 to dd348fd Compare September 2, 2026 07:40
@rugpanov
rugpanov requested a review from anton-107 September 2, 2026 07:44
@anton-107

Copy link
Copy Markdown
Contributor

Reviewed 8dbb696 in a fresh checkout. go vet, go test ./libs/localenv/... ./cmd/environments/..., and CI lint all pass — nothing here is a build break. All uv claims below are measured against a real uv 0.11.21, not inferred.

The mechanism works (verified uv sync --python <abs path> end-to-end: builds the venv from exactly that interpreter). Most of the previous round's findings are genuinely fixed. My main comment is that the new code can mostly be deleted, and one error path is worse than before.


1. The whole discovery machinery is one uv flag

uv sync already does this, with uv's own preference order:

$ uv sync --python 3.12 --no-python-downloads
Using CPython 3.12.13          # the uv-managed one, not /usr/bin 3.12.11
Creating virtual environment at: .venv

$ uv sync --python 3.14 --no-python-downloads
error: No interpreter found for Python 3.14 in managed installations or search path
hint: A managed Python download is available for Python 3.14, but Python downloads are set to 'never'

That deletes uvPython, installedPython, selectInstalledPython (uv.go:54), newerInstalledPython (86), listInstalledPython, pythonListArgs, fallbackPythonConstraint (195), the JSON need in runUvOutput, two subprocesses, and ~120 lines of tests — and gets a better error string than no compatible installed Python found. PythonSelection carries a bool instead of a path; PythonResolution is still derivable from installErr == nil, so the JSON contract is unchanged.

It also settles the managed-vs-patch question from last round the right way for free: uv honors python-preference = "managed" itself.

Trade-off, stated honestly: "nothing installed" then surfaces as E_PROVISION at sync rather than E_PYTHON_INSTALL, without the download stderr attached. If that code fidelity matters, uv python find --system --no-python-downloads 'cpython@==3.12.*' gives you the path in one call instead. --system is mandatory there — without it, inside a project directory it returns the project's own venv:

$ uv python find --no-python-downloads 'cpython@==3.12.*'      # in a dir with .venv
/tmp/findvenv/.venv/bin/python3
$ uv python find --system --no-python-downloads 'cpython@==3.12.*'
/home/…/.local/share/uv/python/cpython-3.12-…/bin/python3.12

uv python list has no such trap (tested with a .venv present and with VIRTUAL_ENV set — venvs are excluded), so the current choice is the safe one. Just much heavier than it needs to be.

2. The joined error mislabels its own cause

Last round asked for the list command's stderr to be reachable. It is now — but pasted into the install command's sentence. Actual rendered output when --managed-python isn't supported:

uv python install 3.12 failed: error: Failed to download cpython-3.12.13-linux-x86_64-gnu
  Caused by: Request failed after 3 retries
  Caused by: error sending request for url (https://github.com/astral-sh/…)
error: unexpected argument '--managed-python' found: uv python install 3.12: exit status 2
uv python list --only-installed … cpython@==3.12.*: exit status 2

--managed-python was never passed to python install. And when discovery simply finds nothing, the one actionable line lands last, after the download stack:

uv python install 3.12 failed: error: Failed to download …: uv python install 3.12: exit status 2
no compatible installed Python found

errors.Join into a single action string (uv.go:172/176/180) can't produce a coherent message — the two failures need separate sentences, or the fallback outcome belongs in Msg as a clause with installErr as the wrapped cause.

3. The download failure reason is dropped on the success path

libs/process/background.go:56-63 doesn't log stderr on failure, and installErr is never logged or recorded before uv.go:189 returns. So on a run that succeeds via fallback, nobody can ever learn why the download failed — --debug shows only running: uv python install 3.12. For a feature whose premise is "the network broke the download", that's the fact worth keeping. One log.Debugf with processStderr(installErr), or fold it into a warning (see #5).

4. No real-uv coverage for a real-uv-only feature

provision_integration_test.go:22 calls itself "the one place the real merge → provision → validate path is exercised". The PR adds nothing there, and it's the only thing that would catch the request-grammar / flag-support / JSON-shape class of bug. The fallback is forceable hermetically — no external network:

$ UV_PYTHON_INSTALL_MIRROR=https://127.0.0.1:9/nope uv python install 3.13
  Caused by: tcp connect error
  Caused by: Connection refused (os error 111)

5. Warnings already is this channel

Warning{Code, Message} (result.go:235) is a closed categorical code set, already rendered in text output (output.go:48-50) and already in the JSON contract. A W_PYTHON_INSTALLED_FALLBACK gives the extension the identical categorical signal without a new top-level key, a new render function, and a new field — and carries the download reason from #3 in Message. The counter-argument is that pythonResolution also encodes the positive uv_install_succeeded; worth saying out loud whether any consumer branches on that.

6. The telemetry rationale is unimplemented

pkgmanager.go:39 justifies the field's categoricality against telemetry, but buildSetupLocalEvent (cmd/environments/telemetry.go:27-46) never reads it. Either wire it in — "how often does the download fail in the field" is the most valuable thing this PR could measure, and it's the answer to #8 — or drop the telemetry sentence.

7. fallbackPythonConstraint costs more than it earns

Real artifact values are >=3.12 (provision_integration_test.go:53) and ==3.12.* (merge_test.go:24). Both already work bare. Next to ==<minor>.*, every clause except a patch-level != is redundant — and no published artifact uses one.

Measured residuals:

requires-python request built uv 0.11.21
==3.12 cpython@==3.12,==3.12.* [], exit 0
===3.12 cpython@===3.12,==3.12.* [], exit 0
>=3.12,foo cpython@>=3.12,foo,==3.12.* [], exit 0

The first two are semantically correct rejections — only 3.12.0 satisfies ==3.12, and uv sync would reject the interpreter too — so not bugs. But TestFallbackPythonConstraint names them "preserves exact minor" / "preserves arbitrary equality" as if they were the win, when what they guarantee is that the fallback finds nothing.

The third is a real divergence: clauseRe doesn't match foo, so it's forwarded verbatim, while PythonMinorFromRequires (envkey.go:76-79) skips unmatched clauses. Two functions, same input, different contracts — and parseConstraints only guarantees a floor exists, not that every clause parses.

Verified fine, so not concerns: >= 3.12 (whitespace), ~=3.12, !=3.11,>=3.12, and >=3 / ==3.* (unmatched by clauseRe but valid to uv).

8. Scope: the fallback fires only when the exact target minor is installed

validate hard-asserts pyVer != expectedPyMinor (pipeline.go:540), so the ==<minor>.* narrowing is load-bearing — but a machine with 3.13 and no 3.12 gets nothing. Worth stating the expected hit-rate somewhere, since that's the feature's whole value.


Smaller

  • The success message names no interpreter. The summary prints Python 3.12 from Resolved.PythonVersion (the target minor), so a user can't tell whether they got managed 3.12.13 or /usr/bin/python3.12 3.12.11 — exactly what they need when deciding whether to trust the venv, or when an import later breaks on a distro build. The path is local-only; it needn't enter the Result to be printed.
  • variant is ignored. uv's records carry "variant":"default"; a freethreaded build of the same version ties and can win on iteration order. Moot under Bump github.com/databrickslabs/terraform-provider-databricks from 0.5.7 to 0.5.8 #1.
  • Cancelling during uv sync after a fallback prints ✗ Setup canceled. followed by Python download failed; used a compatible installed Python instead.
  • appendProcessStderr (uv.go:500) hand-rolls tree-walking with raw type assertions where the file previously used errors.AsType. Only needed because of the join in Bump gopkg.in/ini.v1 from 1.66.4 to 1.66.5 #2.
  • Three byte-identical return PythonSelection{}, uvFailure(ErrPythonInstall, errors.Join(installErr, err), …) lines (172/176/180).
  • pipeline.go:504 forwards selection.Executable unvalidated — empty gives uv sync --python "" as an opaque E_PROVISION.
  • JSON unmarshal failures don't include what uv actually printed.
  • pythonResolution is inserted mid-struct in Result, whose doc says field order matches the spec's schema — worth confirming the spec puts it between venvPath and phases.

Fixed since last round

Changelog fragment; process.WithStub replacing runFn, with the UV_INDEX_URL bridge now covered; doc comments on every new symbol; Provision rationale restored; empty path now continues; ctx-cancel early return (uv.go:171); managed tie-break now live (system group walked first); chosen interpreter logged (188); fallback rendered on failure too; PythonResolutionUnspecified added.

Verified, not problems

uv warnings go to stderr so stdout stays parseable; version_parts/path keys are correct for 0.11.21; uv python list excludes venvs; the untrusted remote requires-python lands in a single cpython@-prefixed argv element, so no flag or shell injection.

@rugpanov
rugpanov force-pushed the python-install-fallback branch from dd348fd to a04b1f2 Compare September 2, 2026 08:06
@rugpanov

rugpanov commented Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @anton-107 — all blocking findings are addressed:

Two judgment calls — tell me if you'd rather go the other way:

Re-requesting your review.

@rugpanov
rugpanov force-pushed the python-install-fallback branch 2 times, most recently from 8eedff0 to 83f657b Compare September 2, 2026 09:46
@anton-107

Copy link
Copy Markdown
Contributor

Re-checked at 83f657b4. go vet and go test ./libs/localenv/... ./cmd/environments/... pass.

All five blocking findings from the first round are resolved — thanks. Not blocking on anything except one regression below, which is small.

One note on the thread first: your 08:29 comment describes code that 83f657b4 then deleted. The requires-python normalization, "managed group walked last", and the empty-path continue aren't at HEAD — the commit replaced the whole discovery path with a single uv python find --system --no-python-downloads 'cpython@==3.12.*', which dissolves #1, #6, and #8 rather than fixing them as described. Same outcome, and simpler than what I suggested, but worth correcting in the thread so the next reader can tell what shipped. #3 is fixed for real: runFn is gone, and the new process.WithStub tests cover both the python find argv and the UV_INDEX_URL bridge. You also took --system, which is the mandatory part.


Good to fix: #11 has regressed, and #14 depends on it

appendProcessStderr was deleted and the new uvStderr helper is applied only to installErr, never to findErr. So the fallback command's stderr is unreachable again. Measured via a stub carrying real stderr on both calls:

uv python install 3.12 failed: error: Failed to download cpython-3.12.13
  Caused by: Connection refused
no compatible installed Python found: uv python install 3.12: exit status 2
uv python find --system --no-python-downloads cpython@==3.12.*: exit status 2

error: unexpected argument '--no-python-downloads' found appears nowhere — ProcessError.Error() is only Command: Err, and errors.Join never reaches Stderr. Output is byte-identical when nothing is merely installed, so those two cases still can't be told apart.

This matters mainly because it's the premise for declining #14:

With #11 fixed, the list command's stderr now surfaces, so an older uv fails with a readable error instead of silently.

That isn't true at HEAD, so #14 is undecided rather than declined. Either the stderr becomes reachable (and I'm happy to leave #14 waved off) or the version gate comes back — but not neither. Applying uvStderr to findErr is enough; I'd take that and drop #14.

Secondary, same block: the : from PipelineError.Error() lands after "no compatible installed Python found", attaching both joined causes to that sentence. The separate-sentences split your new comment describes doesn't survive rendering.

Minor / nits

  • pkgmanager.go:37-39 still justifies the field's categoricality against telemetry, but buildSetupLocalEvent still never reads PythonResolution. The comment asserts something the code doesn't do — delete the sentence or wire the field. (Wiring it would answer "how often does the download fail in the field", which is the most valuable thing this PR could measure, and settles the scope question below.)
  • No real-uv coverage. provision_integration_test.go is untouched; it's the only thing that would catch the request-grammar / flag-support class of bug. Forceable hermetically: UV_PYTHON_INSTALL_MIRROR=https://127.0.0.1:9/nope uv python install 3.13. Discretionary.
  • Patch-level exclusions are now ignored. cpython@==<minor>.* drops the constraint entirely, so a !=3.12.4 artifact can select 3.12.4 and fail at uv sync. This is the lossiness I flagged as a caveat in round 1 — but no published artifact uses a patch-level !=, and it fails loudly rather than building a wrong venv. Fine as-is; noting it so the trade-off is on the record.
  • Scope: the fallback still fires only when the exact target minor is installed (validate hard-asserts the minor). A machine with 3.13 and no 3.12 gets nothing. Worth a sentence somewhere on expected hit rate.
  • The success summary names no interpreterPython 3.12 is the target minor, so a user can't tell whether they got managed 3.12.13 or /usr/bin/python3.12, which is exactly what they need to decide whether to trust the venv.
  • Cancelling during uv sync after a fallback still prints ✗ Setup canceled. followed by Python download failed; used a compatible installed Python instead.
  • pythonResolution is inserted mid-struct in Result, whose doc says field order matches the spec's schema — worth confirming the spec puts it between venvPath and phases.
  • Bump github.com/databrickslabs/terraform-provider-databricks from 0.5.8 to 0.5.9 #5 (Warnings instead of a new top-level key) not adopted — your call, and defensible.

@rugpanov
rugpanov force-pushed the python-install-fallback branch from 83f657b to 3980a16 Compare September 2, 2026 12:15
rugpanov and others added 6 commits September 2, 2026 14:44
Replace the hand-rolled discovery (`uv python list` for managed and system
interpreters, JSON parsing, and version selection) with a single
`uv python find --system --no-python-downloads cpython@==<minor>.*` call.
uv already applies its own managed-first preference and excludes project venvs,
so this deletes selectInstalledPython, newerInstalledPython, listInstalledPython,
pythonListArgs, fallbackPythonConstraint, and the JSON records they parsed.

Also: log the download failure reason on the fallback-success path, and keep the
download failure and empty-search outcome in separate sentences so the message no
longer reads as if `python install` rejected an argument it never received.

Co-authored-by: Isaac <no-reply@databricks.com>
uv python find exits non-zero when no interpreter matches (verified with
uv 0.12.8), so the empty-stdout branch was dead. Trust the exit code per
the repo convention against speculative fallbacks.

Co-authored-by: Isaac <no-reply@databricks.com>
- Suppress the fallback note on cancellation: after Ctrl-C, "Setup
  canceled" was followed by a contradictory "Python download failed;
  used ... installed Python instead". Now shown only on real failures.
- Fix the combined install+search error message: the two-sentence split
  did not survive PipelineError.Error() (which appends the wrapped cause
  after Msg), gluing the joined causes onto the second sentence. Collapse
  to one attributed line ("... failed: <stderr>; no compatible installed
  Python found either: <stderr>") so each stderr stays attributed and the
  cause suffix reads naturally, matching the uvFailure convention.
- Name the fallback interpreter in the text summary (new json:"-" Result
  field) so the user can tell which interpreter backs the venv; the
  structured result stays categorical via PythonResolution.

Co-authored-by: Isaac <no-reply@databricks.com>
@rugpanov
rugpanov force-pushed the python-install-fallback branch from 55618e1 to 4a78597 Compare September 2, 2026 12:56

@anton-107 anton-107 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-checked at 4a79597. go vet, gofmt, go test ./libs/localenv/... ./cmd/environments/..., and go test ./acceptance -run TestAccept/localenv all pass. uv claims below are measured against a real uv 0.11.21.

The one thing I asked for last round is fixed. Nothing blocking from me. This round I also drove the feature end-to-end against real uv rather than only reading it — details first, because they change what's left to say.


Verified end-to-end against real uv

Pointed the download mirror at a dead address and ran the actual code path (EnsurePythonProvisionValidate), no network needed:

UV_PYTHON_INSTALL_MIRROR=https://127.0.0.1:9/nope

EnsurePython("3.13")                      → {Executable:/usr/bin/python3.13 Resolution:installed_fallback}
Provision(dir, "/usr/bin/python3.13")     → nil
Validate(dir)                             → {PythonMinor:3.13}

So the request grammar, --system, the path hand-off into uv sync, and the minor validate then hard-asserts all line up on a real uv. (PostProvision failed only because this sandbox has no route to pypi.org — unrelated.)

One trap if you turn this into the integration test below: the minor must be one with no uv-managed install, or uv python install exits 0 (idempotent "already installed") and the fallback never fires even with a dead mirror. I tried 3.12 first — uv-managed here — and got uv_install_succeeded. 3.13 is system-only on this box, so it downloads → fails → falls back.

#11 is fixed — confirmed by rendering

Both stderrs are now reachable and attributed. Via a stub carrying real stderr on both calls:

uv python install 3.12 failed: error: Failed to download cpython-3.12.13-linux-x86_64-gnu
  Caused by: Request failed after 3 retries
  Caused by: Connection refused (os error 111); no compatible installed Python found either: error: unexpected argument '--no-python-downloads' found: uv python install 3.12: exit status 2
uv python find --system --no-python-downloads cpython@==3.12.*: exit status 2

The "unsupported flag" and "nothing is installed" cases are now distinguishable, which is what #11 was about. #14 stays waved off, as promised — the premise for declining it now holds.

Residual, cosmetic: the cause tail still misattributes. uv python install 3.12: exit status 2 lands immediately after the find stderr, so it reads as though python install caused unexpected argument '--no-python-downloads'. errors.Join's first element is installErr and PipelineError.Error() appends the whole join after Msg, so the commit message's "the cause suffix reads naturally" doesn't quite survive rendering. Both facts are present and readable, so I wouldn't hold the PR — if you want it clean, joining findErr, installErr (uv.go:127) puts the find command line next to the find clause.

New: the json:"-" invariant isn't actually tested

result_test.go:60 asserts assert.NotContains(t, string(b), "python3.12"), but TestResultEmitsPythonResolutionCategorically never sets PythonInterpreter — so it passes for the wrong reason. Verified by mutation; swap the tag and the whole package stays green:

$ sed -i 's|`json:"-"`|`json:"pythonInterpreter,omitempty"`|' libs/localenv/result.go
$ go test ./libs/localenv/ -run 'TestResultEmitsPythonResolutionCategorically|TestResultOmitsUnknownPythonResolution|TestNewResultEmitsEmptyArraysNotNull'
ok

One line — result.PythonInterpreter = "/usr/bin/python3.12" — makes it real, and it's the assertion guarding the field's whole reason for existing.

New: the stated reason for json:"-" is contradicted by backupPath

result.go:319 — a path would leak machine layout to JSON consumers

backupPath is already an absolute local path in the same object: p.res.BackupPath = filepath.ToSlash(backup) (pipeline.go:480) over filepath.Join(p.ProjectDir, …) (pipeline.go:292), with projectDir = os.Getwd() (cmd/environments/sync.go:144). venvPath is not the counterexample — it's deliberately relative per spec §6.1 and says so at pipeline.go:580.

Withholding the interpreter is still defensible: the extension adopts .venv/bin/python, not the base interpreter, so it doesn't need the path. I'd just state that reason, because the current one invites someone to either "fix" backupPath or re-litigate this field.

Minor: the one debug line that explains the download failure

uv.go:129 inlines uv's multi-line stack into a parenthetical mid-sentence. Real shape, captured from the run above:

msg="uv: Python download failed (error: Failed to install cpython-3.13.14-linux-x86_64-gnu\n  Caused by: Request failed after 3 retries in 5.1s\n  Caused by: Failed to download https://127.0.0.1:9/nope/…\n  Caused by: client error (Connect)\n  Caused by: tcp connect error\n  Caused by: Connection refused (os error 111)); using installed interpreter /usr/bin/python3.13"

And when uv fails without stderr (a SIGKILLed process, say) it degrades to an empty parenthetical:

msg="uv: Python download failed (); using installed interpreter /usr/bin/python3.13"

This is the line that exists specifically to answer "why did the download fail", so it's worth logging the stderr as its own record, or dropping the parens when it's empty.

Withdrawing two of my own findings

  • pipeline.go:509 forwarding selection.Executable unvalidated (raised in rounds 2 and 3) — not reachable, dropping it. I probed uv python find for an exit-0-with-empty-stdout path and there isn't one: no match is rc=2 (error: No interpreter found for CPython ==3.14.* in managed installations or search path), and even a malformed request is rc=2 (No interpreter found for executable name 'cpython@==.*'). A nil findErr always carries a path.
  • variant being ignored — moot. The single uv python find call delegates variant preference to uv, so there's nothing left here to get wrong.

Fixed since last round

  • #11: the find command's stderr is reachable and attributed to its own clause.
  • Cancellation no longer prints the fallback note. Pipeline.Run funnels every ctx.Err() != nil into E_CANCELED (pipeline.go:124-142), and renderInstalledPythonFallback is only reached in the non-canceled branch — so the Ctrl-C-during-uv sync case is covered, not just Ctrl-C during install. TestRenderCanceledOmitsInstalledPythonFallback pins it.
  • The success summary names the interpreter, on the failure path too.
  • pkgmanager.go no longer justifies the field against telemetry.

Still open, all discretionary

  • No real-uv coverage. provision_integration_test.go is untouched; the run at the top of this comment is what it would look like, and the uv-managed-minor trap is what makes it non-obvious to write.
  • Patch-level != exclusions are dropped by cpython@==<minor>.*. On the record: fails loudly at uv sync, and no published artifact uses one.
  • Scope: the fallback still fires only when the exact target minor is installed. A machine with 3.13 and no 3.12 gets nothing, and there's still nothing anywhere on expected hit rate.
  • Telemetry doesn't read PythonResolution — you took the "delete the sentence" option, which is what I offered. Noting only that "how often does the download fail in the field" stays unmeasured.
  • pythonResolution and the new json:"-" field both sit mid-struct in a Result whose doc says field order matches the spec's schema — still not checkable in-repo.
  • #5 (Warnings instead of a new top-level key) not adopted — your call, and defensible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants