Skip to content

Commit 95b3dee

Browse files
author
Han Wang
committed
fix(pt_expt): keep the graph-lower routing bool out of traced forwards; CI fixes
Three CI failures from the review-round batch: 1. uses_graph_lower() read the persisted 'graph_lower_disabled' buffer via bool(tensor). The predicate runs inside traced forwards (the dense-adapter gate in DescrptDPA1.call), so under torch.export the read became a data-dependent bool(FakeTensor) guard -- GuardOnDataDependentSymNode Eq(u0, 1) on every test_exportable[excl_types*] parametrization. The routing predicate is a plain python bool again; the buffer stays the persistence vehicle, re-synced at load time inside _load_from_state_dict where the incoming tensor is real. All persistence regressions (Trainer restart, state_dict round-trip, pre-knob back-compat) still pass. 2. test_dos_graph relied on non-energy models DEFAULT-flipping to the graph route, which the energy gate deliberately removed; the parity test now opts into the graph route explicitly (neighbor_graph_method='dense'), which remains supported for non-energy models (eager, output-agnostic). 3. pre-commit: missing ANN annotations on the new _load_from_state_dict overrides.
1 parent ad0a954 commit 95b3dee

3 files changed

Lines changed: 48 additions & 37 deletions

File tree

deepmd/pt_expt/descriptor/dpa1.py

Lines changed: 16 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -48,29 +48,28 @@ def disable_graph_lower(self) -> None:
4848
super().disable_graph_lower()
4949
self.graph_lower_disabled.fill_(True)
5050

51-
def uses_graph_lower(self) -> bool:
52-
"""Graph-lower eligibility; the persisted buffer is the source of truth.
53-
54-
``load_state_dict`` (Trainer restart) only restores tensor values, so
55-
the dpmodel-side bool is re-synced from the buffer here before
56-
delegating to the base-class eligibility logic.
57-
58-
Returns
59-
-------
60-
bool
61-
Whether the descriptor routes through the carry-all graph lower.
62-
"""
63-
if bool(self.graph_lower_disabled):
64-
self._graph_lower_disabled = True
65-
return super().uses_graph_lower()
66-
67-
def _load_from_state_dict(self, state_dict, prefix, *args, **kwargs) -> None:
51+
def _load_from_state_dict(
52+
self,
53+
state_dict: dict[str, Any],
54+
prefix: str,
55+
*args: Any,
56+
**kwargs: Any,
57+
) -> None:
6858
# Back-compat: checkpoints written before the knob was persisted lack
6959
# the buffer; default to the fresh module's value (graph enabled)
7060
# instead of failing the strict load.
7161
key = prefix + "graph_lower_disabled"
7262
if key not in state_dict:
7363
state_dict[key] = self.graph_lower_disabled.detach().clone()
64+
else:
65+
# Re-sync the dpmodel-side routing bool from the RESTORED value
66+
# here, at load time, where the incoming tensor is real. The
67+
# routing predicate itself must stay a plain python bool:
68+
# ``uses_graph_lower()`` runs inside traced forwards (the dense
69+
# adapter gate), and reading the buffer there would emit a
70+
# data-dependent ``bool(FakeTensor)`` guard that breaks
71+
# torch.export (GuardOnDataDependentSymNode Eq(u0, 1)).
72+
self._graph_lower_disabled = bool(state_dict[key])
7473
super()._load_from_state_dict(state_dict, prefix, *args, **kwargs)
7574

