Run cram tests in parallel#1995
Conversation
jameshadfield
force-pushed
the
speed-up-tests
branch
from
b893a2e to
5cff670
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1995 +/- ##
==========================================
- Coverage 72.92% 72.90% -0.02%
==========================================
Files 85 85
Lines 10714 10714
Branches 2096 2096
==========================================
- Hits 7813 7811 -2
- Misses 2535 2537 +2
Partials 366 366 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jameshadfield
force-pushed
the
speed-up-tests
branch
from
5cff670 to
87347b7
Compare
jameshadfield
requested review from
joverlee521 and
victorlin
and removed request for
joverlee521
|
See #1996 for an alternative that Tom proposed a while back. I'll still review this as there are additional features compared to the simple drop-in replacement. |
Thanks! At the least, the last 4 commits of this PR should go in (or equivelents of them), as they're each solid improvements. As for the parallel runner itself, I prefer the approach of this PR (python, using techniques we now use in Augur code itself) to a perl+bash+parallel implementation. |
victorlin
left a comment
victorlin
left a comment
There was a problem hiding this comment.
I don't feel strongly about implementation detail, but I prefer the interface of our custom script as a drop-in replacement (e.g. 4177bcb). If we use the custom script in CI we should also use it for local tests, so my suggestions below assume that will be the case.
jameshadfield
force-pushed
the
speed-up-tests
branch
from
87347b7 to
9632d8f
Compare
See #1994 for context, and an explanation for the hardcoded SLOW_TESTS Lots of tests use cram's $TMP directory to create files which seems like a collision risk, as noted in <#1176 (comment)> Note that the name of the $TMP directory is misleading - although it is temporary, it is shared across all tests so you'll have to explicitly remove files at the end of each test to avoid affecting other tests. The initial directory of each test is a unique directory within $TMP. however here we run every test file in a separate cram process (`subprocess.run(["cram", ..., str(test_file)])`) and thus $TMP is unique per test in this parallel setup. Over time it'd probably still be nice to change our usage of $TMP to $CRAMTMP/$TESTFILE, but that's not needed here. Code written entirely by Claude Opus 4.6
`augur tree` creates IQ-TREE intermediate files (.log, .iqtree.log, -delim.fasta, .treefile, etc.) next to the input alignment file. When tests pointed at the shared data/ directory, parallel runs would collide on those intermediates. All code by Claude Opus 4.6
These tests all ran in the source directory (the setup script used `pushd "$TESTDIR"`) which posed a collision risk if they can now run in parallel. They now run in a (unique) $CRAMTMP/$TESTFILE folder (the cram default). Fix by Claude opus 4.6
Missing `--output-node-data` arguments were causing a node-data JSON to be written based off the input alignment path, i.e. tests/functional/refine/data/aligned.node_data.json
The previous messaging ("default: None") implied that no tree/node-data
JSON would be created, which isn't the case.
jameshadfield
force-pushed
the
speed-up-tests
branch
from
9632d8f to
74484d5
Compare
|
CI failures are due to rabies (ingest) and both unrelated to this PR and observed in multiple PRs this morning. Merging this PR now to speed up tests in Augur, but as per dev-chat last week this runner script may be replaced by prysk/pytest shortly |
2 participants
See #1994 for context. We see typical speedups from 22min to ~5min for test jobs:
But others failed, and the failures were stochastic. At leasttests/functional/tree/cram/iqtree-model-auto.tfailed once. My hunch is that some of the tests aren't correctly using the tmp directoriescramprovides, and thus certain jobs running at the same time can fail.A number of follow-up commits make all our (cram) tests parallel-safe