Skip to content

Commit d725272

Browse files
author
Han Wang
committed
fix: three canonical-bridging P1s from review round 2
1. Sugar expansion keeps trainer-owned top-level `lora` at the composition level instead of forwarding it to the learned child (which the pt bridge builder rejects), restoring the concise dpa4+bridging_method+lora form. The key routing is now a four-way table (composition / consumed / trainer / learned-child) whose coverage of the standard+dpa4 argcheck schemas is pinned by test_routing_covers_the_argcheck_schema: a new argcheck model key fails the test until it gets an explicit routing decision, instead of silently landing on the child. The guard immediately surfaced use_compile and enable_tf32 as previously-undecided keys (both routed to the learned child, preserving behavior). 2. pt LinearEnergyModel.update_sel: the shared-config reconstruction loop now skips the `inner_potential` child too. Normalization always inserts `shared_dict: {}`, so the default CLI path entered the reconstruction loop and dereferenced the analytical child's missing descriptor (KeyError). New test runs on a NORMALIZED config. 3. The shared dpmodel/pt_expt linear builder rejects bridged compositions with a third child (e.g. pairtab): no common execution route exists (pairtab is dense-only, the bridged pair graph-only), matching the pt builder's exact-two-child constraint and message.
1 parent 2f32baf commit d725272

7 files changed

Lines changed: 198 additions & 9 deletions

File tree

