Skip to content

tests: Stop spawned MPI ranks regenerating the reference norms - #3072

Open
ggorman wants to merge 1 commit into
mainfrom
fix-mpi-norms-race
Open

ggorman wants to merge 1 commit into
mainfrom
fix-mpi-norms-race

Conversation

@ggorman

@ggorman ggorman commented Oct 17, 2026

Copy link
Copy Markdown
Contributor

gen_serial_norms in tests/test_mpi.py runs at import time and regenerates norms{1,2,3}.npy whenever the stored day differs from today. The ranks spawned by the parallel marker (mpiexec -n 4 python -m pytest <test>) import the module again, so a session that starts just before 00:00 UTC and reaches TestIsotropicAcoustic after midnight makes all four ranks regenerate the "serial" reference at once, under MPI. Their collectives pair up out of step and whichever rank writes last fixes a garbage reference for the rest of the session.

This hit three unrelated PRs on 2026-10-16 (pytest-mpi runs 38979222207, 38979245108, 38979251087): test_adjoint_F[(4, *)-1] failed five times each with norm(u) = 1577.6036 (correct) against Eu of 9.0e153, 1563.19 and 4.9e108, identical within a job. Every passing job that night had its session start and its adjoint tests on the same UTC date.

Changes:

  • Spawned ranks never regenerate: they use the file written by the parent pytest process even if the day rolled over. The DEVITO_PYTEST_FLAG environment variable that conftest.py already sets for them is the marker (MPI is not initialised yet at import time, so COMM_WORLD cannot be used there).
  • The parent writes the file to a private temporary name and renames it into place, so a concurrent reader, e.g. another xdist worker, never sees a partial file.

Verification on main d7f53a3 plus this patch:

  • Ranks launched by hand (DEVITO_PYTEST_FLAG=1 DEVITO_MPI=basic mpiexec -n 4 ...) against a norms file backdated by one day: on main all four ranks regenerate it; with this patch the file is untouched and test_adjoint_F[(4, basic)-1] passes, three runs each.
  • pytest -n 8 tests/test_mpi.py -k test_adjoint_codegen from a clean directory, eight workers regenerating at once: 2 passed, three complete norms files, no leftover temporaries.
  • pytest tests/test_mpi.py -k TestIsotropicAcoustic through the parallel marker: 19 passed in 11 min.

🤖 Generated with Claude Code

@ggorman ggorman added testing MPI mpi-related CI continuous integration labels Oct 17, 2026
@ggorman
ggorman requested a review from JDBetteridge October 17, 2026 11:35
@codecov

codecov Bot commented Oct 17, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 77.77778% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.23%. Comparing base (d7f53a3) to head (6690ce7).

Files with missing lines Patch % Lines
tests/test_mpi.py 77.77% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3072      +/-   ##
==========================================
- Coverage   84.23%   84.23%   -0.01%     
==========================================
  Files         258      258              
  Lines       57070    57076       +6     
  Branches     4874     4874              
==========================================
+ Hits        48074    48079       +5     
- Misses       8169     8170       +1     
  Partials      827      827              
Flag Coverage Δ
pytest-gpu-aomp-amdgpuX 68.51% <ø> (+0.01%) ⬆️
pytest-gpu-gcc- 79.00% <77.77%> (+<0.01%) ⬆️
pytest-gpu-icx- 78.96% <77.77%> (+0.03%) ⬆️
pytest-gpu-nvc-nvidiaX 69.08% <ø> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI continuous integration MPI mpi-related testing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant