refactor(model): unify dpmodel backend factories#5786
Conversation
|
Caution Review failedAn error occurred during the review process. Please try again later. ✨ 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.
🧹 Nitpick comments (1)
source/tests/common/dpmodel/test_model_factory.py (1)
71-79: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd symmetric test coverage for the ZBL
NotImplementedErrorbranch.Only the
spin_model_factory is Nonebranch is tested; the siblingzbl_model_factory is Nonebranch (model_factory.py lines 58-61) has no direct test.♻️ Suggested additional test
def test_unsupported_standard_variant(self) -> None: with self.assertRaisesRegex( NotImplementedError, "Spin model is not implemented yet" ): get_model( {"spin": {}}, base_model=_BaseModel, standard_model_factory=_factory("standard"), ) + + def test_unsupported_zbl_variant(self) -> None: + with self.assertRaisesRegex( + NotImplementedError, "ZBL model is not implemented yet" + ): + get_model( + {"use_srtab": "table"}, + base_model=_BaseModel, + standard_model_factory=_factory("standard"), + )🤖 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/dpmodel/test_model_factory.py` around lines 71 - 79, Add a test alongside test_unsupported_standard_variant that exercises get_model with a ZBL configuration and zbl_model_factory set to None, while providing the other required factory arguments. Assert that it raises NotImplementedError with the expected ZBL-not-implemented message, covering the sibling branch in get_model.
🤖 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.
Nitpick comments:
In `@source/tests/common/dpmodel/test_model_factory.py`:
- Around line 71-79: Add a test alongside test_unsupported_standard_variant that
exercises get_model with a ZBL configuration and zbl_model_factory set to None,
while providing the other required factory arguments. Assert that it raises
NotImplementedError with the expected ZBL-not-implemented message, covering the
sibling branch in get_model.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 66d07fa5-94c0-4fc4-97a7-2ff40358cf54
📒 Files selected for processing (7)
deepmd/dpmodel/model/model.pydeepmd/dpmodel/model/model_factory.pydeepmd/jax/model/model.pydeepmd/pt_expt/model/get_model.pydeepmd/tf2/model/model.pysource/tests/common/dpmodel/test_model_factory.pysource/tests/pt_expt/model/test_get_model_zbl.py
9d6222c to
083e5b2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
source/tests/common/dpmodel/test_model_factory.py (1)
123-131: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd symmetric coverage for the ZBL unsupported branch.
test_unsupported_standard_variantonly exercises thespinbranch (zbl_model_factory=Nonewith"use_srtab"in data is untested). Given this shared factory now backs dpmodel, JAX, TF2, and pt_expt routing, testing bothNotImplementedErrorbranches would guard against regressions in the precedence/error logic.🧪 Suggested addition
def test_unsupported_standard_variant(self) -> None: with self.assertRaisesRegex( NotImplementedError, "Spin model is not implemented yet" ): get_model( {"spin": {}}, base_model=_BaseModel, standard_model_factory=_factory("standard"), ) + with self.assertRaisesRegex( + NotImplementedError, "ZBL model is not implemented yet" + ): + get_model( + {"use_srtab": "table"}, + base_model=_BaseModel, + standard_model_factory=_factory("standard"), + )🤖 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/dpmodel/test_model_factory.py` around lines 123 - 131, Add a symmetric test alongside test_unsupported_standard_variant that calls get_model with data containing "use_srtab" and zbl_model_factory=None, while providing the required base_model and standard_model_factory arguments. Assert that this ZBL route raises NotImplementedError with the expected unsupported-ZBL error message, preserving coverage of both unsupported branches and their precedence.
🤖 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`:
- Around line 105-120: Update get_zbl_model’s zbl_model construction to forward
data["smin_alpha"] alongside sw_rmin and sw_rmax, preserving configured softmin
values instead of relying on the constructor default.
---
Nitpick comments:
In `@source/tests/common/dpmodel/test_model_factory.py`:
- Around line 123-131: Add a symmetric test alongside
test_unsupported_standard_variant that calls get_model with data containing
"use_srtab" and zbl_model_factory=None, while providing the required base_model
and standard_model_factory arguments. Assert that this ZBL route raises
NotImplementedError with the expected unsupported-ZBL error message, preserving
coverage of both unsupported branches and their precedence.
🪄 Autofix (Beta)
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
Run ID: 29148648-bb52-4911-9dee-b80c916543a8
📒 Files selected for processing (7)
deepmd/dpmodel/model/model.pydeepmd/dpmodel/model/model_factory.pydeepmd/jax/model/model.pydeepmd/pt_expt/model/get_model.pydeepmd/tf2/model/model.pysource/tests/common/dpmodel/test_model_factory.pysource/tests/pt_expt/model/test_get_model_zbl.py
🚧 Files skipped from review as they are similar to previous changes (1)
- source/tests/pt_expt/model/test_get_model_zbl.py
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5786 +/- ##
==========================================
- Coverage 79.76% 78.29% -1.47%
==========================================
Files 1023 1051 +28
Lines 117463 120417 +2954
Branches 4325 4351 +26
==========================================
+ Hits 93691 94282 +591
- Misses 22229 24565 +2336
- Partials 1543 1570 +27 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Centralize descriptor and fitting parameter injection, standard-model selection, ZBL assembly, and model-type routing across dpmodel, pt_expt, JAX, and TF2. Keep backend modules limited to their native class registries and supported special cases. Coding-Agent: Codex Codex-Version: codex-cli 0.144.1 Model: gpt-5.6-sol Reasoning-Effort: xhigh
083e5b2 to
6e5a4a9
Compare
|
Addressed the review-summary feedback and further consolidated the backend factories in 6e5a4a9:
All inline threads have been replied to and resolved. Targeted dpmodel, JAX, pt_expt, spin, ZBL, DPA4/SeZM, linear, and TF2 smoke validations pass, as do all commit hooks. Coding agent: Codex |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
deepmd/dpmodel/model/model_factory.py (1)
89-125: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueRedundant deep-copy of
datainget_zbl_model.
datais deep-copied at Line 100, butget_zbl_modelonly ever reads fromdataafterward (no mutation), andget_model_componentsalready performs its own internal deep-copy at Line 33. This duplicates the copy of the whole config dict on every ZBL build for no benefit;get_standard_modeldoesn't need this pattern either. Also confirms the earliersmin_alphaforwarding gap from a prior review is now fixed at Line 122.♻️ Proposed fix
"""Construct a ZBL model from backend-native atomic model classes.""" - data = copy.deepcopy(data) descriptor, fitting, fitting_type = get_model_components(🤖 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 89 - 125, Remove the redundant copy.deepcopy call from get_zbl_model. Keep passing the original data into get_model_components and preserve all existing read-only lookups and smin_alpha forwarding unchanged.
🤖 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.
Nitpick comments:
In `@deepmd/dpmodel/model/model_factory.py`:
- Around line 89-125: Remove the redundant copy.deepcopy call from
get_zbl_model. Keep passing the original data into get_model_components and
preserve all existing read-only lookups and smin_alpha forwarding unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 20244ca3-1bf1-4ea5-a5d2-007b35fce206
📒 Files selected for processing (8)
deepmd/dpmodel/model/__init__.pydeepmd/dpmodel/model/model.pydeepmd/dpmodel/model/model_factory.pydeepmd/jax/model/model.pydeepmd/pt_expt/model/get_model.pydeepmd/tf2/model/model.pysource/tests/common/dpmodel/test_model_factory.pysource/tests/pt_expt/model/test_get_model_zbl.py
🚧 Files skipped from review as they are similar to previous changes (6)
- source/tests/pt_expt/model/test_get_model_zbl.py
- deepmd/jax/model/model.py
- deepmd/tf2/model/model.py
- source/tests/common/dpmodel/test_model_factory.py
- deepmd/dpmodel/model/model.py
- deepmd/pt_expt/model/get_model.py
wanghan-iapcm
left a comment
wanghan-iapcm
left a comment
There was a problem hiding this comment.
Nice consolidation. I verified the model-registry keys align across all four backends (dipole/polar/dos/ener/property, with direct_force_ener remapped to ener) so get_class_by_type resolves the same classes the old if/elif did, and the init imports make the registrations run. Routing precedence (spin->zbl->standard), the explicit-type fallback, and the pt_expt DPA4/SeZM/linear special cases are all preserved. One non-blocking note for a follow-up: get_zbl_model now honors a configured smin_alpha (data.get('smin_alpha', 0.1)) which the old dp/jax/tf2 path silently dropped -- a good fix, but the pt backend still ignores smin_alpha, so the two backends are now inconsistent on that key. LGTM.
Dismissing to re-review through the /code-review skill per process.
wanghan-iapcm
left a comment
wanghan-iapcm
left a comment
There was a problem hiding this comment.
A few non-blocking notes from re-reviewing this refactor. No blocking issues — registry-key alignment, kwarg/type-pop semantics, routing precedence, and the spin/ZBL special cases are all preserved. The items below sit just under my posting bar; raising them for optional follow-up.
Cover shared model-factory error branches, preserve non-default ZBL softmin values across the dpmodel-driven backends, and avoid an unnecessary ZBL config copy. Coding-Agent: Codex Codex-Version: codex-cli 0.144.4 Model: gpt-5.6-sol Reasoning-Effort: xhigh
|
Possible reviewers based on changed lines, exact file history, and exact-file review history:
No review request was made automatically. Coding agent: Codex |
wanghan-iapcm
left a comment
wanghan-iapcm
left a comment
There was a problem hiding this comment.
All three points addressed and verified at HEAD.
The two error branches now have tests (test_model_level_type_embedding_is_rejected, test_zbl_rejects_non_energy_fitting). The smin_alpha regression is covered where it actually matters - 0.37 is asserted in the shared factory test and in both the JAX and TF2 ZBL tests, so the propagation this PR newly activates for dpmodel/jax/tf2 is pinned rather than relying on the pt_expt-only assertion. And the descriptor-derived pair-table path is both covered and documented; thanks for calling out the #4339 behaviour explicitly in the description, since that part isn't pure restructuring.
One observation rather than a request: the same commit also drops data = copy.deepcopy(data) from get_zbl_model. That is safe for construction - get_model_components deepcopies internally, get_zbl_model only reads data, and test_does_not_mutate_input guards it - but the constructed model now holds the caller's type_map list itself instead of a copy, so the two alias afterwards. Worth keeping in mind, and generally worth landing production changes under a non-test(...) commit so they are visible in the log.
4 participants
Summary
Validation
Coding agent: Codex
Codex version: codex-cli 0.144.4
Model: gpt-5.6-sol
Reasoning effort: xhigh