fix(jax,tf2): deep-copy and propagate type_map in ZBL model factories#5725
Conversation
The JAX and TF2 ZBL model factories, unlike the standard and dpmodel factories, mutated the caller's config in place (popping descriptor and fitting_net "type") and did not inject type_map into the descriptor and fitting sub-configs. The in-place mutation is inconsistent with the standard-model factories, and the missing type_map leaves the descriptor and fitting without the model type map that dpmodel/standard construction provides. Deep-copy the input and inject type_map into the descriptor and fitting configs in both get_zbl_model factories, mirroring the dpmodel ZBL path. The issue also suspected the retained JAX checkpoint metadata could be corrupted by the in-place mutation. That does not happen: the JAX trainer passes a deepcopy of the model params into the factory, so the stored model_def_script is never mutated. No change is made for that sub-claim. Adds source/tests/jax/test_zbl_model.py and a TF2-gated source/tests/consistent/test_tf2_zbl_model.py asserting the factory leaves the input dict unchanged and that the constructed descriptor and fitting carry the model type_map. Both assertions fail on master (input mutated; sub-config type_map is None). Fix deepmodeling#5676
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe JAX and TF2 ChangesZBL Model Factory Fixes
Estimated code review effort: 2 (Simple) | ~12 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant get_zbl_model
participant Descriptor
participant FittingNet
participant DPZBLModel
Caller->>get_zbl_model: data (config dict)
get_zbl_model->>get_zbl_model: deepcopy(data)
get_zbl_model->>Descriptor: ntypes, type_map
get_zbl_model->>Descriptor: construct(descriptor_kwargs)
get_zbl_model->>FittingNet: type_map
get_zbl_model->>FittingNet: construct(fitting_kwargs)
get_zbl_model->>DPZBLModel: assemble(Descriptor, FittingNet)
DPZBLModel-->>Caller: model instance
Related Issues: Suggested labels: bug, JAX, TF2, tests Suggested reviewers: njzjz, iProzd 🎉 A tiny fix, deep and wise, 🚥 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5725 +/- ##
==========================================
- Coverage 81.26% 81.15% -0.11%
==========================================
Files 988 988
Lines 110877 110889 +12
Branches 4234 4233 -1
==========================================
- Hits 90103 89996 -107
- Misses 19249 19369 +120
+ Partials 1525 1524 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Problem
Fixes #5676. The JAX and TF2 ZBL model factories (
get_zbl_model), unlike the standard-model factories and the dpmodel ZBL path, mutated the caller's config dict in place (popping the descriptor andfitting_nettype) and did not injecttype_mapinto the descriptor and fitting sub-configs. The in-place mutation is inconsistent with the neighboring factories, and the missingtype_mapleaves the descriptor and fitting without the model type map that standard/dpmodel construction provides.Fix
Deep-copy the input and inject
type_mapinto the descriptor and fitting configs in bothget_zbl_modelfactories, mirroring the dpmodel ZBL path.The issue also suspected the retained JAX checkpoint metadata could be corrupted by the in-place mutation. That does not actually happen: the JAX trainer passes a
deepcopyof the model params into the factory, so the storedmodel_def_scriptis never mutated. No change is made for that sub-claim.Test
Adds
source/tests/jax/test_zbl_model.pyand a TF2-gatedsource/tests/consistent/test_tf2_zbl_model.py, each asserting the factory leaves the input dict unchanged and that the constructed descriptor and fitting carry the modeltype_map. Both assertions fail on master (the input is mutated and the sub-configtype_mapisNone) and pass with the fix. The TF2 test is gated onINSTALLED_TF2(DEEPMD_TEST_TF2=1).Summary by CodeRabbit
Bug Fixes
Tests