Tests: stop xdist workers from deleting each other's ARC project directories - #990
Closed
calvinp0 wants to merge 3 commits into
Closed
Tests: stop xdist workers from deleting each other's ARC project directories#990calvinp0 wants to merge 3 commits into
calvinp0 wants to merge 3 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #990 +/- ##
=======================================
Coverage 64.46% 64.46%
=======================================
Files 119 119
Lines 39636 39641 +5
Branches 10276 10276
=======================================
+ Hits 25550 25555 +5
Misses 11102 11102
Partials 2984 2984
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
calvinp0
force-pushed
the
fix_ts_test_shared_fixture
branch
2 times, most recently
from
August 17, 2026 09:54
713312a to
eb17036
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
calvinp0
force-pushed
the
fix_ts_test_shared_fixture
branch
3 times, most recently
from
August 17, 2026 10:30
020b1a8 to
d643fb2
Compare
`pyproject.toml` runs the suite with `--dist=worksteal`, which distributes
individual tests rather than whole files. Every worker that receives any test of
a class therefore runs that class's `setUpClass` *and* its `tearDownClass`.
`TestTSChecks` builds its fixtures inside two directories under
`ARC_PATH/Projects/`, named `arc_project_for_testing_delete_after_usage4` and
`..._usage5`, and rmtrees both in `tearDownClass`. So the first worker to finish
its share of the class deleted the `freq.out` files that another worker's
still-running `test_compute_rxn_e0` and `test_check_rxn_e0` had just copied
there. Arkane then found no input and left every E0 as None, which surfaced as a
TypeError subtracting a float from None, and as `ts_checks['E0']` being None.
The same two directory names are hard-coded in `scheduler_test.py` and
`species_test.py`, which rmtree them as well, so the collision crosses modules
as much as it crosses workers.
`TestNMD` and `TestPlotter` collide the same way. `TestNMD` builds a Gaussian job
adapter under `Projects/tmp_nmd_project` and rmtrees it, so a worker that
finished its share first removed the tree while another worker's `setUpClass` was
still building into it, raising FileExistsError for every test that worker had
left. `TestPlotter` uses `Projects/arc_project_for_testing_delete_after_usage`,
a sibling of `..._usage4` and `..._usage5` rather than an ancestor, so the three
rmtrees do not nest and each name is scoped on its own; there another worker's
`tearDownClass` removed the `N4H6.yml` copy that
`test_augment_arkane_yml_file_with_mol_repr` had just made.
`TestPlotter` also wrote three outputs into `ARC_TESTING_PATH` itself, alongside
the input data it reads: `bde_report_test.txt`, `irc/rxn_1_irc_animation.out`,
and the `water.log` / `acetylene.log` / `N-Valeric_Acid.log` that
`make_multi_species_output_file` slices out next to its input path. The last
three are asserted present by `test_make_multi_species_output_file` and asserted
absent by `test_delete_multi_species_output_file`, which holds while the two run
in sequence but not while they run concurrently on separate workers. All of these
now go under the worker's own project directory, which `tearDownClass` already
removes wholesale, so its `files_to_remove` list is empty and gone.
Deriving the directory from `PYTEST_XDIST_WORKER` gives each worker its own tree
and leaves serial runs unchanged. The two helpers live in `arc/common.py`
because that module already owns `ARC_PATH` and `ARC_TESTING_PATH`; both are
used directly, one for a project name and one for a path, and the private copy
in `functional/restart_test.py` is folded into them rather than left as a third.
Per-test fixtures were the alternative and were rejected, but not on cost.
`setUpClass` measures 0.21 s, not the four seconds an earlier draft of this
message claimed, so twenty-five per-test fixtures would add about five seconds of
serial suite time rather than ninety; `pytest arc/checks/ts_test.py --durations=0
--durations-min=0.0` charges class setup to the first test's setup phase, and the
4.4 s of the earlier draft was `test_compute_rxn_e0`'s *call* duration. The
reason per-test fixtures do not work is that they can only scope a directory
within the class that declares it, while every collision here is between classes:
`scheduler_test.py` and `species_test.py` rmtree the two names `ts_test.py` uses,
and `TestNMD` and `TestPlotter` each own their tree outright.
Measured with `taskset -c 0-3 python -m pytest <target> -n 6 --dist worksteal`:
arc/checks/ts_test.py before: 2 failed, 23 passed
1 failed, 15 passed, 9 errors
2 failed, 23 passed
2 failed, 16 passed, 7 errors
1 failed, 16 passed, 8 errors
2 failed, 17 passed, 6 errors
after: 25 passed, in all six runs
arc/checks/nmd_test.py before: 14 passed, 10 errors
14 passed, 10 errors
15 passed, 9 errors
after: 24 passed, in all six runs
arc/plotter_test.py before: 1 failed, 8 passed
2 failed, 7 passed
1 failed, 8 passed
after: 9 passed, in all six runs
arc/checks/ before: 51 passed
51 passed
51 passed
51 passed
43 passed, 8 errors
51 passed
49 passed, 2 errors
49 passed, 2 errors
49 passed, 2 errors
after: 51 passed, in all nine runs
A single test file fails far more readily than a whole directory: with 24 or 25
tests to spread over six workers, worksteal splits the class every time, while
`arc/checks/` is red in four runs of nine. Across all 2800 tests of the CI
invocation `pytest arc/ -n auto --dist=worksteal` the chunks are large enough
that a class normally lands on one worker, and that invocation did not reproduce
any of these failures. The full suite is also unchanged before and after this
commit: the same five deterministic `torch_ani_test` failures from a local
environment gap, and the same intermittent
`adapter_test::test_determine_job_status`, which is a separate shared-directory
race not addressed here.
`TestARC` and `TestScale` both delete a fixed project directory under
`ARC_PATH/Projects` from a class-level fixture, and `--dist=worksteal` runs those
fixtures on every worker that receives any test of the class. The deletion is
therefore not paired with the test that created the tree.
`TestARC` rmtrees six project directories in `setUpClass` *and* in
`tearDownClass`, so a worker starting or finishing its slice removes the trees
the other workers are running in. `ARC.__init__` creates the project directory
and then opens `arc.log` inside it, which is where the race lands:
FileNotFoundError: [Errno 2] No such file or directory:
'.../Projects/unit_test_specific_job/arc.log'
FileNotFoundError: [Errno 2] No such file or directory:
'.../Projects/arc_test/arc.log'
FileNotFoundError: [Errno 2] No such file or directory:
'.../Projects/arc_model_chemistry_test/arc.log'
`Projects/test` and `Projects/arc_test` are each built by several tests of the
class as well, and `test_determine_model_chemistry_and_freq_scale_factor` reads
back the `arc.log` that `ARC(project='test')` wrote there. `ARC` derives the
project directory from the project name when no directory is given, so the names
themselves are scoped rather than the paths; the two restart dictionaries that do
pass a path explicitly are scoped through `get_test_project_directory`. The
substring assertion on `arc2.project_directory` still matches, because the worker
ID is appended rather than substituted.
`TestScale` builds `Projects/scaling_factors_arc_testing_delete_after_usage` in
`test_summarize_results` and rmtrees it in `tearDownClass`, so a worker holding
only `test_get_species_list` deletes the `scaling_factors_0.info` file that
`test_summarize_results` asserts on. That window is narrow - the file is written
and read back in consecutive statements - and 20 runs did not catch it; the
directory is scoped because the shape of the exposure is identical, not because
it was observed to fail.
Measured with `taskset -c 0-3 python -m pytest <target> -n 6 --dist worksteal`:
arc/main_test.py before: 11 passed
1 failed, 10 passed
11 passed (x11)
3 failed, 8 passed
11 passed (x8)
after: 11 passed, in all six runs
arc/utils/scale_test.py before: 4 passed, in all twenty runs
after: 4 passed, in all three runs
Serial results are unchanged: 11 passed and 4 passed respectively.
The adapter test modules build their scratch project directory from a fixed path
under `arc/testing/` rather than under `ARC_PATH/Projects`, so they are exposed to
the same `--dist=worksteal` race as the project directories: `setUpClass` runs
once per worker that receives any test of a class, the teardowns rmtree that tree,
and a worker finishing its slice deletes input files out from under another
worker's running test. `adapter_test.py` and `orca_neb_test.py` register the
removal with `addClassCleanup`, `common_test.py`, `gaussian_test.py`,
`molpro_test.py` and `orca_test.py` with `tearDownClass`; all six run per worker.
`common_test.py` and `gaussian_test.py` also happened to pick the same directory
name, `test_GaussianAdapter`, which collides between the two modules however the
tests are distributed. Scoping alone does not separate them, since both would
resolve to the same worker-suffixed path, so `common_test.py`'s two directories
are renamed to `test_GaussianAdapter_common` and `test_MolproAdapter_common`.
The worker suffix comes from `get_test_project_name` in `arc/common.py`, the same
helper the `Projects` directories use, composed with `ARC_TESTING_PATH` at the
point of use. No second reader of `PYTEST_XDIST_WORKER` is introduced.
Measured with `taskset -c 0-3 python -m pytest <the six modules> -n 6 --dist
worksteal`:
before: 3 failed, 62 passed, 9 errors
1 failed, 66 passed, 7 errors
3 failed, 71 passed
6 failed, 68 passed
2 failed, 58 passed, 14 errors
4 failed, 70 passed
after: 74 passed, in all six runs
Serially the same six modules give 74 passed before and after. This also covers
the intermittent `adapter_test::test_determine_job_status`, which reads the PBS
time-limit fixture that `setUpClass` copies into
`test_JobAdapter_ServerTimeLimit`.
calvinp0
force-pushed
the
fix_ts_test_shared_fixture
branch
from
August 22, 2026 06:45
d643fb2 to
d59f5ee
Compare
Member
Author
|
Superseded by #1008, which now carries this work. Same problem, different mechanism. This PR added The helper itself was dropped rather than reshaped: #1008 now covers 26 files across 7 commits, including everything unique to this PR except four files that belong elsewhere: Measured on #1008: The CodeQL alert raised here (unused |
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
2 participants
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.
pyproject.tomlruns the suite with-n auto --dist=workstealand CI uses-n 6. worksteal distributes individual tests, so one class's tests land on several worker processes and every one of those workers runs that class'ssetUpClassandtearDownClass. Any class with a fixed project directory plus a fixture that deletes it is therefore exposed: a worker starting or finishing its slice rmtrees a tree another worker is still testing inside.It does not take two modules sharing a directory name — one class is enough. That is why the fix is broader than the collisions that are visible by grep.
Reproduced on
main, not inferred:FileNotFoundError: .../Projects/unit_test_specific_job/arc.log.TestARCrmtrees six project directories insetUpClassandtearDownClass, andARC.__init__creates the directory and then opensarc.loginside it, so another worker's fixture lands in that window.Three commits, no file touched by more than one
b45ce92aaddsget_test_project_name/get_test_project_directorytoarc/common.py— the module that already ownsARC_PATHandARC_TESTING_PATH— and applies them ints_test,nmd_test,plotter_test,scheduler_test,species_testandfunctional/restart_test, whose private copy of the same logic is folded into the helpers rather than left as a second implementation.46d1effecoversmain_testandscale_test.ARCderives its project directory from the project name when none is given, so the names are scoped rather than the paths.d643fb21covers the adapter modules, whose scratch directories live underarc/testing/rather thanARC_PATH/Projects.common_test.pyandgaussian_test.pyhad additionally both chosentest_GaussianAdapter, which worker scoping alone cannot separate, socommon_test.py's two directories take a_commonsuffix.Measurements
taskset -c 0-3 python -m pytest <target> -n 6 --dist worksteal; per-commit numbers are in the commit messages.main_test.pySerial results are identical before and after (339 passed), so this changes nothing about what the tests assert.
Reuse
arc/common.pyis the module that owns the path constants, and after this change the repository has exactly one production reader ofPYTEST_XDIST_WORKER— inarc/common.py. The adapter modules composeARC_TESTING_PATHwithget_test_project_namerather than re-reading the environment variable or adding a wrapper around a one-lineos.path.join. Searched by behaviour ("scope a test directory per worker", "build a test project path") and for the raw operation (PYTEST_XDIST_WORKER,ARC_PATH, 'Projects',ARC_TESTING_PATH, 'test_) before writing anything.Deliberately not changed
scheduler_test's remaining fixed directories are per-test and cleaned byaddCleanupin the same worker and the same test that created them; every name was checked and none is shared between two tests.common_test's two remaining sites are the helpers' own tests.restart_test's remaining sites already take their name fromget_test_project_name; the'Projects'literal there is just the join.main_test.py'sassertIn('arc_project_for_testing_delete_after_usage', arc2.project_directory)is a substring assertion on a project name, not a directory, and still holds — the worker suffix is appended, not substituted.Unscoped
'Projects'sites in test modules: 46 → 19, with all 19 in the categories above.Follow-up, not in this PR
Roughly ten further adapter test modules —
xtb_test,obabel_test,mockter_test,cfour_test,ase_test,uma_test,pyscf_test,kinbot_test,xtbgsm_test— carry the identical fixed-scratch-directory race and are untouched here to keep this PR reviewable.Separately,
d643fb21also removes a deadarc0 =binding inside awith self.assertRaises(InputError):block inmain_test.py. It is pre-existing onmain; this PR only changed theproject=argument on that line, which was enough for CodeQL to re-present the statement as new (alert 1956,py/unused-local-variable). The object never exists — the constructor raises — so the binding is removed rather than suppressed.