Skip to content

fix(c-api): contain model deviation frame errors#5794

Merged
njzjz merged 3 commits into
deepmodeling:masterfrom
njzjz:codex/code-scan-5622
Jul 19, 2026
Merged

fix(c-api): contain model deviation frame errors#5794
njzjz merged 3 commits into
deepmodeling:masterfrom
njzjz:codex/code-scan-5622

Conversation

@njzjz

@njzjz njzjz commented Jul 14, 2026

Copy link
Copy Markdown
Member

Summary

  • validate model-deviation frame counts before any model, neighbor-list, or input-array access
  • report unsupported counts through DP_*CheckOK instead of throwing a C++ exception across the C ABI
  • cover all 12 public entry points that accept nframes, including float/double, DeepPot/DeepSpin, neighbor-list/non-neighbor-list, and v2/v3 variants

Root cause and fix

The four shared model-deviation helpers threw std::runtime_error for multiple frames before reaching DP_REQUIRES_OK. Because that exception was outside the existing C API error boundary, a C caller could observe an unwinding C++ exception rather than a retrievable error string.

This change adds one shared validation helper. Any nframes != 1 value now stores a precise error in the model-deviation object's shared exception state and returns immediately. Keeping the check at the top of each shared helper also makes zero and negative counts safe: no pointer range is constructed and no model or neighbor-list object is accessed.

The four deprecated v1 APIs are not in the 12-entry regression matrix because they do not accept nframes and always delegate with one frame.

Regression coverage

The previous tests exercised C API exception formatting and model errors, but did not call the model-deviation compute entry points with an unsupported frame count. Consequently, they never crossed the exact boundary where std::runtime_error escaped.

The new regression uses a fresh error carrier for every public entry point, asserts that no exception escapes, and verifies the exact CheckOK message. On the previous implementation, the nframes = 2 matrix fails immediately at ASSERT_NO_THROW; the additional -1 and 0 cases protect the early-validation invariant.

Validation performed:

  • focused regression: 4 tests passed
  • expanded tests in test_deepmd_exception.cc: 11 tests passed
  • complete no-backend C API target: 19 passed, 215 skipped
  • clang-format 22.1.5 check
  • ruff format --check .
  • ruff check .
  • git diff --check

Closes #5622.

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

Summary by CodeRabbit

  • Bug Fixes
    • Model-deviation C API now rejects unsupported frame counts using the standard error-checking flow, avoiding exceptions reaching C callers.
    • “Exactly one frame” validation is enforced consistently across DeepPot and DeepSpin compute variants, including neighbor-list and precision-specific variants.
    • Validation is performed early to prevent unintended input processing for invalid frame counts.
  • Tests
    • Added boundary and parameterized test coverage to ensure all relevant multi-frame model-deviation C API variants fail with the expected error for unsupported frame counts.

Report unsupported model-deviation frame counts through the C API error state before accessing model, neighbor-list, or array inputs. Add coverage for every public multiframe variant and invalid representative counts.

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 19:00

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
@github-actions github-actions Bot added the C 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: b12cf831-4e2c-4285-9aaf-2c788f43dfd7

📥 Commits

Reviewing files that changed from the base of the PR and between 33dcd2a and 21935a6.

📒 Files selected for processing (2)
  • source/api_c/src/c_api.cc
  • source/api_c/tests/test_deepmd_exception.cc
🚧 Files skipped from review as they are similar to previous changes (2)
  • source/api_c/src/c_api.cc
  • source/api_c/tests/test_deepmd_exception.cc

📝 Walkthrough

Walkthrough

Model-deviation C API compute variants now reject frame counts other than one by recording an error and returning early. Tests verify error propagation across DeepPot and DeepSpin variants, including negative, zero, and multi-frame inputs.

Changes

Model-deviation frame validation

Layer / File(s) Summary
Centralized frame validation and compute integration
source/api_c/src/c_api.cc
Adds a shared exactly-one-frame validator and integrates it into DeepPot, DeepSpin, and neighbor-list model-deviation compute paths.
C API boundary regression coverage
source/api_c/tests/test_deepmd_exception.cc
Tests error reporting across public model-deviation variants and verifies rejection of frame counts -1, 0, and 2.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested labels: Core

