refactor(model): express analytical bridging as an explicit linear_ener composition - #5964
Conversation
…er composition Close deepmodeling#5948. A bridged model IS a linear composition, but it was spelled as a bridging_method flag on a non-composite model type, so the type requested was not the type returned, and every builder accepting the flag re-implemented the composition (and drifted, see deepmodeling#5947). - Register inner_potential as a config-level model type (argcheck), so it can be named as a linear_ener child: {type: inner_potential, mode: zbl, r_inner, r_outer}. - The linear builder derives the learned sibling descriptor's InnerClamp/BridgingSwitch radii from the inner_potential child at build time: the radii are written once, one source of truth. - Keep bridging_method as sugar expanded by ONE shared normalizer (deepmd.utils.bridging.expand_bridging_method) at every backend's get_model entry; the non-composite builders (get_standard_model, get_sezm_model) now fail fast on the flag instead of composing or silently dropping it (pt's standard route used to drop it). - dpmodel gains a real linear_ener config builder (it had none); the linear child-parsing core is shared between dpmodel and pt_expt. - pt realizes the canonical form via its existing SeZMModel implementation, so pt checkpoints and physics are unchanged; both spellings serialize to the identical wire dict (pinned by tests in all three backends). - Migrate examples/water/dpa4/input-zbl.json and doc/model/dpa4.md to the canonical spelling.
for more information, see https://pre-commit.ci
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds ChangesLinear energy bridging
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Caller
participant get_model
participant expand_bridging_method
participant get_linear_model
participant LinearEnergyAtomicModel
Caller->>get_model: submit model configuration
get_model->>expand_bridging_method: expand bridging_method
expand_bridging_method-->>get_model: return linear_ener configuration
get_model->>get_linear_model: dispatch linear_ener composition
get_linear_model->>LinearEnergyAtomicModel: build learned and inner-potential children
LinearEnergyAtomicModel-->>get_linear_model: return composed atomic model
get_linear_model-->>Caller: return energy model
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
source/tests/pt_expt/model/test_get_model_bridging.py (1)
209-227: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a rejection test for non-
sumweights.The pt suite pins this contract in
source/tests/pt/model/test_get_model_bridging.py::test_canonical_requires_sum_weights, but no dpmodel or pt_expt test does. After the shared builder gains the guard described ondeepmd/dpmodel/model/model.pylines 225-232, add the same test here so the three backends stay aligned.🤖 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/pt_expt/model/test_get_model_bridging.py` around lines 209 - 227, Add a test near test_canonical_matches_sugar_serialize that verifies the canonical model configuration rejects non-"sum" weights, matching test_canonical_requires_sum_weights in the pt suite. Use the existing shared model-building helpers and assert the expected exception or validation failure after changing the canonical weights value.deepmd/utils/argcheck.py (1)
3728-3741: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe schema allows
inner_potentialas a top-level model type.
model_args_plugin.register("inner_potential")adds the type to the top-level model Variant as well as to thelinear_enermodelslist. The doc string states the type is usable only as a sub-model, but no schema rule enforces this. A user who writes"model": {"type": "inner_potential", ...}passes argcheck and then fails inside the backend factory with an unrelated message.Consider rejecting the type at the dispatcher entry with an explicit message that points to
linear_ener.🤖 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 `@deepmd/utils/argcheck.py` around lines 3728 - 3741, Reject top-level use of the inner_potential model in the argument-checking dispatcher, while continuing to allow it in the linear_ener models list. Add an explicit validation error for model type inner_potential that directs users to configure it as a linear_ener sub-model, before backend construction is reached.deepmd/dpmodel/model/model.py (1)
187-192: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winA non-DPA4 learned sibling fails with an unrelated
TypeError.Lines 191-192 write
inner_clamp_r_innerandinner_clamp_r_outerinto the learned child's descriptor config unconditionally. Only the DPA4/SeZM descriptor accepts those keywords. For any other descriptor type the keys reach the constructor throughmodel_components_factoryand raiseTypeError: __init__() got an unexpected keyword argument 'inner_clamp_r_inner'. The pt backend raises a clearNotImplementedErrorfor this case (deepmd/pt/model/model/__init__.pylines 322-327).Add the same capability check before deriving the radii, so the message names the real constraint.
🤖 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 `@deepmd/dpmodel/model/model.py` around lines 187 - 192, The learned descriptor clamp radii are currently injected for every descriptor type, causing non-DPA4/SeZM constructors to reject unexpected keywords. In the composition logic around learned_descriptor, add the same DPA4/SeZM capability check used by the pt backend before reading inner_cfg or setting inner_clamp_r_inner and inner_clamp_r_outer, and raise the clear NotImplementedError for unsupported descriptors.deepmd/pt/model/model/__init__.py (1)
291-320: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign the docstring with the raised exception type.
The
Raisessection listsValueErrorforshared_dict, and line 296 listsNotImplementedErroronly for a non-DPA4 learned sibling. Line 318 raisesNotImplementedErrorforshared_dict. Moveshared_dictto theNotImplementedErrorentry, or raiseValueErrorto match the documented contract.♻️ Proposed docstring fix
ValueError If the composition shape is not the bridging one (child counts, - ``weights``, ``shared_dict``). + ``weights``). NotImplementedError - If the learned sibling is not of the DPA4/SeZM family: the pt - backend has no bridging implementation for other descriptors. + If the learned sibling is not of the DPA4/SeZM family (the pt + backend has no bridging implementation for other descriptors), or + if ``shared_dict`` is combined with an ``inner_potential`` child.🤖 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 `@deepmd/pt/model/model/__init__.py` around lines 291 - 320, Align the function docstring’s Raises section with the implementation: document the shared_dict rejection under NotImplementedError rather than ValueError, while retaining ValueError for invalid composition shape and NotImplementedError for unsupported learned descriptor families.deepmd/pt_expt/model/get_model.py (1)
299-301: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winImport a public helper instead of the private
_build_linear_atomic_model.
_build_linear_atomic_modelis private by name but is now a cross-package contract betweendeepmd/dpmodel/model/model.pyand this module. A future rename or signature change inside dpmodel breaks pt_expt silently, and the leading underscore tells maintainers the opposite. Export the builder under a public name, for example next toBackendModelFactoryindeepmd/dpmodel/model/model_factory.py, and import that name in both backends.🤖 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 `@deepmd/pt_expt/model/get_model.py` around lines 299 - 301, Replace the private _build_linear_atomic_model dependency with a public builder exported from deepmd.dpmodel.model.model_factory alongside BackendModelFactory, then update both backend imports and call sites to use the public symbol while preserving the existing builder behavior and signature.source/tests/common/test_bridging.py (1)
144-162: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a case for the uppercase type aliases.
expand_bridging_methodlowercases the model type before the membership test, so"DPA4"and"SeZM"are accepted and are preserved verbatim on the learned child. No test pins that behavior. Parametrizetest_standard_type_is_supportedwith"DPA4"and"SeZM"to lock both the acceptance and the preserved spelling.🤖 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/test_bridging.py` around lines 144 - 162, The test_standard_type_is_supported test currently covers only the lowercase standard type; parametrize it with "DPA4" and "SeZM" and assert the learned child’s models[0]["type"] preserves each input spelling verbatim while confirming expansion succeeds.
🤖 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.
Inline comments:
In `@deepmd/dpmodel/model/model.py`:
- Around line 225-232: The shared _build_linear_atomic_model must reject
compositions with inner_potential children unless weights is "sum": inside its
inner_indices block, validate data.get("weights", "mean") and raise ValueError
using the exact message from the pt backend when it differs from "sum". Update
deepmd/dpmodel/model/model.py lines 225-232; deepmd/pt_expt/model/get_model.py
lines 327-332 requires no direct change, but add a pt_expt test mirroring
test_canonical_requires_sum_weights to verify the inherited guard.
In `@deepmd/pt_expt/model/get_model.py`:
- Around line 307-332: Restore shared_dict preprocessing in get_linear_model()
before calling _build_linear_atomic_model(): detect shared_dict, emit the
existing warning, and invoke preprocess_shared_params() so multi-task heads
receive shared component configurations. Preserve the current spin normalization
and ensure the processed model_params, rather than the unexpanded config, is
delegated to _build_linear_atomic_model().
---
Nitpick comments:
In `@deepmd/dpmodel/model/model.py`:
- Around line 187-192: The learned descriptor clamp radii are currently injected
for every descriptor type, causing non-DPA4/SeZM constructors to reject
unexpected keywords. In the composition logic around learned_descriptor, add the
same DPA4/SeZM capability check used by the pt backend before reading inner_cfg
or setting inner_clamp_r_inner and inner_clamp_r_outer, and raise the clear
NotImplementedError for unsupported descriptors.
In `@deepmd/pt_expt/model/get_model.py`:
- Around line 299-301: Replace the private _build_linear_atomic_model dependency
with a public builder exported from deepmd.dpmodel.model.model_factory alongside
BackendModelFactory, then update both backend imports and call sites to use the
public symbol while preserving the existing builder behavior and signature.
In `@deepmd/pt/model/model/__init__.py`:
- Around line 291-320: Align the function docstring’s Raises section with the
implementation: document the shared_dict rejection under NotImplementedError
rather than ValueError, while retaining ValueError for invalid composition shape
and NotImplementedError for unsupported learned descriptor families.
In `@deepmd/utils/argcheck.py`:
- Around line 3728-3741: Reject top-level use of the inner_potential model in
the argument-checking dispatcher, while continuing to allow it in the
linear_ener models list. Add an explicit validation error for model type
inner_potential that directs users to configure it as a linear_ener sub-model,
before backend construction is reached.
In `@source/tests/common/test_bridging.py`:
- Around line 144-162: The test_standard_type_is_supported test currently covers
only the lowercase standard type; parametrize it with "DPA4" and "SeZM" and
assert the learned child’s models[0]["type"] preserves each input spelling
verbatim while confirming expansion succeeds.
In `@source/tests/pt_expt/model/test_get_model_bridging.py`:
- Around line 209-227: Add a test near test_canonical_matches_sugar_serialize
that verifies the canonical model configuration rejects non-"sum" weights,
matching test_canonical_requires_sum_weights in the pt suite. Use the existing
shared model-building helpers and assert the expected exception or validation
failure after changing the canonical weights value.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1e258e88-0fc6-44ba-b700-05cdd71a3e3e
📒 Files selected for processing (11)
deepmd/dpmodel/model/model.pydeepmd/pt/model/model/__init__.pydeepmd/pt_expt/model/get_model.pydeepmd/utils/argcheck.pydeepmd/utils/bridging.pydoc/model/dpa4.mdexamples/water/dpa4/input-zbl.jsonsource/tests/common/dpmodel/test_zbl_bridging.pysource/tests/common/test_bridging.pysource/tests/pt/model/test_get_model_bridging.pysource/tests/pt_expt/model/test_get_model_bridging.py
njzjz-bot
left a comment
njzjz-bot
left a comment
There was a problem hiding this comment.
I found several actionable regressions in the canonical bridging workflow. In particular, the migrated example fails during the default neighbor-stat pass, canonical PT checkpoints are no longer recognized as SeZM/DPA4 for inference or .pt2 freeze, and multiple accepted options are silently ignored.
I used three independent subagent review passes on the same head and then reproduced the findings independently. The new bridging tests pass (71 tests), the existing linear/example regression selection passes (35 tests and 204 subtests), and Ruff reports clean; those tests do not exercise the failing CLI/checkpoint paths described inline.
Codex quota is about to reset, so I am using the remaining token budget to complete a concentrated review pass over the outstanding PRs.
Coding agent: Codex
Codex version: codex-cli 0.144.6
Model: gpt-5.6-sol
Reasoning effort: xhigh
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #5964 +/- ##
==========================================
- Coverage 79.64% 79.53% -0.11%
==========================================
Files 1085 1086 +1
Lines 126583 127382 +799
Branches 4592 4598 +6
==========================================
+ Hits 100811 101313 +502
- Misses 24120 24416 +296
- Partials 1652 1653 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…s the explicit equivalent Both spellings stay supported: the concise 'type: dpa4' + bridging_method form is the recommended user interface, and the explicit linear_ener + inner_potential composition is the canonical internal form it expands to (one shared normalizer defines the equivalence). Revert the example to the concise form; document both.
…odelFactory The linear child-parsing core was a private function in deepmd/dpmodel/model/model.py that pt_expt imported cross-module. The factory is the established home for registry-parameterized composition builders (get_zbl_model already builds the srtab two-child composition there), so get_linear_atomic_model joins it: dpmodel and pt_expt now call _model_factory.get_linear_atomic_model(data), with the backend classes bound once at factory construction. No behavior change.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
deepmd/dpmodel/model/model_factory.py (2)
229-236: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the atomic-model accessors for
rcutandsel. Replace the direct.descriptoraccess withbuilt[learned_indices[0]].get_rcut()and.get_sel()to avoid coupling the shared factory to backend storage details.🤖 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 `@deepmd/dpmodel/model/model_factory.py` around lines 229 - 236, Update the loop constructing InnerPotentialAtomicModel so it obtains rcut and sel through built[learned_indices[0]].get_rcut() and get_sel() directly, removing the intermediate .descriptor access and preserving the existing constructor behavior.
199-200: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCentralize the radius defaults. The values currently match across the schema, normalizer, and factory. Use shared constants to prevent future drift; explicit
linear_enerconfigs can still reach these fallbacks directly.🤖 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 `@deepmd/dpmodel/model/model_factory.py` around lines 199 - 200, Replace the hard-coded 0.5 and 0.8 fallbacks in the learned descriptor construction with the shared radius-default constants already used by the schema and normalizer. Update the relevant inner_cfg fallback lookups while preserving explicit linear_ener configuration values and the existing float conversion.
🤖 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.
Inline comments:
In `@deepmd/dpmodel/model/model_factory.py`:
- Line 182: Update the learned_indices construction in the model factory to
include descriptor-bearing children only when their index is not in
inner_indices. Preserve the existing inner_potential detection and ensure
overlapping children are excluded so the guard in the surrounding factory logic
raises the intended ValueError.
- Around line 237-245: Update the LinearEnergyAtomicModel construction in the
model factory to detect compositions whose linear_ener children include an
inner_potential and require weights to be explicitly set to "sum"; reject
omitted or "mean" weights instead of applying the current default. Preserve
existing weight handling for non-bridging compositions.
---
Nitpick comments:
In `@deepmd/dpmodel/model/model_factory.py`:
- Around line 229-236: Update the loop constructing InnerPotentialAtomicModel so
it obtains rcut and sel through built[learned_indices[0]].get_rcut() and
get_sel() directly, removing the intermediate .descriptor access and preserving
the existing constructor behavior.
- Around line 199-200: Replace the hard-coded 0.5 and 0.8 fallbacks in the
learned descriptor construction with the shared radius-default constants already
used by the schema and normalizer. Update the relevant inner_cfg fallback
lookups while preserving explicit linear_ener configuration values and the
existing float conversion.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 76e77054-4fc4-4371-891c-676ab2697819
📒 Files selected for processing (3)
deepmd/dpmodel/model/model.pydeepmd/dpmodel/model/model_factory.pydeepmd/pt_expt/model/get_model.py
🚧 Files skipped from review as they are similar to previous changes (2)
- deepmd/dpmodel/model/model.py
- deepmd/pt_expt/model/get_model.py
…ders
Guards (shared builder, dpmodel + pt_expt via BackendModelFactory):
- a bridged composition requires weights: "sum" ("mean" silently
halved both energy terms; pt already rejected it)
- an inner_potential child must not carry a descriptor (was a KeyError)
- a bridging_method flag on a linear child is rejected instead of
silently dropped (also guarded in the pt linear builder)
pt canonical realization:
- reject lora on the learned child (the trainer reads top-level lora
only, so it would silently train without adapters)
- reject a child type_map that differs from the composition's instead
of silently overwriting it
pt checkpoint consumers: _is_sezm_model_params, is_sezm_checkpoint and
freeze_sezm_to_pt2 now recognize the canonical bridged spelling via the
shared is_bridged_sezm_config predicate, so DeepPot no-jit routing and
the .pt2 freeze route work for canonical checkpoints.
update_sel (pt + pt_expt linear models): skip inner_potential children
instead of crashing with KeyError: 'descriptor' on the default CLI path.
pt_expt: DPA4/SeZM-family linear children now route through
get_sezm_model via a descriptor_child_builder hook on the shared
builder, restoring the family defaults and the loud rejections of
lora / use_compile / preset_out_bias.
argcheck: bridging args shared with the standard variant (the
documented standard sugar previously failed strict normalization);
inner_potential is now child-only (top-level model.type:
inner_potential is rejected at normalization, not at build).
|
All review findings are addressed in b25bd482a, each pinned by a new test. Resolution per finding: Fixed
Skipped (with reason)
The failing |
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
deepmd/dpmodel/model/model_factory.py (1)
234-251: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReject child
type_mapvalues that differ from the compositiontype_map.Line 239 only fills a missing child map. Line 251 accepts an explicit, different child map. A reordered map with the same length does not fail construction, but the learned child then interprets atom-type indices differently from
LinearEnergyAtomicModelandInnerPotentialAtomicModel.Validate equality before building any child. Add a regression test with reordered child types.
Proposed fix
if "type_map" not in sub: sub["type_map"] = copy.deepcopy(type_map) + elif sub["type_map"] != type_map: + raise ValueError( + "A linear_ener sub-model `type_map` must match the " + "composition `type_map`." + )🤖 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 `@deepmd/dpmodel/model/model_factory.py` around lines 234 - 251, The child-building loop must reject any explicit child type_map that differs from the composition type_map, including reordered entries. In the child handling around descriptor_child_builder and atomic_model, validate an existing sub["type_map"] for exact equality before constructing the child; retain the deep-copied composition map only when it is missing, and raise the established model-construction error for mismatches. Add a regression test covering a reordered child type_map.
🤖 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.
Inline comments:
In `@deepmd/pt_expt/model/get_model.py`:
- Around line 300-302: Update the child-type selection in the callback around
get_sezm_model so it also reads sub["descriptor"]["type"] when the child-level
type is absent, normalizes recognized DPA4/SeZM variants to the expected model
type, and then routes the child through get_sezm_model. Add a regression test
covering a descriptor-typed child with no child-level type.
---
Outside diff comments:
In `@deepmd/dpmodel/model/model_factory.py`:
- Around line 234-251: The child-building loop must reject any explicit child
type_map that differs from the composition type_map, including reordered
entries. In the child handling around descriptor_child_builder and atomic_model,
validate an existing sub["type_map"] for exact equality before constructing the
child; retain the deep-copied composition map only when it is missing, and raise
the established model-construction error for mismatches. Add a regression test
covering a reordered child type_map.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bab72aa1-32d6-42fd-9f9c-41d9d3713767
📒 Files selected for processing (13)
deepmd/dpmodel/model/model_factory.pydeepmd/pt/entrypoints/freeze_pt2.pydeepmd/pt/infer/deep_eval.pydeepmd/pt/model/model/__init__.pydeepmd/pt/model/model/dp_linear_model.pydeepmd/pt_expt/model/dp_linear_model.pydeepmd/pt_expt/model/get_model.pydeepmd/utils/argcheck.pydeepmd/utils/bridging.pysource/tests/common/dpmodel/test_zbl_bridging.pysource/tests/common/test_bridging.pysource/tests/pt/model/test_get_model_bridging.pysource/tests/pt_expt/model/test_get_model_bridging.py
🚧 Files skipped from review as they are similar to previous changes (1)
- deepmd/pt/model/model/init.py
… family builder A child spelled 'type: standard' with a DPA4/SeZM descriptor (the shape the sugar on 'type: standard' expands to) bypassed get_sezm_model and so lost the family defaults and the loud lora/use_compile/ preset_out_bias rejections. The child-builder hook now keys on the descriptor type too, matching the pt backend's routing.
…pcm/deepmd-kit into refactor-bridging-linear-ener
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)
deepmd/pt_expt/model/get_model.py (1)
332-362: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winInject
use_spininto defaulted DPA4/SeZM children.
get_linear_modelinjectsuse_spinonly when a child already contains"descriptor". However,_dpa4_family_child_builderaccepts a type-only DPA4/SeZM child and letsget_sezm_modelcreate its descriptor defaults later. That descriptor then missesuse_spin, so the native-spin capability check can reject an otherwise valid composition.Initialize the descriptor before setting
use_spinfor learned DPA4/SeZM children. Do not modifyinner_potentialorpairtabchildren. Add a regression test for a type-only DPA4 child with top-level native spin.Proposed fix
for sub in model_params["models"]: - if "descriptor" in sub: + child_type = str(sub.get("type", "")).lower() + if "descriptor" in sub or child_type in ("dpa4", "sezm"): + sub.setdefault("descriptor", {}) sub["descriptor"]["use_spin"] = use_spin🤖 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 `@deepmd/pt_expt/model/get_model.py` around lines 332 - 362, Update the native-spin child-processing logic in get_linear_model to initialize a missing descriptor for learned DPA4/SeZM children before assigning use_spin, while leaving inner_potential and pairtab children unchanged. Preserve existing descriptors and ensure type-only children receive the spin setting before _dpa4_family_child_builder creates defaults; add a regression test covering a type-only DPA4 child with top-level native spin.
🤖 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 `@deepmd/pt_expt/model/get_model.py`:
- Around line 332-362: Update the native-spin child-processing logic in
get_linear_model to initialize a missing descriptor for learned DPA4/SeZM
children before assigning use_spin, while leaving inner_potential and pairtab
children unchanged. Preserve existing descriptors and ensure type-only children
receive the spin setting before _dpa4_family_child_builder creates defaults; add
a regression test covering a type-only DPA4 child with top-level native spin.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b8cdb3d6-8794-483e-8fa5-bd5df8930dc3
📒 Files selected for processing (2)
deepmd/pt_expt/model/get_model.pysource/tests/pt_expt/model/test_get_model_bridging.py
🚧 Files skipped from review as they are similar to previous changes (1)
- source/tests/pt_expt/model/test_get_model_bridging.py
njzjz-bot
left a comment
njzjz-bot
left a comment
There was a problem hiding this comment.
Three blocking correctness issues were independently confirmed and reproduced against the current PR head: the recommended bridging+LoRA form regresses, the normalized default neighbor-stat path still crashes, and the shared builder accepts bridge compositions with no executable graph/dense route. Details are attached inline.
Coding agent: Codex
Codex version: codex-cli 0.144.6
Model: gpt-5.6-sol
Reasoning effort: xhigh
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.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
OutisLi-Bot
left a comment
OutisLi-Bot
left a comment
There was a problem hiding this comment.
Review (head d725272)
Re-checked the previously blocking items against current head (neighbor-stat / update_sel with inner_potential, concise form + top-level lora, canonical checkpoint / DeepEval / .pt2 recognition, mixed bridge graphs, nested sugar rejection, DPA4-family child routing). Those look fixed in code, and CI on this head looks green.
I did a second adversarial pass focused on training/infer regressions and silent config drops on the recommended dpa4 + bridging_method path and did not find a remaining blocker of that class.
Approve.
njzjz-bot
left a comment
njzjz-bot
left a comment
There was a problem hiding this comment.
Three independent review passes plus a final verification pass found five reproducible correctness issues on the current head: dpmodel neighbor-stat dispatch crashes for the newly registered composition, dpmodel/pt_expt accept a bridged type-map shape that cannot execute, the PT spin route bypasses exclusion validation, explicit canonical configs silently drop learned-model options, and pt_expt silently ignores LoRA after sugar expansion. Details are attached inline.
Coding agent: Codex
Codex version: codex-cli 0.144.6
Model: gpt-5.6-sol
Reasoning effort: xhigh
- 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).
…ctor-bridging-linear-ener
Self-audit of the linear_ener key x backend x route matrix (the same probe pattern the review rounds used) found four residual holes; all are closed by explicit rejections instead of silent drops: - the shared linear builder (dpmodel/pt_expt) now rejects top-level `lora`, non-empty `shared_dict`, and child-level `lora` -- the pt backend's consumers (trainer lora, linear shared_dict) do not exist in these backends, so all three silently built a different model than the config asked for (empty shared_dict from strict normalization still passes). - the shared builder mirrors pt's DPA4/SeZM family restriction on the bridged learned sibling: other families died on an obscure unknown-kwarg TypeError from the clamp injection. - tf registers linear_ener but cannot build inner_potential children; its update_sel now rejects them explicitly before any neighbor statistics run, instead of an 'unknown model type' dispatch error. - a table-walk test runs EVERY _LEARNED_CHILD_KEYS entry through route_canonical_learned_options (copy-down and conflict branches), so a future per-key special case cannot land untested. All rejections are pinned by tests verified to fail without the guards.
njzjz
left a comment
njzjz
left a comment
There was a problem hiding this comment.
[P1] Preserve top-level canonical options after argcheck normalization
route_canonical_learned_options() cannot distinguish a child value explicitly set by the user from a default inserted by argcheck normalization. On the normal CLI path, normalization populates learned-child defaults before this helper runs. A valid canonical config such as a top-level data_stat_protect: 0.123 can therefore acquire the child schema default (for example 0.01) and then be rejected as a parent/child conflict.
This makes the advertised composition-level routing unusable for learned-owned options whenever the requested top-level value differs from the child default. It potentially affects data_stat_protect, data_stat_nbatch, preset_out_bias, use_compile, enable_tf32, and the other fields in _LEARNED_CHILD_KEYS.
Please route these options before child defaults are injected, avoid duplicating the relevant defaults at the child level for canonical compositions, or retain enough provenance to reject only two explicitly configured conflicting values. Please add a regression test that first normalizes a canonical config with a non-default top-level learned option and then calls get_model().
Delegated review submitted on behalf of @njzjz by ChatGPT.
Agent: ChatGPT
Version: 1.2026.209 (client build)
Model: GPT-5.6 Pro
Strict normalization injects schema defaults on BOTH the composition top level and the learned child before any builder runs, so the previous strict conflict check rejected every explicitly set top-level learned-owned option on the normal CLI path (top-level 0.123 vs the injected child default 0.01) -- the advertised composition-level routing was unusable. route_canonical_learned_options now recovers the lost provenance by comparing each level against the key's argcheck default (collected once from the model schema, with a consistency check): a level holding exactly the default is treated as not explicitly configured and the other level wins; only two explicit non-default values still raise. Known residual ambiguity: explicitly setting a level to the default value is indistinguishable from not setting it. Regression per review: normalize a canonical config with a top-level data_stat_protect=0.123 (asserting the child default injection), then get_model() must build with 0.123; verified to fail before the fix.
|
@njzjz Confirmed and fixed in edf02fb, using the provenance-recovery option you listed.
The regression test is exactly the one you asked for: Known residual ambiguity, documented in the helper's docstring: explicitly setting a level to exactly the default value is indistinguishable from not setting it, and loses to an explicit non-default on the other level. |
njzjz
left a comment
njzjz
left a comment
There was a problem hiding this comment.
Re-reviewed the fix at head edf02fb7 with particular attention to the normal argcheck-normalize → model-build path and to failure modes introduced by schema-default inference.
The previous blocker is resolved: an injected child default no longer creates a false conflict with an explicit top-level learned option, while two different non-default values still fail loudly. The new integration regression test exercises the actual normalized canonical configuration before get_model(), rather than testing only the routing helper in isolation.
I also checked the less-obvious risks in the implementation: the schema traversal is finite for the current model schema, inconsistent duplicate defaults fail explicitly rather than being guessed, the result is cached only after successful collection, and the routing helper is shared by the affected backend paths. The documented ambiguity between “unset” and “explicitly set to the schema default” is inherent once normalization has erased provenance; it only affects duplicate parent/child declarations and is not a merge blocker here.
All current checks for this head have completed without a failure. I did not find another blocking correctness regression in the updated change.
Approved.
Delegated review submitted on behalf of @njzjz by ChatGPT.
Agent: ChatGPT
Model: GPT-5.6 Pro
Conflict in deepmd/pt_expt/model/get_model.py: master (deepmodeling#5964) removed _compose_bridging in favour of the canonical linear_ener route, so the branch-side edit to that function is obsolete -- took master's deletion. The new route regressed this branch's use_amp contract, caught by TestUseAmpSurvivesAssembly: get_linear_atomic_model hardcoded the dpmodel InnerPotential/LinearEnergy atomic classes, so pt_expt's composition was a raw dpmodel instance that LinearEnergyModel(atomic_model_=...) had to convert -- and conversion round-trips through deserialize(serialize()), which keeps only the portable record. Added inner_potential_model and linear_atomic_model to the backend-class injection the factory already does for atomic_model/pairtab_model/zbl_model, and pt_expt now passes its wrapped classes, so the composition is assembled from backend-native children with no conversion.
6 participants
Close #5948.
A bridged model IS a linear composition, but it was spelled as a
bridging_methodflag on a non-composite model type. The type you requested was not the type you got, and every builder that accepted the flag re-implemented the composition (and drifted; see #5947).Canonical spelling
Changes, by issue task
inner_potentialis a config-level model type. Registered in argcheck (mode,r_inner,r_outer), so it can be named as alinear_enerchild. It is buildable only inside a composition.inner_clamp_r_inner/_outerfrom theinner_potentialchild at build time. The radii are written once; one source of truth.pair_exclude_typesbelongs to the composition. The canonical builders do no promotion; the composition-level key governs both children by construction. The legacy promotion semantics of the pttype: "dpa4"builder survive only inside the sugar expansion, so the two bridged routes of Deprecate descriptor-scoped exclude_types in favour of the model-level build-seam owner #5947 can no longer diverge (the ~80 eV builder disagreement is gone: both spellings of the flag now expand identically). Full deletion of the non-bridged promotion stays with Deprecate descriptor-scoped exclude_types in favour of the model-level build-seam owner #5947's deprecation cycle.bridging_methodis sugar with ONE owner.deepmd.utils.bridging.expand_bridging_methodexpands the flag into the canonical form at every backend'sget_modelentry. The non-composite builders (get_standard_model,get_sezm_model) fail fast on the flag instead of composing — or, as pt's standard route used to do, silently dropping it.linear_enerconfig builder (it previously had none); its child-parsing core is shared with pt_expt. pt realizes the canonical form through its existingSeZMModelimplementation, so pt checkpoints and physics are unchanged.Native-scheme spin combines with the canonical form: a top-level
spinsection on alinear_enerconfig wraps the composition asNativeSpinEnergyModel(dpmodel/pt_expt) or routes to the SeZM spin builder (pt).Both spellings are supported, by design
The concise
type: "dpa4"+bridging_methodform is the recommended user interface (it is shorter, and existing inputs/checkpoints keep working with no migration). The explicitlinear_ener+inner_potentialform is the canonical internal semantics: all builders construct only it, so the concise form is pure rewriting and cannot drift.examples/water/dpa4/input-zbl.jsonkeeps the concise form;doc/model/dpa4.mddocuments the concise form first and shows the explicit equivalent.Tests
source/tests/common/test_bridging.py(new): 16 normalizer unit tests — key routing, promotion, mismatch error, spin passthrough, rejections, inactive-flag passthrough.source/tests/common/dpmodel/test_zbl_bridging.py: canonical-vs-sugar tests — identical energy (exact equality) and identical serialized wire dict, plus shape rejections and the standard-builder fail-fast.source/tests/pt/model/test_get_model_bridging.py(new): canonical →SeZMModelwith identical serialization to the sugar form; rejections (weights != "sum", non-DPA4 sibling, two inner children); plainlinear_enerunaffected.source/tests/pt_expt/model/test_get_model_bridging.py: updated to the new contract (flag ontype: standardnow composes throughget_model); canonical composition, canonical native-spin, serialize parity, builder fail-fasts.test_zbl_bridging/test_get_model_bridging/test_get_model_dpa4/test_linear_model, pt sezm model + spin + linear suites, consistencytest_linear_ener,test_examples.Known limitations
SeZMModel, so its linear builder maps the canonical config back ontoSeZMModelconstructor arguments. Physics and checkpoint format are byte-identical to the flag form (pinned by a serialize-equality test), but pt's internal ownership still differs from dpmodel/pt_expt.descriptor.exclude_typesdifferent from the composition'spair_exclude_typesraises in pt (SeZM mismatch check) while dpmodel/pt_expt accept the two scopes independently (Deprecate descriptor-scoped exclude_types in favour of the model-level build-seam owner #5947 territory).descriptor.exclude_typespromotion inget_sezm_model(pt, pt_expt) is untouched; removing it is Deprecate descriptor-scoped exclude_types in favour of the model-level build-seam owner #5947's staged deprecation.dp trainrun or GPU.pt2export with a canonical config in this PR; construction, argcheck, serialization, and energy parity are unit-tested. Two locally failing AOTI freeze tests are the known pre-existing torch 2.11 CPU-SIMD inductor bug (they pass withcpp.simdlen = 1; master fails identically).shared_dictcombined with aninner_potentialchild is rejected, not supported.Summary by CodeRabbit
New Features
Documentation