Skip to content

ci: add job-level timeout backstop and pytest-timeout - #309

Merged
jayhesselberth merged 3 commits into
mainfrom
ci-pytest-timeout
Sep 14, 2026
Merged

jayhesselberth merged 3 commits into
mainfrom
ci-pytest-timeout

Conversation

@jayhesselberth

@jayhesselberth jayhesselberth commented Sep 14, 2026

Copy link
Copy Markdown
Member

Summary

  • Add a 20-minute job-level timeout-minutes to ci.yml's test job as a hard backstop behind pytest-timeout's per-test enforcement — it covers hangs during collection or the pre-test install/Rust-build step that pytest itself can't catch.
  • Add pytest-timeout (in the test extra, so CI's uv pip install -e ".[test,rust,onnx]" actually installs it — see "Code review findings" below) with a 120s per-test cap, timeout_method = "thread" since the platform-default signal method (SIGALRM) can't reliably interrupt a hang stuck in a C-level lock — exactly the shape of the fix: unify predict's config resolution and chunk extraction (#269, #268) #308 deadlock.

Prompted by PR #308's "Run tests" job hanging for ~55 minutes with nothing to stop it until a human noticed and cancelled it (gh run cancel). Root cause there was a fork-safety hazard (a multiprocessing.Pool forked after libtorch's thread pool had already initialized) that's already fixed elsewhere — this PR is the systemic safeguard so a different cause with the same symptom gets caught automatically instead of wasting up to an hour of CI compute before a human notices.

How the 120s value was picked

Ran the full suite with --durations=10 on a dedicated 8-core allocation, against the same extras CI installs (test,rust,onnx):

1707 passed, 45 skipped in 322.89s (0:05:22)
21.57s  tests/test_distributed_training.py::test_two_rank_checkpoint_has_the_single_rank_keys
21.14s  tests/test_distributed_training.py::test_two_rank_run_records_its_world_size
16.69s  tests/test_distributed_training.py::test_two_rank_weighted_ce_gradients_match_the_single_rank_gradients
16.21s  tests/test_distributed_training.py::test_batchnorm_still_wraps_on_the_cpu_harness_without_conversion
16.05s  tests/test_distributed_training.py::test_two_rank_gradients_match_the_single_rank_gradients

The slowest real test is ~22s (the two-rank DDP checkpoint/gradient-parity tests in test_distributed_training.py, which spin up a genuine 2-process process group). 120s leaves >5x headroom for a slower/more contended CI runner while still failing well inside the 20-minute job backstop.

timeout_method: left as the explicit "thread" rather than the per-platform default, because on Linux (CI) that default is actually "signal" — which only interrupts the interpreter at the next bytecode boundary, and would not reliably catch a hang stuck in a C-level lock the way #308's fork deadlock was.

Verification

Temporarily added a test with @pytest.mark.timeout(5) + time.sleep(9999), confirmed pytest-timeout's thread method killed it (dumped every thread's stack, exited 1) well before the sleep could complete, then removed the test before committing — it is not part of this diff.

Code review findings (fixed before landing)

/code-review on the diff caught two real bugs the initial version had:

  1. The plugin wasn't actually installed in CI. pytest-timeout was first added via uv add --dev (a uv dependency-group), but CI's install step runs uv pip install -e ".[test,rust,onnx]" — plain PEP 621 extras, which never pull in [dependency-groups]. The whole per-test timeout would have been a silent no-op in CI (just a PytestConfigWarning: Unknown config option), leaving only the job-level backstop. Fixed by moving it to [project.optional-dependencies].test; verified by installing into a fresh venv with CI's exact command and confirming pytest's startup banner lists the plugin (timeout: 120.0s, timeout method: thread).
  2. Three existing hang-regression tests race the new global timeout. test_inference.py::test_parallel_path_alone_writes_tags, test_inference.py::test_a_fork_after_a_waited_shutdown_completes, and test_parallel_prep.py::test_python_backend_real_pool_matches_rust_chunk_set each carry their own subprocess.run(..., timeout=120) as a deliberate hang guard for known fork-deadlock classes (one of them is literally the fix: unify predict's config resolution and chunk extraction (#269, #268) #308 regression test). With the new global 120s timeout, a real recurrence would have the whole pytest process killed via os._exit() at the same 120s mark instead of letting the test's own subprocess.TimeoutExpired produce its purpose-built diagnostic. Marked all three @pytest.mark.timeout(150), documented inline.

Also fixed a smaller finding: .github/actions/pytest-skip-summary couldn't distinguish "pytest finished, nothing skipped" from "pytest was killed mid-run" — a thread-method kill would have shown a falsely reassuring "no skipped/xfailed tests" job summary. It now checks for pytest's final N passed/failed... in X.XXs line before drawing that conclusion.

Full suite re-run after all fixes: 1707 passed, 45 skipped (unchanged), 285.77s.

release.yml

release.yml's test job already carries timeout-minutes: 45 (added previously, alongside its wheel-build jobs). Left it as-is rather than tightening it to 20: it mirrors ci.yml's job but runs far less often (only on version tags), so it's more likely to hit a genuinely cold Swatinem/rust-cache and pay full Rust-compile time on top of the test run, unlike ci.yml's job which runs on every PR push and almost always has a warm cache. It's already a bounded hard backstop rather than the unbounded default GitHub Actions timeout, which was the actual gap this PR closes.

Changelog

changelog.d/309.changed.md (towncrier fragment, not a direct CHANGELOG.md edit — this repo just migrated to towncrier specifically to avoid CHANGELOG.md merge collisions across concurrent PRs).

🤖 Generated with Claude Code

https://claude.ai/code/session_01JJssLRQzDyJFFnruK7SoDu

PR #308 hung ~55 minutes with nothing to stop it; add a 20-minute
job-level timeout plus a 120s per-test pytest-timeout (thread method,
since SIGALRM can't interrupt a C-level lock).
… races

pytest-timeout was declared under [dependency-groups].dev, which CI's
install step never installs (it runs `uv pip install -e ".[test,rust,onnx]"`,
not `uv sync`) -- the whole per-test timeout was a silent no-op in CI.
Move it to the `test` extra instead, verified by simulating CI's exact
install command in a fresh venv.

Also mark the three existing hang-regression tests that carry their own
subprocess.run(timeout=120) (test_inference.py's
test_parallel_path_alone_writes_tags and
test_a_fork_after_a_waited_shutdown_completes, test_parallel_prep.py's
test_python_backend_real_pool_matches_rust_chunk_set) with
@pytest.mark.timeout(150), so their own internal timeout fires and reports
cleanly instead of racing the new global 120s timeout.

And distinguish "pytest finished with nothing to report" from "pytest was
killed mid-run" in the job-summary action, so a future timeout kill doesn't
get a falsely reassuring "no skipped/xfailed tests" summary.
@jayhesselberth
jayhesselberth merged commit 2ab253b into main Sep 14, 2026
3 checks passed
@jayhesselberth
jayhesselberth deleted the ci-pytest-timeout branch September 14, 2026 01:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant