[WIP] Add dpa1 + lammps inference #4556
Conversation
📝 WalkthroughWalkthroughThis pull request encompasses modifications across multiple files in the DeepMD project, focusing on pre-commit hook configurations, model freezing functionality, and utility function adjustments. The changes include updating the pre-commit configuration to modify hook exclusions and disable certain hooks, enhancing the Changes
Suggested Labels
Suggested Reviewers
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
deepmd/pd/utils/nlist.py (1)
Line range hint
244-251: Bug: Similar condition issue in build_directional_neighbor_list.The same issue exists here where
decomp.numelwill always return True.Apply similar fix:
- if decomp.numel(coord_neig) > 0: + if coord_neig.shape[1] > 0: xmax = paddle.max(coord_cntl) + 2.0 * rcut else: xmax = ( paddle.zeros([1], dtype=coord_neig.dtype, device=coord_neig.place) + 2.0 * rcut )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.pre-commit-config.yaml(3 hunks)deepmd/pd/entrypoints/main.py(1 hunks)deepmd/pd/model/descriptor/se_a.py(2 hunks)deepmd/pd/utils/decomp.py(1 hunks)deepmd/pd/utils/nlist.py(3 hunks)source/api_cc/src/DeepPotPD.cc(1 hunks)source/lmp/plugin/CMakeLists.txt(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (21)
- GitHub Check: Build wheels for cp310-manylinux_aarch64
- GitHub Check: Build wheels for cp311-win_amd64
- GitHub Check: Build wheels for cp311-macosx_x86_64
- GitHub Check: Test Python (6, 3.12)
- GitHub Check: Test Python (6, 3.9)
- GitHub Check: Test Python (5, 3.12)
- GitHub Check: Test Python (5, 3.9)
- GitHub Check: Test Python (4, 3.12)
- GitHub Check: Test Python (4, 3.9)
- GitHub Check: Test Python (3, 3.12)
- GitHub Check: Test Python (3, 3.9)
- GitHub Check: Test Python (2, 3.12)
- GitHub Check: Analyze (python)
- GitHub Check: Test Python (2, 3.9)
- GitHub Check: Build C library (2.14, >=2.5.0rc0,<2.15, libdeepmd_c_cu11.tar.gz)
- GitHub Check: Build C library (2.18, libdeepmd_c.tar.gz)
- GitHub Check: Test Python (1, 3.12)
- GitHub Check: Analyze (c-cpp)
- GitHub Check: Test C++ (false)
- GitHub Check: Test Python (1, 3.9)
- GitHub Check: Test C++ (true)
🔇 Additional comments (7)
deepmd/pd/entrypoints/main.py (2)
365-371: Good documentation of input/output shapes.The documentation clearly specifies the expected shapes and data types for the model's outputs.
373-384: LGTM: Well-structured static graph conversion.The input specifications for the model's forward method are clear and comprehensive.
source/api_cc/src/DeepPotPD.cc (1)
141-141: Clarify why message passing is forcibly disabled.The code overrides the value retrieved from buffer with
0, effectively disabling message passing. This change needs explanation:
- Is this a temporary workaround?
- What are the implications of disabling message passing?
- Should this be configurable?
Please document the rationale for this change.
deepmd/pd/model/descriptor/se_a.py (1)
22-22: LGTM: New import for decomp utility.The addition of the
decompimport aligns with the standardization of tensor element counting across the codebase..pre-commit-config.yaml (2)
68-74: Confirm if prettier and bibtex-tidy hooks can be safely disabled.The removal of these formatting hooks could impact code consistency. Please verify:
- Are there alternative formatting tools in place?
- Is this a temporary or permanent change?
Also applies to: 86-104
✅ Verification successful
The prettier and bibtex-tidy hooks can be safely disabled
The codebase maintains robust formatting through other tools:
- Source code formatting is handled by language-specific tools (ruff, clang-format, etc.)
- Basic validation remains via pre-commit-hooks (check-yaml, trailing whitespace, etc.)
- The disabled hooks only affected documentation and metadata files, not critical code
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for files that would be affected by removed hooks # Files that would be affected by prettier echo "=== Files affected by prettier ===" fd -e md -e yaml -e css --exclude "source/3rdparty" --exclude ".github/workflows" --exclude ".clang-format" # Files that would be affected by bibtex-tidy echo "=== Files affected by bibtex-tidy ===" fd -e bibLength of output: 3266
Script:
#!/bin/bash # Check for alternative formatting tools and temporary indicators echo "=== Checking pre-commit config for other formatters ===" cat .pre-commit-config.yaml echo -e "\n=== Checking GitHub workflows for formatting jobs ===" fd -e yml -e yaml . .github/workflows/ echo -e "\n=== Searching for TODO/FIXME comments about formatting ===" rg -i "todo|fixme|temporary|format" .pre-commit-config.yamlLength of output: 5949
10-10: Verify the need for excluding dpa1.*.json files.The addition of
dpa1.*\.json$to exclusions suggests new file types being introduced. Please confirm if these files require end-of-file fixing exemption.✅ Verification successful
Exclusion of dpa1.json files is appropriate*
The dpa1*.json files are test fixtures and example configurations used across multiple test suites. These files need to maintain specific formatting for test comparisons, making their exclusion from end-of-file fixing appropriate.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for dpa1 JSON files and their EOL status # Find any dpa1 JSON files fd "dpa1.*\.json$" # Check if these files have specific EOL requirements for f in $(fd "dpa1.*\.json$"); do echo "=== $f ===" tail -c1 "$f" | xxd doneLength of output: 1410
Script:
#!/bin/bash # Check file endings and content of dpa1 JSON files for f in $(fd "dpa1.*\.json$"); do echo "=== $f ===" echo "Last line:" tail -n 1 "$f" | cat -A echo -e "\nFirst few lines:" head -n 3 "$f" doneLength of output: 67986
Script:
#!/bin/bash # Check if files are tracked and look for related generation scripts echo "=== Git status of dpa1 files ===" for f in $(fd "dpa1.*\.json$"); do echo "$f:" git check-ignore "$f" || echo "tracked" done echo -e "\n=== Looking for related build/generation scripts ===" rg -l "dpa1.*\.json" --type make --type python --type shellLength of output: 1162
Script:
#!/bin/bash # Search for related code with correct file type flags echo "=== Looking for related code ===" rg -l "dpa1.*json" -g "*.py" -g "*.sh" -g "Makefile" -g "*.mk" echo -e "\n=== Check for JSON schema or validation ===" rg -l "dpa1" -g "*.py" -g "*.json" | grep -v "dpa1.*\.json$"Length of output: 3232
source/lmp/plugin/CMakeLists.txt (1)
11-12: LGTM: Improved clarity in LAMMPS download location message.The change to use
${lammps_download_SOURCE_DIR}provides more accurate information about the download location, as it directly references the FetchContent-managed directory.
| return True | ||
| # return paddle.numel(x) |
There was a problem hiding this comment.
Critical: The numel function's behavior has been altered incorrectly.
The function now returns True instead of the actual number of elements, which:
- Violates the function's contract (name implies counting elements)
- Changes return type from int to bool without type hints
- May break dependent code expecting an integer count
Consider this alternative implementation:
def numel(x: paddle.Tensor) -> int:
if paddle.in_dynamic_mode():
return np.prod(x.shape)
- return True
+ return paddle.numel(x)Committable suggestion skipped: line range outside the PR's diff.
| if decomp.numel(coord) > 0: | ||
| xmax = paddle.max(coord) + 2.0 * rcut | ||
| else: | ||
| xmax = paddle.zeros([], dtype=coord.dtype).to(device=coord.place) + 2.0 * rcut |
There was a problem hiding this comment.
Bug: Condition will always evaluate to True.
Due to changes in decomp.numel, this condition will always be true, potentially leading to incorrect handling of empty tensors.
Use direct tensor size check:
- if decomp.numel(coord) > 0:
+ if coord.shape[1] > 0:
xmax = paddle.max(coord) + 2.0 * rcut
else:
xmax = paddle.zeros([], dtype=coord.dtype).to(device=coord.place) + 2.0 * rcut📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if decomp.numel(coord) > 0: | |
| xmax = paddle.max(coord) + 2.0 * rcut | |
| else: | |
| xmax = paddle.zeros([], dtype=coord.dtype).to(device=coord.place) + 2.0 * rcut | |
| if coord.shape[1] > 0: | |
| xmax = paddle.max(coord) + 2.0 * rcut | |
| else: | |
| xmax = paddle.zeros([], dtype=coord.dtype).to(device=coord.place) + 2.0 * rcut |
| # coord = paddle.load("./coord.pdin") | ||
| # atype = paddle.load("./atype.pdin") | ||
| # box = paddle.load("./box.pdin") | ||
| # od = model.forward(coord, atype, box, do_atomic_virial=True) | ||
| # for k, v in od.items(): | ||
| # if isinstance(v, paddle.Tensor): | ||
| # paddle.save(v, f'{k}.pdout') | ||
| # exit() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove commented-out debug code.
Debug code should not be committed to production, even if commented out.
- # coord = paddle.load("./coord.pdin")
- # atype = paddle.load("./atype.pdin")
- # box = paddle.load("./box.pdin")
- # od = model.forward(coord, atype, box, do_atomic_virial=True)
- # for k, v in od.items():
- # if isinstance(v, paddle.Tensor):
- # paddle.save(v, f'{k}.pdout')
- # exit()| @@ -744,7 +745,7 @@ def forward( | |||
| "Compressed environment is not implemented yet." | |||
| ) | |||
| else: | |||
| if rr.numel() > 0: | |||
| if decomp.numel(rr) > 0: | |||
There was a problem hiding this comment.
💡 Codebase verification
Revert to using direct tensor.numel() method
The decomp.numel() implementation is problematic:
- Always returns
Truein static mode, which breaks the emptiness check - No advantages over using tensor's built-in
numel()method - Inconsistent with other numel usage patterns in the codebase
🔗 Analysis chain
Verify behavior of decomp.numel matches tensor.numel.
The change from rr.numel() > 0 to decomp.numel(rr) > 0 modifies how tensor elements are counted. This is in a critical path of the forward method.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the behavior of decomp.numel
# Search for other usages of numel to ensure consistency
# Check implementation of decomp.numel
rg -A 5 "def numel" "deepmd/pd/utils/decomp.py"
# Check other usages of numel in the codebase
rg "\.numel\(\)" "deepmd/pd/"
Length of output: 555
…ncy when using fp32
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
examples/water/se_atten/input_torch.json (1)
29-29: LGTM! Explicit precision settings will help with inference consistency.The addition of explicit float32 precision settings to both the descriptor and fitting_net sections is a good practice, especially for C++ inference. This ensures consistent numerical behavior between training and production environments.
Consider documenting the performance implications of float32 vs float64 precision in the model documentation, particularly for LAMMPS integration scenarios.
Also applies to: 39-39
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
deepmd/pd/model/descriptor/descriptor.py(1 hunks)deepmd/pd/model/descriptor/repformers.py(1 hunks)deepmd/pd/model/descriptor/se_a.py(3 hunks)deepmd/pd/model/descriptor/se_atten.py(1 hunks)deepmd/pd/model/descriptor/se_t_tebd.py(1 hunks)examples/water/se_atten/input_torch.json(2 hunks)source/api_cc/src/DeepPotPD.cc(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- deepmd/pd/model/descriptor/repformers.py
🚧 Files skipped from review as they are similar to previous changes (2)
- source/api_cc/src/DeepPotPD.cc
- deepmd/pd/model/descriptor/se_a.py
👮 Files not reviewed due to content moderation or server errors (3)
- deepmd/pd/model/descriptor/descriptor.py
- deepmd/pd/model/descriptor/se_t_tebd.py
- deepmd/pd/model/descriptor/se_atten.py
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Analyze (python)
- GitHub Check: Build source distribution
- GitHub Check: Analyze (c-cpp)
🔇 Additional comments (1)
examples/water/se_atten/input_torch.json (1)
28-28: LGTM! Valid JSON syntax.The added trailing comma after the temperature setting is valid JSON syntax and follows common style guidelines.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @@ -138,6 +138,7 @@ | |||
| // initialize hyper params from model buffers | |||
| ntypes_spin = 0; | |||
| DeepPotPD::get_buffer<int>("buffer_has_message_passing", do_message_passing); | |||
| // this->do_message_passing = 0; | |||
Check notice
Code scanning / CodeQL
Commented-out code Note
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## devel #4556 +/- ##
==========================================
- Coverage 84.60% 84.60% -0.01%
==========================================
Files 680 680
Lines 64467 64468 +1
Branches 3538 3538
==========================================
- Hits 54545 54544 -1
- Misses 8781 8782 +1
- Partials 1141 1142 +1 ☔ View full report in Codecov by Sentry. |
Add cpp inference with lammps
Summary by CodeRabbit
Pre-Commit Configuration
end-of-file-fixerhookprettierandbibtex-tidyhooksCode Changes
Configuration Updates
Build Configuration