fix(api_c): preserve exact bytes in DP_ReadFileToChar2#5763
Merged
wanghan-iapcm merged 3 commits intoJul 12, 2026
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
📝 WalkthroughWalkthroughThe C API adds charge-spin-aware DeepPot and model-deviation compute functions, exposes charge-spin dimensions, and preserves exact file bytes in file-reading APIs with C++ and Python regression coverage. ChangesCharge-spin C API
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant C_API
participant ComputeHelper
participant DeepPotBackend
participant Outputs
C_API->>ComputeHelper: submit coordinates and charge_spin
ComputeHelper->>DeepPotBackend: convert and forward charge-spin vector
DeepPotBackend-->>Outputs: return energy, force, virial, and atomic outputs
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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: 2
🧹 Nitpick comments (1)
source/tests/test_c_api_readfile.py (1)
37-102: 📐 Maintainability & Code Quality | 🔵 TrivialExtract shared library loading and function signature setup into a fixture.
Both tests duplicate
_load_c_lib()calls and theargtypes/restypeconfiguration. A session-scoped fixture would load the library once and centralize the setup.♻️ Proposed refactor
+@pytest.fixture(scope="module") +def c_lib(): + lib = _load_c_lib() + lib.DP_ReadFileToChar2.argtypes = [ctypes.c_char_p, ctypes.POINTER(ctypes.c_int)] + lib.DP_ReadFileToChar2.restype = ctypes.c_void_p + lib.DP_DeleteChar.argtypes = [ctypes.c_void_p] + lib.DP_DeleteChar.restype = None + return lib + + -def test_readfiletochar2_preserves_trailing_whitespace(tmp_path): +def test_readfiletochar2_preserves_trailing_whitespace(tmp_path, c_lib): """Files ending in whitespace must return exact size and bytes.""" - lib = _load_c_lib() - - # Set up function signatures ------------------------------------------- - # Use c_void_p for the return type so ctypes does not truncate the - # buffer at the first null byte (c_char_p would do that). - lib.DP_ReadFileToChar2.argtypes = [ctypes.c_char_p, ctypes.POINTER(ctypes.c_int)] - lib.DP_ReadFileToChar2.restype = ctypes.c_void_p - lib.DP_DeleteChar.argtypes = [ctypes.c_void_p] - lib.DP_DeleteChar.restype = None + # c_void_p return type avoids truncation at the first null byte. ...🤖 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/tests/test_c_api_readfile.py` around lines 37 - 102, Extract the duplicated library loading and ctypes signature setup from test_readfiletochar2_preserves_trailing_whitespace and test_readfiletochar2_preserves_no_trailing_whitespace into a session-scoped pytest fixture. Configure DP_ReadFileToChar2 and DP_DeleteChar once in the fixture, then inject the prepared library into both tests while preserving their existing assertions and cleanup.
🤖 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/api_c/src/c_api.cc`:
- Line 2686: Guard the assignment in the file-reading function containing *size
= file_content.size() by checking whether file_content.size() exceeds INT_MAX
before converting it to int. For oversized content, return the existing error
sentinel and surface a clear error through the function’s established
error-reporting mechanism; otherwise assign the safely bounded size.
- Around line 1774-1854: Add declarations to source/api_c/include/c_api.h for
the new DP_DeepPotCompute3, DP_DeepPotComputef3, DP_DeepPotComputeNList3,
DP_DeepPotComputeNListf3, all DP_DeepPotModelDevi*3 entry points, and the new
GetDimChgSpin accessors, matching the definitions’ signatures and existing API
declaration style.
---
Nitpick comments:
In `@source/tests/test_c_api_readfile.py`:
- Around line 37-102: Extract the duplicated library loading and ctypes
signature setup from test_readfiletochar2_preserves_trailing_whitespace and
test_readfiletochar2_preserves_no_trailing_whitespace into a session-scoped
pytest fixture. Configure DP_ReadFileToChar2 and DP_DeleteChar once in the
fixture, then inject the prepared library into both tests while preserving their
existing assertions and cleanup.
🪄 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: 547099a2-1f8a-44da-86da-dc88fb1ea9fb
📒 Files selected for processing (3)
source/api_c/src/c_api.ccsource/api_cc/tests/test_read_file_to_string.ccsource/tests/test_c_api_readfile.py
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5763 +/- ##
==========================================
- Coverage 79.65% 79.57% -0.09%
==========================================
Files 1015 1020 +5
Lines 115781 116383 +602
Branches 4272 4303 +31
==========================================
+ Hits 92229 92613 +384
- Misses 22008 22223 +215
- Partials 1544 1547 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
njzjz-bot
commented
Jul 11, 2026
njzjz-bot
left a comment
njzjz-bot
left a comment
Contributor
Author
There was a problem hiding this comment.
I found one test-coverage issue in the C++ regression test. The effective merge result otherwise looks correct.
Coding agent: Codex
Codex version: codex-cli 0.144.0-alpha.4
Model: gpt-5.6-sol
Reasoning effort: xhigh
njzjz
pushed a commit
to njzjz-bot/deepmd-kit
that referenced
this pull request
Jul 11, 2026
- Add INT_MAX overflow guard in DP_ReadFileToChar2 - Add missing version-3 charge-spin C API declarations to header - Refactor test_c_api_readfile.py to use a module-scoped pytest fixture - Fix implicit return in _load_c_lib (CodeQL/Ruff) Coding-Agent: opencode opencode-Version: 1.17.18 Model: ustc/deepseek-v4-pro Reasoning-Effort: max
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/api_c/include/c_api.h`:
- Around line 687-701: Update the Doxygen buffer-size documentation for all
affected C API declarations, including the symbols around the documented
input/output parameters and their corresponding variants, to include nframes
consistently: cell as nframes x 9, energy as nframes, force as nframes x natoms
x 3, virial as nframes x 9, atomic_energy as nframes x natoms, and atomic_virial
as nframes x natoms x 9. Apply the same corrections at every referenced
declaration, including the additional ranges noted in the review.
🪄 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: 3620e9b2-a23a-455a-ac1f-ab70bf8b4a65
📒 Files selected for processing (3)
source/api_c/include/c_api.hsource/api_c/src/c_api.ccsource/tests/test_c_api_readfile.py
🚧 Files skipped from review as they are similar to previous changes (2)
- source/tests/test_c_api_readfile.py
- source/api_c/src/c_api.cc
DP_ReadFileToChar2 reported the original file size but returned a buffer from string_to_char, which trims trailing whitespace before allocating. The C++ wrapper then reconstructed a std::string with the reported (larger) size, causing an over-read of the shorter allocation. Fix by adding string_to_char_exact, a helper that preserves every byte without trimming, and use it in DP_ReadFileToChar2 and DP_ReadFileToChar. Error-message paths still use the trimming string_to_char since whitespace trimming is desirable there. Added both a C++ test (test_read_file_to_string.cc) and a Python test (test_c_api_readfile.py) that verify trailing whitespace is preserved. Fixes deepmodeling#5620. Generated with opencode using model glm-5.2.
- Add INT_MAX overflow guard in DP_ReadFileToChar2 - Correct multi-frame buffer documentation for charge-spin DeepPot APIs - Refactor test_c_api_readfile.py to use a module-scoped pytest fixture - Fix implicit return in _load_c_lib (CodeQL/Ruff) Coding-Agent: opencode opencode-Version: 1.17.18 Model: ustc/deepseek-v4-pro Reasoning-Effort: max Coding-Agent: Codex Codex-Version: codex-cli 0.144.1 Model: gpt-5.6-sol Reasoning-Effort: xhigh
njzjz
force-pushed
the
fix/readfile-exact-bytes
branch
from
d3c85ce to
f7cd760
Compare
Test exact byte preservation through the native C API suite instead of ctypes in the Python tests. Use std::numeric_limits for the file-size guard so GCC and Clang builds compile portably. Coding-Agent: Codex Codex-Version: codex-cli 0.144.1 Model: gpt-5.6-sol Reasoning-Effort: xhigh
njzjz
force-pushed
the
fix/readfile-exact-bytes
branch
from
ece5230 to
223f88f
Compare
wanghan-iapcm
approved these changes
Jul 12, 2026
wanghan-iapcm
left a comment
wanghan-iapcm
left a comment
Collaborator
There was a problem hiding this comment.
Verified the fix: DP_ReadFileToChar2 now reports the exact byte count and returns a matching untrimmed buffer via string_to_char_exact, resolving the over-read in read_file_to_string (#5620). The INT_MAX guard correctly precedes the int cast, and the doc-comment nframes corrections match the Compute3/ComputeNList3 implementations. Regression tests (c_api_preserves_exact_bytes, legacy_c_api_preserves_trailing_whitespace) fail on the buggy code and pass with the fix.
Minor, non-blocking: the file-too-large branch (size > INT_MAX) is unexercised by tests, but a 2GB fixture is impractical and the branch mirrors the existing error path.
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
3 participants
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 #5620
Problem
DP_ReadFileToChar气Char2reported the original file size but returned a buffer fromstring_to_char, which trims trailing whitespace before allocating/copying. The C++ wrapper (read_file_to_stringindeepmd.hpp) then reconstructed astd::stringwith the reported (larger) size, causing an over-read of the shorter allocation.Fix
Added
string_to_char_exact, a new helper that preserves every byte without trimming, and use it inDP_ReadFileToChar2andDP_ReadFileToChar. Error-message paths still use the trimmingstring_to_charsince whitespace trimming is desirable there.Test
Added both a C++ test (
test_read_file_to_string.cc) and a Python test (test_c_api_readfile.py) that verify trailing whitespace is preserved. The Python test usesctypesto callDP_ReadFileToChar2directly and the C API.Verified: the test fails against the buggy code (over-reads garbage bytes) and passes with the fix.
Attribution
Generated with opencode using model
glm-5.2.Recommended reviewers
@njzjz (maintainer of the C API)
Summary by CodeRabbit
charge_spinsupport to DeepPot and model-deviation C API computations.charge_spin(including neighbor-list variants).