Skip to content

fix(lammps): free model deviation buffers#5803

Open
njzjz wants to merge 1 commit into
deepmodeling:masterfrom
njzjz:codex/code-scan-5644
Open

fix(lammps): free model deviation buffers#5803
njzjz wants to merge 1 commit into
deepmodeling:masterfrom
njzjz:codex/code-scan-5644

Conversation

@njzjz

@njzjz njzjz commented Jul 14, 2026

Copy link
Copy Markdown
Member

Closes #5644.

Summary

  • initialize all six model-deviation gather buffer pointers to nullptr;
  • centralize allocation and growth in a shared ensure_model_deviation_buffers() helper;
  • reuse process-count buffers across repeated init_style() calls instead of overwriting live allocations;
  • grow the four atom-sized buffers through LAMMPS Memory::grow() when the global atom count increases;
  • release all six buffers from the shared base-class destructor;
  • use the same lifecycle path for both DeepMD and DeepSpin pair styles;
  • add a repeated 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 call init_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 nullptr initialization 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

  • built the DeePMD LAMMPS plugin, TensorFlow backend, and TensorFlow op against stable_22Jul2025_update2;
  • built a matching Update2 LAMMPS shared library and Python wrapper;
  • existing test_pair_deepmd_model_devi: passed;
  • new test_pair_deepmd_model_devi_atomic_reinit_clear_smoke: passed;
  • combined targeted result: 2 passed;
  • clang-format 22.1.5 check;
  • ruff format .;
  • ruff check .;
  • git diff --check;
  • independent final review: no findings.

Coding agent: Codex
Codex version: codex-cli 0.144.1
Model: gpt-5.6-sol
Reasoning effort: xhigh

Summary by CodeRabbit

  • Bug Fixes

    • Improved cleanup and reuse of model-deviation output resources during repeated simulations and reinitialization.
    • Prevented resource leaks and stale buffer issues when clearing and restarting simulations.
    • Improved handling of model-deviation data as system sizes change.
  • Tests

    • Added coverage for repeated DeepMD initialization, model-deviation output, and simulation clearing.

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
Copilot AI review requested due to automatic review settings July 14, 2026 21:07

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@dosubot dosubot Bot added the bug label Jul 14, 2026
@coderabbitai

coderabbitai Bot commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 50355684-67d4-470b-9774-9670c2b7faea

📥 Commits

Reviewing files that changed from the base of the PR and between 3122138 and 43d6a41.

📒 Files selected for processing (5)
  • source/lmp/pair_base.cpp
  • source/lmp/pair_base.h
  • source/lmp/pair_deepmd.cpp
  • source/lmp/pair_deepspin.cpp
  • source/lmp/tests/test_lammps.py

📝 Walkthrough

Walkthrough

The change centralizes model-deviation communication buffer allocation, resizing, and destruction, initializes buffer pointers safely, updates DeepMD and DeepSpin call paths, and adds a repeated-run cleanup smoke test.

Changes

Model-deviation buffer lifecycle

Layer / File(s) Summary
Centralize buffer lifecycle
source/lmp/pair_base.h, source/lmp/pair_base.cpp
Adds shared buffer management helpers, initializes buffer pointers to nullptr, uses the helper during init_style(), and destroys buffers in the destructor.
Pair-style integration and lifecycle validation
source/lmp/pair_deepmd.cpp, source/lmp/pair_deepspin.cpp, source/lmp/tests/test_lammps.py
Routes DeepMD and DeepSpin atomic model-deviation paths through shared buffer sizing and adds a smoke test covering repeated runs followed by lammps.clear().

Estimated code review effort: 3 (Moderate) | ~20 minutes

Suggested labels: C++, LAMMPS

Suggested reviewers: copilot, wanghan-iapcm

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly matches the main change: freeing model deviation buffers in LAMMPS.
Linked Issues check ✅ Passed The PR addresses #5644 by null-initializing, centralizing allocation, freeing buffers in the base destructor, and adding lifecycle coverage.
Out of Scope Changes check ✅ Passed The changes stay focused on the buffer leak fix and related test coverage without obvious unrelated additions.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@njzjz njzjz requested a review from wanghan-iapcm July 14, 2026 22:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Code scan] Free LAMMPS model-deviation atomic communication buffers

3 participants