localenv: populate setup-local warnings and durationMs - #6176
Conversation
Approval status: pending
|
Integration test reportCommit: 9d6bd19
8 interesting tests: 4 RECOVERED, 4 SKIP
Top 3 slowest tests (at least 2 minutes):
|
Both fields were declared in the --output json contract but never populated: warnings was always [] and durationMs always 0. They back the ERD's "setup result" metrics (merge-quality warning count + the ~3min duration claim), so the extension had nothing real to report. DECO-27875. - durationMs: add an injectable clock (Pipeline.Now, default time.Now) and stamp wall time across every Run exit path. Tests inject a fixed clock; the acceptance JSON goldens normalize the value via a [DURATION_MS] repl so they stay deterministic. - warnings: emit a documented, closed set of categorical codes from the merge phase — W_REQUIRES_PYTHON_OVERRIDDEN and W_DBCONNECT_PIN_OVERRIDDEN when the user's pins differ from the env's and are replaced, and W_USER_CONSTRAINT_CONFLICT when a [project].dependencies pin is provably disjoint from the env's constraint for that package. Detection is a read-only compare of the pre-merge pyproject against the fetched constraints (detectMergeWarnings); MergeManaged still owns the byte edits. The conflict check is conservative PEP 440 (== / ~= / bounds) and only fires on a provable disjoint, so an ambiguous range is never a false positive — uv remains the real resolver. Warnings fire for both --dry-run and real runs, and only for existing projects (greenfield has nothing of the user's to override). Contract-additive and scoped to libs/localenv; the command is still hidden. DECO-27875 Co-authored-by: Isaac
… input
Follow-up to the review of the setup-local warnings work. Four defects, all in
the advisory/telemetry path (no effect on the merged file or the run's outcome):
- A PEP 735 {include-group = ...} entry in [dependency-groups].dev made the
strict decode fail, and detectMergeWarnings returned no warnings at all — even
though [project] had already parsed and MergeManaged still rewrote all three
managed regions. The user's pins were replaced with no advisory, and the
warning histogram under-counted exactly the projects using a standard uv
feature. The user's file now decodes through userPyprojectTOML, which models
dev as []any and skips non-string entries; the artifact keeps the strict struct
since we control its shape.
- parseClause accepted a single-segment "~=" (e.g. "~=2"), which PEP 440 does
not define. compatibleReleaseContains returns "not contained" for such a base,
and the disjointness callers read that as proof of disjointness — so
overlapping ranges were reported as conflicts ("requests~=2" vs
"requests==2.31.0"). Such a clause is now refused so it stays an unknown range,
restoring the "only provably disjoint" invariant the code documents.
- Duplicate constraint-dependencies entries for one package let the last one win,
making the outcome depend on artifact ordering (["pyarrow<21","pyarrow>=20"]
reported a conflict; the reverse order did not). Entries for a package compose
as a conjunction, so they are now joined and treated as an unknown range.
- Extras were left in the specifier, so "pyarrow[compute]==17.0.0" never parsed
and a real conflict went unreported. Extras select optional features and never
narrow the version range, so they are stripped. Requirements carrying an
environment marker are now skipped instead of half-parsed, since we do not
evaluate markers and the pin may not apply to the resolving interpreter.
Also drops the [project].dependencies field added to pyprojectTOML, which the
new user-side struct supersedes.
Verified by brute-forcing 5184 ordered specifier pairs (including the malformed
single-segment "~=" forms) against a dense version grid: every "disjoint" verdict
is backed by the absence of a witness version satisfying both sides, and ~=
semantics match PEP 440's worked examples via an independent interval model.
Co-authored-by: Isaac
…s-connect pin
Review follow-up. PEP 735 {include-group = "..."} is an indirection, so the
user's pin may live in a group the dev group merely references:
[dependency-groups]
dev = [{include-group = "spark"}]
spark = ["databricks-connect==16.1.0"]
The previous scan only walked dev's own string entries, so it found no pin and
emitted no W_DBCONNECT_PIN_OVERRIDDEN — while MergeManaged still injects the
env's pin into dev, leaving the file with two pins for the same package and no
advisory that one replaced the other. groupRequirements now resolves the
reference chain, normalizing group names per PEP 503 (which PEP 735 also
specifies) and skipping already-visited groups so an include-group cycle
terminates. Decoding dependency-groups as a map is what makes the other groups
reachable; the walk is deterministic despite the map because warning order is
fixed downstream.
Also scopes the environment-marker check to the post-extras remainder rather
than the raw requirement, so it tests only where a marker can appear. Behavior
is unchanged for every input either form decides — a ";" in a url requirement is
still skipped, which costs nothing since a url has no comparable version range.
This closes a false negative that predates the warnings work rather than a
regression from it: the parent commit reported nothing for the same input too.
Co-authored-by: Isaac
54ccf7a to
9d6bd19
Compare
anton-107
left a comment
anton-107
left a comment
There was a problem hiding this comment.
Reviewed the branch locally and verified each point by running it rather than reading. The durationMs half looks correct and self-contained — the defer covers every exit path including the E_CANCELED reclassification. My concerns are all in the warnings half.
Blocking: #1, #2, #5 (inline below). The short version: as shipped, W_USER_CONSTRAINT_CONFLICT will rarely fire on the constraint shape our artifacts actually publish, and W_DBCONNECT_PIN_OVERRIDDEN makes a factually wrong claim in the exact case commit 9d6bd19 was added to handle. Since both back a metric, the failure mode is silent — bad numbers, nothing visibly broken.
Non-blocking: #3, #4, #6, #7 — dead code and comments that describe behavior the code doesn't have. Cheap to fix, worth doing before merge.
None of the fixes are large or architectural; the helpers needed (groupRequirements, compareRelease) are already in the file.
One suggestion: consider splitting. durationMs + the repl + the 5 golden updates is correct and could land now; the warnings work gets its own PR with the blocking items addressed. That also matches the one-change-per-PR rule, since these are two independent contract fields.
Verification: go test ./libs/localenv/ ./cmd/environments/ passes on the branch. For #3 and #4 I confirmed by deleting the code and re-running the suite.
| if strings.TrimSpace(entry) != strings.TrimSpace(c.DatabricksConnect) { | ||
| warnings = append(warnings, Warning{ | ||
| Code: WarnDBConnectPinOverridden, | ||
| Message: fmt.Sprintf("databricks-connect %q was replaced by the environment's %q", strings.TrimSpace(entry), c.DatabricksConnect), |
There was a problem hiding this comment.
BLOCKING (#1) — this message is factually wrong in the case it was written for.
I ran MergeManaged on the exact input from TestDetectMergeWarningsIncludeGroupIndirection, and the merged output is:
dev = ["databricks-connect==17.0.0", {include-group = "spark"}]
spark = ["databricks-connect==16.1.0"]The user's 16.1.0 pin is still there. Nothing was replaced — a second, contradicting pin was inserted alongside it, and uv sync will fail to resolve. 9d6bd19's own commit message concedes this ("the merged file would carry two databricks-connect pins"), but the warning still tells the user their pin was cleanly overridden.
So in this scenario the user gets a reassuring advisory and then a hard resolution failure, while the actual hazard — two pins for one package — has no signal at all. TestDetectMergeWarningsIncludeGroupIndirection asserts the inaccurate wording, so it locks the bug in.
Walking the indirection is the right instinct; the conclusion drawn from it is wrong. This case needs its own code (a duplicate-pin warning), not the override code — they're different conditions with different user actions.
There was a problem hiding this comment.
Confirmed and fixed in #6191. The merged output is exactly as you show — the merge inserts a second pin and leaves the users 16.1.0 in place, so "was replaced" was untrue and hid the resolution failure.
Split into its own condition rather than reworded: W_DBCONNECT_PIN_DUPLICATED, emitted when the pin is reachable only through an include-group. A pin sitting directly in devs own array still reports W_DBCONNECT_PIN_OVERRIDDEN, since that one really is rewritten in place. The distinction is directRequirements vs groupRequirements. The test now asserts the message does not contain "was replaced", so the wording cannot regress.
| } | ||
| } | ||
|
|
||
| warnings = append(warnings, constraintConflicts(p.Project.Dependencies, c.ConstraintDeps)...) |
There was a problem hiding this comment.
BLOCKING (#2) — conflict detection skips dependency groups entirely.
Only [project].dependencies is scanned. I confirmed that pyarrow==17.0.0 in [dependency-groups].dev against an env constraint of pyarrow~=21.0.0 produces zero warnings.
uv applies constraint-dependencies to the whole resolution, so a group pin breaks uv sync exactly like one in [project].dependencies. Given that this command's whole audience is projects with a dev group (that's where the dbconnect pin lives), that's likely where conflicting pins land in practice.
The fix is nearly free: groupRequirements already exists a few lines up for the dbconnect check and just isn't reused here.
There was a problem hiding this comment.
Confirmed and fixed in #6191. constraintConflicts now receives slices.Concat(p.Project.Dependencies, groupRequirements(p.DependencyGroups, devGroup)), so the dev group is scanned including pins behind an include-group. Tests cover a conflicting dev-group pin and one reached by indirection.
| // pair it cannot decide — a "!=", a multi-clause range, an unparseable release — | ||
| // returns false, so an uncertain case is never reported as a conflict. uv remains | ||
| // the real resolver; this only surfaces the obvious clashes as an advisory. | ||
| func rangesDisjoint(userSpec, envSpec string) bool { |
There was a problem hiding this comment.
BLOCKING (#5) — can't decide the constraint shape our own artifacts use.
Verified against the current implementation:
| user | env | provably disjoint? | reported |
|---|---|---|---|
>=20 |
<19 |
yes | false |
>=4 |
<3 |
yes | false |
~=17.0 |
~=21.0 |
yes | false |
Every other localenv fixture publishes bound-style constraints — pyarrow<19, pandas<3 in serverless-json/test.toml. The tell is that the new fixture had to use pyarrow~=21.0.0 for the warning to fire at all: the test was fit to the detector rather than to what the artifacts contain.
Being conservative on genuinely ambiguous ranges is the right call, and I'm not asking for a full PEP 440 solver. But opposite-direction bounds and ~= vs ~= are trivially decidable, and compareRelease is already right here. Since the stated purpose is a merge-quality metric, a detector that misses the common shape under-reports invisibly — the dashboard just shows a low conflict count and nobody can tell it's wrong.
There was a problem hiding this comment.
Confirmed and fixed in #6191. The fixture point is the part that convinced me: every pre-existing fixture publishes pyarrow<19 / pandas<3, and the only ~= entry in the tree is the one I added — so the test was fitted to the detector, exactly as you said.
Rather than adding the two missing operator pairs, the core is now interval arithmetic: each clause converts to a release range with explicit endpoint inclusivity, and disjointness is one endpoint comparison. That decides all three rows in your table plus ~= vs a bound, and it replaced clausesDisjoint, satisfies, and compatibleReleaseContains outright.
One subtlety worth flagging since it is easy to get wrong: I first normalized to half-open ranges by synthesizing a successor, which is unsound — ">3.12" becoming ">=3.12.1" excludes 3.12.0.5, which does satisfy ">3.12" (compareRelease zero-pads). Shrinking an interval turns a real overlap into a false conflict, so inclusivity is tracked per endpoint instead. TestRangesDisjoint now covers the touching-endpoint cases (>=2.0 vs <=2.0 overlaps; >2.0 vs <=2.0 does not).
| case "==": | ||
| // a pins exactly a.rel; disjoint iff that version does not satisfy b. | ||
| return !satisfies(b, a.rel) | ||
| case "~=": |
There was a problem hiding this comment.
Non-blocking (#3) — this branch is dead.
It only fires when b.op == "==", and the swapped call clausesDisjoint(b, a) already decides that pair through satisfies's ~= case, reaching the same compatibleReleaseContains.
I brute-forced all 6400 pairs over 8 operators × 10 releases against a copy with the branch removed: zero divergence, and the package suite passes without it.
Separately: the single-segment ~= guard in parseClause (~line 269) is load-bearing — but it protects the satisfies path, not this branch. Its comment credits "the disjointness callers" via compatibleReleaseContains, which will mislead the next reader once this branch is gone. Worth correcting while you're here.
There was a problem hiding this comment.
Confirmed and fixed in #6191. Verified the same way before changing anything: the swapped clausesDisjoint(b, a) call reaches case "==" → satisfies → the ~= case → the same compatibleReleaseContains, so the branch could never be the deciding path.
Moot now — the interval rewrite for #5 removed clausesDisjoint, satisfies, and compatibleReleaseContains entirely. The parseClause single-segment ~= guard is kept and its comment now explains the real reason: clause.interval builds the ceiling from rel[:len(rel)-1], so a one-segment base would yield an empty prefix and a nonsense bound.
| } | ||
| // Sort by package name (embedded in Message after "dependency ") for stable | ||
| // output; the count and code are what the contract carries, order is cosmetic. | ||
| sortWarningsByMessage(warnings) |
There was a problem hiding this comment.
Non-blocking (#4) — unnecessary, and both comments describe behavior it doesn't have.
The comment says "map iteration and slice order upstream are unstable." They aren't: constraintConflicts iterates userDeps, a []string straight from the TOML decode, in declaration order. envByName is only ever used for lookup, never iterated. Tests pass with the sort removed.
It also doesn't do what it claims. Both this comment and the one above constraintConflicts say "sorted by package name," but it sorts the whole Message string in ASCII order — so "Zeta==1.0.0" sorts before "alpha==1.0.0" (verified). Net effect is discarding the user's declaration order, which is the more useful order for reading warnings, for no determinism gain.
There was a problem hiding this comment.
Confirmed and removed in #6191. Both halves of your point check out: envByName is only ever indexed, never ranged over, and userDeps arrives in declaration order from the TOML decode — so there was no instability to protect against. And it sorted the whole Message, so "Zeta==1.0.0" really did sort before "alpha==1.0.0".
Warnings now come out in the users declaration order, and both comments were rewritten to describe that instead. I did double-check that removing the sort does not expose the map iteration in groupRequirements — that one only indexes its map and walks arrays in order, so it is deterministic on its own.
| // Now returns the current time; it exists so tests can inject a deterministic | ||
| // clock (acceptance goldens would otherwise carry a real, changing durationMs). | ||
| // nil means time.Now — see now(). | ||
| Now func() time.Time |
There was a problem hiding this comment.
Non-blocking (#6) — the stated rationale isn't the real one.
The comment says acceptance goldens "would otherwise carry a real, changing durationMs," but the [DURATION_MS] repl is what makes the goldens deterministic — this field isn't involved. Now is never set anywhere outside pipeline_test.go:276.
So an injectable field on the exported Pipeline struct exists so one test can assert 250, which largely verifies that Sub().Milliseconds() works. Given the repo's stance on speculative flexibility, I'd either drop the field and assert the duration is sane, or keep it and reword the comment to say what it's actually for.
The timing logic itself is right — measuring in Run via defer correctly covers the failure and cancellation paths.
There was a problem hiding this comment.
Confirmed. Now was set only at pipeline_test.go:276, and you are right that the repl — not the clock — is what makes the goldens deterministic, so the stated rationale was wrong.
Took the drop-it option in #6190, given the repo stance on speculative flexibility: an exported field on Pipeline existing for one test did not earn its keep. Timing is now plain time.Now() / time.Since. The replacement test delays the constraint fetch by 25ms and asserts the duration exceeds that and stays within the observed wall time — a >= 0 assertion would have passed against the old hardcoded 0 and proved nothing. A second test covers the preflight-error path.
| [[Repls]] | ||
| # environments setup-local emits a real wall-time durationMs; normalize it so the | ||
| # JSON goldens are deterministic. Runs before the generic numeric repls (Order 10). | ||
| Old = '"durationMs": \d+' |
There was a problem hiding this comment.
Non-blocking (#7) — localenv is the only thing emitting this key, so this fits better in acceptance/localenv/test.toml. Global scope means a future command that emits durationMs gets it silently normalized in its goldens without anyone opting in.
(The Order = 8 choice is correct — it has to beat the Order = 10 numeric repls. Worth keeping that note wherever it lands.)
There was a problem hiding this comment.
Confirmed and moved in #6190 — it now lives in a new acceptance/localenv/test.toml, and the Order = 8 note moved with it, including why it has to beat the root Order = 10 numeric repls.
|
Closing in favor of the split you suggested — the two contract fields are independent and only one of them needed rework:
Each of your seven points reproduced exactly as described. Two were on me in ways worth naming: I had reported #1 as fixed in One limitation to flag on both PRs: the SDK bump on |
3 participants
Changes
Populates the two dead fields in the
environments setup-local --output jsoncontract (both were declared but never set —warningsalways[],durationMsalways0). Scoped to the still-hidden command.durationMs— inject a clock (Pipeline.Now, defaulttime.Now) and stamp wall time on everyRunexit path. Tests use a fixed clock; the acceptance JSON goldens normalize the value via a[DURATION_MS]repl so they stay deterministic.warnings— emit a documented, closed set of categorical codes from the merge phase:W_REQUIRES_PYTHON_OVERRIDDEN/W_DBCONNECT_PIN_OVERRIDDEN— the user's pin differed from the env's and was replaced.W_USER_CONSTRAINT_CONFLICT— a[project].dependenciespin is provably disjoint from the env'sconstraint-dependenciesfor that package (conservative PEP 440:==/~=/ bounds; ambiguous ranges never fire, so no false positives — uv stays the real resolver).Detection is a read-only compare of the pre-merge
pyproject.tomlagainst the fetched constraints (detectMergeWarnings);MergeManagedstill owns the byte edits. Warnings fire for both--dry-runand real runs, and only for existing projects (greenfield has nothing to override).Why
These back the ERD's "setup result" metric group (merge-quality warning count + the ~3-min duration claim). The extension's setup-result telemetry (DECO-27787) had to omit
warningsCountbecause the CLI never emitted real warnings. DECO-27875; unblocks the reopened DECO-27787 follow-up.Tests
detectMergeWarnings(override + conflict + greenfield/constraints-only/unparseable cases) and arangesDisjointtable (exact/compat-release/bounds/!=/multi-clause/wildcard). Pipeline tests: warnings surface in the result, greenfield stays empty, anddurationMsis deterministic under an injected clock.merge-warnings-jsonscenario asserts all three codes through--output json; 5 existing JSON goldens updated for the[DURATION_MS]repl.go test ./libs/localenv/... ./cmd/environments/...+localenv/helpacceptance pass;golangci-lintanddeadcodeclean; darwin +GOOS=windowsbuild.This PR was written by Claude Code.