Add a --package override CLI argument - #3
Merged
Merged
Conversation
A change deep in the dependency chain — the `gitMinimal` or `python3Packages.setuptools` bumps npb had nothing to offer on — puts more attrs in the changed set than a review machine can build, which made npb unusable for exactly the reviews its durable store should help most with (#2). So the changed set is now filterable, like nixpkgs-review's: `-p`/`--package` restricts a review to given attrs, `-P`/`--skip-package` drops given ones. Attrs are matched **exactly** — no subtrees, no globs, no prefixes. That follows the house style rather than cutting a corner: nowhere in npb's Rust is an attr path anything but an opaque key, and where structure is genuinely needed it comes from the evaluator, as nix-eval-jobs' pre-split `attrPath` or a `lib.attrByPath (lib.splitString ".")` inside a generated expression. A filter is a poor place to break that, since the string it would parse is ambiguous: the full-set eval stores nix-eval-jobs' *quoted* attr, so a dotted name (`rubyPackages."http_parser.rb"`, 34 such attrs in one aarch64-linux eval) and a dotted path look alike. Subtree matching stays additive if someone asks for `-P python3Packages`. The filter runs once, over the diff, before the `tests` expansion — which is what makes tests come along for free. That phase expands only what survives, so `-p git` reviews `git` and every `git.tests.*` row without the filter knowing what a test row is called, `-P git` reviews neither, and no package's tests are enumerated except the selected ones — the expensive part of the phase, and most of what `-P` buys. One predicate, one place it runs; the cost is that `-P` can't drop an individual test row, since that row doesn't exist yet when the filter runs. Two invariants the placement protects. It filters the changed set, never the eval: a partial eval cached under a `(tree, system, profile)` key would poison every later diff with phantom "removed" packages, so a cold filtered review still pays both full evals and skips only what is keyed on the changed set. And exclusion removes a target, not a dependency: a `-P`'d package a surviving target needs is still built inside that closure, and its observation is drvpath-keyed like any other, so a later run without the flag finds it already decided. Filtering narrows a report; it never discards knowledge. The report discloses the filter unfolded above the sections, and the reproduction command echoes both flags — without them it would reproduce a different report. The prose for that disclosure, for `--help`, and for the two README sections is left as `TODO(samestep)` placeholders, since none of that text is mine to write. Assisted-by: Claude:opus-5
|
I just ran with the following command as part of testing NixOS/nixpkgs#545428: I've copied the output below. The vast majority of packages specified on the command line aren't appearing in the results...
|
Owner
Author
|
Oh I see. With the semantics you want, we'd need another table in the SQLite database. I'll see what I can do. |
The first cut of this filtered the changed set by attr name, and the first person to try it got two rows back from eighteen `-p` attrs (#3). Nothing was broken: a whole-set walk only descends into attrsets marked `recurseForDerivations`, and nixpkgs marks exactly two of the python sets, so `python3Packages.dirty-equals` — which is literally the same derivation as the `python314Packages` one the report did show — is in no changed set, and neither is `python311Packages.dirty-equals`, which evaluates fine and really was a rebuild of the PR under test. Measured on one cached eval: 11305 rows under `python313Packages`, 11304 under `python314Packages`, zero under `python3Packages`, `python311Packages`, `python312Packages`, `python315Packages` or any `pypy*Packages`. A name-matching filter cannot reach any of them. nixpkgs-review had the near half of the answer: with `-p` it evaluates the named attrs and intersects the changed set on drvpath, dying when one lands outside. Keying on attr *names* was the mistake — drvpath is npb's founding decision — and evaluating the named attrs goes one better than intersecting: it can review attrs no changed set contains. So `-p` and `-P` become two states of one `Coverage`, exclusive by construction, answering different questions: - `-P` still narrows a delta. Same changed set from two whole-set evals, minus the attrs named. - `-p` is a selection. npb resolves each named attr on both trees through the same `lib.attrByPath` selector the instantiate phase uses — so attr-path splitting stays on the Nix side and the Rust keeps treating attrs as opaque keys — pairs the sides into rows, and skips the whole-set eval entirely. A cold selector run is now sub-second to seconds where a delta review pays minutes for two ~114k-attr walks. Every named attr gets a row, including the two a diff can't produce: equal drvs on both sides (`-p hello` on an unrelated PR reports ✅→✅, verified end to end, rather than nothing at all) and nothing on either side, which is how a typo reports itself as ➖→➖ with no separate diagnostic. A named attr must be a derivation: a `recurseForDerivations` set comes back with a multi-element `attrPath` whose first element is what was asked for, so it's refused without parsing anything. Resolutions are cached in a new additive `sel_drv` table, and that is not an optimization. A selection writes no eval file — a partial eval under a `(tree, system, profile)` key is the one thing that poisons every later diff — so without somewhere to put them, every re-run would re-import nixpkgs, exactly the cost `drvs_needing_instantiation` exists to avoid, and the near-instant re-run npb promises would be gone. The table holds the same trichotomy the eval-file format does (a drv; no drv but it threw; absent), and needs no completeness marker because every read is a lookup by exact attr. Measured: 0.70s to resolve a fresh attr on both trees, 0.10s once cached, and 0.72s for the full warm re-run of a real `-p hello` report. One consequence recorded rather than papered over: a selector key has no eval file, so `--clean` — whose budget is the eval-file corpus — doesn't reach its rows. The rows are tiny, keyed, and re-derivable in seconds, and a dated eviction stays additive if that ever stops being true. Assisted-by: Claude:opus-5
A narrowed review announced itself twice: once in an unfolded block above the sections, and again in the reproduction command that already echoes `-p`/`-P` along with every other flag that shapes what a report contains. The first was the odd one out — `--no-tests` and the `--allow-*` profile narrow a report just as much and have always disclosed themselves through the repro command alone. For a selection there's a second reason it was redundant: every named attr gets a row, so an attr that resolved to nothing is already visible as `➖ → ➖` rather than as a silent omission the reader needs warning about. Also removes the two report strings that were waiting on prose, and de-globs a `pypy*Packages` shorthand in DESIGN that could be misread as syntax `-p` accepts — it matches attrs exactly, with no globbing of any kind. Assisted-by: Claude:opus-5
A three-system selector run where one system's attrs were all cached showed that system as an empty `0 / 0` group, stuck blue while its `resolve` phase line sat yellow after the run had finished. Both are the same cause: a group with no items gets no shard, and a group that never gets a shard is never marked running or done, so it stays WAIT — and the parent rollup reads a waiting child as "still running" (`live::eff_state`). `instantiate_prepare` filters its empty requests out for exactly this reason and `reveal_system_tests` never creates the leaf at all; `resolve` was the one fan-out that built a node per side unconditionally. It now selects its groups through `live_requests`, so a fully-cached side contributes no node, a side cached on one tree only shows the other tree, and an all-cached run still short-circuits before the phase exists. Only a *mixed* cache state could show this — every side cold, or every side warm, both look right — which is why the local tests missed it: they were all one or the other. Assisted-by: Claude:opus-5
Owner
Author
|
@me-and how's this?
|
|
Ah, this is amazing, thank you! I think I'd expected it to also pick up, say, |
A selection reported every attr it was named, whether or not the change touched it, while the tests those attrs dragged in were diffed like anything else. That inconsistency was the visible symptom; the cause was a wrong idea of what `-p` is for. Its value is **reach** — it evaluates attrs directly, so it can review ones a whole-set walk never enumerates, like `python311Packages.dirty-equals` in a set nixpkgs doesn't mark for recursion — and reach is entirely separable from reporting non-changes. The attr that prompted all this *did* differ across the two trees; nothing about fixing that case required listing unaffected attrs. nixpkgs-review draws the line in the same place, and harder: a `-p` attr its rebuild set doesn't contain is a `die`, not a row. So the resolved attrs are diffed, which is what `evalfile::changed_set` already does structurally for a delta — this isn't a new rule, it's the existing one applied where the selection path had skipped it. `⏩→⏩` and `➖→➖` go back to being unreachable in both modes, and an unchanged named attr never reaches the `tests` phase, exactly as it wouldn't have in a delta. `coverage.only()` is now consulted in exactly one place, the switch that picks how candidate attrs are found; everything downstream treats the two modes identically. A `-p` that resolves to nothing anywhere is then just no rows. Making that fatal was tried and backed out: it can't be stated per system, since a Linux-only package reviewed on Darwin resolves to nothing there and is perfectly legitimate, so the condition needs a quantifier over every side of every `--system` — and a rule that awkward to state is a rule about the wrong thing. npb is a diff; the absence of rows is the answer, and the report's reproduction command records what was asked for. Verified against the case that started this: `-p python311 -p python311Packages.dirty-equals` drops `python311` (identical on both sides, tests not even enumerated) and keeps the dirty-equals row plus its changed `tests.pytest`. Assisted-by: Claude:opus-5
`-p` can name a `tests` attr directly — that is the point of it, since a test
whose package didn't change is exactly what a changed set can't reach — but
doing so puts a test derivation in the changed set, and the `tests` phase then
enumerates *that* derivation's `passthru.tests`. For the
`overrideAttrs`-with-`doCheck` idiom those resolve straight back to itself, so
`-p python313Packages.pydantic-core.tests.pytest` produced a second row,
`…tests.pytest.tests.pytest`, with the identical drv on each side. Same
derivation, so the report's alias collapsing printed the pair on one line:
- `x.tests.pytest` = `x.tests.pytest.tests.pytest`
Correct, and silly. New with this branch: a whole-set walk can't produce such a
row, because it doesn't descend into `passthru.tests`, and the phase's input is
computed before test rows are folded in, so a delta's test rows are never
themselves expanded.
A test whose derivation *is* its package's, on both sides, is that package under
a second name: no fact npb doesn't already have, since facts are keyed on
drvpath, and the report was only printing the alias. So drop it. `test_drv`
already stores each row's `pkg_attr`, so this needs no attr-path parsing —
`tests_drvs_for` just carries the package along and the fold compares
derivations. Both sides must match: a test sharing one side's derivation with
its package but not the other's is a real difference, as is a one-sided test.
The redundant enumeration still happens — recognising a named attr as a test
before evaluating it would mean reading structure out of an attr path, which npb
doesn't do. It costs one evaluation of an attr the user explicitly asked for.
Assisted-by: Claude:opus-5
Owner
Author
|
Ah... great question. So, For this particular example, nothing in for attr in python313Packages.pydantic-core python313Packages.pydantic-core.tests.pytest; do
echo "--- $attr"
for rev in ed5ca467b977^ ed5ca467b977; do
nix eval --impure --raw --expr \
"with import (builtins.fetchGit { url = ./.; rev = \"$(git rev-parse $rev)\"; }) {}; $attr.drvPath"
echo " <- $rev"
done
doneOne nice thing about Separately, the report I posted above is also inconsistent on this topic, since it includes |
The issue asked for `-p` and `-P` together for parity with nixpkgs-review, on the guess that implementing one would make the other free. Asked directly, the requester had never used `-P` there and couldn't construct a case for it that `-p` wouldn't cover — and the guess didn't hold up anyway: the two answer different questions, one filtering a delta and one replacing it, which is why they had to be made mutually exclusive. Since a shipped flag is one npb is committed to keeping working forever (§1), the cheapest time to drop an unwanted one is before it ships. So `Coverage` loses `Except` and `keeps`, the delta path loses its filtering pass, and the whole thing gets smaller: two states instead of three, one accessor instead of two, and no exclusivity rule for the CLI to enforce. `-p` also loses its `conflicts_with = "clean"`, which had been carried over from before `--clean` became `exclusive = true` and was the last flag still spelling that out by hand. DESIGN keeps a short note on why `-P` existed and why it went, since "parity with nixpkgs-review" will come up again. Assisted-by: Claude:opus-5
samestep
changed the title
-p / -P--package override CLI argument
That does require enumerating all the tests and passing them explicitly, though. In my ideal world, passing Failing that, it would be good to be able to pass (I do wonder about actually making it possible to check for all changed tests; I could believe there would be value in having that as an option that could be enabled with another command line switch, for when the eval-time tradeoff is worthwhile, but I expect that's a completely separate enhancement!)
I can see the new behaviour is more consistent, but I think I preferred the old! If I'm passing I do want to say this is all nitpicking at the moment; the new function you've added is fantastic from my perspective, and will make some of my Nixpkgs contributions significantly easier. You definitely don't have to indulge my every whim! |
Owner
Author
|
Sorry for the delay! Just as a quick update, I thought more about it and I've decided I agree with you on pretty much all these points :) Hopefully I'll have time to finish this PR tomorrow. |
`-p` was doing two jobs: choosing which attrs to review, and quietly imposing a different rule on them once chosen. Named attrs were resolved and then *diffed*, so an attr that turned out identical on both sides vanished — and with it any chance of its `passthru.tests` being enumerated, since the `tests` phase expands whatever the changed set holds. Both halves of that were wrong, per the person who asked for the flag (#3): - Naming an attr is a question — "I think this might have moved; tell me it didn't" — and a diff can't answer it. The confirmation is the point. - Naming a package should pull in its tests the way a changed package's are pulled in, without having to enumerate and name each one. So `-p` now replaces exactly one step and nothing downstream. npb picks the attrs to review, adds their `passthru.tests` unless `--no-tests`, builds, and reports; `-p` replaces the *first* step, the way `--head` replaces the working-tree guess without changing what happens to the revision it names. A selection reports every attr it named, and pairs its test rows instead of diffing them, since those tests were given with the package. A delta is untouched: there a row is a change, and an unchanged test isn't one. Two consequences worth naming. `⏩→⏩` and `➖→➖` become reachable in a selection (§8) — the latter is how a misspelled attr shows itself, which is the second reason not to make that an error. And a selection now builds the tests of a package the change didn't touch: `-p python311` reports the interpreter plus its thirteen tests, twelve ✅→✅ and one 🚫→🚫 that was invisible before precisely because it is unaffected. Assisted-by: Claude:opus-5
samestep
marked this pull request as ready for review
August 29, 2026 11:59
Owner
Author
|
OK @me-and this should be ready now! Let me know if there's anything else before I merge this. |
|
This seems perfect, thank you so much! Output log example
|
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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
Resolves #2.