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

Đang mở
  • Có gì không ổn?
#5,977 0 bình luận 0 reaction 0 người được giao Xem trên GitHub

Maintainer thường phản hồi trong vòng 3 ngày

Chưa có ai nhận issue này.

Đánh giá

Độ khó
4/5
Thời gian dự kiến
3-5 ngày
Mức phù hợp với người mới
45/100
Loại issue
Tái cấu trúc
Độ rõ ràng
Khá rõ ràng
Mức độ hoạt động
Sôi nổi
Công nghệ
python, pytorch, tensorflow
Lĩnh vực
backend, machine-learning

Hướng nghiên cứu

Đọc deepmd/dpmodel/model/model_factory.py và so sánh bốn điểm khởi tạo trong deepmd/dpmodel/model/model.py, deepmd/jax/model/model.py, deepmd/tf2/model/model.py và deepmd/pt_expt/model/get_model.py. Xem lại ngữ cảnh của #5960 và #5964, sau đó chọn và cố định quyết định về InnerPotentialAtomicModel bằng một test. Hoàn thành nghĩa là mọi injection đều bắt buộc, không còn đường dẫn khởi tạo fallback nào và các class bị thiếu sẽ thất bại với TypeError.

Do mô hình lập chỉ mục viết ra từ nội dung của issue.

Mô tả

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).
Ngôn ngữ chính
Python
Star
2.1k
Fork
663
Merge trung bình
3 ngày 11 giờ
Pull request đã merge (30 ngày)
25

Chuẩn bị môi trường

Mở trong Codespaces

Khởi chạy dev container của dự án ngay trên trình duyệt, bằng tài khoản GitHub của bạn.

Bắt đầu từ đâu

  1. Đọc hết issue, rồi đọc hướng dẫn đóng góp của dự án.
  2. Bình luận trên issue rằng bạn sẽ nhận — tránh hai người làm cùng một việc.
  3. Fork repository và làm thay đổi trên một nhánh.
  4. Mở pull request có tham chiếu số hiệu của issue.

Issue khác của deepmodeling/deepmd-kit

Tất cả issue của deepmodeling/deepmd-kit

Issue tương tự

Thêm issue về Python

Nhận issue mới trong hộp thư của bạn

Bản tóm tắt ngắn những issue GitHub phù hợp với người mới.