Skip to content

Commit 187ad36

Browse files
author
Han Wang
committed
fix: canonical-bridging review round 3
- dpmodel LinearEnergyModel gains a composite update_sel (twin of pt_expt): update learned children, skip inner_potential/pairtab, aggregate min_nbor_dist; BaseModel.update_sel dispatch no longer crashes with KeyError 'descriptor' on normalized linear configs. - the shared linear builder rejects a bridged learned child whose type_map differs from the composition's (the graph route rejects the non-identity remap on every forward; fail at construction like pt). - pt SeZM pair-exclusion reconciliation is factored into ONE helper used by all three builders (plain, native spin, virtual spin), so a pair_exclude_types vs descriptor.exclude_types mismatch fails fast on the spin routes too instead of being silently overwritten. - canonical top-level learned-model options (data_stat_protect, preset_out_bias, use_compile, ...) are routed to the learned child by route_canonical_learned_options with conflict checks, in both the pt bridged builder and the shared factory; silently dropped before. - pt_expt get_model rejects trainer-owned lora after bridge expansion (pt_expt has no LoRA support; it silently trained a plain model). - module-level assert pins the routing tables disjoint (also resolves the CodeQL unused-variable finding on _LEARNED_CHILD_KEYS).
1 parent d725272 commit 187ad36

9 files changed

Lines changed: 304 additions & 11 deletions

File tree

deepmd/dpmodel/model/dp_linear_model.py

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,9 @@
2222
from deepmd.dpmodel.model.make_model import (
2323
make_model,
2424
)
25+
from deepmd.utils.data_system import (
26+
DeepmdDataSystem,
27+
)
2528

2629
DPLinearModel_ = make_model(LinearEnergyAtomicModel, T_Bases=(NativeOP, BaseModel))
2730

@@ -45,3 +48,48 @@ def __init__(
4548
) -> None:
4649
DPModelCommon.__init__(self)
4750
DPLinearModel_.__init__(self, *args, **kwargs)
51+
52+
@classmethod
53+
def update_sel(
54+
cls,
55+
train_data: DeepmdDataSystem,
56+
type_map: list[str] | None,
57+
local_jdata: dict,
58+
) -> tuple[dict, float | None]:
59+
"""Update the selection and perform neighbor statistics.
60+
61+
Updates each learned child in place, skipping analytical
62+
(``inner_potential``) and pair-table children, and aggregates the
63+
minimum neighbor distance (twin of the pt_expt implementation).
64+
65+
Parameters
66+
----------
67+
train_data : DeepmdDataSystem
68+
data used to do neighbor statistics
69+
type_map : list[str], optional
70+
The name of each type of atoms
71+
local_jdata : dict
72+
The local data refer to the current class
73+
74+
Returns
75+
-------
76+
dict
77+
The updated local data
78+
float
79+
The minimum distance between two atoms
80+
"""
81+
local_jdata_cpy = local_jdata.copy()
82+
type_map = local_jdata_cpy["type_map"]
83+
min_nbor_dist = None
84+
for idx, sub_model in enumerate(local_jdata_cpy["models"]):
85+
if sub_model.get("type") == "inner_potential":
86+
# analytical child: no descriptor, no selection to update
87+
continue
88+
if "tab_file" not in sub_model:
89+
sub_model, temp_min = DPModelCommon.update_sel(
90+
train_data, type_map, local_jdata_cpy["models"][idx]
91+
)
92+
local_jdata_cpy["models"][idx] = sub_model
93+
if min_nbor_dist is None or temp_min <= min_nbor_dist:
94+
min_nbor_dist = temp_min
95+
return local_jdata_cpy, min_nbor_dist

deepmd/dpmodel/model/model_factory.py

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,9 @@
1010
Any,
1111
)
1212

