ci: run the NVHPC container lanes without MPI - #1822
Conversation
Every failure these lanes have produced has been an MPI test. post_process segfaults under mpirun on the GitHub-hosted runners -- "3D -> 2 MPI Ranks" (CE232828) and "MPI Consistency -> 3D -> Viscous" (0090B316) -- while the same tests pass on the GNU and Intel lanes and on the self-hosted clusters that run MPI at scale. The harness already skips ppn>1 cases when built without MPI (toolchain/mfc/test/test.py), and both failing tests are ppn=2, so --no-mpi drops exactly those and keeps the rest of --test-all. That is what these lanes are for: compile coverage across 15 NVHPC releases, where breaks have reached CI before. MPI runtime coverage is unaffected elsewhere. Only the cpu lanes change; the gpu lanes are build-only and never ran tests.
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Adjusts the NVHPC CPU CI lanes to avoid running MPI tests inside the NVHPC container on GitHub-hosted runners, where MPI execution has been intermittently crashing, while preserving the intended compiler-coverage signal of these lanes.
Changes:
- Add
--no-mpito NVHPC CPU build--dry-runinvocation. - Add
--no-mpito NVHPC CPU test invocation to skipppn>1MPI cases. - Add inline workflow commentary documenting the rationale for disabling MPI in these lanes.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| # --no-mpi: every failure these lanes have produced has been an MPI test. | ||
| # post_process segfaults under mpirun on the GitHub-hosted runners -- | ||
| # "3D -> 2 MPI Ranks" and "MPI Consistency -> 3D -> Viscous" -- while the | ||
| # same tests pass everywhere else, including the self-hosted clusters that | ||
| # actually run MPI at scale. The harness skips ppn>1 cases without MPI | ||
| # (test.py), so this drops exactly those and keeps the rest of --test-all, | ||
| # which is what these lanes exist for: compile coverage across 15 NVHPC | ||
| # releases. MPI runtime coverage stays on the GNU, Intel and self-hosted | ||
| # lanes. |
| docker exec nvhpc bash -c ' | ||
| source /etc/nvhpc-env.sh | ||
| /bin/bash mfc.sh test -v --dry-run -j $(nproc) --test-all | ||
| /bin/bash mfc.sh test -v --dry-run -j $(nproc) --test-all --no-mpi |
| source /etc/nvhpc-env.sh | ||
| ulimit -s unlimited || ulimit -s 65536 || true | ||
| /bin/bash mfc.sh test -v --max-attempts 3 -j $(nproc) --test-all | ||
| /bin/bash mfc.sh test -v --max-attempts 3 -j $(nproc) --test-all --no-mpi |
Addresses review on MFlowCode#1822: --test-all --no-mpi was repeated in the build and test steps, so the two could drift. They now come from MFC_NVHPC_TEST_FLAGS, passed into the long-lived container at docker run so it expands inside the container, where these commands are evaluated. Divergence here is not cosmetic: a no-MPI build tested with MPI would run ppn>1 cases against a binary that has none. Also trims the rationale comment to the essentials and points at the PR for the incident detail.
|
Both taken, in Duplicated flags — real hazard, not just style: if the build and test steps drifted, a no-MPI build would be tested with MPI and One detail worth recording, since the obvious implementation would not have worked: a job-level YAML anchors were the other suggestion, but GitHub Actions does not support them in workflow files. Comment length — fair, trimmed from nine lines to the essentials with the incident detail left in this PR. I kept the one non-obvious consequence inline (why the two steps must agree), since that is the thing a future editor could break without noticing. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1822 +/- ##
=======================================
Coverage 62.26% 62.26%
=======================================
Files 84 84
Lines 21558 21558
Branches 3188 3195 +7
=======================================
Hits 13423 13423
Misses 5937 5937
Partials 2198 2198 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Every failure the NVHPC container lanes have produced has been an MPI test.
post_processsegfaults undermpirunon the GitHub-hosted runners — exit 139, thenh5dumpcannot openp0/0.silobecause the file was never written. Observed on:3D -> 2 MPI RanksCE232828MPI Consistency -> 3D -> Viscous0090B316It is intermittent, spans NVHPC 24.9 / 24.11 / 25.11, and the build always succeeds — the crash is at run time. The same tests pass on the GNU and Intel lanes and on the self-hosted clusters that run MPI at scale, so this is specific to running MPI in these containers on GitHub's runners.
The change
Add
--no-mpito the NVHPC cpu build and test steps. The harness already skipsppn > 1cases when built without MPI (toolchain/mfc/test/test.py), and both failing tests areppn=2, so this drops exactly those and keeps the rest of--test-all.That preserves what these lanes exist for: compile coverage across 15 NVHPC releases, where breaks have reached CI before (the
GPU_DECLAREordering break and two ICEs). MPI runtime coverage is unaffected — the GNU, Intel and self-hosted lanes all still run it.Only the
cpulanes change; thegpulanes are build-only and never ran tests.What this does not do
It does not diagnose the segfault. A bisect against #1762 was inconclusive — it did not reproduce locally under NVHPC 24.1, which is not one of the affected versions, so that result says nothing either way. If the crash is a real
post_processbug rather than runner flakiness, this hides it on these lanes; it would still be caught by the GNU, Intel and self-hosted MPI lanes, which exercise the same code paths.