Repository navigation
Conversation
The guard proved the shard selections partition the slow tier, which is not the same as proving every shard runs something. A selection that has emptied out disappears from both sides of that comparison at once: the union still equals the unsharded set, the shards are still pairwise disjoint, every ignored path is still named by somebody, and one runner installs this project for two minutes to run no tests. That case was reachable only by renaming a whole file while the catch-all picked it back up, and it was caught incidentally by pytest's exit code 5 in the shard's own test step. It stops being incidental the moment a shard names a test class rather than a file, which is the next change here: a class is one edit away from holding no slow tests -- every test in it gaining the `smoke` marker is enough -- and nothing about the partition would look wrong. So a fourth invariant, with its own error naming the shard and printing the selection that came back empty, and reported before the install and the model warm rather than behind a suite run. Two supporting changes the class case needs: Invariant 3 now compares file paths. A shard argument may be `<file>::<Class>` and the catch-all's ignore list can only name whole files, so the part before `::` is what the two lists have in common; without that reduction, naming one class of an ignored file would report the file as orphaned. Whether the file's other classes are claimed by anybody is invariant 1's question and it answers it exactly. A failed collection now names the shard. Selecting by class is what makes pytest's exit 4 reachable -- a class that has been renamed or removed is a usage error rather than an empty selection -- and the message should say which leg of the matrix to go and look at.
The four selections were 120 / 109 / 105 / 74 test-seconds, so the heaviest leg was 1.6x the lightest and bound the gate by itself at 152 s of a 165 s run. That leg was one file, tests/test_pipeline_e2e.py, so there was nothing to move off it without splitting inside it. It splits by class: five classes, all of which take their output directory from the job_dir/isolated_input fixtures, so none of them depends on another or on running in file order. Classes rather than node IDs, because a node ID moves the first time somebody parametrizes a test and the selection would rot with nothing looking wrong. Three of them join tests/test_worker_lifecycle.py and two join tests/test_compile_values.py and tests/test_thermo.py, which is the best two-way split available of the eight divisible units: 110 and 111 test-seconds against an unavoidable 109. Four legs, and that is the end of it. The split is bounded by its largest indivisible unit, which is now tests/test_auto3D.py: 21 slow tests as module-level functions, no class to name, so nothing short of node IDs or a new marker divides its 109 s. With ~34 s of fixed setup per shard that is a ~143 s floor on the binding job, and four legs are already within a couple of seconds of it. Five would spend a whole extra runner to arrive at the same number and six the same again -- past four the arithmetic stops being what binds, so the honest reading of the measurement is that the returns do not flatten out, they stop. Leg 2 is untouched for that reason, and the catch-all keeps the smallest share of test time because it alone carries the partition check and the notebook. The ignore list is unchanged: the same five files, now claimed by file on two legs and by class on two. A class is only ever named on a leg that names it explicitly, never on the catch-all: `--ignore=` of a file beats an explicit node ID inside that file, so a class listed there would be dropped silently rather than run. The partition check already refuses the other half of that trap, a selection that collects nothing.
`UV_VERSION` was pinned because an unpinned installer in front of every gate fails on luck rather than on a diff. What the pin did not come with is anything that re-checks it. The proof that uv and pip resolve the same dependency set was produced once, by a throwaway job deleted after it reported, so all that survives of it is prose in a report and three numbers in a comment. A bump re-opens that question, and `--index-strategy` behavior is exactly the kind of thing a resolver release moves. So the retired job comes back, triggered by the bump itself. It builds each extras set twice in one runner -- pip as the independent reference, the pinned uv as the subject -- freezes both with the same pip, because `uv pip freeze` and `pip freeze` disagree about pip and setuptools themselves and that difference says nothing about the resolution, and fails with the differing lines. It also re-asserts the two properties the extra index exists for, a `+cpu` torch and no `nvidia_*` or `triton`, and on the no-aimnet leg asserts that aimnet is absent and unimportable on both sides rather than that the counts match: that leg stands for a job whose whole value is which package it ends up without, and matching counts would say nothing about which distribution left. Detection is on the pinned value read from both workflow files at the pull request's base and at its head, not on a path filter. These files are edited for every sort of reason, so a filter would fire three heavy installs on comment changes while still missing a pull request that touched only the other file. Reading both values also makes the cheap mistake visible for free: the two files are meant to be bumped together, and a pull request that moves one and not the other now fails instead of leaving a silent disagreement. Nothing runs on an ordinary pull request beyond the detection itself, and neither job is a required context. That is load-bearing for the heavy one: it is skipped almost always, and a skipped check reports no verdict, so as a required context it would leave branch protection waiting rather than merging. Both names are distinct from the five required ones, so publish.yml's reduction has nothing of theirs to collapse.
The balance of the matrix above is a claim about seconds, and until now the only place those seconds existed was a report. Apportioning one file's classes from a local run is what that leaves you with, and it gets the direction wrong: the five classes of tests/test_pipeline_e2e.py divide 58/42 on a developer box pinned to four cores and 46/54 on a runner, which was enough to put 25 s on the wrong leg and cost more gate time than the rebalance saved. So each shard prints its own slowest tests. It costs nothing -- pytest already times every test -- and it means the next person to re-balance reads the numbers out of a run instead of guessing them. Top 25 rather than all of them: a split is decided by the heavy tests, and the catch-all collects 2,295.
The seconds in the matrix comment were a local run's class shares scaled to a runner's file total. The first run with `--durations` says what they actually are, and the shipped split holds up: 109 / 102 / 109 / 107 against a mean of 107. What does not hold up is the reasoning written around it. There are two indivisible units, not one. tests/test_auto3D.py at 102 s was already named. The catch-all is the other at 107 s: it is `tests/` minus the named files, so it shrinks only by naming more of them, and the three worth naming are the ones deliberately kept together for the job directories they write into tests/files/. The floor on the binding leg is therefore the catch-all, and the heaviest leg is 2.2 s above it. That changes the answer on shard count rather than confirming it. A fifth leg would divide only the remainder that already sits below the floor, so it could move at most those 2.2 s -- and it would add a fifth runner to a gate that is `max` over all of them. The same catch-all selection, the same 2,295 tests, took 62 s on one run and 107 s on the next, so every extra leg is another chance to draw the runner that sets the gate. Past four, more shards make the expected gate worse.
Three runs put the same leg on top every time, which is not what a balanced split does, and the per-test durations say why. test_compile_values.py is the volatile unit: its torch.compile test was 33 s on one run and 43 s on the next, taking the file from 37 s to 51 s, while test_thermo.py sat at 25.5 and then 26.0. Grouping it with three other middle-weight units left that leg carrying every unit that could grow at once. So test_compile_values.py and test_worker_lifecycle.py trade legs. The volatile file now travels with the two lightest end-to-end classes, and the four units that each move by a second or two share the other leg. Checked against both measured runs rather than one: the legs come out 110 / 108 / 109 / 107 on the first and 101 / 74 / 105 / 82 on the second, where the previous grouping gave 109 / 108 / 109 / 107 and then 79 / 74 / 127 / 82. The partition is unchanged -- 6 + 21 + 20 + 2296 = 2343, and the unfiltered collect-only totals still sum to 2,459 -- and so is the ignore list, since both files were already named.
The version assertion added with the pin check could never pass: `uv --version` prints `uv 0.13.7 (x86_64-unknown-linux-gnu)`, so comparing the whole line against `uv <pin>` fails on the build target, which is not what is being pinned. Exercising the check with a real bump is what surfaced it -- the three legs failed before reaching the freeze comparison they exist for. Second field only, which is the version and nothing else.
This reverts commit 4df505e. The bump existed to make the check above fire on a real pull request, which it did: the detection job read base='0.13.6' head='0.13.7' from both files and turned the heavy legs on, and all three proved uv 0.13.7 resolves what pip does -- 118 / 117 / 106 third-party distributions, identical freezes, torch 2.13.0+cpu, no nvidia_* and no triton, and aimnet absent and unimportable on both sides of the no-aimnet leg. Reverted because raising the pin is not this change's business. Nothing here claims 0.13.7 should be adopted; the point was that a pull request which does raise it now has to show its work. The pin returns to 0.13.6, the version every timing and freeze in the reports was measured with.
The comment claimed the new pairing "balances under both measurements". A third run disagreed, and the reason is worth writing down rather than re-splitting again: each leg is a separate runner and they are not equally fast. The `Install` and model-warm steps are identical work on all four legs, and they vary by about 15% within a run and 10% between runs, which is enough to move any one leg's measured seconds by more than the imbalance being removed. Normalizing the test times by those two steps across seven runs, the binding leg used to carry about 30 s more than an even quarter of the tier and now carries about 15-20 s. That is the real prize. It is also smaller than the spread it sits in: gates were 163-179 s before this branch and 179-198 s on it, and that difference tracks the same runner-speed index rather than the split. So the comment now says that, and it drops the claim it cannot support: with runner speed normalized out, this pairing and the obvious alternative land in the same band, and the volatile torch.compile file travelling with the lightest classes is a tie-break on predictability rather than a measured win.
The per-leg seconds in the matrix comment were unit sums computed under the previous grouping, written before this one had ever run. It has now run three times, and the numbers are different in a way that matters: the leg holding test_worker_lifecycle.py, test_thermo.py and three end-to-end classes measured 126-132 s every time and is the binding leg of the four. The comment claimed the legs "sit level" with the floor. They do not. Which also corrects the case for four legs rather than confirming it. The binding leg is the divisible remainder, so a fifth leg would genuinely take about 20 s off it -- the earlier claim that it "could move almost nothing" was wrong. Four is still the choice, but as a trade that is now written down: that 20 s costs a permanent extra runner on every pull request, and it sits inside the 10-15% runner-speed variation measured on steps that are identical work on every leg, which on a 130 s leg is 13-20 s by itself. So the gate would not reliably show it. Every figure here now comes from the `--durations` output of a run of this split, normalized by the `Install` and model-warm steps where normalized numbers are quoted.
Contributor
Author
|
Superseded by #244 and closed unmerged. Not to be merged. The shard rebalance this branch carries was measured and declined. It halved the #244 carries only the parts that stand on their own, cherry-picked onto a clean This branch is left in place because the measurements are linked from it. They |
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
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.
Closes the two points the CI optimization left open. They are connected: the
first is what makes the second safe.
Full measurements and reasoning:
.superpowers/sdd/ci-rebalance-report.md.1. The shard guard refuses an empty shard
assert_slow_shards_partition.pyproved the selections partition the slow tier,which is not the same as proving every shard runs something. A selection that has
emptied out leaves both sides of that comparison at once — the union still equals
the unsharded set, the shards are still disjoint, every ignored path is still
named — and a runner installs this project for two minutes to run nothing.
That mattered little while shards selected whole files, where pytest's exit 5
caught it incidentally. It stops being incidental the moment a shard names a test
class: a class is one edit away from holding no slow tests. So a fourth
invariant, with its own
::error::naming the shard and printing the selectionthat came back empty, reported before the install and the model warm.
Two supporting changes the class case needs: invariant 3 now compares file paths,
because a shard argument may be
<file>::<Class>while the ignore list can onlyname whole files; and a failed collection now names the shard, since class
selection is what makes pytest's exit 4 reachable.
2. The shards are rebalanced — and the honest result is mixed
tests/test_pipeline_e2e.pywas one leg at 120 test-seconds and bound the gate byitself, so balancing meant splitting inside it. It splits by class — all five
classes take their output directory from the
job_dir/isolated_inputfixtures,so none depends on another or on file order.
Four legs, not five or six. The split is bounded by two indivisible units:
tests/test_auto3D.py(21 slow tests as module-level functions, no class toname) and the catch-all (
tests/minus the named files, which shrinks only bynaming more of them). With the shipped split the remainder legs are already level
with both, so a fifth leg would divide work that is no longer the maximum — and
every leg is an independent draw on runner speed, so a gate that is
maxover Nlegs gets worse as N grows. The same 2,295-test catch-all selection has measured
62 s, 82 s, 94 s, 97 s, 98 s and 107 s across six runs.
What it is worth. Normalizing test times by the
Installand model-warm steps(identical work on every leg), the binding leg used to carry 29.9 s more than an
even quarter of the tier and now carries 17.0 s. That is the whole prize.
What it is not worth. Six runs on this branch gave gates of 193 / 196 / 198 /
186 / 179 / 197 s against a 165-181 s baseline. The gate did not improve. The mean
runner-speed index accounts for ~9 of the ~12% gap, but no run here beat the
baseline's best, and the rebalance cannot be shown to have moved the gate at all.
Point 2 is separable if that trade is not wanted — see concern 1 in the report.
The tier now prints
--durations=25, because the first split was apportioned froma local run and got the direction wrong (58/42 locally against 46/54 on a runner).
3. The uv pin is re-checked when it moves, and only then
UV_VERSIONwas pinned so a resolver release could not fail a build on luck, butnothing re-checked it: the proof that uv and pip resolve the same set was produced
once by a throwaway job that was then deleted.
That job is recovered verbatim from its commit and triggered by the bump itself —
pip as the independent reference, the pinned uv as the subject, both frozen with
the same pip, failing with the differing lines;
+cputorch and nonvidia_*ortritonre-asserted; and on theno-aimnetlegaimnet's absence asserted onboth sides rather than counts compared. Detection is on the pinned value read from
both workflow files at the base and the head, not a path filter, and it also
catches a pull request that bumps one file and not the other.
Exercised with a real bump, which caught a bug in the one assertion I added beyond
the recovered job (
uv --versionprints the build target, so an exact-string matchcould never pass). Fixed, re-run, and all three legs then proved uv 0.13.7
resolves what pip does — 118 / 117 / 106 distributions, exactly the retired job's
figures. The bump is reverted; the pin stays at 0.13.6.
Nothing runs on an ordinary pull request beyond the detection itself. Neither job
is a required context, which is load-bearing for the heavy one: a skipped check
reports no verdict, so as a required context it would leave branch protection
waiting rather than merging.
Unchanged
The aggregator keeps the name
slow tests (NNP integration)byte-for-byte andstill treats
failure,cancelledandskippedas red; shard names staydistinct; markers, schedules,
publish.yml,cache-keepalive.ymland the fiverequired check names are untouched; no
pytest-xdist;UV_VERSIONis 0.13.6 onthe head. Collect-only totals still sum to 2,459 and the slow union is 2,343.