13+
from deepmd.utils.bridging import (
14+
route_canonical_learned_options,
15+
)
1316
from deepmd.utils.spin import (
1417
Spin,
1518
)
@@ -233,13 +236,22 @@ def get_linear_atomic_model(
233236
learned_descriptor = children[learned_indices[0]]["descriptor"]
234237
learned_descriptor["inner_clamp_r_inner"] = float(inner_cfg.get("r_inner", 0.5))
235238
learned_descriptor["inner_clamp_r_outer"] = float(inner_cfg.get("r_outer", 0.8))
239+
route_canonical_learned_options(data, children[learned_indices[0]])
236240

237241
built: dict[int, Any] = {}
238242
for i, sub in enumerate(children):
239243
if i in inner_indices:
240244
continue
241245
if "type_map" not in sub:
242246
sub["type_map"] = copy.deepcopy(type_map)
247+
elif inner_indices and i == learned_indices[0] and sub["type_map"] != type_map:
248+
# The analytical child always uses the composition's type_map,
249+
# and the graph route rejects a non-identity remap at forward
250+
# time; fail at construction like the pt builder does.
251+
raise ValueError(
252+
"A bridged linear_ener composition requires the learned "
253+
"child's type_map to match the composition type_map."
254+
)
243255
if "descriptor" in sub:
244256
child = None
245257
if descriptor_child_builder is not None:

deepmd/pt/model/model/__init__.py

Lines changed: 43 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@
3838
)
3939
from deepmd.utils.bridging import (
4040
expand_bridging_method,
41+
route_canonical_learned_options,
4142
)
4243
from deepmd.utils.spin import (
4344
Spin,
@@ -346,6 +347,7 @@ def _get_bridged_linear_model(model_params: dict) -> BaseModel:
346347
"The pt backend requires the learned child's `type_map` to "
347348
"match the bridged linear_ener composition's `type_map`."
348349
)
350+
route_canonical_learned_options(model_params, learned)
349351
inner_cfg = inner_cfgs[0]
350352
learned["type"] = "dpa4"
351353
learned["type_map"] = copy.deepcopy(model_params["type_map"])
@@ -479,16 +481,31 @@ def get_standard_model(model_params: dict) -> BaseModel:
479481
return model
480482

481483

482-
def get_sezm_model(model_params: dict) -> BaseModel:
483-
model_params_old = model_params
484-
model_params = copy.deepcopy(model_params)
485-
model_params.setdefault("descriptor", {})
486-
model_params.setdefault("fitting_net", {})
487-
model_params["descriptor"].setdefault("type", "dpa4")
484+
def _reconcile_sezm_pair_exclude_types(model_params: dict) -> list[list[int]]:
485+
"""Reconcile ``pair_exclude_types`` with ``descriptor.exclude_types``.
488486
489-
ntypes = len(model_params["type_map"])
490-
model_params["descriptor"]["ntypes"] = ntypes
491-
model_params["descriptor"]["type_map"] = copy.deepcopy(model_params["type_map"])
487+
A DPA4/SeZM config may spell pair exclusions at the model level
488+
(``pair_exclude_types``) or on the descriptor (``exclude_types``).
489+
Every SeZM builder (plain, native spin, virtual spin) resolves the two
490+
through this ONE helper so that a mismatch always fails fast instead
491+
of one spelling silently overwriting the other.
492+
493+
Parameters
494+
----------
495+
model_params : dict
496+
The DPA4/SeZM model config; ``model_params["descriptor"]`` must
497+
exist.
498+
499+
Returns
500+
-------
501+
list[list[int]]
502+
The reconciled real-type pair exclusion list.
503+
504+
Raises
505+
------
506+
ValueError
507+
If both spellings are given and differ.
508+
"""
492509
descriptor_exclude_types = [
493510
list(pair) for pair in (model_params["descriptor"].get("exclude_types") or [])
494511
]
@@ -503,6 +520,20 @@ def get_sezm_model(model_params: dict) -> BaseModel:
503520
)
504521
else:
505522
pair_exclude_types = descriptor_exclude_types
523+
return pair_exclude_types
524+
525+
526+
def get_sezm_model(model_params: dict) -> BaseModel:
527+
model_params_old = model_params
528+
model_params = copy.deepcopy(model_params)
529+
model_params.setdefault("descriptor", {})
530+
model_params.setdefault("fitting_net", {})
531+
model_params["descriptor"].setdefault("type", "dpa4")
532+
533+
ntypes = len(model_params["type_map"])
534+
model_params["descriptor"]["ntypes"] = ntypes
535+
model_params["descriptor"]["type_map"] = copy.deepcopy(model_params["type_map"])
536+
pair_exclude_types = _reconcile_sezm_pair_exclude_types(model_params)
506537
model_params["pair_exclude_types"] = pair_exclude_types
507538
model_params["descriptor"]["exclude_types"] = copy.deepcopy(pair_exclude_types)
508539

@@ -621,7 +652,7 @@ def _get_sezm_native_spin_model(model_params: dict) -> BaseModel:
621652
model_params["descriptor"]["type_map"] = copy.deepcopy(model_params["type_map"])
622653
model_params["descriptor"]["use_spin"] = use_spin
623654

624-
pair_exclude_types = model_params.get("pair_exclude_types", [])
655+
pair_exclude_types = _reconcile_sezm_pair_exclude_types(model_params)
625656
model_params["pair_exclude_types"] = pair_exclude_types
626657
if pair_exclude_types:
627658
model_params["descriptor"]["exclude_types"] = copy.deepcopy(pair_exclude_types)
@@ -694,9 +725,10 @@ def _get_sezm_virtual_spin_model(model_params: dict) -> BaseModel:
694725
virtual_scale=model_params["spin"]["virtual_scale"],
695726
allow_missing_label=model_params["spin"].get("allow_missing_label", False),
696727
)
728+
real_pair_exclude_types = _reconcile_sezm_pair_exclude_types(model_params)
697729
model_params["type_map"] += [item + "_spin" for item in model_params["type_map"]]
698730
pair_exclude_types = spin.get_pair_exclude_types(
699-
exclude_types=model_params.get("pair_exclude_types", None)
731+
exclude_types=real_pair_exclude_types or None
700732
)
701733
model_params["pair_exclude_types"] = pair_exclude_types
702734
model_params["descriptor"]["exclude_types"] = pair_exclude_types

