fix(lammps): free model deviation buffers#5803
Conversation
Centralize allocation, growth, reuse, and destruction of atomic model-deviation communication buffers for DeepMD and DeepSpin pair styles. Coding-Agent: Codex Codex-Version: codex-cli 0.144.1 Model: gpt-5.6-sol Reasoning-Effort: xhigh
📝 WalkthroughWalkthroughThe change centralizes model-deviation communication buffer allocation, resizing, and destruction, initializes buffer pointers safely, updates DeepMD and DeepSpin call paths, and adds repeated-run cleanup and LSan coverage. ChangesModel-deviation buffer lifecycle
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
There was a problem hiding this comment.
Reviewed via the code-review skill. Correct and complete: all six model-devi buffers are nullptr-initialized, ensure_model_deviation_buffers() reuses counts/displacements (nprocs is fixed for the run) and memory->grow()s the four atom buffers (grow-on-nullptr == create in LAMMPS), and destroy_model_deviation_buffers() frees all six from the base destructor (destroy-on-nullptr is a safe no-op, so no double-free), covering both PairDeepMD and PairDeepSpin. The sweep found no other unfreed model-devi buffers, and the destructor-never-freed gap has existed since #628. Both the atomic and non-atomic branches are exercised (the latter via existing teardown).
One non-blocking note: the new reinit/clear smoke test genuinely exercises the fixed paths, but it cannot fail pre-fix in CI -- the leak is silent (not a crash/UAF), and the LAMMPS pytest step is gated with 'if: not matrix.check_memleak', so the -fsanitize=leak leg never runs it while every leg that runs it has no sanitizer. As written it is a crash-safety/coverage guard, not a leak-regression guard (which the PR body candidly acknowledges). Wiring source/lmp/tests into the check_memleak leg (or a dedicated LSan job) would turn it into a real regression guard for this leak class. LGTM.
Execute the focused model-deviation reinitialization and clear test in the TensorFlow/PyTorch leak-sanitizer leg so the fixed buffer leak becomes CI-observable. Coding-Agent: Codex Codex-Version: codex-cli 0.144.4 Model: gpt-5.6-sol Reasoning-Effort: xhigh
|
I addressed the non-blocking CI coverage note in The focused The workflow YAML, formatting, Ruff, and repository commit hooks passed locally; the actual LSan execution is delegated to CI. Coding agent: Codex |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/test_cc.yml (1)
100-103: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse matrix variables for environment configuration.
For consistency with other test steps in this workflow and to prevent potential mismatches if the build matrix configuration changes in the future, consider dynamically setting the framework flags based on the matrix variables instead of hardcoding them.
♻️ Proposed refactor
- ENABLE_TENSORFLOW: "1" - ENABLE_PYTORCH: "1" - ENABLE_JAX: "1" - ENABLE_PADDLE: "0" + ENABLE_TENSORFLOW: ${{ matrix.enable_tensorflow && '1' || '0' }} + ENABLE_PYTORCH: ${{ matrix.enable_pytorch && '1' || '0' }} + ENABLE_JAX: ${{ matrix.enable_tensorflow && '1' || '0' }} + ENABLE_PADDLE: ${{ matrix.enable_paddle && '1' || '0' }}🤖 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 @.github/workflows/test_cc.yml around lines 100 - 103, Update the framework environment flags in the workflow test step to derive their enabled values from the corresponding build matrix variables rather than hardcoded strings. Apply this consistently to ENABLE_TENSORFLOW, ENABLE_PYTORCH, ENABLE_JAX, and ENABLE_PADDLE while preserving the matrix’s current enable/disable behavior.
🤖 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.
Nitpick comments:
In @.github/workflows/test_cc.yml:
- Around line 100-103: Update the framework environment flags in the workflow
test step to derive their enabled values from the corresponding build matrix
variables rather than hardcoded strings. Apply this consistently to
ENABLE_TENSORFLOW, ENABLE_PYTORCH, ENABLE_JAX, and ENABLE_PADDLE while
preserving the matrix’s current enable/disable behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d2d247a8-76a9-4f05-b7a9-e963b23af725
📒 Files selected for processing (1)
.github/workflows/test_cc.yml
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5803 +/- ##
==========================================
- Coverage 79.83% 78.58% -1.25%
==========================================
Files 1029 1054 +25
Lines 117576 121758 +4182
Branches 4311 4411 +100
==========================================
+ Hits 93869 95689 +1820
- Misses 22156 24502 +2346
- Partials 1551 1567 +16 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Possible reviewers based on changed lines, exact file history, and exact-file review history:
No review request was made automatically. Coding agent: Codex |
Pull request was converted to draft
Remove the workflow-only LSan regression step while retaining the underlying LAMMPS buffer fix and its test. Coding-Agent: Codex Codex-Version: codex-cli 0.144.4 Model: gpt-5.6-sol Reasoning-Effort: xhigh Signed-off-by: njzjz-bot <njzjz.bot@gmail.com>
Closes #5644.
Summary
nullptr;ensure_model_deviation_buffers()helper;init_style()calls instead of overwriting live allocations;Memory::grow()when the global atom count increases;run(0)/clear()smoke test for reinitialization and pair destruction.Root cause
init_style()created the communication arrays whenever atomic model-deviation output was enabled, but repeated LAMMPS runs can callinit_style()again on the same pair object. Those creates overwrote the stored pointers. The compute paths separately destroyed and recreated four arrays on atom-count growth, while the pair destructor released none of the six arrays.The pair object is the sole owner of these buffers. Allocation, reuse, growth, and destruction are now handled in one base-class implementation, with
nullptrinitialization making every cleanup path safe.Why existing tests missed this
The existing model-deviation tests validate numerical output. Leaking a LAMMPS-managed buffer does not change those values or necessarily crash, so the old implementation passes ordinary functional tests.
The new repeated-init/clear test is intentionally only a lifecycle smoke: it exercises repeated
init_style()and pair destruction and can catch obvious double-free or use-after-free regressions. It is not presented as a leak detector, and the old implementation also passes it while leaking.A direct stable old-fail/new-pass demonstration requires a dedicated LeakSanitizer build with appropriate handling of unrelated TensorFlow/Python/LAMMPS allocations. No RSS or process-memory threshold was added because it would be allocator- and timing-dependent.
Validation
stable_22Jul2025_update2;test_pair_deepmd_model_devi: passed;test_pair_deepmd_model_devi_atomic_reinit_clear_smoke: passed;ruff format .;ruff check .;git diff --check;Coding agent: Codex
Codex version: codex-cli 0.144.1
Model: gpt-5.6-sol
Reasoning effort: xhigh
Summary by CodeRabbit
Bug Fixes
Tests