fix(pt_expt): dispatch property models to DeepProperty inference#5724
fix(pt_expt): dispatch property models to DeepProperty inference#5724wanghan-iapcm wants to merge 1 commit into
Conversation
A pt_expt property checkpoint could be built and trained but not evaluated:
pt_expt DeepEval.model_type dispatched only energy/DOS/dipole/polar/WFC and
raised RuntimeError("Unknown model type") for property outputs, and the
evaluator did not expose the property metadata getters DeepProperty needs.
Underneath, the pt_expt PropertyModel itself only implemented get_var_name, not
get_task_dim or get_intensive.
Add get_task_dim and get_intensive to the pt_expt PropertyModel (mirroring the
PyTorch model), import DeepProperty in the pt_expt evaluator and dispatch to it
when the property variable name appears in the model output, and expose
get_var_name / get_task_dim / get_intensive that delegate to the reconstructed
model. The dispatch and getters are guarded by hasattr so non-property and
unknown model types are unaffected (they keep matching their own branches or
fall through to "Unknown model type"), and the guard on self._dpmodel keeps
metadata-only mode (no reconstructed model) raising a clear NotImplementedError
rather than mis-dispatching.
Adds source/tests/pt_expt/infer/test_deep_eval_property.py, a full
serialize -> .pte -> DeepEval round trip asserting model_type is DeepProperty,
the three getters, and the eval output shape. Without the fix the DeepEval
construction raises "Unknown model type". Verified pt property inference works
end to end (same mechanism) and the pt_expt energy inference suite is unaffected.
Fix deepmodeling#5671
📝 WalkthroughWalkthroughThis PR adds property-model support to the experimental PyTorch (pt_expt) backend's DeepEval inference wrapper, mirroring existing stable PyTorch/Paddle support. It adds a DeepProperty import, model_type dispatch logic, three new metadata accessor methods, corresponding PropertyModel methods, and a new end-to-end test. ChangesProperty model support in pt_expt DeepEval
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Test as TestDeepEvalProperty
participant DeepEval
participant PropertyModel
Test->>DeepEval: load .pte checkpoint
DeepEval->>PropertyModel: get_var_name()
PropertyModel-->>DeepEval: var_name
DeepEval->>DeepEval: model_type() checks var_name in output types
DeepEval-->>Test: returns DeepProperty
Test->>DeepEval: get_task_dim() / get_intensive()
DeepEval->>PropertyModel: get_task_dim() / get_intensive()
PropertyModel-->>DeepEval: task_dim / intensive flag
Test->>DeepEval: eval(...)
DeepEval-->>Test: property output
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
deepmd/pt_expt/infer/deep_eval.py (1)
711-718: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueProperty dispatch requires the reconstructed dpmodel; metadata-only
.pte/.pt2property archives still hitRuntimeError("Unknown model type").The new
elifbranch only fires whenself._dpmodel is not None. In metadata-only mode (_init_from_metadata, used whenmodel.jsonis absent), a property model's output type would still be present inmodel_output_type, but there is noget_var_name()to check against, so it falls through toraise RuntimeError("Unknown model type")— the exact error this PR intends to fix. This is consistent with the existing pattern thatget_var_name/get_task_dim/get_intensiveare also unavailable in metadata-only mode, so it's a known limitation rather than a regression, but the resulting generic error message will confuse users who hit it via a metadata-only archive.💡 Optional: give metadata-only property models a clearer error
elif ( self._dpmodel is not None and hasattr(self._dpmodel, "get_var_name") and self._dpmodel.get_var_name() in model_output_type ): return DeepProperty + elif self._dpmodel is None and "task_dim" in self.metadata: + raise NotImplementedError( + "Property model dispatch requires the reconstructed dpmodel " + "(model.json); this archive was loaded in metadata-only mode." + ) else: raise RuntimeError("Unknown model type")🤖 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/infer/deep_eval.py` around lines 711 - 718, The property-type dispatch in the model-type selection logic is too strict because it only checks self._dpmodel.get_var_name() when self._dpmodel is available, so metadata-only .pte/.pt2 archives fall through to the generic RuntimeError("Unknown model type"). Update the dispatch around the self._dpmodel branch to handle metadata-only property archives explicitly, either by recognizing the property model from the available metadata/model_output_type path or by raising a clearer, targeted error from this selector in DeepEval. Keep the fix localized to the model-type resolution logic and preserve the existing behavior for non-property models.
🤖 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/pt_expt/infer/deep_eval.py`:
- Around line 711-718: The property-type dispatch in the model-type selection
logic is too strict because it only checks self._dpmodel.get_var_name() when
self._dpmodel is available, so metadata-only .pte/.pt2 archives fall through to
the generic RuntimeError("Unknown model type"). Update the dispatch around the
self._dpmodel branch to handle metadata-only property archives explicitly,
either by recognizing the property model from the available
metadata/model_output_type path or by raising a clearer, targeted error from
this selector in DeepEval. Keep the fix localized to the model-type resolution
logic and preserve the existing behavior for non-property models.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5da33ea0-3592-4528-83f9-9761860d1752
📒 Files selected for processing (3)
deepmd/pt_expt/infer/deep_eval.pydeepmd/pt_expt/model/property_model.pysource/tests/pt_expt/infer/test_deep_eval_property.py
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5724 +/- ##
==========================================
- Coverage 81.26% 79.19% -2.07%
==========================================
Files 988 988
Lines 110877 110902 +25
Branches 4234 4234
==========================================
- Hits 90103 87830 -2273
- Misses 19249 21552 +2303
+ Partials 1525 1520 -5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
njzjz-bot
left a comment
There was a problem hiding this comment.
Thanks for the fix. I think this should be handled consistently across the DeepEval backends before merging.
This PR adds the DeepProperty dispatch and the property metadata getters for pt_expt, but the same gap still exists in the other backend DeepEval.model_type implementations I checked:
deepmd/dpmodel/infer/deep_eval.pydeepmd/jax/infer/deep_eval.pydeepmd/tf2/infer/deep_eval.py
Those backends still only dispatch energy / dos / dipole / polar / wfc, so a property model reaching the generic DeepEval(...) path would still raise Unknown model type. They also do not currently expose the get_var_name() / get_task_dim() / get_intensive() hooks needed by DeepProperty.change_output_def().
Please extend the same DeepProperty dispatch logic and metadata getter support to dpmodel, jax, and tf2 as well, with tests where the backend supports property inference. If any of these backends intentionally cannot support property models, please make that explicit in the code/tests/docs rather than leaving the same Unknown model type failure mode.
Authored by OpenClaw (model: custom-chat-jinzhezeng-group/gpt-5.5)
Problem
Fixes #5671. A pt_expt property checkpoint could be constructed and trained by the backend stack but not evaluated.
pt_exptDeepEval.model_typedispatched only energy, DOS, dipole, polar, and WFC outputs and then raisedRuntimeError("Unknown model type")for a property model, and the evaluator did not expose theget_intensive/get_var_name/get_task_dimgetters thatDeepPropertyneeds. Underneath, the pt_exptPropertyModelitself only implementedget_var_name, notget_task_dimorget_intensive, so simply mirroring the dispatch would not have been enough.Fix
pt_expt/model/property_model.py: addget_task_dim(fitting output dimension) andget_intensive(from the output def), mirroring the PyTorch property model.pt_expt/infer/deep_eval.py: importDeepProperty, dispatch to it when the property variable name appears in the model output, and exposeget_var_name/get_task_dim/get_intensivethat delegate to the reconstructed model.The dispatch branch and the getters are guarded by
hasattr(and byself._dpmodel is not None), so:"Unknown model type"rather than raising anAttributeError;NotImplementedErrorfor the property getters instead of mis-dispatching.Test
Adds
source/tests/pt_expt/infer/test_deep_eval_property.py, a fullserialize -> .pte -> DeepEvalround trip assertingmodel_type is DeepProperty, the three getters, and the eval output shape. Without the fix, constructing theDeepEvalraisesRuntimeError("Unknown model type").Verification for the reviewer's peace of mind: pt-backend property inference works end to end via the same mechanism (dispatch + getters + eval), and the full pt_expt energy inference suite (
source/tests/pt_expt/infer/test_deep_eval.py, 92 passed / 2 skipped) is unaffected by the added branch.Summary by CodeRabbit
New Features
Bug Fixes
Tests