Suggested reviewers: iprozd, 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 is concise and accurately reflects the main change: handling model-deviation C API frame errors.
Linked Issues check ✅ Passed The PR matches #5622 by preventing uncaught exceptions, routing unsupported nframes through DP_*CheckOK, and adding regression tests.
Out of Scope Changes check ✅ Passed The code changes stay within the reported scope, limited to model-deviation C API validation and its regression tests.
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.

@codecov

codecov Bot commented Jul 14, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.11650% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.44%. Comparing base (f7d8a53) to head (21935a6).

Files with missing lines Patch % Lines
source/api_c/src/c_api.cc 69.23% 0 Missing and 4 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5794      +/-   ##
==========================================
- Coverage   78.67%   78.44%   -0.23%     
==========================================
  Files        1053     1053              
  Lines      120889   120983      +94     
  Branches     4383     4386       +3     
==========================================
- Hits        95109    94907     -202     
- Misses      24232    24520     +288     
- Partials     1548     1556       +8     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@njzjz
njzjz requested a review from wanghan-iapcm July 14, 2026 22:54
wanghan-iapcm
wanghan-iapcm previously approved these changes Jul 15, 2026

@wanghan-iapcm wanghan-iapcm left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Correct and thorough. The four model-devi helpers threw std::runtime_error from extern C entry points, so a C caller saw an exception unwind across the ABI instead of a retrievable error. Routing through validate_model_devi_nframes, which sets dp->exception and returns, matches the existing C-API error mechanism -- I confirmed DP_DeepPot/SpinModelDevi inherit DP_DeepBaseModelDevi and DP_DeepBaseModelDeviCheckOK returns string_to_char(dpbase->exception). Tightening nframes>1 to nframes!=1 is a real improvement: it rejects 0/negative before the std::vector(coord, coord+natoms*3) construction, which the -1/0 parametrized cases exercise against null pointers. The regression genuinely fails pre-fix (nframes=2 escapes the throw so ASSERT_NO_THROW fails; -1/0 would crash on null-pointer range construction) and covers all 12 nframes-accepting entry points across float/double, Pot/Spin, nlist/non-nlist, v2/v3. LGTM.

@wanghan-iapcm
wanghan-iapcm dismissed their stale review July 15, 2026 15:38

Dismissing to re-review through the /code-review skill per process.

@wanghan-iapcm wanghan-iapcm left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Re-reviewed via the code-review skill. Correct and complete: the four model-devi helpers threw std::runtime_error from extern "C" entry points (UB across the C ABI, unretrievable); routing through validate_model_devi_nframes (sets dp->exception, returns) with nframes!=1 fixes it and also rejects 0/negative before any pointer-range construction. Verified DP_DeepPot/SpinModelDevi inherit DP_DeepBaseModelDevi and DP_DeepBaseModelDeviCheckOK returns string_to_char(exception). A reviewer built both versions and confirmed the tests fail pre-fix (12-variant ASSERT_NO_THROW failures; nframes=-1/0 SIGSEGV) and pass post-fix; all 12 nframes-accepting entry points are covered (exhaustive), and the accept path stays covered by the existing hpp tests. One non-blocking consistency note inline. LGTM.

Comment thread source/api_c/src/c_api.cc
Prefix direct model-deviation validation errors with the same DeePMD-kit Error marker used by exception-based C-API failures.

Coding-Agent: Codex
Codex-Version: codex-cli 0.144.4
Model: gpt-5.6-sol
Reasoning-Effort: xhigh
@njzjz

njzjz commented Jul 18, 2026

Copy link
Copy Markdown
Member Author

Possible reviewers based on changed lines, exact file history, and exact-file review history:

  • @OutisLi — 24 reviews on exact changed files (source/api_c/src/c_api.cc, source/api_c/tests/test_deepmd_exception.cc).

No review request was made automatically.

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

Resolve the C API exception-test conflict by retaining both the model-deviation frame validation coverage and the newer InputNlist move-assignment regression.

Coding-Agent: Codex
Codex-Version: codex-cli 0.144.4
Model: gpt-5.6-sol
Reasoning-Effort: xhigh
@njzjz
njzjz requested a review from OutisLi July 18, 2026 07:25
@njzjz
njzjz added this pull request to the merge queue Jul 19, 2026
Merged via the queue into deepmodeling:master with commit 5a1bbf9 Jul 19, 2026
56 of 57 checks passed
@njzjz
njzjz deleted the codex/code-scan-5622 branch July 19, 2026 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Code scan] Do not throw uncaught C++ exceptions through model-deviation C APIs

4 participants