fix(tf2): register se_t, se_t_tebd, and se_atten_v2 descriptors#5721
Conversation
TF2 standard-model construction resolves descriptor classes through BaseDescriptor.get_class_by_type(<type>) on the TF2 descriptor registry, which is a separate registry from the dpmodel and JAX ones. The se_t, se_t_tebd, and se_atten_v2 wrappers were defined without the @BaseDescriptor.register(...) decorators their JAX counterparts carry, so their config type names (se_e3, se_at, se_a_3be, se_e3_tebd, se_atten_v2) could not be resolved and TF2 model construction failed with an unknown-descriptor error, even though the wrapper classes exist and are exported by deepmd.tf2.descriptor. Add the missing decorators, matching the JAX registrations for the same descriptor names. Adds a registration test asserting every TF2 descriptor type name resolves via get_class_by_type; it fails on master for the five unregistered names and passes with the fix. The test is gated on INSTALLED_TF2 and run with DEEPMD_TEST_TF2=1. Existing TF2 consistency tests never caught this because they instantiate the wrapper classes directly rather than through the registry. Fix deepmodeling#5677
|
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)
📝 WalkthroughWalkthroughAdds ChangesTF2 Descriptor Registration
Estimated code review effort: 1 (Trivial) | ~5 minutes Related issues: Suggested labels: TF2, bug, tests Suggested reviewers: njzjz, wanghan-iapcm Poem:
🚥 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 #5721 +/- ##
==========================================
- Coverage 81.26% 81.14% -0.13%
==========================================
Files 988 988
Lines 110877 110884 +7
Branches 4234 4232 -2
==========================================
- Hits 90103 89973 -130
- Misses 19249 19387 +138
+ Partials 1525 1524 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Problem
Fixes #5677. TF2 standard-model construction resolves descriptor classes through
BaseDescriptor.get_class_by_type(<type>), and the TF2 descriptor registry is a distinct registry from the dpmodel and JAX ones (each backend builds its own viamake_base_descriptor). These_t,se_t_tebd, andse_atten_v2wrappers were defined without the@BaseDescriptor.register(...)decorators their JAX counterparts carry, so their config type names could not be resolved and TF2 model construction failed with an unknown-descriptor error even though the wrapper classes exist and are exported bydeepmd.tf2.descriptor. The affected type names arese_e3,se_at,se_a_3be(allse_t),se_e3_tebd(se_t_tebd), andse_atten_v2.Fix
Add the missing
@BaseDescriptor.register(...)decorators, matching the JAX registrations for the same descriptor names. These_e3_tebdregistration goes on the outerDescrptSeTTebdonly (not the block), mirroring JAX.Test
Adds
source/tests/consistent/test_tf2_descriptor_registration.py, which asserts that every TF2 descriptor config type name resolves viaget_class_by_type. It fails on master for the five previously-unregistered names and passes with the fix. The test is gated onINSTALLED_TF2(run withDEEPMD_TEST_TF2=1). Existing TF2 consistency tests never caught this because they instantiate the wrapper classes directly rather than through the registry string lookup.Summary by CodeRabbit