localenv: fall back to installed Python - #6457
Conversation
83ef594 to
3191670
Compare
|
Reviewed the full diff. Two of these I'd treat as blocking; the rest are ordinary review comments. 🔴 Blocking1.
|
| 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:32 — runFn 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:504 — selection.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
left a comment
There was a problem hiding this comment.
Thanks for the detailed review. The following findings need to be addressed before this can merge:
- #1 —
uv.go:167, constraint concatenation silently disabling fallback. Real bug: the accepted bare/==/===requires-pythonforms 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 onlycpython@==<minor>.*is lossy: the validate phase comparesmajor.minoronly, so it will not catch an excluded/capped patch, anduv sync --python <path>would then reject it. Prefer normalizingrequires-pythoninto a valid specifier (trim whitespace + rewrite the bare/exact forms) and keep the==<minor>.*pin. - #2 — missing
.nextchanges/fragment. Required by.agents/rules/changelog.mdfor this user-visible behavior + JSON-contract change. - #3 —
runFnreimplementsprocess.WithStub. Switch to the existing seam; it also gives coverage to the newuv python listargv wiring and theUV_INDEX_URLbridge, 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.managedbranch 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
pathaborts the entire selection.continuepast unusable entries and fail only if none remain, or keep the strict check with a one-line comment documenting the--only-installedinvariant.
Requesting changes on these.
8dbb696 to
dd348fd
Compare
|
Reviewed The mechanism works (verified 1. The whole discovery machinery is one uv flag
That deletes It also settles the managed-vs-patch question from last round the right way for free: uv honors Trade-off, stated honestly: "nothing installed" then surfaces as
2. The joined error mislabels its own causeLast 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
3. The download failure reason is dropped on the success path
4. No real-uv coverage for a real-uv-only feature
5.
|
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.12fromResolved.PythonVersion(the target minor), so a user can't tell whether they got managed 3.12.13 or/usr/bin/python3.123.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 theResultto be printed. variantis 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 syncafter a fallback prints✗ Setup canceled.followed byPython download failed; used a compatible installed Python instead. appendProcessStderr(uv.go:500) hand-rolls tree-walking with raw type assertions where the file previously usederrors.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:504forwardsselection.Executableunvalidated — empty givesuv sync --python ""as an opaqueE_PROVISION.- JSON unmarshal failures don't include what uv actually printed.
pythonResolutionis inserted mid-struct inResult, whose doc says field order matches the spec's schema — worth confirming the spec puts it betweenvenvPathandphases.
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.
dd348fd to
a04b1f2
Compare
|
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. |
8eedff0 to
83f657b
Compare
|
Re-checked at 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 Good to fix: #11 has regressed, and #14 depends on it
This matters mainly because it's the premise for declining #14:
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 Secondary, same block: the Minor / nits
|
83f657b to
3980a16
Compare
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>
55618e1 to
4a78597
Compare
anton-107
left a comment
There was a problem hiding this comment.
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 (EnsurePython → Provision → Validate), 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:509forwardingselection.Executableunvalidated (raised in rounds 2 and 3) — not reachable, dropping it. I probeduv python findfor 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 nilfindErralways carries a path.variantbeing ignored — moot. The singleuv python findcall 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.Runfunnels everyctx.Err() != nilintoE_CANCELED(pipeline.go:124-142), andrenderInstalledPythonFallbackis only reached in the non-canceled branch — so the Ctrl-C-during-uv synccase is covered, not just Ctrl-C during install.TestRenderCanceledOmitsInstalledPythonFallbackpins it. - The success summary names the interpreter, on the failure path too.
pkgmanager.gono longer justifies the field against telemetry.
Still open, all discretionary
- No real-uv coverage.
provision_integration_test.gois 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 bycpython@==<minor>.*. On the record: fails loudly atuv 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. pythonResolutionand the newjson:"-"field both sit mid-struct in aResultwhose doc says field order matches the spec's schema — still not checkable in-repo.- #5 (
Warningsinstead of a new top-level key) not adopted — your call, and defensible.
Changes
uv python installas the first attemptuv python findfor 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 returnsWhy
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/...