deepmd/pt_expt/model/get_model.py

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -385,6 +385,14 @@ def get_model(data: dict) -> BaseModel:
385385
The data to construct the model.
386386
"""
387387
data = expand_bridging_method(data)
388+
if data.get("lora") is not None:
389+
# The expansion keeps trainer-owned `lora` at the composition top
390+
# level (the pt trainer reads it there); pt_expt has no LoRA
391+
# support, so reject it here instead of silently training a plain
392+
# full model.
393+
raise NotImplementedError(
394+
"`lora` is not supported for DPA4/SeZM in the pt_expt backend."
395+
)
388396
return _model_factory.get_model(
389397
data,
390398
standard_model_factory=get_standard_model,

deepmd/utils/bridging.py

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -117,6 +117,48 @@ def _is_dpa4_family(sub: dict) -> bool:
117117
"finetune_head",
118118
)
119119
_NON_CHILD_KEYS = _COMPOSITION_KEYS + _CONSUMED_KEYS + _TRAINER_KEYS
120+
# The routing tables are pairwise disjoint: a key has exactly one owner.
121+
assert not set(_LEARNED_CHILD_KEYS) & set(_NON_CHILD_KEYS)
122+
123+
124+
def route_canonical_learned_options(composition: dict, learned: dict) -> None:
125+
"""Route learned-model options from a canonical composition to its child.
126+
127+
A canonical ``linear_ener`` config accepts generic model options (e.g.
128+
``data_stat_protect``, ``preset_out_bias``) at the composition top
129+
level, but the learned child is their one owner: a bridged builder
130+
reads them from the child config only. This helper copies each
131+
learned-owned key present at the top level onto ``learned`` (in
132+
place) when the child does not set it, and raises when both levels
133+
set different values — a silent drop or a silent override would both
134+
unpin the ownership contract.
135+
136+
Parameters
137+
----------
138+
composition : dict
139+
The canonical ``linear_ener`` model config.
140+
learned : dict
141+
The learned child's config; modified in place.
142+
143+
Raises
144+
------
145+
ValueError
146+
If a learned-owned key is set at both levels with different
147+
values.
148+
"""
149+
for key in _LEARNED_CHILD_KEYS:
150+
if key not in composition:
151+
continue
152+
if key in learned:
153+
if learned[key] != composition[key]:
154+
raise ValueError(
155+
f"`{key}` is set both on the linear_ener composition "
156+
f"({composition[key]!r}) and on its learned child "
157+
f"({learned[key]!r}) with different values. The learned "
158+
"child owns this option: set it on the child only."
159+
)
160+
else:
161+
learned[key] = copy.deepcopy(composition[key])
120162

121163

122164
def expand_bridging_method(data: dict) -> dict:

source/tests/common/dpmodel/test_zbl_bridging.py

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -784,3 +784,49 @@ def test_inner_child_with_descriptor_raises_cleanly(self) -> None:
784784
cfg["models"][1]["descriptor"] = {"type": "dpa4"}
785785
with pytest.raises(ValueError, match="must not carry"):
786786
get_model(cfg)
787+
788+
def test_canonical_rejects_mismatched_learned_type_map(self) -> None:
789+
"""A remapped learned-child type_map builds a model the graph
790+
route rejects on every forward; fail at construction instead.
791+
"""
792+
cfg = _canonical_config()
793+
cfg["models"][0]["type_map"] = list(reversed(cfg["type_map"]))
794+
with pytest.raises(ValueError, match="type_map"):
795+
get_model(cfg)
796+
797+
def test_canonical_conflicting_top_level_option_raises(self) -> None:
798+
"""A learned-owned option set differently at both levels must
799+
fail loudly instead of one value silently winning.
800+
"""
801+
cfg = _canonical_config()
802+
cfg["data_stat_protect"] = 0.123
803+
cfg["models"][0]["data_stat_protect"] = 0.456
804+
with pytest.raises(ValueError, match="data_stat_protect"):
805+
get_model(cfg)
806+
807+
def test_update_sel_dispatches_and_skips_inner_child(self, monkeypatch) -> None:
808+
"""``BaseModel.update_sel`` dispatches ``linear_ener`` to a
809+
composite implementation that updates the learned child and
810+
skips the analytical one (the default neighbor-stat phase would
811+
otherwise crash with ``KeyError: 'descriptor'``).
812+
"""
813+
from deepmd.dpmodel.model.dp_model import (
814+
DPModelCommon,
815+
)
816+
from deepmd.utils.argcheck import (
817+
model_args,
818+
)
819+
820+
seen = []
821+
822+
def _fake_update_sel(train_data, type_map, sub):
823+
seen.append(copy.deepcopy(sub))
824+
return sub, 0.9
825+
826+
monkeypatch.setattr(DPModelCommon, "update_sel", staticmethod(_fake_update_sel))
827+
cfg = model_args().normalize_value(_canonical_config(), trim_pattern="_*")
828+
updated, min_dist = BaseModel.update_sel(None, cfg["type_map"], cfg)
829+
assert min_dist == 0.9
830+
assert len(seen) == 1 # only the learned child
831+
assert "descriptor" in seen[0]
832+
assert updated["models"][1]["type"] == "inner_potential"

source/tests/common/test_bridging.py

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -272,3 +272,33 @@ def test_non_dpa4_learned_child_is_not(self) -> None:
272272
cfg = self._canonical()
273273
cfg["models"][0] = {"type": "standard", "descriptor": {"type": "se_e2_a"}}
274274
assert not is_bridged_sezm_config(cfg)
275+
276+
277+
def test_route_canonical_learned_options_copies_and_conflicts() -> None:
278+
"""The learned child owns the generic model options: a top-level value
279+
is copied onto the child when absent, accepted when equal, and
280+
rejected when the two levels differ.
281+
"""
282+
from deepmd.utils.bridging import (
283+
route_canonical_learned_options,
284+
)
285+
286+
composition = {
287+
"type": "linear_ener", # composition-owned: never routed
288+
"type_map": ["Ni", "O"], # composition-owned: never routed
289+
"data_stat_protect": 0.123, # learned-owned: routed
290+
"preset_out_bias": {"energy": [1.0, 2.0]}, # learned-owned: routed
291+
}
292+
learned = {"descriptor": {"type": "dpa4"}}
293+
route_canonical_learned_options(composition, learned)
294+
assert learned["data_stat_protect"] == 0.123
295+
assert learned["preset_out_bias"] == {"energy": [1.0, 2.0]}
296+
assert learned["preset_out_bias"] is not composition["preset_out_bias"]
297+
assert "type_map" not in learned
298+
299+
# equal values at both levels pass
300+
route_canonical_learned_options(composition, learned)
301+
302+
learned["data_stat_protect"] = 0.456
303+
with pytest.raises(ValueError, match="data_stat_protect"):
304+
route_canonical_learned_options(composition, learned)

source/tests/pt/model/test_get_model_bridging.py

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -327,3 +327,47 @@ def _fake_update_sel(train_data, type_map, sub):
327327
assert len(seen) == 1 # only the learned child
328328
assert "descriptor" in seen[0]
329329
assert updated["models"][1]["type"] == "inner_potential"
330+
331+
332+
def test_canonical_top_level_option_routes_to_learned_child() -> None:
333+
"""Generic learned-model options at the canonical top level reach the
334+
learned child (the child is their one owner).
335+
"""
336+
cfg = _canonical_config()
337+
cfg["data_stat_protect"] = 0.123
338+
model = get_model(cfg)
339+
assert model.atomic_model.data_stat_protect == 0.123
340+
341+
342+
def test_canonical_conflicting_top_level_option_raises() -> None:
343+
"""A learned-owned option set differently at both levels must fail
344+
loudly instead of one value silently winning.
345+
"""
346+
cfg = _canonical_config()
347+
cfg["data_stat_protect"] = 0.123
348+
cfg["models"][0]["data_stat_protect"] = 0.456
349+
with pytest.raises(ValueError, match="data_stat_protect"):
350+
get_model(cfg)
351+
352+
353+
@pytest.mark.parametrize(
354+
"scheme",
355+
[
356+
"native", # spin as an equivariant descriptor feature
357+
"deepspin", # classical virtual-atom representation
358+
],
359+
)
360+
def test_canonical_spin_rejects_mismatched_pair_exclusions(scheme: str) -> None:
361+
"""Both spin routes must fail fast on a pair-exclusion mismatch like
362+
the no-spin route, not silently overwrite the descriptor's exclusions.
363+
"""
364+
cfg = _canonical_config()
365+
cfg["pair_exclude_types"] = [[0, 0]]
366+
cfg["models"][0]["descriptor"]["exclude_types"] = [[0, 1]]
367+
cfg["spin"] = {
368+
"scheme": scheme,
369+
"use_spin": [True, False],
370+
"virtual_scale": 0.3,
371+
}
372+
with pytest.raises(ValueError, match="must match"):
373+
get_model(cfg)

0 commit comments

Comments
 (0)