deepmodeling / deepmodeling/deepmd-kit
Make BackendModelFactory class injection complete and required (no silent dpmodel fallback)
- Dominant language
- Python
- Stars
- 2k
- Forks
- 649
- Avg merge
- 6d 18h
- Merged PRs (30d)
- 15
Description
## Summary
`BackendModelFactory` (`deepmd/dpmodel/model/model_factory.py`, introduced in #5964) constructs backend model objects from injected backend classes. The injection list is incomplete and partially optional, which allows a wrapping backend to silently fall back to constructing dpmodel-class objects. This is the root cause of the `use_amp` loss fixed in #5960 and should be closed off structurally.
## Problem
Current `BackendModelFactory.__init__`:
- required: `descriptor_base`, `fitting_base`, `model_base`, `backend_name`
- optional (default `None` → dpmodel class): `atomic_model`, `pairtab_model`, `zbl_model`, `linear_atomic_model`
- not injectable at all (hardcoded import inside the factory body): `InnerPotentialAtomicModel`
For a backend that wraps dpmodel classes (pt_expt), an omitted injection is not an error: the factory builds a dpmodel-class composition, and the backend's model wrapper later converts it via `deserialize(serialize())`. That conversion keeps only the portable record, so any runtime state the live children hold is silently dropped. This is exactly how #5960's bug happened: the factory hardcoded the dpmodel `LinearEnergyAtomicModel`, and a configured `use_amp: false` was lost during pt_expt assembly — numerically invisible to every cross-backend parity test, because the loss changes precision policy, not the math.
A half-injected factory is worse than no sharing: it looks canonical while constructing wrong-backend objects, and the failure mode is silent degradation instead of an error.
## Proposal
1. Make **all** class-injection parameters required (no `None` default, no fallback import inside the factory):
- `atomic_model`, `pairtab_model`, `zbl_model` — already passed explicitly by all four instantiation sites (`deepmd/dpmodel/model/model.py`, `deepmd/jax/model/model.py`, `deepmd/tf2/model/model.py`, `deepmd/pt_expt/model/get_model.py`), so this is zero-cost.
- `linear_atomic_model` — currently passed only by pt_expt. dpmodel/jax/tf2 must state their choice explicitly (for them the dpmodel class is correct — the point is that "I use the native class" becomes a visible, reviewed decision instead of a silent default).
2. Remove the fallback imports (`LinearEnergyAtomicModel as LinearEnergyAtomicModelDP`, `linear_atomic_model or ...`) from the factory body.
3. **Open question — `InnerPotentialAtomicModel`:** it is constructed inside the factory (`model_factory.py:325`) from a hardcoded import. Today this is harmless because the model is purely analytical (no trainable arrays, no runtime state), so the wrapping backend's conversion is lossless (verified in #5960, where an unused `inner_potential_model` parameter was removed for exactly that reason). But "the conversion happens to be lossless today" was also the implicit assumption for the linear class before #5960. Either:
- (a) inject it like the others, for uniformity; or
- (b) keep it hardcoded and pin the exemption with a comment + a negative test asserting the class carries no non-portable state.
## Acceptance
- `BackendModelFactory(...)` with a missing class raises `TypeError` at import time of the backend module — a missed injection is a hard failure, not a silent fallback.
- No dpmodel-class fallback import remains inside any factory construction path (grep-clean for the `or LinearEnergyAtomicModelDP` pattern).
- The `InnerPotentialAtomicModel` decision (3a or 3b) is implemented and pinned by a test.
## References
- #5964 — introduced the shared factory / canonical `linear_ener` composition.
- #5960 — the `use_amp` silent-loss bug caused by the hardcoded linear class; fixed by injecting `linear_atomic_model` (pt_expt only).
Contributor guide
Assessment
This issue has not been assessed yet.