Commit 9245a7b
fix(test): tear down LAMMPS before MPI.Finalize() in mpirun test runners (deepmodeling#5455)
## Summary
- Adds an explicit `del lammps` before `MPI.Finalize()` in all four
mpirun-driven LAMMPS test runners (`run_mpi_pair_deepmd.py`,
`run_mpi_pair_deepmd_spin.py`, `run_mpi_pair_deepmd_dpa3_pt2.py`,
`run_mpi_pair_deepmd_spin_dpa3_pt2.py`).
- Fixes a teardown-order race that intermittently manifests as
**subprocess exit code 136 (SIGFPE)** for
`test_pair_deepmd_mpi_dpa3_spin_empty_subdomain` on the GitHub Actions
CUDA runner image.
## Background
Recent CI runs on multiple unrelated PRs (deepmodeling#5446, deepmodeling#5450) hit the
identical failure signature:
```
short test summary info ============================
FAILED source/lmp/tests/test_lammps_spin_dpa3_pt2.py::test_pair_deepmd_mpi_dpa3_spin_empty_subdomain
- subprocess.CalledProcessError: ... returned non-zero exit status 136.
```
- Reproduces ~1 in 5 runs on the GitHub Actions CUDA image
(`nvidia/cuda:12.9.1-cudnn-devel-ubuntu22.04`).
- **Does not reproduce on a V100 Bohrium dev box** — 60/60 consecutive
passes.
So it's a pre-existing flake, not caused by either of the recent PRs.
## Root cause (empirically confirmed)
The runner ends with:
```python
forces_global = lammps.lmp.gather_atoms(...)
...
MPI.Finalize()
```
`lammps` is still alive when `MPI.Finalize()` returns. Python then
garbage-collects it during interpreter shutdown, which triggers
`LAMMPS::~LAMMPS` → `Finish::end()` → **`MPI_Allreduce`** for timing
aggregation. By that time, MPI has already been finalized, which is
undefined behavior.
I instrumented the runner with timestamped prints to verify the order
directly. Without the fix:
```
t=3311.770 R1: BEFORE MPI.Finalize
t=3311.778 R0/R1: AFTER MPI.Finalize ← MPI is finalized
t=3311.778 R0/R1: PY ATEXIT
… process exit, LAMMPS destructor runs HERE
```
With the fix:
```
t=3423.100 R1: AFTER del lammps (LAMMPS destructor done) ← MPI still up
t=3423.108 R0/R1: BEFORE MPI.Finalize
t=3423.108 R0/R1: AFTER MPI.Finalize
```
So the LAMMPS destructor now runs while MPI is still up, which is what
its `MPI_Allreduce`/`MPI_Gather` calls require.
The reason this manifests as SIGFPE only on the CUDA CI image (not on
V100) is most likely that the CI image (or one of its preloaded
libraries) enables FP-exception trapping; on V100 the same
MPI-after-Finalize errors return silently. The flake is
environment-specific, but the underlying antipattern is unconditional
and worth fixing in any environment.
## Test plan
- [x] Local CPU: 29/29 LAMMPS tests pass (`test_lammps_dpa3_pt2.py`,
`test_lammps_spin_dpa3_pt2.py`)
- [x] Remote V100: 50/50 stress runs of the previously-failing test
- [x] Empirical confirmation that the fix flips the
LAMMPS-destructor-vs-MPI.Finalize ordering (see Background)
- [ ] CI: re-run the spin LAMMPS suite multiple times to confirm the
SIGFPE no longer appears
## Known limitations
- Cannot directly observe the SIGFPE on V100, so the fix has not been
observed *preventing* the actual crash — only correcting the antipattern
that we have strong reason to believe causes it.
- If the failure persists after merge, the next candidate root cause is
CUDA stream destruction order, and we should revisit.
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
* **Bug Fixes**
* Improved MPI cleanup sequence in multiple test runners to prevent
finalization-related crashes when executing tests in distributed MPI
environments.
<!-- review_stack_entry_start -->
[](https://app.coderabbit.ai/change-stack/deepmodeling/deepmd-kit/pull/5455?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack)
<!-- review_stack_entry_end -->
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
Co-authored-by: Han Wang <wang_han@iapcm.ac.cn>1 parent 3384efb commit 9245a7b
4 files changed
Lines changed: 19 additions & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
62 | 62 | | |
63 | 63 | | |
64 | 64 | | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
65 | 68 | | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
225 | 225 | | |
226 | 226 | | |
227 | 227 | | |
| 228 | + | |
| 229 | + | |
| 230 | + | |
| 231 | + | |
228 | 232 | | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
62 | 62 | | |
63 | 63 | | |
64 | 64 | | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
65 | 68 | | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
144 | 144 | | |
145 | 145 | | |
146 | 146 | | |
| 147 | + | |
| 148 | + | |
| 149 | + | |
| 150 | + | |
| 151 | + | |
| 152 | + | |
| 153 | + | |
| 154 | + | |
| 155 | + | |
147 | 156 | | |
0 commit comments