[VPEX][1/8] Add local-env foundation: result types and env-key mapping#5823
Conversation
Integration test reportCommit: afca77c
8 interesting tests: 4 RECOVERED, 4 SKIP
Top 10 slowest tests (at least 2 minutes):
|
87c371d to
865a2cd
Compare
First of a stacked series adding `databricks local-env python sync`, which provisions a local Python environment matched to a Databricks compute target. The feature lands across small, single-concern PRs; each layer is independently reviewable and adds no user-facing surface until the final PR wires the command in. This PR is the foundation the rest of the stack builds on: - result.go: the result types and the --json / E_* error contract that every phase reports through (Result, PipelineError, ErrorCode, PhaseName, PhaseStatus, Mode, TargetInfo, ResolvedInfo, Plan, Warning), plus the command-path constants (local-env / python / sync) defined once. - envkey.go: mapping a compute target to an environment key and parsing the Python minor from a requires-python specifier. Nothing imports this package yet, so the CLI is unchanged. The unexported filesystem/artifact constants and the canonical phase-order slice live with the pipeline that consumes them (a later PR in the stack). Co-authored-by: Isaac
865a2cd to
22f99d9
Compare
…pecifiers Review of the foundation layer flagged that PythonMinorFromRequires took the first MAJOR.MINOR in the string via first-match regex. For a multi-clause requires-python where the exclusive upper bound comes first — e.g. "<3.13,>=3.10" — it returned 3.13, the version the "<3.13" clause forbids, because PEP 440 clause order is arbitrary. The result feeds PM.EnsurePython, so the tool could target a Python the constraint excludes. Prefer a lower-bound / pinning clause (>=, >, ==, ~=, ===) and only fall back to the first version when none is present. Adds multi-clause test coverage; the prior tests exercised only single-bound specifiers. Co-authored-by: Isaac
…dden version Round-2 review of the foundation layer noted that when a requires-python has no lower-bound/pin clause at all (only upper-bound or exclusion, e.g. "<3.13,!=3.12"), PythonMinorFromRequires fell back to the first number and returned 3.13 — a version the specifier forbids. Such a spec has no floor to install from, so it now errors rather than guessing. A bare "3.12" (no operator) is still accepted as a valid floor. Co-authored-by: Isaac
…ower bound
Round-3 review found two edge cases in the regex-based PythonMinorFromRequires:
with multiple lower bounds (">=3.8,>=3.11") it returned the first (3.8) rather
than the effective floor (3.11), and a bare floor alongside an exclusion
("!=3.11,3.12") was wrongly rejected as having no floor.
Replaced the layered regexes with a small clause parser: split on commas,
classify each clause by operator (>=,>,==,~=,=== and bare = floor; <,<=,!=
never a floor), and return the highest floor. A spec with no floor clause
("<3.13", "!=3.12") still errors. Covers multi-lower-bound, bare-floor +
exclusion, ordering, and whitespace.
Co-authored-by: Isaac
Round-4 review noted PythonMinorFromRequires returned 3.10 for ">3.10", but PEP 440's ">" excludes the entire given release series (neither 3.10 nor any 3.10.x satisfies ">3.10"), so the lowest installable minor is 3.11. The clause parser now bumps the minor by one for a strict ">" bound. Co-authored-by: Isaac
anton-107
left a comment
There was a problem hiding this comment.
Thanks for the clean, dependency-free foundation — easy to review in isolation. Two items I'd like addressed before this lands: one real (if narrow) correctness bug in PythonMinorFromRequires, and one --json contract nit. Details inline.
| if op == ">" { | ||
| minor++ |
There was a problem hiding this comment.
clauseRe captures only MAJOR.MINOR, so the patch component is invisible here. For a bare-minor strict bound like >3.10 this minor++ is correct (PEP 440 excludes all of 3.10.x, as the comment above explains). But for >3.10.5 the effective floor should stay 3.10 — 3.10.6 satisfies the constraint — yet this bumps it to 3.11, selecting the wrong Python version.
It's uncommon in real requires-python, but it's a genuinely incorrect result rather than a rounding choice. Could you either capture the patch in clauseRe and only bump the minor when the patch is absent, or document the limitation explicitly? A >3.10.5 → 3.10 case in TestPythonMinorFromRequires would lock it in.
There was a problem hiding this comment.
Good catch — fixed. clauseRe now captures the patch component and the minor is only bumped for a strict > when the patch is absent, so >3.10.5 correctly stays 3.10 (3.10.6 satisfies it) while a bare >3.10 still yields 3.11. Added >3.10.5 / >3.10.5,<3.13 / >=3.10.2 cases to TestPythonMinorFromRequires. (commit 900518b)
| Phases []PhaseStatus `json:"phases"` | ||
| Warnings []Warning `json:"warnings"` |
There was a problem hiding this comment.
Phases and Warnings have no omitempty and are slices, so a zero-value Result marshals them as "phases": null, "warnings": null rather than []. For a versioned --json contract, null-vs-empty-array is a classic ambiguity that trips up consumers (and golden-file diffs).
If the pipeline (#5828) always initializes both to non-nil before marshalling, this is moot — could you confirm that here, or otherwise guarantee they always emit as []?
There was a problem hiding this comment.
Confirmed and hardened. The pipeline (#5828) always seeds both to non-nil (initialPhases() and []Warning{}), so --json never emits null in practice. To guarantee it at the type level rather than by convention, I added a NewResult() constructor here that seeds both slices, a doc note on the invariant, and a test asserting the JSON emits [] (and that a bare Result{} is the null case to avoid). #5828 now constructs its Result via NewResult(). (commit 900518b)
…json arrays
Two items from review of the foundation layer:
- PythonMinorFromRequires bumped the minor for any strict ">" bound, but a
patch-qualified bound like ">3.10.5" is still satisfied by 3.10.6, so the
floor should stay 3.10 (only a bare ">3.10" excludes the whole 3.10.x
series). clauseRe now captures the patch component and the minor is bumped
only when it is absent. Adds >3.10.5 / >=3.10.2 test cases.
- Result.Phases and Result.Warnings are non-omitempty slices, so a bare
Result{} would marshal them as "null" rather than "[]", an ambiguity in the
--json contract. Added NewResult() which seeds both to empty slices, a doc
note on the invariant, and a test asserting the JSON emits [] not null. The
pipeline (later in the stack) constructs its Result through this.
Co-authored-by: Isaac
|
@anton-107 both items addressed in commit |
anton-107
left a comment
There was a problem hiding this comment.
Both review items are addressed cleanly:
- Patch-qualified strict bound —
clauseRenow captures the patch component and the minor is bumped only for a bare>3.10(not>3.10.5), with>3.10.5/>3.10.5,<3.13/>=3.10.2test cases pinning it. Correct per PEP 440. --jsonarrays —NewResult()seedsphases/warningsto non-nil, a struct doc-note records the invariant, and a test asserts[]rather thannull(with a bare-Result{}sanity check). Confirmed the pipeline constructs through it.
LGTM — thanks for the quick turnaround.
Integration test reportCommit: 4c2d06e
58 interesting tests: 44 FAIL, 6 KNOWN, 6 flaky, 2 SKIP
Top 50 slowest tests (at least 2 minutes):
|
## Why - `local-env` must turn the user's compute selection into a single environment key before it can fetch anything, and the selection can come from several places with a defined precedence. - Isolating resolution behind a narrow seam keeps it testable without a live workspace and keeps SDK details out of the engine. ## What - **`target.go`** — `ResolveTarget` with ordered precedence `--cluster` → `--serverless` → `--job` → bundle target, producing a `TargetInfo` + env key. - Compute lookups go through the narrow `ComputeClient` interface (stubbable in tests). - `ValidateTargetFlags` rejects more than one target flag; `ResolveTarget` runs it up front so a non-Cobra caller can't silently resolve the wrong target. - Classic-compute jobs read the Spark version from the documented first return of `GetJobSparkVersion` (not the recorded-version third return). ## Testing strategy - Unit tests against a stub `ComputeClient` covering each precedence branch, the mutually-exclusive-flags error, and the job classic-compute contract (`target_test.go`). - Gates: `go build`, `go test`, `golangci-lint`, `deadcode`, `gofmt` — all green. - Reviewed with codex to a clean pass. --- ## About this stack This is one of a series of small, stacked PRs that together add the `databricks local-env python sync` command — it provisions a local Python environment (Python version, `databricks-connect` pin, and dependency constraints) matched to a selected Databricks compute target. The work was split from one large branch into single-concern layers so each is independently reviewable; the command is kept hidden until the final PR so nothing is user-visible mid-stack. **Review bottom-up.** Each PR targets the previous one as its base branch, so its diff shows only that layer. | # | PR | What | |---|----|------| | 1 | databricks#5823 | foundation: result types + env-key mapping | | 2 | **databricks#5824 ← you are here** | compute-target resolution | | 3 | databricks#5826 | constraint fetch + offline cache | | 4 | databricks#5827 | formatting-preserving pyproject.toml merge | | 5 | databricks#5828 | six-phase pipeline + detection + package-manager interface | | 6 | databricks#5832 | uv backend + CLI command (registered hidden) | | 7 | databricks#5833 | acceptance tests | | 8 | databricks#5835 | unveil (unhide + help + changelog) | This pull request and its description were written by Isaac.
…icks#5826) ## Why - Once a target resolves to an env key, `local-env` needs the pinned Python version, `databricks-connect` version, and dependency constraints published for that key. - The fetch must degrade gracefully offline and distinguish “this environment isn't published” from “the network is down,” because those call for different user action. - The artifact host must be a Databricks-owned, access-controlled location and must never default to a personal repo — whoever controls the host controls what the CLI installs. ## What - **`constraints.go`** — fetches the per-environment `pyproject.toml`, parses `requires-python`, the `databricks-connect` pin, and `[tool.uv]` `constraint-dependencies`, and caches it on disk. - **Host parameterization** — no host is hardcoded: `RepoConstraintBaseURL` reads the repo (`owner/name`) from the temporary `DATABRICKS_LOCALENV_CONSTRAINT_REPO` env var and builds a `raw.githubusercontent.com/<repo>/main` URL; the built-in default is empty. When unset it returns `""` and `FetchConstraints` reports the missing source as a fetch-phase `E_FETCH` error (so there is no untrusted default, and the failure flows through the normal phase/JSON reporting). Once `databricks/environments` can publish, that becomes the hardcoded default and the env var is no longer required. - Failure classification: **404** → `E_ENV_UNSUPPORTED` (no cache fallback — a distinct non-transient condition); **transport / non-404** → `E_FETCH` with fallback to the last-good cached copy. - Robustness: validate the body (parse + require `requires-python`) **before** caching so a bad 2xx can't poison the cache; atomic cache write (mkdir + temp-file + rename); dedicated `http.Client` with a 30s timeout; body read bounded by `io.LimitReader` at 1 MiB; `databricks-connect` matched by leading package name under PEP 503 normalization (so `Databricks_Connect` matches, `databricks-connectors` does not); cache filename = readable slug + sha256 suffix to prevent collisions. ## Testing strategy - Unit tests with an `httptest` server: 200-parse, 404 → `E_ENV_UNSUPPORTED`, transport failure + cache fallback, missing-`requires-python` rejection, PEP 503 name matching, cache-dir creation, collision-free filenames, oversized-body rejection (`constraints_test.go`). - Host resolution: `TestRepoConstraintBaseURL` (env var → URL, unset → `""`, whitespace treated as unset) and `TestFetchConstraintsNoSourceConfigured` (empty host → `E_FETCH` naming the env var). - Gates: `go build`, `go test`, `golangci-lint`, `deadcode`, `gofmt` — all green. - Reviewed with codex to a clean pass (several fetch/cache edge-case fixes landed from review). --- ## About this stack This is one of a series of small, stacked PRs that together add the `databricks local-env python sync` command — it provisions a local Python environment (Python version, `databricks-connect` pin, and dependency constraints) matched to a selected Databricks compute target. The work was split from one large branch into single-concern layers so each is independently reviewable; the command is kept hidden until the final PR so nothing is user-visible mid-stack. **Review bottom-up.** Each PR targets the previous one as its base branch, so its diff shows only that layer. | # | PR | What | |---|----|------| | 1 | databricks#5823 | foundation: result types + env-key mapping | | 2 | databricks#5824 | compute-target resolution | | 3 | **databricks#5826 ← you are here** | constraint fetch + offline cache | | 4 | databricks#5827 | formatting-preserving pyproject.toml merge | | 5 | databricks#5828 | six-phase pipeline + detection + package-manager interface | | 6 | databricks#5832 | uv backend + CLI command (registered hidden) | | 7 | databricks#5833 | acceptance tests | | 8 | databricks#5835 | unveil (unhide + help + changelog) | This pull request and its description were written by Isaac.
…atabricks#5827) ## Why - `local-env` must apply the resolved Python version and constraints to the user's `pyproject.toml` without disturbing their own content — comments, ordering, formatting, and unrelated config must survive untouched. - Re-running must be safe and idempotent, and a greenfield project needs a sensible file created from scratch. - This is the most intricate logic in the feature, so it lands in its own PR for focused review. ## What - **`merge.go`** — a formatting-preserving merge that rewrites only the env-owned regions (`requires-python`, the `databricks-connect` entry in `[dependency-groups].dev`, and a marker-bracketed managed `[tool.uv]` block) and preserves every other byte incl. CRLF; idempotent. `RenderFreshPyproject` builds a complete managed file for a greenfield project. - Scoping/robustness: managed `constraint-dependencies` nests header-less inside an existing user `[tool.uv]` (never a duplicate header); single- vs multi-line array detection tracks real bracket depth outside strings/comments; the `databricks-connect` rewrite is confined to `dev` and leaves trailing comments alone; `requires-python`'s inline comment is preserved; table-header parsing tolerates inline comments and recognizes `[[array.of.tables]]`. ## Testing strategy - Unit tests that parse the merged output as TOML (not just substring checks), covering idempotency, CRLF preservation, user-key preservation, the duplicate-`[tool.uv]` case, bracket-in-element arrays, sibling-group/comment non-clobbering, and `[[tool.uv.index]]` children (`merge_test.go`). - Gates: `go build`, `go test`, `golangci-lint`, `deadcode`, `gofmt` — all green. - Reviewed with codex to a clean pass (multiple TOML-corruption edge cases were caught and fixed). --- ## About this stack This is one of a series of small, stacked PRs that together add the `databricks local-env python sync` command — it provisions a local Python environment (Python version, `databricks-connect` pin, and dependency constraints) matched to a selected Databricks compute target. The work was split from one large branch into single-concern layers so each is independently reviewable; the command is kept hidden until the final PR so nothing is user-visible mid-stack. **Review bottom-up.** Each PR targets the previous one as its base branch, so its diff shows only that layer. | # | PR | What | |---|----|------| | 1 | databricks#5823 | foundation: result types + env-key mapping | | 2 | databricks#5824 | compute-target resolution | | 3 | databricks#5826 | constraint fetch + offline cache | | 4 | **databricks#5827 ← you are here** | formatting-preserving pyproject.toml merge | | 5 | databricks#5828 | six-phase pipeline + detection + package-manager interface | | 6 | databricks#5832 | uv backend + CLI command (registered hidden) | | 7 | databricks#5833 | acceptance tests | | 8 | databricks#5835 | unveil (unhide + help + changelog) | This pull request and its description were written by Isaac.
Why
local-envfeature needs a shared vocabulary before any behavior can be built: the result shape, the error taxonomy, and how a compute target maps to an environment key.unused/deadcode-clean with no consumers yet.What
result.go— the--json/E_*output contract:Result,PipelineError,ErrorCode,PhaseName,PhaseStatus,Mode,TargetInfo,ResolvedInfo,Plan,Warning; plus the command-path constants (local-env/python/sync) defined in one place.envkey.go—EnvKeyForServerless/EnvKeyForSparkVersion/NormalizeServerless, andPythonMinorFromRequires(clause-aware: returns the effective highest lower bound of arequires-python).cmd/, so the CLI is unchanged. Filesystem/artifact constants and the phase-order slice deliberately live with their consumer (PR 5).Testing strategy
result_test.go) and env-key mapping incl. multi-clause / strict->/ no-floorrequires-pythoncases (envkey_test.go).go build,go test,golangci-lint,deadcode,gofmt— all green.About this stack
This is one of a series of small, stacked PRs that together add the
databricks local-env python synccommand — it provisions a local Python environment (Python version,databricks-connectpin, and dependency constraints) matched to a selected Databricks compute target. The work was split from one large branch into single-concern layers so each is independently reviewable; the command is kept hidden until the final PR so nothing is user-visible mid-stack.Review bottom-up. Each PR targets the previous one as its base branch, so its diff shows only that layer.
This pull request and its description were written by Isaac.