fix(dpa): handle NULL type properly in lammps plugin#5268
Closed
link89 wants to merge 12 commits intodeepmodeling:masterfrom
Closed
fix(dpa): handle NULL type properly in lammps plugin#5268link89 wants to merge 12 commits intodeepmodeling:masterfrom
link89 wants to merge 12 commits intodeepmodeling:masterfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughMap "NULL" atom types to sentinel -1 instead of type_map.size() in DeepMD-related pair/fix code; add unit tests exercising Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
source/lmp/tests/test_lammps_dpa_pt.py (1)
497-500: Consider adding a basic sanity assertion.The test is a crash-regression smoke test (no assertions), which is acceptable given the bug being fixed (double-free). However, since
run(0)with only H atoms active is a well-defined state, adding even a simpleassert lammps_type_map.eval("pe") != 0(or apytest.approxagainst a known value) would make the test more meaningful and protect against silent incorrect behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lmp/tests/test_lammps_dpa_pt.py` around lines 497 - 500, In test_pair_deepmd_type_map_with_null, after calling lammps_type_map.run(0) add a simple sanity assertion that checks the computed potential energy is sensible (for example assert lammps_type_map.eval("pe") != 0 or use pytest.approx against a known value); place the assertion immediately after lammps_type_map.run(0) to ensure the test still guards against silent incorrect behavior while keeping it as a smoke/regression test for the double-free fix.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@source/lmp/tests/test_lammps_dpa_pt.py`:
- Around line 495-502: Add the missing blank line separating top-level test
functions: ensure there are two blank lines between the function definitions
test_pair_deepmd_type_map_with_null and test_pair_deepmd_real by inserting one
more blank line after the end of test_pair_deepmd_type_map_with_null (the
lammps_type_map.run(0) line), then run ruff format/check to confirm PEP8 E302 is
satisfied.
---
Nitpick comments:
In `@source/lmp/tests/test_lammps_dpa_pt.py`:
- Around line 497-500: In test_pair_deepmd_type_map_with_null, after calling
lammps_type_map.run(0) add a simple sanity assertion that checks the computed
potential energy is sensible (for example assert lammps_type_map.eval("pe") != 0
or use pytest.approx against a known value); place the assertion immediately
after lammps_type_map.run(0) to ensure the test still guards against silent
incorrect behavior while keeping it as a smoke/regression test for the
double-free fix.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
source/lmp/fix_dplr.cppsource/lmp/pair_deepmd.cppsource/lmp/pair_deepspin.cppsource/lmp/tests/test_lammps_dpa_pt.py
for more information, see https://pre-commit.ci
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes handling of NULL atom types in DeepMD-related LAMMPS plugins by using a sentinel mapping value, and adds a regression test to cover the NULL type-map case.
Changes:
- Map
NULLtypes to-1instead oftype_map.size()in multiple LAMMPS plugin components. - Add a LAMMPS Python test to ensure
pair_coeffaccepts aNULLtype entry without crashing.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| source/lmp/tests/test_lammps_dpa_pt.py | Adds a regression test for NULL in the DeepMD type map. |
| source/lmp/pair_deepspin.cpp | Changes NULL type index mapping to -1 sentinel. |
| source/lmp/pair_deepmd.cpp | Changes NULL type index mapping to -1 sentinel. |
| source/lmp/fix_dplr.cpp | Changes NULL type index mapping to -1 sentinel. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: link89 <xuweihong.cn@gmail.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@source/lmp/tests/test_lammps_dpa_pt.py`:
- Around line 498-501: The current test test_pair_deepmd_type_map_with_null only
uses pair_style("deepmd ...") and doesn't exercise the hybrid/scaled + NULL
combination reported in Issue `#4749`; update or add a test that sets pair_style
to a hybrid/scaled form that includes deepmd (e.g., pair_style("hybrid/scaled
deepmd ...")) and then call pair_coeff with the same NULL mapping
(lammps_type_map.pair_coeff("* * H O NULL")) and run(0) to ensure the
hybrid/scaled + NULL path is exercised and does not crash; locate the test
function test_pair_deepmd_type_map_with_null and modify its pair_style
invocation (or create a new test with a distinct name) to use the hybrid/scaled
variant so the exact failing configuration is covered.
Signed-off-by: link89 <xuweihong.cn@gmail.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
source/lmp/tests/test_lammps.py (1)
520-524: Smoke test has no assertions — consider verifying forces on non-NULL atoms or zero forces on NULL atoms.The test correctly exercises the NULL type code path (type-2 atoms in
data_type_map_fileare mapped to NULL), making it an effective crash-regression guard for issue#4749. However, without any assertion, a silent wrong-result regression (e.g., non-zero forces being computed for NULL atoms) would go undetected.A lightweight addition would be to assert that atoms of the NULL type receive zero force:
💡 Suggested assertion for NULL-type atoms
def test_pair_deepmd_type_map_with_null(lammps_type_map) -> None: lammps_type_map.pair_style(f"deepmd {pb_file.resolve()}") lammps_type_map.pair_coeff("* * H NULL") lammps_type_map.run(0) + # type_HO maps type 2 → NULL; atoms at 0-based indices 0 and 3 are type 2 + null_atom_ids = [ + i for i, t in enumerate(type_HO) if t == 2 + ] # 0-based positions of NULL atoms + for ii in range(len(lammps_type_map.atoms)): + atom = lammps_type_map.atoms[ii] + if (atom.id - 1) in null_atom_ids: + assert atom.force == pytest.approx([0.0, 0.0, 0.0])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/lmp/tests/test_lammps.py` around lines 520 - 524, The test function test_pair_deepmd_type_map_with_null currently only runs LAMMPS without assertions; update it to fetch the computed forces after lammps_type_map.run(0) and assert that atoms mapped to NULL (type-2 in the data_type_map_file) have zero force (or that non-NULL atoms have non-zero forces) to prevent silent regressions — locate the test by function name and add a small check using the LAMMPS wrapper's force-accessor (or API used elsewhere in tests) after calling lammps_type_map.run(0) to verify per-atom forces for NULL vs non-NULL types.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@source/lmp/tests/test_lammps.py`:
- Around line 520-524: The test function test_pair_deepmd_type_map_with_null
currently only runs LAMMPS without assertions; update it to fetch the computed
forces after lammps_type_map.run(0) and assert that atoms mapped to NULL (type-2
in the data_type_map_file) have zero force (or that non-NULL atoms have non-zero
forces) to prevent silent regressions — locate the test by function name and add
a small check using the LAMMPS wrapper's force-accessor (or API used elsewhere
in tests) after calling lammps_type_map.run(0) to verify per-atom forces for
NULL vs non-NULL types.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5268 +/- ##
========================================
Coverage 82.00% 82.00%
========================================
Files 750 750
Lines 75082 75263 +181
Branches 3615 3627 +12
========================================
+ Hits 61571 61723 +152
- Misses 12347 12375 +28
- Partials 1164 1165 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: link89 <xuweihong.cn@gmail.com>
Signed-off-by: link89 <xuweihong.cn@gmail.com>
Signed-off-by: link89 <xuweihong.cn@gmail.com>
* Initial plan * fix(api_cc): fix segfault in DeepPotPT when using NULL types with hybrid/scaled Co-authored-by: link89 <3314130+link89@users.noreply.github.com> * debug: add logging to DeepPotPT and test to narrow down crash location Co-authored-by: link89 <3314130+link89@users.noreply.github.com> * fix(api_cc): fix use-after-free in mapping_tensor by persisting mapping_data as member variable Co-authored-by: link89 <3314130+link89@users.noreply.github.com> * fix(api_cc): fix segfault in DeepPotPT when using NULL types with hybrid/scaled pair style Co-authored-by: link89 <3314130+link89@users.noreply.github.com> * fix: ELOOP: too many symbolic links encountered * fix(api_cc): apply fwd_map to mapping_data to convert original to real atom indices Co-authored-by: link89 <3314130+link89@users.noreply.github.com> * fix(api_cc): fix segfault in DeepPotPT when using NULL types with hybrid/scaled pair style Co-authored-by: link89 <3314130+link89@users.noreply.github.com> * fix(api_cc): skip message passing comm_dict when NULL-type atoms are present When nall_real < nall (NULL-type atoms filtered out), the LAMMPS sendlist is in original-space and may contain atom indices >= nall_real. Passing this sendlist to forward_lower causes OOB access inside the DPA-2 model. Fix: detect has_null_atoms = (nall_real < nall) and skip comm_dict when NULL atoms exist. The ghost atoms are already in the coordinate tensor, so the model can still compute correctly without explicit message passing. Also add sendlist diagnostic logging (first entry per swap, valid range) to expose the index-space mismatch for future debugging. Co-authored-by: link89 <3314130+link89@users.noreply.github.com> * fix(api_cc): remap sendlist from original-space to real-space when NULL atoms present Instead of skipping comm_dict when NULL-type atoms are present, properly remap the sendlist from original-space to real-space using fwd_map: - Iterate sendlist[s][j] per swap, apply fwd_map to each entry - Filter out NULL-type atoms (fwd_map[orig_idx] == -1) - Build remapped_sendnum_data and remapped_sendlist_data (persistent class members to back the tensors and avoid use-after-free) - Always pass comm_dict to forward_lower when do_message_passing For single-process tests (nswap=0), total_send=0 so the remapping is a no-op. For multi-process with NULL atoms, the sendlist is now correctly in real-space. Co-authored-by: link89 <3314130+link89@users.noreply.github.com> * fix: add bounds check for orig_idx before fwd_map access in sendlist remapping Co-authored-by: link89 <3314130+link89@users.noreply.github.com> * Delete _codeql_detected_source_root Signed-off-by: link89 <xuweihong.cn@gmail.com> * Delete source/source Signed-off-by: link89 <xuweihong.cn@gmail.com> * fix(api_cc): apply suggested patch - use torch::tensor() and always remap sendlist Key fixes following @link89's patch suggestion: 1. lmp_list.sendlist is int** (jagged array per swap), NOT a flat buffer. The previous code using from_blob(lmp_list.sendlist, {total_send}, ...) was treating the int** pointer-to-pointers as a flat int array - wrong even for the non-NULL case. 2. Always remap sendlist through fwd_map (not just when has_null_atoms), correctly iterating sendlist[iswap] per swap. 3. Use torch::tensor() to copy data into tensors, avoiding any lifetime issues with from_blob. 4. Rename buffers to mp_sendlist_remap_ / mp_sendnum_remap_ (int32_t) matching the suggested naming convention. 5. Add #include <cstdint> to DeepPotPT.h and #include <numeric> to .cc Co-authored-by: link89 <3314130+link89@users.noreply.github.com> * debug: remove all code fixes, keep diagnostic logs only Per @link89's request: revert all code fixes so we can observe the raw crash behavior with the diagnostic logs in place. Reverted: - Removed sendlist remap loop (back to original from_blob(lmp_list.sendlist,...)) - Removed mapping_data persistent member (back to local vector mapping) - Reverted mapping index back to original: mapping[ii] = lmp_list.mapping[fwd_map[ii]] - Removed mp_sendlist_remap_ and mp_sendnum_remap_ class members from header - Removed #include <cstdint> from header (not in original) Kept/added diagnostic logs: - has_null_atoms flag and NULL atom count logging - total_send logging - DIAG: sendlist[s][0] per swap showing first entry vs nall_real - DIAG: first 5 mapping values showing fwd_map[ii] used - nlist copy/shuffle/padding checkpoints - forward_lower call/return checkpoints - bkw mapping force checkpoint Co-authored-by: link89 <3314130+link89@users.noreply.github.com> * log full fwd_map Signed-off-by: link89 <xuweihong.cn@gmail.com> * debug: print all real/virtual atom indices and full comm_dict contents Per @link89's request: add full diagnostic dumps of: 1. All original atom indices with their atype, fwd_map value, and REAL/VIRTUAL label 2. All bkw_map entries showing real->original mapping 3. Full comm_dict contents: send_proc, recv_proc, send_num, recv_num, firstrecv 4. Per-swap sendlist with all entries, annotated with (valid) or (OOB!) relative to nall_real Co-authored-by: link89 <3314130+link89@users.noreply.github.com> * debug: add try-catch around forward_lower and print exception details before re-throw Co-authored-by: link89 <3314130+link89@users.noreply.github.com> * debug: print mapping_tensor shape and values before forward_lower call Co-authored-by: link89 <3314130+link89@users.noreply.github.com> * debug: add logging to Python forward_lower to print atype/nlist/mapping/comm_dict Co-authored-by: link89 <3314130+link89@users.noreply.github.com> * fix(api_cc): fix compile error - use has_value()/value() for c10::optional mapping_tensor Co-authored-by: link89 <3314130+link89@users.noreply.github.com> * fix(api_cc): fix Dict<IValue,IValue> default-construction error - use lambda for try-catch around forward_lower Co-authored-by: link89 <3314130+link89@users.noreply.github.com> * feat(api_cc): add make_comm_dict method to remap sendlist from original to real-atom space Co-authored-by: link89 <3314130+link89@users.noreply.github.com> * fix(api_cc): simplify fwd_map lookup in make_comm_dict per review feedback Co-authored-by: link89 <3314130+link89@users.noreply.github.com> * update recvsum for virtual atoms * update comm_dict directly * update comm_dict directly * fix memory lifecycle issue * simplify memory mamangment * simplify memory mamangment * fix memory issue and add more logs * fix segmenet fault * optimize memory usage * refactor(api_cc,pt): remove debug logs and split update_comm_dict for performance Co-authored-by: link89 <3314130+link89@users.noreply.github.com> * remove log in test case * remove _codeql_detected_source_root * refactor duplicated code * refactor duplicated code * fix(tf): fix MessageToString float_format compatibility with protobuf 5.x Co-authored-by: link89 <3314130+link89@users.noreply.github.com> * revert(tf): revert MessageToString fix in convert.py Co-authored-by: link89 <3314130+link89@users.noreply.github.com> --------- Signed-off-by: link89 <xuweihong.cn@gmail.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: link89 <3314130+link89@users.noreply.github.com> Co-authored-by: weihong.xu <xuweihong.cn@gmail.com>
for more information, see https://pre-commit.ci
Contributor
Author
|
Hi @iProzd @njzjz , this PR is intended to address the issue reported in #4749. I have implemented the fix specifically within |
Collaborator
|
I'll close this PR. See #4889 for details. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fix: #4749
related to: #4889
Summary by CodeRabbit
Bug Fixes
Tests