Skip to content

feat(dpa4): DPA4/SeZM on the NeighborGraph lower — graph-native port, multi-rank, native spin, charge-spin/bridging#5884

Queued
wanghan-iapcm wants to merge 220 commits into
deepmodeling:masterfrom
wanghan-iapcm:feat-graph-dpa4
Queued

feat(dpa4): DPA4/SeZM on the NeighborGraph lower — graph-native port, multi-rank, native spin, charge-spin/bridging#5884
wanghan-iapcm wants to merge 220 commits into
deepmodeling:masterfrom
wanghan-iapcm:feat-graph-dpa4

Conversation

@wanghan-iapcm

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

Copy link
Copy Markdown
Collaborator

Summary

Brings the DPA4/SeZM descriptor fully onto the NeighborGraph ("graph") lower — the sel-free, edge-native route that dpa1 (#5583) and dpa2 (#5779) already use — and adds the conditioning inputs DPA4 needs on that route. Stacked on the merged dpa2 graph work (#5779); this PR is DPA4's turn on the same infrastructure.

Four layers, built and reviewed incrementally:

  1. DPA4 graph-native port — dense call becomes a thin adapter over one graph-native math owner (_call_graph_impl); the model-level graph seam drives DPA4 through call_graph. Parity vs the dense route at fp64 1e-12; parity vs the pt reference at ~6e-15.
  2. Multi-rank (with-comm) graph .pt2 — per-block border_op ghost-feature exchange on the graph lower; graph-kind .pt2 embeds the nested with-comm artifact. Dense (nlist) lower stays comm-less and fails fast on multi-rank; bridging models also fail fast (a rank cannot observe a ghost owner's full outgoing-edge set).
  3. Native spin (magnetic moments) — per-local-atom spin (nf, nloc, 3) conditions the descriptor and is a second autograd leaf giving force_mag = -dE/dspin. Rides the graph route only; frozen to a graph-kind .pt2 (spin at positional ABI index 10), evaluated in Python (DeepEval) and C++ (DeepSpinPTExpt graph route), run in LAMMPS (pair_style deepspin). Single-rank. DPA4NativeSpinModel in dpmodel + pt_expt.
  4. Charge-spin FiLM + SFPG bridging — the last dense-only conditioning inputs, now wired onto the graph route (the edge-native trunk already implemented both; this threads charge_spin and flips the capability gates).

Validation

  • dpmodel ↔ pt reference DPA4 parity: 387 tests.
  • native-spin autograd force_mag vs finite difference: 5.5e-10 (atol 1e-6); pt weight-copied parity 1e-12.
  • charge-spin graph-vs-dense parity 1e-12; SFPG bridging frozen-sphere invariance <1e-10 (ablation reopens the leak >1e-6).
  • C++ (CUDA, Tesla T4): deeppot_universal DPA4 dense + graph rows 38 passed / 8 skipped (NoPbc profile); native-spin DeepSpinPTExpt graph gtest 16 passed (double/float × PBC/NoPbc).
  • LAMMPS (CUDA): DPA4 graph energy/force vs live DeepEval ~1e-13; native-spin deepspin energy/force/force_mag; 2-rank multi-rank fail-fast aborts cleanly.
  • Full Python native-spin + charge-spin + bridging battery green on CUDA (device-conditional tolerances where the graph-route index_add atomics introduce 1-2 fp64 ULP run-to-run nondeterminism).

Known limitations

  • Single-rank only for spin / charge-spin FiLM / bridging. Multi-rank spin-graph (ghost-spin exchange) is a follow-up; the graph-kind spin .pt2 carries has_comm_artifact=false and C++ fails fast on multi-rank. Bridging is intrinsically single-rank (per-node freeze fold over the full outgoing-edge set).
  • Charge-spin FiLM + native spin combined is rejected at build (add_chg_spin_ebd + native spin) — a small follow-up.
  • dpmodel is energy-only for the native-spin wrapper; force/force_mag come from pt_expt autograd.
  • Whole-model conversion of pt-serialized sezm_native_spin checkpoints is not claimed — the deserialize alias covers the type string with dpmodel structure only.
  • Sel-free carry-all divergence when sel binds — the graph route uses the full physical edge set, so it diverges from a sel-truncated dense route only when sel actually binds (the same deliberate divergence as dpa1/dpa2).
  • The legacy pt-side edge_vec .pt2 lower schema (DPA4's original inference rail) is quarantined with DeprecationWarning, not removed — its removal is a follow-up gated on repointing the pt-SeZM freeze (and, for its spin part, on graph-spin support, now landed here).

Summary by CodeRabbit

  • New Features
    • Added native per-atom spin conditioning for DPA4/SeZM on the NeighborGraph route, including magnetic outputs (e.g., force_mag) and spin masking.
    • Introduced native-spin .pt2 export/inference support on the graph route (with appropriate single-rank/MPI behavior).
    • Extended graph-route input/output handling for spin and charge+spin routing.
  • Bug Fixes
    • Improved energy+spin loss for flat per-atom label shapes via consistent reshaping.
  • Documentation
    • Expanded guidance on graph vs dense capabilities, native-spin limitations, and legacy edge_vec deprecation.
  • Tests
    • Added/extended coverage for graph routing, spin outputs, export parity, and multi-rank fail-fast/empty-rank scenarios.

Han Wang and others added 30 commits July 12, 2026 14:18
…erLayer.call_graph parity

Add parametrized test case 'no_chnnl_2_no_gg1' to test_repformer_layer_call_graph_parity
that exercises two previously untested branches:

1. update_chnnl_2=False: Tests the short-circuit path where g2/h2 updates are
   skipped and returned unchanged (lines 2397-2401 in repformers.py call_graph).
   This is the critical production path for the LAST layer of every DPA2 model.

2. cal_gg1=False (gg1=None): Tests the graph path when no gg1-dependent updates
   are needed (all of update_g1_has_drrd, update_g1_has_conv, update_g1_has_attn,
   update_g2_has_g1g1 are False). With g1_out_mlp=False, g1_mlp is seeded with
   the identity [g1] for xp.concat.

Test passes all 30 cases (29 baseline + 1 new).
gen_dpa2.py section B builds a graph-eligible dpa2 (use_three_body=False;
three-body is graph-ineligible), exports the graph-form .pt2 (which embeds
the nested with-comm artifact automatically for message-passing models) plus
an independent dense-nlist .pt2 of the same weights, cross-checks atomic
energies/forces/total virial between the two at gen time (NaN-safe), and
writes deeppot_dpa2_graph.expected from the graph artifact eval.  Unlike the
dpa1 precedent the .expected is not copied from the nlist oracle: for a
message-passing model the graph path's full-to-source per-edge atomic-virial
decomposition legitimately differs from the dense per-atom decomposition
(only the sum agrees), and cross-path e/f noise (~1e-10) sits at the
universal gtest's 1e-10 double tolerance.

The dpa2_graph_ptexpt VariantDeepPotCase row mirrors dpa1_graph_ptexpt
(same tolerances and capability flags).
- Add nall_real == 0 guard at the top of graph with-comm arm (lines 922-929)
  to throw a clear error instead of silently crashing with Dim violation
- Fix stale comment (lines 1050-1051) to reflect that ghost forces fold back
  via both plain and with-comm graph routes
…h-comm route

The with-comm GRAPH artifact derives n_local from the nlocal comm tensor
IN-GRAPH (owned-node energy mask); after move_to_device_pass that
derivation is a device kernel, so feeding it a CPU tensor makes the
kernel read a host pointer as device memory -> CUDA illegal memory
access on every multi-rank run (first live exercise of the route, T4).
The other six comm tensors are consumed only by the opaque border_op
whose host code dereferences their data_ptr, so they stay on CPU --
same placement the dense with-comm route uses for all eight.

Verified on Tesla T4: minimal AOTI repro crashes with CPU nlocal and
matches the plain graph artifact bit-for-bit with device nlocal.
… DEVICE

_trace_and_compile_graph built its synthetic NeighborGraph trace inputs on
the module-level DEVICE constant instead of the device the model's own
parameters/buffers actually live on. In real training these always match
(the trainer moves the model to DEVICE before compiling), but any caller
that intentionally holds the model on a different device (e.g. a test
pinning the model to CPU while running on a CUDA host, mirroring the .pt2
export's model.to("cpu") pattern) hits a device split between the traced
graph tensors and the model params.

DPA2.call_graph's tebd gather (dpmodel/descriptor/dpa2.py:1159) surfaced
this first: xp.take(tebd_table, atype_local, axis=0) requires tebd_table
(a model param, left un-asarray'd to avoid detaching its gradient -- the
documented dpa1 lesson) and atype_local (moved to device(graph.edge_vec))
to share a device. Under test_compiled_training_graph_smoke the model is
pinned to CPU but the trace sample was still built on the global DEVICE
(cuda:0 on a GPU box), so tebd_table stayed CPU while atype_local was CUDA.

DPA1's call_graph has the identical gather pattern and would fail the same
way, but no existing dpa1 test calls _trace_and_compile_graph directly with
a device-pinned model, so the bug was latent there since PR-B (deepmodeling#5604).

Fix: add _model_trace_device(), reading the device off the model's own
parameters/buffers (falling back to DEVICE only if the model exposes
neither), and use it to build the trace sample instead of the global
DEVICE. No change to the gather itself -- gradient flow through
type_embedding is untouched.
…back test

dpa2 became graph-eligible (uses_graph_lower=True), so neighbor_list=None
now routes to the carry-all graph while explicit DefaultNeighborList() forces
the dense route. The smooth repformer attention diverges intentionally between
these paths (sel-independent on graph, sel-padded on dense), amplified through
message passing.

Observed divergence at this fixture:
- energy rel ~1e-4, abs ~9.5e-4
- force abs ~5e-3
- virial abs ~1.3e-2

Set atol=2e-2, rtol=1e-3 to accommodate dpa2's larger divergence compared to
dpa1_smooth/se_atten_v2.
…adapter

DPA2's repinit is hardwired to attn_layer=0, where the dense se_atten body
leaks a phantom padding-neighbor -davg/dstd residual that the graph path
deliberately omits (the physically-correct behavior, accepted as
KNOWN_GRAPH_DENSE_DIVERGENT). So _call_graph_adapter is bit-exact vs
_call_dense only in the trivial-statistics regime (davg=0, dstd=1); for any
model with non-trivial stats it diverges.

The T7 call gate routed the dense .call() through _call_graph_adapter for
graph-eligible configs, which made the cross-backend consistency reference
diverge from the leaky tf/pt/pd/jax dense bodies (source/tests/pt|pd/model/
test_dpa2.py::test_consistency, 71% mismatch) and crashed the universal
zero_forward on empty systems. The graph-native route is reached exclusively
through call_graph (pt_expt forward_atomic_graph + C++ graph .pt2), never
through call, so .call() now always runs _call_dense. _call_graph_adapter is
retained as the bit-exact-regime reference exercised by TestDPA2AdapterBitExact.

Updates TestDPA2CallRouting.test_call_routing_graph_eligible to assert the
corrected routing (call == _call_dense) and corrects the _call_graph_adapter
bit-exactness docstring.
The C++ memleak matrix runs gen scripts with LD_PRELOAD=liblsan (the
sanitizer-instrumented deepmd op .so needs the LSAN runtime). Evaluating the
AOTInductor-compiled deeppot_dpa2_graph.pt2 BACKWARD (forces) under that
runtime segfaults inside the repformer's fused backward kernel
(cpp_fused_..._slice_backward_...): an AOTI-compiled-code vs LeakSanitizer
allocator incompatibility, NOT a graph-code bug. The identical backward is
finite and correct in eager, in the non-memleak C++ ctest, and in LAMMPS;
dpa1's simpler graph .pt2 does not trip it. Leak-checking torch's own compiled
kernels is meaningless (the memleak build exists to leak-check deepmd's C++
ops), so excluding the dpa2 graph artifact from the memleak build loses no real
coverage.

- gen_dpa2.py: skip the graph section (Section B) when LD_PRELOAD contains
  liblsan; the dense DPA2 .pt2 (section A) is still generated.
- test_deeppot_universal.cc: add skip_if_artifact_missing to VariantDeepPotCase
  and set it on the dpa2_graph_ptexpt row, so SetUp GTEST_SKIPs (instead of
  failing) when the artifact is absent under memleak. Every other case still
  hard-fails on a missing artifact.

LAMMPS/ipi tests already run only when !check_memleak, so no other memleak
consumer of the artifact remains.
Han Wang added 6 commits July 26, 2026 01:38
Three capabilities existed only on the energy twin.

A rank with no owned+ghost atoms threw RANK-LOCALLY, which is worse than
a crash: DeepPotPTExpt all-reduces the minimum node count first, and its
comment explains why -- a rank-local throw leaves the non-empty peers
blocked forever in the per-layer border_op collectives. Spin now runs the
same allreduce_min_int preflight, cached on graph_comm_preflight_done_,
so an empty subdomain fails on every rank together instead of hanging the
job.

applyPairExclusionNlist was called three times in DeepPotPTExpt and zero
times here, though init already builds pair_exclude_table_ from metadata.
The three dense branches fed the nlist tensor raw. Masked today because
DPA4 native spin has no dense lower, but any spin .pt2 on the nlist
schema with pair_exclude_types would silently include excluded pairs.

A truly empty rank (nall_real == 0) on the single-rank graph branch fed
n_node = 0 into an artifact exported with Dim(n_node_total, min=1) and
died inside it; it now short-circuits with zeroed outputs, force_mag
included.

The phantom-padding gate was investigated and deliberately NOT narrowed
to DeepPot's form. It looked ungated, but shuffle_exclude_empty leaves
jlist empty exactly when padding triggers (it requires nloc_real == 0),
so there are no stale indices to shift; DeepSpin pads at the host-buffer
level before any tensor exists, which is lower-kind-agnostic by
construction, and copying DeepPot's edge-branch gate would regress the
empty-subdomain case validated in deepmodeling#5485.

Verified: 62 DPA4 spin/ZBL gtests pass locally, now that the venv's
colliding openmpi/mpich headers are sorted out and this box can run them.
Two holes from the variant audit.

Native spin + ZBL had NO coverage below the pt_expt Python layer -- no
.pt2 fixture, no C++ gtest, no LAMMPS test -- and its only export-seam
test is skipped when CI=true, so CI validated the three-way stack by
eager Python alone. gen_dpa4_spin_zbl.py freezes the fixture and
test_deepspin_dpa4_zbl_ptexpt.cc consumes it (8 typed cases across PBC,
NoPBC and the LAMMPS nlist overload).

ZBL bridging had no LAMMPS coverage at all, though its fixture already
existed and the C++ layer already consumed it.
test_lammps_dpa4_zbl_pt2.py adds two single-rank cases plus a fail-fast
assertion under mpirun -- bridged models are single-rank by design, and
that limitation was previously folklore rather than a test.

The generator pins what would otherwise make the tests vacuous: the
analytical term must reproduce an independent double-loop ZBL reference
(including the jittered out_bias of BOTH the analytical child and the
composition -- omitting them misses by 0.277 eV), force_mag must be
non-zero on spin-carrying atoms and EXACTLY zero on others, and the
archive must carry no with-comm artifact.

fp32 uses a magnitude-scaled bound (1e-4 + 5e-7*|expected|) because one
fp32 ULP at this fixture's 1.4e3 force magnitude is 1.22e-4 -- the
suite's flat 1e-4 would demand sub-ULP agreement. fp64 stays at 1e-10,
still ~400x tighter than the representation.
…ition

Two more capabilities the ZBL composition dropped.

atom_exclude_types was NOT forwarded, and my earlier commit 749fd49
justified that with the claim that forwarding would double-apply. That was
wrong. The composition's own atom_excl was None, the learned child masked
only itself, and the analytical child never heard about the exclusion, so
an atom the user excluded still collected its share of the ZBL energy: on
an Ni-O dimer at 0.9 A, excluding type 1 moved the energy only 80.553 ->
80.201 while the ~80 eV analytical term stayed. Masking to zero is
idempotent, so the child keeping its own copy is harmless -- forwarding
applies it once, at the layer that owns the shared graph. Now 80.201 ->
40.219; the residual is correct, being Ni's half of a pair term whose
other half belonged to the excluded atom, exactly as an excluded atom
still contributes to its neighbours' learned energies.

get_intensive and get_compute_stats_distinguish_types fell through to the
base defaults instead of asking the children, so a bridged model would fit
its out-stat bias with the wrong extensivity and the wrong
type-distinguishing rule. Aggregated deliberately differently: intensive
iff EVERY child is (a sum cannot be intensive while one child scales),
type-distinguishing if ANY child needs it (the stricter rule is safe for
children that do not care).

The atom-exclusion regression fails without the fix. The stat-capability
one does NOT discriminate for an energy DPA4 -- its children happen to
agree with the base defaults -- so it is a structural pin only.
get_intensive aggregated with all(), so a composition mixing an intensive
with an extensive child quietly reported False -- the same
plausible-default failure mode as the capability gaps fixed alongside it.

Such a sum is not physically meaningful, so the composition must not exist
at all: LinearAtomicModel.__init__ now validates that every child agrees,
next to the existing mixed-type check, and raises naming the offenders.
The accessor is then a plain read of an invariant rather than a vote.

Regression asserts CONSTRUCTION raises, with an anti-vacuity check that
agreeing children still compose and report their value.
The composition now validates get_intensive() on every child at
construction, and _RecordingAtomicModel is a duck-typed stub that
implements only mixed_types()/get_type_map(). __init__ already required
mixed_types(), so requiring the rest of the interface is consistent;
completing the stub keeps the fail-fast honest rather than weakening it to
skip children that do not implement the interface.

@OutisLi OutisLi 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.

[P1] Preserve explicit lower_kind="nlist" instead of treating graph capability as a requirement

model_uses_graph_lower(m) establishes that a model can/defaults to use the graph lower; it does not establish that the dense lower is unavailable. This branch therefore rewrites every non-graph choice, including an explicit lower_kind="nlist", for every graph-eligible DPA1/DPA2/DPA4 model.

I reproduced this at 0053e2799 with a plain non-spin, non-ZBL DPA4 checkpoint: freeze(..., lower_kind="nlist") passes lower_kind="graph" to deserialize_to_file and changes a suffixless output to .pt2. That conflicts with the CLI contract (--lower-kind nlist is the documented default), _resolve_lower_kind (only auto selects automatically; explicit kinds are preserved), and this PRs own DPA4 export test, which states that the dense kind stays reachable for back-compat. This is numerically observable for models such as DPA1, where the documented graph/dense attention semantics can differ by about 1e-4.

Please keep an explicit nlist request intact. If graph should become the preferred default, expose/use auto for that policy, and represent the distinct graph-required/dense-supported capability explicitly so only models that truly lack a dense lower are forced to graph. This also avoids replacing the earlier concrete-type special case with an overly broad capability test.

@OutisLi OutisLi 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.

[P1] Do not collapse per-child fparam defaults to the first child

The new any(...) plus first-default aggregation in LinearEnergyAtomicModel.has_default_fparam() / get_default_fparam() is not a valid model-level default for a general linear_ener. The composition exposes one external fparam tensor shared by all children, but when it is omitted each fitting uses its own configured default.

I reproduced this at 0053e2799 with two valid learned se_atten energy children whose defaults are [0.0] and [1.0]. The parent reports has_default_fparam=True and get_default_fparam=[0.0]; consequently get_additional_data_requirement marks the input optional and injects [0.0] into both children. The same model gives 2.1515824682051807 for fparam=None (each child uses its own default) and 2.1507697800584133 after the parent fallback, a silent 8.13e-4 change. The compiled training wrappers and frozen metadata use this parent accessor too, so training/exported inference can disagree with eager omission semantics. The PT reference reports no model-level default for this composition and therefore requires an explicit shared fparam in training.

Please consider only active consumers (get_dim_fparam() > 0) and advertise a parent default only when every active child has a compatible dimension and the same default value. Dimension-zero analytical children should be ignored so the intended learned-model plus ZBL composition still inherits the learned default. If active children disagree or one lacks a default, expose no parent default (or reject the incompatible composition explicitly). Please cover differing defaults, matching defaults, and a dimension-zero analytical child.

…hildren

has_default_fparam/get_default_fparam aggregated with any() plus the FIRST
child's value. The composition exposes ONE external tensor to all
children, so advertising one child's default made
get_additional_data_requirement mark the input optional and inject that
value into every child -- silently overriding the others' own defaults.
Reported for two learned se_atten children defaulting to [0.0] and [1.0]:
omitting fparam gave 2.1515824682 (each child using its own) versus
2.1507697801 after the parent fallback, an 8.13e-4 change that also
reaches the compiled training wrappers and the frozen metadata.

Now a parent default is advertised only when every ACTIVE consumer
(get_dim_*() > 0) has a default AND they all agree in shape and value;
otherwise none is exposed, so omission stays omission and each child
applies its own. Dimension-zero children are not consumers and are
excluded, so the intended learned+ZBL composition still inherits the
learned default. The same aggregation applies to charge_spin, which had
the identical defect.

Regressions cover the four cases: differing defaults, matching defaults, a
dimension-zero analytical child, and an active child with no default.

Review 4781085983.
@wanghan-iapcm

Copy link
Copy Markdown
Collaborator Author

Re: this review — fixed in f937b89. Your diagnosis was right and the bug was mine: the accessors were forwarded in 5f837d5 but aggregated with any() plus the first child's value, which is not a valid model-level default for a shared input.

A parent default is now advertised only when every active consumer (get_dim_*() > 0) has one and they all agree in shape and value; otherwise none is exposed, so omission stays omission and each child applies its own default. Dimension-zero children are excluded as non-consumers, so the intended learned + ZBL composition still inherits the learned default, as you asked.

I applied the same aggregation to charge_spin, which had the identical defect and which your report did not mention.

Regressions cover the four cases you listed — differing defaults, matching defaults, a dimension-zero analytical child, and an active child with no default:

case has_default_fparam get_default_fparam
[0.0] / [1.0] False None
[1.0] / [1.0] True [1.0]
learned + ZBL (dim-0 child) True [0.5]
active child, no default False None

Two things I did not do, so the record is accurate: the regressions use a stub child and pin the accessor contract, not your end-to-end 8.13e-4 divergence with two se_atten children — that number in the commit message is your measurement, not mine. And the downstream readers (get_additional_data_requirement, the compiled training wrappers, the frozen metadata) follow from the accessors but are not separately pinned.

Your other P1 (4781068808, explicit lower_kind="nlist") is a policy question I have referred to the author, since the requested graph-required-vs-dense-supported capability was implemented earlier in this branch and removed deliberately. I will follow up on that thread separately rather than silently pick a side.

Han Wang and others added 2 commits July 26, 2026 14:39
The freeze entrypoint forces a graph-capable model onto the graph lower
even when the caller asked for "nlist", because the dense lower is
deprecated in this backend. That override was announced at info level, so
in practice it was silent -- a caller who explicitly passed nlist learned
about it only from the output suffix changing to .pt2.

The two lowers are not numerically identical (dpa1's graph and dense
attention semantics differ by ~1e-4), so an overridden request has to be
visible. Log it at WARNING, naming the requested kind and saying the
outputs may differ.

Regression stubs deserialize_to_file rather than compiling: it pins the
resolution, the warning and the resulting suffix in about a second, and a
real compile would only add minutes without testing more. Verified it
fails when the call is downgraded back to log.info.

This narrows but does not close review 4781068808, which asks for the
explicit request to be honoured rather than merely reported.
Comment thread source/tests/pt_expt/model/test_dpa4_export.py Fixed
Comment thread source/tests/pt_expt/model/test_dpa4_export.py Fixed
Han Wang added 2 commits July 26, 2026 15:09
The flag still claimed 'nlist' was the default and described 'graph' as
opt-in for eligible models, while the entrypoint has forced every
graph-capable model onto the graph lower since 881e208. The help text
now matches: it is consulted only for models that cannot use the graph
lower, and requesting 'nlist' for a graph-capable one is overridden with
a warning.

Also records WHY the gate is 'can use graph' rather than a narrower
'graph is the only route': the dense lower is deprecated and every DPA
model is expected to become graph-capable, so a graph-required capability
would encode a distinction that stops existing.
The new override-logging test imported deepmd.pt_expt.entrypoints.main and
deepmd.pt_expt.utils.serialization as modules while the file already
imports names from both, which CodeQL reports as importing a module with
both 'import' and 'import from'.

Patch by string target instead, which needs neither module import: the
mock target names the source module directly (freeze imports
deserialize_to_file inside the function, so it must be patched there), and
caplog takes the logger name as a literal.
@wanghan-iapcm
wanghan-iapcm requested a review from OutisLi July 26, 2026 07:14
…sion

The previous commit made the DEFAULT require agreement among active
consumers but left the DIMENSION on max() across all children, so two
children wanting different widths of the one shared tensor composed
happily and the wider silently won.

__init__ now rejects consumers that disagree, for fparam and aparam alike,
next to the intensive/extensive check. Children of dimension 0 do not
consume the input and are ignored, so dpa4 + ZBL inherits dpa4's dimension
AND its default rather than being constrained by the analytical term.
max() in the accessors is then exact rather than a guess -- the only other
value present is 0 from a non-consumer -- which the docstrings now say.

aparam has no default concept on the base atomic model, so only its
dimension is validated.

Two test doubles needed completing: they predate the accessors the
composition now validates. Completing them keeps the fail-fast honest
rather than weakening it to skip children that do not implement the
interface.

@OutisLi OutisLi 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.

Reviewed the latest HEAD. The previously requested changes have been addressed.

@OutisLi OutisLi 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.

[P2] Preserve the PT-aligned forward-method ordering in dpa4.py. The PT SeZM implementation deliberately presents this code top-down: construction, the peer forward entrypoints, their shared implementation, the core block computation, and then lower-level helpers. The dpmodel port currently orders the section as call -> _run_graph -> call_graph -> _forward_blocks, so the private _run_graph separates the two peer entrypoints and makes readers traverse roughly 260 lines of internals before reaching call_graph. Please make this a move-only organization change and use __init__ -> call -> call_graph -> _run_graph -> _forward_blocks -> remaining helpers. Do not reorganize it around DPA2 conventions; the goal is to keep the port structurally aligned with the intentionally arranged PT implementation. This should not change runtime, checkpoint, or serialization behavior.

@OutisLi OutisLi 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.

[P2] Correct the PT DPA4 freeze and multi-rank capability matrix in dpa4.md. The important block currently says that the default dp --pt freeze output is a dense-kind .pt2 and is single-rank only. The implementation does the opposite: deepmd.pt.entrypoints.main.freeze detects DPA4/SeZM and calls freeze_sezm_to_pt2; SeZMModel.export_lower_input_kind() returns edge_vec (a compact edge-list ABI, not a padded dense nlist), and a plain model reports supports_edge_parallel()==True, so freeze embeds model/extra/forward_lower_with_comm.pt2 and sets has_comm_artifact=true. The PT export tests explicitly assert that artifact. Consequently the standard PT-frozen plain DPA4 archive supports multi-rank LAMMPS. The text also conflates the fixed PT edge_vec export route with pt_expt --lower-kind graph; these are different graph/edge ABIs, but neither should be described as the default PT dense route. Please rewrite this block to distinguish PT legacy edge_vec from the pt_expt NeighborGraph ABI and state the correct multi-rank support for both plain-energy archives.

Han Wang added 2 commits July 26, 2026 16:14
The private _run_graph sat between the two peer forward entrypoints, so a
reader met roughly 260 lines of internals before reaching call_graph. The
PT SeZM implementation is deliberately arranged top-down, and the dpmodel
port should read the same way beside it.

Order is now __init__ -> call -> call_graph -> _run_graph ->
_forward_blocks -> remaining helpers, following PT rather than DPA2
conventions.

Move-only: the two blocks were swapped whole and the resulting file has an
identical multiset of lines and an identical set of 54 method definitions,
so no body, signature or decorator changed. No runtime, checkpoint or
serialization behaviour is affected.

Review 4781239183.
The important block claimed the default `dp --pt freeze` output is a
dense-kind .pt2 and single-rank only. The implementation does the opposite:
freeze detects DPA4/SeZM and calls freeze_sezm_to_pt2,
SeZMModel.export_lower_input_kind() returns "edge_vec" (a compact edge-list
ABI whose topology is built C++-side, not a padded dense nlist), and
supports_edge_parallel() is False only when an inter_potential is present --
so a plain energy model embeds forward_lower_with_comm.pt2 and DOES support
multi-rank LAMMPS.

The block also conflated the PT edge_vec route with pt_expt's NeighborGraph
`--lower-kind graph`. They are different ABIs consumed by different C++
paths, and neither is a dense route; the text now describes them separately
and states multi-rank support for each.

Corrected in the same pass, all invalidated by work in this branch:
charge/spin conditioning and ZBL bridging are graph-eligible (not
dense-only); native spin supports multi-rank since the with-comm graph spin
export landed; and only the deepspin virtual-atom scheme is genuinely
dense-only. Bridging remains single-rank, for the SFPG reason.

Review 4781248431.
@wanghan-iapcm
wanghan-iapcm enabled auto-merge July 26, 2026 08:25
@wanghan-iapcm
wanghan-iapcm added this pull request to the merge queue Jul 26, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jul 26, 2026
… CUDA

Test CUDA failed in the merge queue (it does not run on PR checks, so every
PR check was green): the pt_expt roundtrip resolved the wire type through
the DPMODEL registry, giving a NumPy-backed model, while _pair_energy feeds
tensors on _env.DEVICE. On CPU that is harmless; on CUDA apply_out_stat
evaluates ret[kk] + out_bias[kk][atype], indexing a NumPy out_bias with a
CUDA atype, and NumPy calls __array__ on it -> can't convert cuda:0 tensor
to numpy.

Restore through type(child) instead. The registry lookup is kept as an
explicit assertion so the wire type is still covered, and a same-class
restore is the stricter comparison anyway.
@wanghan-iapcm
wanghan-iapcm enabled auto-merge July 26, 2026 15:30
@wanghan-iapcm
wanghan-iapcm added this pull request to the merge queue Jul 26, 2026
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.

5 participants