refactor(lmp): share test infrastructure#5770
Conversation
Consolidate the common pair_deepmd and pair_deepspin MPI setup while preserving their existing script entry points and model-specific configuration. Authored by OpenClaw (model: gpt-5.6-sol) Coding-Agent: Codex Codex-Version: codex-cli 0.144.1 Model: gpt-5.6-sol Reasoning-Effort: xhigh
for more information, see https://pre-commit.ci
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughIntroduces shared LAMMPS test utilities and a configurable MPI pair-test runner for DeepMD and DeepSpin. Existing tests delegate simulation setup, fixture handling, MPI execution, output parsing, backend gating, and cleanup to the shared infrastructure. ChangesShared LAMMPS test infrastructure
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Test
participant SharedUtilities
participant MPI
participant PyLammps
participant Output
Test->>SharedUtilities: Request LAMMPS setup or MPI run
SharedUtilities->>MPI: Launch configured runner
MPI->>PyLammps: Configure and execute simulation
PyLammps->>Output: Write energy and per-atom results
SharedUtilities->>Test: Parse and return structured results
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5770 +/- ##
==========================================
- Coverage 79.65% 79.56% -0.10%
==========================================
Files 1015 1020 +5
Lines 115781 116359 +578
Branches 4272 4303 +31
==========================================
+ Hits 92229 92577 +348
- Misses 22008 22227 +219
- Partials 1544 1555 +11 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Centralize atomic and spin LAMMPS construction, common water fixture lifecycle, and MPI subprocess parsing across backend and model-format tests. Coding-Agent: Codex Codex-Version: codex-cli 0.144.1 Model: gpt-5.6-sol Reasoning-Effort: xhigh
njzjz-bot
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
source/lmp/tests/lammps_test_utils.py (1)
128-193: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocstring omits the
capture=Truereturn contract.The docstring only describes the parsed
{"pe", "forces", "virials", ...}shape; whencapture=Truethe function instead returns{"returncode", "stdout", "stderr"}. Worth a one-line mention since this is a shared utility other test files rely on.📝 Suggested docstring addition
"""Invoke a DPA MPI runner and parse its energy/per-atom output. The runner output contract is one energy line followed by a rectangular per-atom table. ``output_columns`` names and slices that table while each model-specific wrapper retains its own defaults and explanatory docstring. + + If ``capture`` is True, the parsed result is skipped and the function + instead returns ``{"returncode": int, "stdout": str, "stderr": str}`` + from the raw subprocess invocation. """🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@source/lmp/tests/lammps_test_utils.py` around lines 128 - 193, Update the docstring of run_mpi_pair_runner to document that capture=True returns a dictionary containing returncode, stdout, and stderr instead of the parsed energy and per-atom output fields; leave the existing parsing behavior and parameters unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@source/lmp/tests/test_lammps_spin_nopbc_pt.py`:
- Around line 205-213: Update test_pair_deepmd_mpi to append "--nopbc" to the
existing balance_args when invoking run_mpi_model_deviation, matching the
argument handling in test_lammps_spin_nopbc.py so the MPI runner uses
non-periodic boundaries.
---
Nitpick comments:
In `@source/lmp/tests/lammps_test_utils.py`:
- Around line 128-193: Update the docstring of run_mpi_pair_runner to document
that capture=True returns a dictionary containing returncode, stdout, and stderr
instead of the parsed energy and per-atom output fields; leave the existing
parsing behavior and parameters unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 445af8d4-f2d5-41ac-b91a-917da076ea17
📒 Files selected for processing (27)
source/lmp/tests/lammps_test_utils.pysource/lmp/tests/test_deeptensor.pysource/lmp/tests/test_lammps.pysource/lmp/tests/test_lammps_3types.pysource/lmp/tests/test_lammps_dpa1_graph_pt2.pysource/lmp/tests/test_lammps_dpa2_pt.pysource/lmp/tests/test_lammps_dpa2_pt_nopbc.pysource/lmp/tests/test_lammps_dpa3_pt2.pysource/lmp/tests/test_lammps_dpa3_pt2_nopbc.pysource/lmp/tests/test_lammps_dpa4_pt2.pysource/lmp/tests/test_lammps_dpa_jax.pysource/lmp/tests/test_lammps_faparam.pysource/lmp/tests/test_lammps_faparam_pt.pysource/lmp/tests/test_lammps_faparam_pt2.pysource/lmp/tests/test_lammps_fparam_from_fix_dedn.pysource/lmp/tests/test_lammps_jax.pysource/lmp/tests/test_lammps_model_devi_pt2.pysource/lmp/tests/test_lammps_pd.pysource/lmp/tests/test_lammps_pt.pysource/lmp/tests/test_lammps_pt2.pysource/lmp/tests/test_lammps_spin.pysource/lmp/tests/test_lammps_spin_dpa3_pt2.pysource/lmp/tests/test_lammps_spin_nopbc.pysource/lmp/tests/test_lammps_spin_nopbc_pt.pysource/lmp/tests/test_lammps_spin_nopbc_pt2.pysource/lmp/tests/test_lammps_spin_pt.pysource/lmp/tests/test_lammps_spin_pt2.py
Document the capture-mode return contract for the shared MPI runner. Coding-Agent: Codex Codex-Version: codex-cli 0.144.1 Model: gpt-5.6-sol Reasoning-Effort: xhigh
wanghan-iapcm
left a comment
wanghan-iapcm
left a comment
There was a problem hiding this comment.
Faithful test-infrastructure extraction. Verified no expected-value arrays, tolerances, assertions, or skip markers were altered, and per-test special-casing (masses, atom_map, boundary="f f f", the 15-column spin-dpa3 MPI output layout) is preserved as explicit call-site arguments. The --nopbc addition in test_lammps_spin_nopbc_pt.py::test_pair_deepmd_mpi is a correct fix (commit 2fde99e) that makes the MPI test consistent with the serial nopbc tests in the same file.
Non-blocking: test_lammps_dpa1_graph_pt2.py still keeps a hand-rolled MPI runner instead of delegating to the new run_mpi_pair_runner helper like test_lammps_dpa3_pt2.py and test_lammps_spin_dpa3_pt2.py do; consolidating it would complete the stated goal.
| lammps.timestep(0.0005) | ||
| lammps.fix("1 all nve") | ||
| return lammps | ||
| return make_atomic_lammps(data_file, units, atom_map=atom_map) |
There was a problem hiding this comment.
Non-blocking: this file adopts make_atomic_lammps here, but its MPI path (_run_mpi_subprocess, ~L180) still hand-rolls the mpirun argv building and output parsing, whereas the sibling GNN tests test_lammps_dpa3_pt2.py and test_lammps_spin_dpa3_pt2.py now delegate to the new run_mpi_pair_runner in lammps_test_utils.py. Consolidating this one too would complete the PR's stated goal of sharing the MPI pair-test driver across formats. Not required for merge.
2 participants
Summary:
lammps_test_utils.pyas the shared infrastructure layer for LAMMPS Python testsmpi_pair_deepmd.pyrunnerReduction:
Compatibility preserved:
atom_map="no"continues to omit the unsupportedatom_modify map nocommandValidation:
ruff check .— passedruff format .— 1,599 files left unchangedgit diff --check— passedpython -m compileall -q source/lmp/tests— passedpytest --collect-only -q source/lmp/tests— 244 tests collecteddeepmd/deepspin; this environment issue is outside the refactorCoding agent: Codex
Codex version: codex-cli 0.144.1
Model: gpt-5.6-sol
Reasoning effort: xhigh
Summary by CodeRabbit