Uh oh!
There was an error while loading. Please reload this page.
localenv: populate setup-local durationMs - #6190
Merged
Merged
Conversation
durationMs was declared in the --output json contract but always emitted as 0, so the extension had no CLI-side latency figure to report (it backs the ERD's ~3-min setup claim). Stamp it from a defer in Pipeline.Run, which covers every exit path: success, a phase failure, and the E_CANCELED reclassification. The acceptance JSON goldens would otherwise carry a real, changing value, so a [DURATION_MS] repl normalizes it. It lives in acceptance/localenv/test.toml rather than the root: localenv is the only command emitting the key, and a global repl would silently normalize it for any future command without anyone opting in. Order = 8 beats the root's Order = 10 numeric repls, which would otherwise rewrite the digits first. Splitting this out from the warnings half of the same contract, which needs further work — they are independent fields and the durationMs change stands on its own. DECO-27875 Co-authored-by: Isaac
This was referenced Aug 6, 2026
Collaborator
Integration test reportCommit: 5860bfb
8 interesting tests: 4 RECOVERED, 4 SKIP
Top 3 slowest tests (at least 2 minutes):
|
TestPipelineReportsDurationOnFailure only asserted DurationMs <= elapsed. An unset field is 0, which satisfies that bound trivially, so the test passed with the deferred stamp in Run deleted — it protected nothing on exactly the path it was named for. Fail after the delayed constraint fetch instead of at preflight. A preflight usage error returns near-instantly, so its duration truncates to 0 and no non-trivial lower bound exists; a 500 with an empty cache fails at the fetch phase with E_FETCH, after the run has done measurable work. Both duration tests now fail when the defer is removed. The 25ms delay and its server move to a shared const and helper, since both tests now need them. Co-authored-by: Isaac
anton-107
approved these changes
Aug 7, 2026
rugpanov added a commit
that referenced
this pull request
Aug 7, 2026
Address review on #6191. The override warning was deciding for itself which databricks-connect pin the merge rewrites, using rules that did not match MergeManaged's. Where they disagreed the warning claimed a replacement that never happened — the same false claim the W_DBCONNECT_PIN_DUPLICATED split was added to remove, reached through a different door. MergeManaged now reports the requirement it rewrote in place, and the detector consumes that instead of re-deriving it. Anything the merge does not report as replaced is, by definition, still in the file beside the managed pin, so the retained set falls out by subtraction. A single-quoted element, a top-level dotted `dependency-groups.dev` key and a `Dev` group are all left in place by the merge and now report DUPLICATED rather than OVERRIDDEN. Deciding this from the returned `regions` instead does not work: regionDatabricksConnect is present for a pure insert into an empty dev group, where nothing was overridden, and absent when the first of two pins already matches, where a stale pin survives. It answers whether the region changed, not whether a pin was rewritten. W_DBCONNECT_PIN_DUPLICATED is now gated on the two pins being provably disjoint. Two pins are only a problem when nothing satisfies both; ">=16" beside "~=17.2.0" resolves at 17.2.x, so reporting it sent the user after a conflict that does not exist. Also from the same review: - Conflicts are scanned across every requirement uv locks — [project].dependencies, the optional-dependency extras, and all dependency groups. uv applies constraint-dependencies to the whole resolution, so a pin in a non-dev group fails `uv sync` identically; scanning only dev stayed silent on it. - The user's file decodes through loosely-typed containers. BurntSushi reports a type mismatch on one key as a whole-document error, so a group sub-table ([dependency-groups.docs], the PDM style) dropped every warning, including those that never read it. - Group iteration is sorted, so colliding keys cannot let map order pick which warning is reported. - One conflict per requirement, so a pin declared in several places does not inflate the code histogram consumers build from warnings[]. - The pin the merge replaces is out of scope for the conflict scan; retained pins stay in. - Messages are present tense, since the same detection runs under --dry-run. Corrects the claim that the published artifacts use bound shapes: serverless-v4 and -v5 are entirely "~=" (204/204 and 240/240), so the acceptance fixture was the only source of "pyarrow<19". The interval model still handles both; the justification was wrong, not the design. Adds a "~=" conflict case alongside the bound one. Restores the merge-warnings acceptance scenarios in both --json and text form. They are the only end-to-end coverage of the warnings[] contract, and the generated golden carries "durationMs": 0 — it needs #6190's repl to land first. Co-authored-by: Isaac
github-merge-queueBot
removed this pull request from the merge queue due to failed status checks
Aug 7, 2026
Uh oh!
There was an error while loading. Please reload this page.
github-merge-queueBot
removed this pull request from the merge queue due to failed status checks
Aug 7, 2026
Uh oh!
There was an error while loading. Please reload this page.
github-merge-queueBot
removed this pull request from the merge queue due to failed status checks
Aug 7, 2026
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
rugpanov added a commit
that referenced
this pull request
Aug 10, 2026
#6190 landed, so durationMs is now measured rather than hardcoded to 0. Its acceptance repl normalizes the value, and the merge-warnings-json golden is regenerated to use [DURATION_MS] — the dependency anton-107 flagged on the review. pipeline_test.go conflicted only because both sides appended tests at the same point; all five are kept.
rugpanov added a commit
that referenced
this pull request
Aug 10, 2026
#6190 landed, so durationMs is now measured rather than hardcoded to 0. Its acceptance repl normalizes the value, and the merge-warnings-json golden is regenerated to use [DURATION_MS] — the dependency anton-107 flagged on the review. pipeline_test.go conflicted only because both sides appended tests at the same point; all five are kept.
eng-dev-ecosystem-bot
commented
Aug 10, 2026
Collaborator
Integration test reportCommit: f924d58
20 interesting tests: 6 flaky, 6 FAIL, 5 RECOVERED, 2 SKIP, 1 KNOWN
Top 50 slowest tests (at least 2 minutes):
|
yolocs pushed a commit
to yolocs/dbcli
that referenced
this pull request
Aug 11, 2026
## Changes Populates `warnings[]` in the `environments setup-local --output json` contract. It was declared but always empty, so the extension had no merge-quality signal (DECO-27787 had to omit `warningsCount`). Detection is a read-only compare of the pre-merge `pyproject.toml` against the fetched constraints (`detectMergeWarnings`); `MergeManaged` still owns the byte edits. Warnings fire for both `--dry-run` and real runs, and only for existing projects. **Codes:** - `W_REQUIRES_PYTHON_OVERRIDDEN` — the user's `requires-python` was replaced. - `W_DBCONNECT_PIN_OVERRIDDEN` — a `databricks-connect` pin in the dev group's own array was replaced. - `W_DBCONNECT_PIN_DUPLICATED` — the pin is reachable only through a PEP 735 `include-group`, which `MergeManaged` does not rewrite, so the env's pin is inserted *alongside* it. That leaves two pins for one package and `uv` cannot resolve. - `W_USER_CONSTRAINT_CONFLICT` — a user pin is provably outside the env's constraint for that package. ## Addresses review on databricks#6176 This is the `warnings` half of databricks#6176, split out per @anton-107's suggestion (the `durationMs` half is databricks#6190). All seven review items are addressed: - **#1 (blocking)** — the indirect-pin case no longer claims "was replaced", which was factually untrue: the merge inserts a second pin rather than replacing anything. It now has its own code, `W_DBCONNECT_PIN_DUPLICATED`, since the user action differs (reconcile two pins by hand vs. nothing to do). - **databricks#2 (blocking)** — conflicts are now scanned in the dev group as well as `[project].dependencies`, following `include-group` references. `uv` applies `constraint-dependencies` to the whole resolution, so a group pin breaks `uv sync` identically. - **databricks#5 (blocking)** — disjointness is now decided by interval arithmetic over release ranges rather than a per-operator-pair table, so it decides the shapes the artifacts actually publish: opposite-direction bounds (`pyarrow<19` vs `>=20`), `~=` vs `~=`, and `~=` vs a bound. The main conflict test now uses `pyarrow<19` / `pandas<3` — matching the existing fixtures — instead of the `~=` shape that suited the old detector. - **databricks#3** — the dead `~=` branch is gone; the interval model replaced `clausesDisjoint`, `satisfies`, and `compatibleReleaseContains` entirely. The `parseClause` guard comment no longer cites a removed function. - **databricks#4** — `sortWarningsByMessage` removed. Warnings now come out in the user's declaration order; nothing upstream was ever unstable (`envByName` is only indexed, never iterated). - **databricks#6, databricks#7** — in databricks#6190. Inclusivity is tracked explicitly rather than normalizing to half-open ranges: releases have no successor, so turning `">3.12"` into `">=3.12.1"` would exclude `3.12.0.5`, which *does* satisfy it — and shrinking an interval can turn a real overlap into a false conflict. Anything not provably disjoint (`!=`, multi-clause, unparseable) still yields nothing; `uv` remains the real resolver. ## Tests `TestRangesDisjoint` gains the bound-vs-bound, `~=` vs `~=`, and endpoint-touching cases (`>=2.0` vs `<=2.0` overlaps on the shared version; `>2.0` vs `<=2.0` does not). New tests cover the duplicated-vs-overridden distinction, conflicts in the dev group and behind an `include-group`, and `include-group` cycles. **Not included:** the `merge-warnings-json` acceptance scenario from databricks#6176. Its golden contains a byte-exact `diff` field that has to be generated by running the suite, which I could not do — the SDK bump on main is undownloadable in my environment. Happy to add it in a follow-up, or if a reviewer regenerates goldens. _This pull request was written by Isaac._
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes
Populates
durationMsin theenvironments setup-local --output jsoncontract. It was declared but never set (always0), so the extension had no CLI-side latency figure to report.deferinPipeline.Run, so every exit path is covered: success, a phase failure, and theE_CANCELEDreclassification added in localenv: reap uv subprocesses on SIGINT/SIGTERM instead of orphaning them #6107.[DURATION_MS]repl normalizes it. It lives inacceptance/localenv/test.tomlrather than the root, since localenv is the only command emitting the key — a global repl would silently normalize it for any future command without anyone opting in.Order = 8beats the root'sOrder = 10numeric repls, which would otherwise rewrite the digits first.Why
Backs the ERD's "setup result" metric group (the ~3-min duration claim). DECO-27875.
Split from #6176
#6176 populated both dead fields of this contract (
durationMsandwarnings). Review found thewarningshalf needs substantive work — the conflict detector can't decide the bound-style constraints our artifacts actually publish, and one warning made a factually wrong claim. Those are independent contract fields, so per the one-change-per-PR rule this half lands on its own and the warnings work gets its own PR.Tests
TestPipelineReportsDurationdelays the constraint fetch by 25ms and asserts the duration exceeds it and stays within the run's observed wall time. A>= 0assertion would have passed against the old hardcoded0, so it would prove nothing.TestPipelineReportsDurationOnFailurecovers the preflight-error path, whereRunreturns before any phase executes.Note: the injectable
Pipeline.Nowclock from #6176 is dropped. It was exported API set by exactly one test, and the repl — not the clock — is what makes the goldens deterministic.This pull request was written by Isaac.