7675
def share_params(

deepmd/pt_expt/descriptor/dpa2.py

Lines changed: 16 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -52,29 +52,28 @@ def disable_graph_lower(self) -> None:
5252
super().disable_graph_lower()
5353
self.graph_lower_disabled.fill_(True)
5454

55-
def uses_graph_lower(self) -> bool:
56-
"""Graph-lower eligibility; the persisted buffer is the source of truth.
57-
58-
``load_state_dict`` (Trainer restart) only restores tensor values, so
59-
the dpmodel-side bool is re-synced from the buffer here before
60-
delegating to the base-class eligibility logic.
61-
62-
Returns
63-
-------
64-
bool
65-
Whether the descriptor routes through the carry-all graph lower.
66-
"""
67-
if bool(self.graph_lower_disabled):
68-
self._graph_lower_disabled = True
69-
return super().uses_graph_lower()
70-
71-
def _load_from_state_dict(self, state_dict, prefix, *args, **kwargs) -> None:
55+
def _load_from_state_dict(
56+
self,
57+
state_dict: dict[str, Any],
58+
prefix: str,
59+
*args: Any,
60+
**kwargs: Any,
61+
) -> None:
7262
# Back-compat: checkpoints written before the knob was persisted lack
7363
# the buffer; default to the fresh module's value (graph enabled)
7464
# instead of failing the strict load.
7565
key = prefix + "graph_lower_disabled"
7666
if key not in state_dict:
7767
state_dict[key] = self.graph_lower_disabled.detach().clone()
68+
else:
69+
# Re-sync the dpmodel-side routing bool from the RESTORED value
70+
# here, at load time, where the incoming tensor is real. The
71+
# routing predicate itself must stay a plain python bool:
72+
# ``uses_graph_lower()`` runs inside traced forwards (the dense
73+
# adapter gate), and reading the buffer there would emit a
74+
# data-dependent ``bool(FakeTensor)`` guard that breaks
75+
# torch.export (GuardOnDataDependentSymNode Eq(u0, 1)).
76+
self._graph_lower_disabled = bool(state_dict[key])
7877
super()._load_from_state_dict(state_dict, prefix, *args, **kwargs)
7978

8079
def share_params(

source/tests/pt_expt/model/test_dos_graph.py

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,8 @@
55
routes into the graph path by default. Before the general output transform this
66
KeyError'd on ``"energy"``; now every fitting (dos/dipole/polar/property/...)
77
flows through :func:`fit_output_to_model_output_graph` with no change on the
8-
fitting side. Each model's graph forward (default ``neighbor_graph_method``)
8+
fitting side. Each model's graph forward (EXPLICIT ``neighbor_graph_method``
9+
opt-in; non-energy models default to dense since the energy gate)
910
must match the dense path (``neighbor_graph_method="legacy"``) on every shared
1011
key (carry-all graph at non-binding ``sel`` reproduces the dense neighbor set).
1112
"""
@@ -129,8 +130,14 @@ def test_dos_repro(self) -> None:
129130
[_make_dos, _make_dipole, _make_polar, _make_property],
130131
) # one builder per fitting kind
131132
def test_graph_matches_dense(self, make_model) -> None:
132-
"""Graph (default) output matches the dense (``legacy``) path on every
133+
"""EXPLICIT graph opt-in matches the dense (``legacy``) path on every
133134
shared key, including derivatives for r/c-differentiable fittings.
135+
136+
Non-energy models no longer DEFAULT-flip to the graph route (the
137+
compiled-training trace is energy-specific; see the
138+
``_resolve_graph_method`` energy gate), but the eager graph lower
139+
stays output-agnostic and available via an explicit
140+
``neighbor_graph_method`` -- which is what this parity test pins.
134141
"""
135142
tol = (
136143
{"rtol": 1e-11, "atol": 1e-11}
@@ -139,7 +146,13 @@ def test_graph_matches_dense(self, make_model) -> None:
139146
)
140147
ds = _make_descriptor()
141148
m = make_model(ds)
142-
graph = m.call_common(self.coord, self.atype, None, do_atomic_virial=True)
149+
graph = m.call_common(
150+
self.coord,
151+
self.atype,
152+
None,
153+
do_atomic_virial=True,
154+
neighbor_graph_method="dense",
155+
)
143156
# the dense path differentiates w.r.t. coord -> needs a coord leaf.
144157
dense = m.call_common(
145158
self.coord.detach().requires_grad_(True),

0 commit comments

Comments
 (0)