Make BackendModelFactory class injection complete and required (no silent dpmodel fallback)

Open
  • Is something wrong?
#5,977 0 comments 0 reactions 0 assignees View on GitHub

Maintainers usually reply within 3 days

Nobody has claimed this yet.

Assessment

Difficulty
4/5
Estimated time
3-5 days
Newbie friendliness
45/100
Issue type
Refactor
Clarity
Mostly clear
Activity status
Active
Tech stack
python, pytorch, tensorflow
Domain
backend, machine-learning

Research direction

Read deepmd/dpmodel/model/model_factory.py and compare the four construction sites in deepmd/dpmodel/model/model.py, deepmd/jax/model/model.py, deepmd/tf2/model/model.py, and deepmd/pt_expt/model/get_model.py. Review the #5960 and #5964 context, then choose and pin the InnerPotentialAtomicModel decision with a test. Done means all injections are required, no fallback construction path remains, and missing classes fail with TypeError.

Written by the indexing model from the issue text.

Description

enhancement

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).
Dominant language
Python
Stars
2.1k
Forks
663
Avg merge
3d 11h
Merged PRs (30d)
25

Getting set up

Open in Codespaces

Starts the project's dev container in your browser, under your own GitHub account.

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

More from deepmodeling/deepmd-kit

All issues in deepmodeling/deepmd-kit

Similar issues

More Python issues

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.