fix(dpmodel): dpa1 dense call must not route through divergent graph adapter#5785
Conversation
…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.
📝 WalkthroughWalkthroughDPA1 public ChangesDPA1 routing and parity
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
There was a problem hiding this comment.
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 winStale 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 bytest_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
📒 Files selected for processing (5)
deepmd/dpmodel/descriptor/dpa1.pysource/tests/common/dpmodel/test_dpa1_call_graph_descriptor.pysource/tests/common/dpmodel/test_dpa1_graph_attention_parity.pysource/tests/pd/model/test_dpa1.pysource/tests/pt/model/test_dpa1.py
Codecov Report✅ All modified and coverable lines are covered by tests. 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. 🚀 New features to boost your workflow:
|
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.
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).
Problem
DescrptDPA1.callroutes graph-eligible configs through_call_graph_adapter(decision #14: dense call = thin adapter). That adapter is bit-exact vs_call_denseonly in the trivial-statistics regime (davg == 0): the dense se_atten body leaks a phantom padding-neighbor-davg/dstdresidual (EnvMat.callsubtractsdavgafter the padding rows' geometry is weight-zeroed; with emptyexclude_typesnothing re-masks it, at anyattn_layer) that the graph path deliberately omits. Measured on the consistency fixture: dense vs adapter agree at 0.0 fordavg=0, diverge by ~20 (abs) for nonzerodavg, at bothattn_layer=0and2.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)andcall(...)returned different descriptors for the same physical system.Why it never surfaced
The pt/pd consistency tests invoke
dd2.call()withoutmapping, 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_atomicalways forwardsmapping. Consequence: dpmodel/jax/tf2 inference of a trained dpa1 withset_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.callalways runs_call_dense— it is the cross-backend consistency reference. The graph-native route is reached exclusively throughcall_graph(pt_exptforward_atomic_graphand the graph.pt2); nothing in shipping code consumed thecallflip._call_graph_adapteris 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
test_dpa1.py::test_consistencynow passmapping(the production invocation). Red on the old routing (71%-scale mismatch), green with the fix.call(mapping) == call(None) == _call_denseat zero tolerance with nonzero injecteddavgand ghosts present.test_dpa1_call_graph_descriptor.py,test_dpa1_graph_attention_parity.py) exercise_call_graph_adapterdirectly instead of relying on the removedcallrouting — adapter coverage is unchanged.Notes
uses_graph_lowerstill gates them; it no longer influences the densecall).Summary by CodeRabbit
Bug Fixes
Tests