deepmd/dpmodel/model/model_factory.py

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -214,11 +214,14 @@ def get_linear_atomic_model(
214214
"A linear_ener composition supports at most one "
215215
"`inner_potential` sub-model."
216216
)
217-
if len(learned_indices) != 1:
217+
if len(learned_indices) != 1 or len(children) != 2:
218+
# A third child (e.g. pairtab) has no common execution route
219+
# with the graph-only bridged pair; reject at construction
220+
# like the pt builder does.
218221
raise ValueError(
219222
"An `inner_potential` sub-model bridges exactly one learned "
220-
f"sibling, but got {len(learned_indices)} sub-models with a "
221-
"descriptor."
223+
"sibling: expected a linear_ener composition over "
224+
"[learned, inner_potential]."
222225
)
223226
if str(data.get("weights", "mean")) != "sum":
224227
raise ValueError(

deepmd/pt/model/model/dp_linear_model.py

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -370,6 +370,9 @@ def get_shared_key(shared_ref: str) -> str:
370370
if "type_map" not in ret_jdata:
371371
ret_jdata["type_map"] = deepcopy(type_map)
372372
for idx, original_sub_model in enumerate(original_models):
373+
if original_sub_model.get("type") == "inner_potential":
374+
# analytical child: no descriptor to write back
375+
continue
373376
if "tab_file" in original_sub_model:
374377
continue
375378
updated_sub_model = local_jdata_cpy["models"][idx]

deepmd/utils/bridging.py

Lines changed: 48 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -66,17 +66,57 @@ def _is_dpa4_family(sub: dict) -> bool:
6666
return any(_is_dpa4_family(sub) for sub in children)
6767

6868

69-
# Top-level keys that belong to the composition, not to the learned child.
69+
# Routing of the concise-form top-level keys during sugar expansion. Every
70+
# key the `standard`/`dpa4` argcheck schemas declare must appear in exactly
71+
# one tuple below: a schema-coverage test derives the key universe from
72+
# `deepmd.utils.argcheck` and fails when a new key is left unrouted, so
73+
# adding a model key forces an explicit routing decision here.
74+
75+
# Keys that belong to the composition, not to the learned child.
7076
_COMPOSITION_KEYS = (
7177
"type",
7278
"type_map",
7379
"spin",
7480
"atom_exclude_types",
7581
"pair_exclude_types",
82+
)
83+
# Keys consumed by the expansion itself; they appear in neither the
84+
# composition nor the learned child.
85+
_CONSUMED_KEYS = (
7686
"bridging_method",
7787
"bridging_r_inner",
7888
"bridging_r_outer",
7989
)
90+
# Training-owned keys: the trainer reads them from the top level of the
91+
# model section, so they stay at the composition level and must never be
92+
# forwarded to a sub-model.
93+
_TRAINER_KEYS = ("lora",)
94+
# Keys that configure the learned model and are forwarded to the learned
95+
# child. This tuple is not consulted at expansion time (the child receives
96+
# every key not routed above); it exists so the schema-coverage test can
97+
# assert that every argcheck key has an explicit routing decision.
98+
_LEARNED_CHILD_KEYS = (
99+
"descriptor",
100+
"fitting_net",
101+
"model_branch_alias",
102+
"info",
103+
"use_compile",
104+
"enable_tf32",
105+
"data_stat_nbatch",
106+
"data_stat_protect",
107+
"data_bias_nsample",
108+
"use_srtab",
109+
"smin_alpha",
110+
"sw_rmin",
111+
"sw_rmax",
112+
"preset_out_bias",
113+
"srtab_add_bias",
114+
"type_embedding",
115+
"modifier",
116+
"compress",
117+
"finetune_head",
118+
)
119+
_NON_CHILD_KEYS = _COMPOSITION_KEYS + _CONSUMED_KEYS + _TRAINER_KEYS
80120

81121

82122
def expand_bridging_method(data: dict) -> dict:
@@ -87,8 +127,9 @@ def expand_bridging_method(data: dict) -> dict:
87127
deep-copied and rewritten to the canonical composition form: a
88128
``linear_ener`` model with ``weights: "sum"`` over the learned
89129
sub-model and an ``inner_potential`` sub-model. The exclusion lists
90-
move to the composition level; a top-level ``spin`` section stays at
91-
the top level; every other key stays on the learned child.
130+
move to the composition level; a top-level ``spin`` section and the
131+
training-owned keys (``lora``) stay at the top level; every other key
132+
stays on the learned child.
92133
93134
For backward compatibility with the legacy pt ``type: "dpa4"``
94135
builder, ``descriptor.exclude_types`` is promoted to the composition's
@@ -142,9 +183,7 @@ def expand_bridging_method(data: dict) -> dict:
142183
else:
143184
pair_exclude_types = descriptor_exclude_types
144185

145-
learned = {
146-
key: value for key, value in data.items() if key not in _COMPOSITION_KEYS
147-
}
186+
learned = {key: value for key, value in data.items() if key not in _NON_CHILD_KEYS}
148187
learned["type"] = model_type
149188
learned["type_map"] = copy.deepcopy(data["type_map"])
150189
canonical = {
@@ -165,4 +204,7 @@ def expand_bridging_method(data: dict) -> dict:
165204
}
166205
if "spin" in data:
167206
canonical["spin"] = data["spin"]
207+
for key in _TRAINER_KEYS:
208+
if key in data:
209+
canonical[key] = data[key]
168210
return canonical

source/tests/common/dpmodel/test_zbl_bridging.py

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,29 @@ def test_builder_composes_linear_model():
7676
assert float(dp_child.descriptor.inner_clamp.r_inner) == 0.8
7777

7878

79+
def test_third_child_without_common_route_raises():
80+
"""[learned, inner_potential, pairtab] has no common execution route
81+
(pairtab is dense-only, the bridged pair is graph-only): the builder
82+
must reject it at construction like the pt backend does.
83+
"""
84+
cfg = {
85+
"type": "linear_ener",
86+
"weights": "sum",
87+
"type_map": ["Ni", "O"],
88+
"models": [
89+
{
90+
"type": "dpa4",
91+
"descriptor": copy.deepcopy(ZBL_CONFIG["descriptor"]),
92+
"fitting_net": copy.deepcopy(ZBL_CONFIG["fitting_net"]),
93+
},
94+
{"type": "inner_potential", "mode": "ZBL"},
95+
{"type": "pairtab", "tab_file": "unused.txt", "rcut": 4.0, "sel": 8},
96+
],
97+
}
98+
with pytest.raises(ValueError, match="exactly one learned"):
99+
get_model(cfg)
100+
101+
79102
def test_zbl_child_equals_composition_minus_learned():
80103
"""Composition energy == learned child + analytical child (exact sum)."""
81104
model = get_model(copy.deepcopy(ZBL_CONFIG))

source/tests/common/test_bridging.py

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -103,6 +103,60 @@ def test_other_model_keys_stay_on_learned_child() -> None:
103103
assert "preset_out_bias" not in out
104104

105105

106+
def test_lora_stays_top_level() -> None:
107+
"""`lora` is training-owned: the pt trainer reads it from the top
108+
level of the model section, so the expansion must keep it there and
109+
never forward it to the learned child (which the pt bridge builder
110+
rejects).
111+
"""
112+
data = _flag_config()
113+
data["lora"] = {"rank": 2, "alpha": None}
114+
out = expand_bridging_method(data)
115+
assert out["lora"] == {"rank": 2, "alpha": None}
116+
assert "lora" not in out["models"][0]
117+
118+
119+
def test_routing_covers_the_argcheck_schema() -> None:
120+
"""Every key the `standard`/`dpa4` argcheck schemas declare must have
121+
an explicit routing decision in the expansion. Adding a model key to
122+
argcheck without deciding its routing fails here instead of silently
123+
landing on the learned child (how top-level `lora` once broke).
124+
"""
125+
from deepmd.utils.argcheck import (
126+
model_args,
127+
sezm_model_args,
128+
standard_model_args,
129+
)
130+
from deepmd.utils.bridging import (
131+
_COMPOSITION_KEYS,
132+
_CONSUMED_KEYS,
133+
_LEARNED_CHILD_KEYS,
134+
_TRAINER_KEYS,
135+
)
136+
137+
schema_keys = {"type"}
138+
schema_keys |= set(model_args(exclude_hybrid=True).sub_fields)
139+
schema_keys |= set(standard_model_args().sub_fields)
140+
schema_keys |= set(sezm_model_args().sub_fields)
141+
142+
routing = [
143+
set(_COMPOSITION_KEYS),
144+
set(_CONSUMED_KEYS),
145+
set(_TRAINER_KEYS),
146+
set(_LEARNED_CHILD_KEYS),
147+
]
148+
routed = set().union(*routing)
149+
assert sum(len(s) for s in routing) == len(routed), (
150+
"a key is routed to more than one destination"
151+
)
152+
assert schema_keys == routed, (
153+
f"unrouted argcheck keys: {sorted(schema_keys - routed)}; "
154+
f"routed keys absent from the schema: {sorted(routed - schema_keys)}. "
155+
"Decide the routing in deepmd.utils.bridging and update the "
156+
"corresponding tuple."
157+
)
158+
159+
106160
def test_exclusions_move_to_composition_level() -> None:
107161
data = _flag_config()
108162
data["pair_exclude_types"] = [[0, 1]]

source/tests/pt/model/test_get_model_bridging.py

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -252,6 +252,57 @@ def test_is_sezm_checkpoint_recognizes_canonical_params(tmp_path) -> None:
252252
assert is_sezm_checkpoint(ckpt2)
253253

254254

255+
def test_sugar_with_top_level_lora_builds() -> None:
256+
"""The concise dpa4+bridging form with trainer-owned top-level `lora`
257+
must keep building: the expansion routes `lora` to the composition
258+
level, so the bridge builder never sees it on the learned child.
259+
"""
260+
from deepmd.pt.train.training import (
261+
get_model_for_wrapper,
262+
)
263+
264+
cfg = _sugar_config()
265+
cfg["lora"] = {"rank": 2, "alpha": None}
266+
model = get_model_for_wrapper(copy.deepcopy(cfg))
267+
assert isinstance(model, SeZMModel)
268+
# The trainer injects the adapters later by reading the top level of
269+
# its own (unexpanded) config; expansion must not have mutated it.
270+
assert cfg["lora"] == {"rank": 2, "alpha": None}
271+
272+
273+
def test_update_sel_normalized_config_skips_inner_potential_child(
274+
monkeypatch,
275+
) -> None:
276+
"""The default CLI path hands `update_sel` a NORMALIZED config, where
277+
argcheck always inserts `shared_dict: {}`. Both the update loop and
278+
the shared-config reconstruction loop must skip the analytical child.
279+
"""
280+
from deepmd.pt.model.model import (
281+
LinearEnergyModel,
282+
)
283+
from deepmd.pt.model.model.dp_model import (
284+
DPModelCommon,
285+
)
286+
from deepmd.utils.argcheck import (
287+
model_args,
288+
)
289+
290+
seen = []
291+
292+
def _fake_update_sel(train_data, type_map, sub):
293+
seen.append(copy.deepcopy(sub))
294+
return sub, 0.9
295+
296+
monkeypatch.setattr(DPModelCommon, "update_sel", staticmethod(_fake_update_sel))
297+
cfg = model_args().normalize_value(_canonical_config(), trim_pattern="_*")
298+
assert cfg["shared_dict"] == {} # inserted by normalization
299+
updated, min_dist = LinearEnergyModel.update_sel(None, cfg["type_map"], cfg)
300+
assert min_dist == 0.9
301+
assert len(seen) == 1 # only the learned child
302+
assert "descriptor" in seen[0]
303+
assert updated["models"][1]["type"] == "inner_potential"
304+
305+
255306
def test_update_sel_skips_inner_potential_child(monkeypatch) -> None:
256307
"""Neighbor-stat selection must skip the analytical child instead of
257308
crashing on its missing descriptor.

source/tests/pt_expt/model/test_get_model_bridging.py

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -194,6 +194,19 @@ def _canonical_config() -> dict:
194194
}
195195

196196

197+
def test_third_child_without_common_route_raises() -> None:
198+
"""[learned, inner_potential, pairtab] has no common execution route
199+
(pairtab is dense-only, the bridged pair is graph-only): the shared
200+
builder must reject it at construction like the pt backend does.
201+
"""
202+
cfg = _canonical_config()
203+
cfg["models"].append(
204+
{"type": "pairtab", "tab_file": "unused.txt", "rcut": 4.0, "sel": 8}
205+
)
206+
with pytest.raises(ValueError, match="exactly one learned"):
207+
get_model(cfg)
208+
209+
197210
def test_canonical_composition_builds() -> None:
198211
"""The canonical spelling composes [learned, InnerPotential] with the
199212
clamp radii derived onto the learned child's descriptor.

0 commit comments

Comments
 (0)