Skip to content

fix(dpmodel): dpa1 dense call must not route through divergent graph adapter#5785

Merged
wanghan-iapcm merged 2 commits into
deepmodeling:masterfrom
wanghan-iapcm:fix-dpa1-dense-call
Jul 16, 2026
Merged

fix(dpmodel): dpa1 dense call must not route through divergent graph adapter#5785
wanghan-iapcm merged 2 commits into
deepmodeling:masterfrom
wanghan-iapcm:fix-dpa1-dense-call

Conversation

@wanghan-iapcm

@wanghan-iapcm wanghan-iapcm commented Jul 13, 2026

Copy link
Copy Markdown
Collaborator

Problem

DescrptDPA1.call routes graph-eligible configs through _call_graph_adapter (decision #14: dense call = thin adapter). That adapter is bit-exact vs _call_dense only in the trivial-statistics regime (davg == 0): the dense se_atten body leaks a phantom padding-neighbor -davg/dstd residual (EnvMat.call subtracts davg after the padding rows' geometry is weight-zeroed; with empty exclude_types nothing re-masks it, at any attn_layer) that the graph path deliberately omits. Measured on the consistency fixture: dense vs adapter agree at 0.0 for davg=0, diverge by ~20 (abs) for nonzero davg, at both attn_layer=0 and 2.

The gate also made mapping — an argument that only enables ghost folding on graph routes — silently change the numerics of a dense call: call(..., mapping) and call(...) returned different descriptors for the same physical system.

Why it never surfaced

The pt/pd consistency tests invoke dd2.call() without mapping, so the routing gate (mapping is not None or nall == nloc) is off and the tests exercise the dense body — a code path production never takes: DPAtomicModel.forward_atomic always forwards mapping. Consequence: dpmodel/jax/tf2 inference of a trained dpa1 with set_davg_zero=False (the constructor default) silently used the adapter and differed from the pt/tf dense backends. Cross-backend suites stayed green only because fresh models sit in the trivial-stat regime.

This is the dpa1 twin of the dpa2 routing fix in #5779 (wanghan-iapcm/deepmd-kit@bc0d70ade, where the same class of divergence was caught by CI because the dpa2 consistency test does pass mapping).

Fix

  • DescrptDPA1.call always runs _call_dense — it is the cross-backend consistency reference. The graph-native route is reached exclusively through call_graph (pt_expt forward_atomic_graph and the graph .pt2); nothing in shipping code consumed the call flip.
  • _call_graph_adapter is retained as the bit-exact-regime reference; its docstring now states the actual regime (including the slot-0 statistics simplification, exact for real slot-uniform stat tables).

Tests

  • pt/pd test_dpa1.py::test_consistency now pass mapping (the production invocation). Red on the old routing (71%-scale mismatch), green with the fix.
  • New regression: call(mapping) == call(None) == _call_dense at zero tolerance with nonzero injected davg and ghosts present.
  • Adapter parity tests (test_dpa1_call_graph_descriptor.py, test_dpa1_graph_attention_parity.py) exercise _call_graph_adapter directly instead of relying on the removed call routing — adapter coverage is unchanged.
  • Local sweep: dpa1/se_atten_v2 graph+consistency+universal battery — 94 + 304 + 852 passed.

Notes

Summary by CodeRabbit

  • Bug Fixes

    • Ensured descriptor calculations consistently use the dense path, regardless of mapping or graph-related conditions.
    • Preserved graph-based processing through its dedicated pathway.
    • Prevented mapping settings from changing dense numerical results.
  • Tests

    • Expanded parity and consistency coverage across dense and graph-based calculations.
    • Added regression checks for mapping, ghost atoms, exclusions, and single-rank scenarios.
    • Improved validation of bit-exact results and production-path behavior.

…adapter

DescrptDPA1.call routed graph-eligible configs through _call_graph_adapter
(decision deepmodeling#14: dense call = thin adapter). The adapter is bit-exact vs
_call_dense ONLY in the trivial-statistics regime (davg == 0): the dense
se_atten body leaks a phantom padding-neighbor -davg/dstd residual
(EnvMat.call subtracts davg AFTER the padding rows' geometry is
weight-zeroed; with empty exclude_types nothing re-masks it, at ANY
attn_layer) that the graph path deliberately omits. Measured on the
consistency fixture: dense vs adapter agree at 0.0 for davg=0, diverge by
~20 (abs) for nonzero davg, both attn_layer=0 and 2. The gate also made
'mapping' -- an argument that only enables ghost folding on graph routes --
silently change dense numerics.

The divergence never surfaced because the pt/pd consistency tests invoke
dd2.call() WITHOUT mapping (gate off -> dense route), while production
model-level forwards (DPAtomicModel.forward_atomic) always pass mapping:
dpmodel/jax/tf2 inference of a trained dpa1 with set_davg_zero=False (the
constructor default) silently used the adapter and differed from the pt/tf
dense backends. Cross-backend suites stayed green only because fresh
models sit in the trivial-stat regime.

Fix (same pattern as the dpa2 graph PR):
- DescrptDPA1.call always runs _call_dense; the graph-native route is
  reached exclusively through call_graph (pt_expt forward_atomic_graph and
  the graph .pt2). _call_graph_adapter is retained as the bit-exact-regime
  reference and its docstring now states the actual regime (incl. the
  slot-0 statistics simplification).
- pt/pd consistency tests now pass mapping (the production invocation);
  red on the old routing, green now.
- New regression test: call(mapping) == call(None) == _call_dense with
  nonzero injected davg and ghosts present.
- Adapter parity tests exercise _call_graph_adapter directly instead of
  relying on the removed call routing.
@dosubot dosubot Bot added the bug label Jul 13, 2026
@wanghan-iapcm
wanghan-iapcm requested a review from OutisLi July 13, 2026 12:24
@coderabbitai

coderabbitai Bot commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

DPA1 public call now always uses the dense implementation. Graph adapter parity tests call _call_graph_adapter directly, while regression and implementation consistency tests verify mapping does not alter dense results.

Changes

DPA1 routing and parity

Layer / File(s) Summary
Dense public call contract
deepmd/dpmodel/descriptor/dpa1.py
DescrptDPA1.call always invokes _call_dense; _call_graph_adapter documentation specifies its dense-to-graph role and parity limits.
Graph adapter parity coverage
source/tests/common/dpmodel/test_dpa1_call_graph_descriptor.py, source/tests/common/dpmodel/test_dpa1_graph_attention_parity.py
Parity tests directly compare _call_graph_adapter with _call_dense, while regression tests verify that public call results are unchanged by mapping.
Mapped implementation consistency
source/tests/pd/model/test_dpa1.py, source/tests/pt/model/test_dpa1.py
DP and PT consistency tests pass mapping through descriptor calls and preserve dense numerical comparisons.

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

Possibly related PRs

Suggested labels: Python

🚥 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 Title clearly states the main fix: DPA1 dense call no longer routes through the graph adapter.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ 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.

@coderabbitai coderabbitai Bot 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.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
source/tests/common/dpmodel/test_dpa1_call_graph_descriptor.py (1)

99-103: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Stale comment references a removed test.

This NOTE still points to test_eligible_no_mapping_with_ghosts_falls_back below, but that test was removed and replaced by test_call_is_mapping_insensitive_with_nontrivial_stats (which asserts mapping-insensitivity, not a "falls back to dense" gate). Update the comment to avoid confusing future readers.

📝 Suggested comment fix
-    # NOTE: after the strip PR (`#5747`) and this PR both landed, neither strip
-    # nor exclude_types falls back to dense — both are graph-eligible. The only
-    # non-disabled ineligible config is compression, whose gate is covered by
-    # TestDpa1StripRouting.test_uses_graph_lower_strip_gate, and the no-mapping
-    # ghost fallback by test_eligible_no_mapping_with_ghosts_falls_back below.
+    # NOTE: after the strip PR (`#5747`) and this PR both landed, neither strip
+    # nor exclude_types falls back to dense — both are graph-eligible. The only
+    # non-disabled ineligible config is compression, whose gate is covered by
+    # TestDpa1StripRouting.test_uses_graph_lower_strip_gate. mapping-insensitivity
+    # of the dense `call` is covered by test_call_is_mapping_insensitive_with_nontrivial_stats
+    # below.
🤖 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/common/dpmodel/test_dpa1_call_graph_descriptor.py` around lines
99 - 103, Update the NOTE comment near the DPA1 call-graph descriptor tests to
remove the reference to the deleted test and replace the “ghost fallback”
wording with an accurate reference to
test_call_is_mapping_insensitive_with_nontrivial_stats, reflecting its
mapping-insensitivity assertion.
🤖 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.

Outside diff comments:
In `@source/tests/common/dpmodel/test_dpa1_call_graph_descriptor.py`:
- Around line 99-103: Update the NOTE comment near the DPA1 call-graph
descriptor tests to remove the reference to the deleted test and replace the
“ghost fallback” wording with an accurate reference to
test_call_is_mapping_insensitive_with_nontrivial_stats, reflecting its
mapping-insensitivity assertion.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: b54ffa01-e5a9-471e-8d7c-191ff6994b80

📥 Commits

Reviewing files that changed from the base of the PR and between c0ad8bf and 5c14c07.

📒 Files selected for processing (5)
  • deepmd/dpmodel/descriptor/dpa1.py
  • source/tests/common/dpmodel/test_dpa1_call_graph_descriptor.py
  • source/tests/common/dpmodel/test_dpa1_graph_attention_parity.py
  • source/tests/pd/model/test_dpa1.py
  • source/tests/pt/model/test_dpa1.py

@codecov

codecov Bot commented Jul 13, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.48%. Comparing base (c0ad8bf) to head (5c14c07).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5785      +/-   ##
==========================================
- Coverage   79.76%   79.48%   -0.28%     
==========================================
  Files        1023     1023              
  Lines      117415   117410       -5     
  Branches     4324     4328       +4     
==========================================
- Hits        93657    93328     -329     
- Misses      22215    22526     +311     
- Partials     1543     1556      +13     

☔ 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.

@wanghan-iapcm
wanghan-iapcm enabled auto-merge July 16, 2026 00:13
Resolves the both-add conflict in test_dpa1_call_graph_descriptor.py with
master's pair-exclude merge (deepmodeling#5733, 98c3ab0): keep both tests --
master's test_eligible_no_mapping_with_ghosts_falls_back (docstring
adapted: the public call is now unconditionally dense on this branch)
and this branch's test_call_is_mapping_insensitive_with_nontrivial_stats.
@wanghan-iapcm
wanghan-iapcm added this pull request to the merge queue Jul 16, 2026
Merged via the queue into deepmodeling:master with commit 34d1fdb Jul 16, 2026
47 checks passed
@wanghan-iapcm
wanghan-iapcm deleted the fix-dpa1-dense-call branch July 16, 2026 07:28
wanghan-iapcm pushed a commit to wanghan-iapcm/deepmd-kit that referenced this pull request Jul 16, 2026
Conflict: source/api_cc/src/common.cc -- both sides fixed the same
daparam-stride aparam buffer sizing (deepmodeling#5790 upstream, e95f2ba here);
code identical, comment text taken from upstream so the hunk drops out
of this PR's diff. deepmd/dpmodel/descriptor/dpa1.py auto-merged with
master's deepmodeling#5785 (dense call is unconditionally _call_dense; verified
post-merge).
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.

2 participants