ci: parallelise pytest with xdist and drop 3.15 from the blocking matrix - #725
Conversation
21de5a2 to
e975b9e
Compare
|
❌ NEEDS CHANGES — cross-review on Opus of Sonnet-authored work at head |
|
❌ NEEDS CHANGES — cross-review on Opus of Sonnet-authored work; head Own evidence: repo venv 3.14.7 with 1. Blocking —
|
| run | exit | guard message in output |
|---|---|---|
| serial | 4 | full diagnostic — file:line, capture name, the CI command, two named remedies |
-n auto --dist load |
3 | 0 occurrences across 78 lines; INTERNALERROR … dsession.py:217 assert not crashitem names the test as a crashed item |
CI still goes red, so the gate holds as a gate — but whoever trips it gets an xdist assertion instead of the instructions. Cheapest fix that preserves intent: emit the findings to stderr / terminalreporter before raising, so they survive however the controller renders a worker-side collection error.
2. Blocking — the --dist load comment's reason is stale
It asserts, present tense, that load "is the only xdist distribution mode that is safe on this suite today" because loadfile/loadscope "surface a real order-dependent sys.modules leak (#720)". #720 is fixed in this PR's own merge base 4391dc77b. Measured at this head on the #720 reproduction set (tests/protocols/test_dispatch_registry_unit.py + tests/project, -n 2): loadfile → exit 0, 133 passed; loadscope → exit 0, 133 passed. Keep the pin — pinning the mode is right and load is the measured one — but the reason as written is untrue and will mislead.
Attacked and held
- The pin is unoverridable. No
pytest.ini/setup.cfg/tox.ini;[tool.pytest.ini_options]carries onlytestpaths/python_files, noaddopts. Precedence measured anyway: iniaddopts = --dist loadfile+PYTEST_ADDOPTS=--dist loadscope+ CLI--dist load→ resolveddist='load'; no-CLI control →dist='loadfile', proving the ini was live and genuinely overridden. - xdist does not undermine tests: an unrestored setUpClass module purge leaks sys.modules state across files, blocking --dist loadfile #720's fix — the item nobody had checked.
pytest_sessionstartfires in every worker before collection: 4 workers + controller, distinct pids, each pinningsize=315withpcapkit_present=True, all under module keytests.conftest. The warm-up import runs per worker, so none pins a cold table, and the pin precedes anysetUpClassin each. Safe because the design is per-process and an xdist worker is a process. - Subtest fidelity holds. Deliberate failing subtest: serial and
-n 2 --dist loadboth exit 1 with1 failed, 2 passed, 5 subtests passedand a full traceback. Cosmetic only — ids stringify (i=3→i='3') and subtests render asuin the progress line. compatibility-nightlyis reachable — cron'0 4 * * 6'genuinely exists and theif:matches it. lint.yml is comment-only — YAML parse identical before/after, 0 non-comment diff lines.timeout-minutes: 45untouched on all three pytest jobs, and all three install thetestextra, so there is no-n autowithout xdist present.
Notes, not blockers
- I would not lower the timeout:
--dist loadis non-deterministic, so the tail is wider than the mean; 45 against a ~9 min observed run is the right margin. - Job key reads
compatibility-nightlybut the cron is weekly (Sat 04:00 UTC) — the prose says "weekly" correctly, the key doesn't. With noworkflow_dispatchanywhere in that workflow, 3.15 now cannot be checked on demand at all: up to 7 days' latency and no manual escape hatch. Worth addingworkflow_dispatchand widening the gate. make teststays serial (Pipfilehas nopytest-xdist), so CI now runs a distribution local dev cannot reproduce — notable for a suite whose last three bugs (twelve tests/project/ tests fail withTypeError: type 'ProtocolBase' is not subscriptabledepending on what ran first: the fake-module helpers purge on entry and never restore on exit #660, tests/cli/test_main.py leaves hand-written sys.modules stand-ins behind: 10 errors when paired with test_public_api #688, tests: an unrestored setUpClass module purge leaks sys.modules state across files, blocking --dist loadfile #720) were all order-dependentsys.modulesleaks.--dist loadsplits a class across workers, sosetUpClassruns once per worker holding any of its tests.EndToEndTestCasepurges and reloads there to save ~45s over 92 methods, so the integration job may pay that reload 4×. Unmeasured by me — and an argument forloadscopeon that job specifically, i.e. the mode item 2 forbids.- The
allextra excludestest, so a future job installing.[all]and running pytest would lack xdist. Inert today. - Body is honest on speed: 3.6–3.9× correctly given as the CI expectation, ~7× correctly labelled a 16-core local ceiling, no overstatement. But the one real CI datapoint — 521.79s for 1508 passed / 116 skipped / 5621 subtests on 4 vCPU, with no same-runner serial baseline — appears nowhere on the PR. Worth adding, with its refusal to derive a ratio intact.
- Merge order: ci: parallelise pytest with xdist and drop 3.15 from the blocking matrix #725 first, then ci(unit-tests): stop the integration job re-running the unit tier #727. ci(unit-tests): stop the integration job re-running the unit tier #727 also edits
unit-tests.ymland conflicts with this. ci: parallelise pytest with xdist and drop 3.15 from the blocking matrix #725 is the smaller, already-green edit and it settles the final matrix shape, so rebasing ci(unit-tests): stop the integration job re-running the unit tier #727's partition onto it is mechanical — and a new pytest job that arrives without-n auto --dist loadshows up as a visible inconsistency against three jobs that have it. The reverse order forces re-deciding which partitioned job each flag belongs to, which is the easier thing to get wrong.
❌ NEEDS CHANGES — head e975b9e6e.
e975b9e to
7d1f011
Compare
|
✅ GOOD TO MERGE @ |
|
✅ GOOD TO MERGE @ Supersedes the ❌ NEEDS CHANGES verdict at The gate holds — real violation, guard ACTIVE (
|
| invocation | exit | diagnostic | INTERNALERROR |
crashitem |
|---|---|---|---|---|
| serial | 4 (ERROR:) |
1 | 0 | 0 |
load / loadfile / loadscope / loadgroup / worksteal / each / no / -n auto |
2 (Interrupted:) |
1 | 0 | 0 |
12/12 repeats at -n 4, -n auto, -n 16, worksteal over all of tests/protocols (hundreds of real items to schedule): every one exit 2, no flakiness. No false positives either — a clean selection under -n 4 --dist load is 31 passed, exit 0.
Mechanism — confirmed, and isolated to the one added line
Single-variable A/B, -n 4 --dist load, 5 repeats per arm, only the session.shouldfail assignment differing:
| arm | exit | diagnostic | INTERNALERROR |
crashitem |
|---|---|---|---|---|
| without | 3 | 0 | 39 | 2 — dsession.py:217 assert not crashitem, naming a real scheduled id |
| with | 2 | 1 | 0 | 0 |
Chain is as the comment says: _pytest/main.py:872 modifyitems inside the try:, :879 pytest_collection_finish in the finally:; xdist/remote.py:141-149 copies shouldfail into workeroutput; dsession.py:206-212's for…else breaks past the assert at :217. One refinement — the worker's own shouldfail check (remote.py:207) never runs, since collection raised before pytest_runtestloop, so the entire effect is controller-side. Pre-fix was a genuine race; post-fix the break is unconditional on shouldfail's truthiness, so it cannot fire on any interleaving.
Counts
tests/test_tier_guard_xdist.py— 4 passed with xdist; 2 passed, 2 skipped on the repo venv, skipping forpytest-xdist not installed, not for absent captures.tests/test_tier_guard.py+tests/project/test_module_isolation.py— 31 passed, 15 subtests, serial and under-n 4 --dist load.
The one change I would ask for
>=3 permits xdist 3.0.2–3.5.0, none of which forward shouldfail to the controller at all — there the fix silently reverts to the INTERNALERROR bug, and this PR's own test_xdist_run_reports_the_diagnostic_and_fails goes red rather than skipping, since xdist 3.5 imports fine. The mechanism arrives in 3.6.0 (xdist #1024), which is yanked, so the honest floor is >=3.6.1. It cannot bite this CI (no lockfile, so pip resolves 3.8.0), hence a judgement call and not a blocker. pytest>=8 is safe: the shouldfail setter is identical in 8.0.0 and 9.1.1 and its unset-guard never fires for a non-empty string.
Attacked and held
- Both
e975b9e6eblockers fixed.grep -rn "720" .github/workflows/→ nothing; the soleloadfile/loadscopemention is the future-proofing sentence, which is accurate —xdist/plugin.py:318-320already defaultsdist=loadunder-n, so the pin guards exactly the future default change it claims. CLI beatsPYTEST_ADDOPTSbeats iniaddopts(measured), so it is the strongest pin available. - Serial unchanged: one
conftest.pyin the repo, no hookwrappers/pytest_plugins/entry points, the single call site keyword-only;sessionis a declared hookspec arg (_pytest/hookspec.py:268) and pluggy binds by name. Message string is character-identical to pre-change. hasattr(config, 'workerinput')is pytest core's own idiom (cacheprovider.py:417,stepwise.py:64,junitxml.py:424);xdist/remote.py:424is the only assignment site anywhere.- 3.15 drop is safe: ruleset
23497679is the only gate (branches/main/protection404s), and all 15 required contexts are already reporting on this PR's own run with 2 green. No required context ever came from acontinue-on-errorleg. All three pytest jobs —gateincluded — install.[test], so-n autonever runs without xdist.
Notes, not blockers
Interrupted:vsERROR:framing — content is fully intact (file:line, the CI command, both remedies). But "unfixable from a conftest" is overstated:pytest_keyboard_interruptis a documented conftest hook (hookspec.py:1214) andterminal.py:1011-1023is what writes the! … !banner, so it could be relabelled. Nothing is lost, so I would not spend the code on it — your call.- Unchanged from last review:
make test/Pipfilestill carry no xdist, so CI runs a distribution local dev cannot reproduce;compatibility-nightly's cron is weekly (Sat 04:00 UTC) with noworkflow_dispatch. - lint.yml's "newest non-experimental version in the test matrix" is now vestigial — after this PR nothing in that matrix is experimental. True, just redundant.
✅ GOOD TO MERGE @ 7d1f011cf.
11fc9a6 to
391a1b3
Compare
…blocking matrix Contention, not job duration, was the bottleneck (#703). Changes: - Add pytest-xdist>=3.6.1 (not >=3: 3.0.2-3.5.0 lack the workerinput forwarding the fix below needs, and PyPI has yanked 3.6.0); run `-n auto --dist load` in all three pytest jobs. - Drop Python 3.15 from unit-tests.yml's blocking matrices; its compileall/import check moves to a schedule-only job in python-compatibility.yml. - Fix the tier guard's UsageError diagnostic under xdist (swallowed by a controller race, `assert not crashitem` in dsession.py) by setting session.shouldfail inside a worker before raising. - Close a second race: XdistSubprocessTests' probe module and SuiteIsCleanTests' filesystem scan can land in different xdist workers and overlap. tests/conftest.py adds a readers-writer flock mutex around both, without touching test_tier_guard.py. Verified: test_tier_guard.py and test_tier_guard_xdist.py pass serial and under xdist; the probe/scan race reproduced in 14/32 trials pre-fix, 0/139 post-fix over repeated runs.
391a1b3 to
5ac52f5
Compare
|
✅ GOOD TO MERGE @ |
|
✅ GOOD TO MERGE @ Cross-review on Opus (author: Sonnet). Supersedes the ✅ at
Two things the description undersells. Unmeasured before, now measured — and your call, not mine: on the post-#727 Caveats, none blocking:
✅ GOOD TO MERGE @ |
make pylint,make mypy,make isort)make testpasses, and a test case covers the change — not run per instruction (no full-suite run); see counts belowWhat is the purpose of your pull request?
ci— workflows or build toolingDescription of your pull request and other information
Two of #715's four items, plus three review findings. Rebased onto #727 (main
bf57b4542); itsintegration-job selection rewrite conflicted with this PR's-n auto --dist loadaddition to the same step — resolved by keeping #727'sfixture_tier_paths()selection/error-checking and appending-n auto --dist loadto its finalpytestline.pytest-xdist-n auto --dist loadontest/integration/gate; pinned>=3.6.1(3.0.2-3.5.0 lack theworkerinputforwarding the tier-guard fix needs, and PyPI yanked 3.6.0 itself)Findings fixed:
pytest_collection_modifyitems'sUsageErrorwas swallowed by an xdist controller race (assert not crashitemindsession.py). Now setssession.shouldfailinside a worker before raising.SuiteIsCleanTests(filesystem scan) vsXdistSubprocessTests(writes a real violation to disk) raced under-n auto --dist load— hit on CI,Integration Python 3.13. Fixed with a readers-writerflockmutex intests/conftest.py, without touchingtests/test_tier_guard.py. Note: ci(unit-tests): stop the integration job re-running the unit tier #727 narrowedintegration's selection to 20 entries that no longer include either racing test, so the hazard now lives only intest/gate, both still full-suite under-n auto --dist load— the mutex covers both regardless of selection.Tests:
test_tier_guard.py+test_tier_guard_xdist.py, 42 passed/57 subtests under real xdist (40 passed/2 skipped/57 subtests in the repo's own no-xdist venv). Race reproduced 10/10 and 14/32 with the mutex disabled, 0 failures across 350+ repeated concurrent trials with it enabled, including an independent cross-model review that reran the same toggle experiment.test_module_isolation.py: 6 passed.Related to #715; item 1 (integration partitioning) is done by #727, item 2 (gate dedup) remains.