Regenerate .test_durations, which was blind to a whole module - #140
Closed
amburger66 wants to merge 2 commits into
Closed
Regenerate .test_durations, which was blind to a whole module#140amburger66 wants to merge 2 commits into
amburger66 wants to merge 2 commits into
Conversation
CI balances its eight shards with pytest-split's least_duration algorithm over the committed .test_durations (predicators.yml:31). That file was generated on 6 August and carried 4707 entries with ZERO for anything under code_sim_learning -- a module that arrived with #135. pytest-split gives an unlisted test a fallback duration, so a block of ~33 identically-estimated tests is exactly the input that makes bin-packing swing. The consequence is that adding any test reshuffles which tests share a shard PROCESS, and that regrouping has been surfacing a pre-existing fragility in the NSRT/GNN path: dill raising "cannot pickle '_abc._abc_data'" inside nsrt_learning_approach._learn_nsrts. Measured on a PR adding two tests, its shard 8 held 913 tests against master's 910, with 95 of ~175 file-lines differing in test count -- so test_gnn_action_policy_approach ran in shard 8 in both, behind a substantially different set of tests. The split is deterministic given (test set, .test_durations), which is why that shard failed twice identically rather than flapping. Worth knowing for anyone checking this: shard 8's FILE lists are byte-identical between the two. pytest-split splits at test granularity, so identical file lists prove nothing. Count tests. Now 5641 entries, 153 of them under code_sim_learning. Generated from a full single-process GREEN run at 1d809f3 (1574 passed), and that mattered: the first attempt came back with the 9-failure cluster, and a test that fails fast records the time it took to FAIL. test_oracle_samplers logged 0.369 s on that run against 2.252 s on the green one, which would have under-weighted it in every future split -- baking the problem in while claiming to fix it. Two things this does NOT do. It does not fix the dill fragility; it re-rolls the grouping, and the arrangement it lands on may or may not be green. And the durations come from a developer machine rather than a CI runner: least_duration uses relative weights and machine differences are broadly uniform, so the balance should transfer, but anything with a different CPU/GPU profile locally than on the runner may be mis-weighted.
amburger66
marked this pull request as ready for review
August 18, 2026 16:02
Collaborator
|
ok I'll trust you on this :D |
yichao-liang
approved these changes
Aug 18, 2026
amburger66
enabled auto-merge (squash)
August 18, 2026 18:23
amburger66
disabled auto-merge
August 18, 2026 19:12
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.
Regenerates
.test_durations, which CI uses to balance its eight shards and which had drifted badly out of date.Why
.github/workflows/predicators.yml:31runs:least_durationbin-packs tests into groups using the committed.test_durations. The file inmasterwas generated on 6 August and holds 4707 entries — with zero for anything undercode_sim_learning, a module that arrived with #135. pytest-split assigns unlisted tests a fallback duration, so a block of ~33 identically-estimated tests is exactly the input that makes bin-packing swing.The visible consequence is that adding any test reshuffles which tests share a shard process, and that regrouping has been surfacing a pre-existing fragility in the NSRT/GNN learning path —
dillraisingTypeError: cannot pickle '_abc._abc_data' objectinsidensrt_learning_approach._learn_nsrts.Measured on a PR that adds two tests, comparing its shard 8 against master's:
So
test_gnn_action_policy_approachruns in shard 8 in both, but with a substantially different set of tests preceding it in the same process. Because the split is deterministic given (test set,.test_durations), the same regrouping recurs every run — which is why that shard failed twice identically rather than flapping.Note the file-level view is misleading here: shard 8's file lists are byte-identical between the two. pytest-split splits at test granularity, so identical file lists prove nothing. Count tests.
What this does and does not fix
Does: makes the split reflect the actual test set, so a PR adding tests no longer perturbs grouping out of proportion to what it changed.
Does not: fix the
dillfragility. This re-rolls the grouping; it does not make the NSRT/GNN path robust to what precedes it in a process. That is a real bug inmasterand is worth its own investigation — the reproducible handle is CI's shard 8.Caveats worth reviewing
least_durationcares about relative weights and machine speed differences are broadly uniform, so the balance should transfer — but anything with a different CPU/GPU profile locally than on the runner may be mis-weighted.1d809f3, so no duration here is the time a test took to fail.Unrelated, and still unexplained
There is a separate local symptom that this does not explain and that should not be conflated with it: the full suite run in one process, at a fixed commit, with identical execution order and
PYTHONHASHSEED=0pinned, has produced 9 failures on one run and 0 on the next. Execution order, pytest-randomly, hash randomisation and git hooks are all ruled out as mechanisms. That one is open.