docs(argcheck): document backend support matrices - #5915
Conversation
Add explicit cached backend support labels for argument documentation and cover the verified backend matrices with focused tests. Coding-Agent: Codex Codex-Version: codex-cli 0.144.6 Model: gpt-5.6-sol Reasoning-Effort: xhigh
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughBackend documentation is centralized in a registry, formatted through a validated helper, and applied across argument definitions. New tests validate ordering, visibility, unknown keys, representative documentation prefixes, and spacing. ChangesBackend documentation generation
Estimated code review effort: 4 (Complex) | ~45 minutes 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/utils/argcheck.py`:
- Line 1372: Normalize the embedded supported_backends() text in the affected
documentation strings: update doc_smooth_type_embedding and the corresponding
strings near the other listed sites so mid-sentence text has exactly one
separating space, while the list-of-bools descriptions have one space before the
backend label and none before the following colon. Account for
supported_backends()’s trailing space by trimming it where needed and adding
literal spacing only where required.
- Around line 3449-3454: Update the DPA4/SeZM fitting configuration around
doc_fitting and fitting_args_plugin.register("property", ...) so generated
documentation does not imply TensorFlow 2 support for DPA4 property fitting. Use
DPA4-specific backend labels or registration while preserving the existing
PyTorch and PyTorch Exportable support descriptions.
🪄 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 Plus
Run ID: 12f096e6-c7aa-4c97-a45c-4abd067497b3
📒 Files selected for processing (2)
deepmd/utils/argcheck.pysource/tests/common/test_argcheck_backend_docs.py
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5915 +/- ##
==========================================
- Coverage 79.21% 78.96% -0.25%
==========================================
Files 1069 1069
Lines 124070 124084 +14
Branches 4522 4522
==========================================
- Hits 98278 97982 -296
- Misses 24171 24482 +311
+ Partials 1621 1620 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
wanghan-iapcm
left a comment
wanghan-iapcm
left a comment
There was a problem hiding this comment.
I went through the changed support sets mechanically rather than by eye -- extracting every rendered (Supported Backend: ...) label from the base and the head by walking the full argument tree, then diffing the two mappings and checking each changed entry against the actual registration decorators.
Most of it holds up well. All ten descriptor variants match their BaseDescriptor.register decorators exactly, including the non-obvious exclusions; all seven loss variants match the trainer dispatchers; the model variants, the spin arguments, finetune_head, atom_exclude_types / pair_exclude_types, gradient_max_norm, stat_file and rglob_patterns all check out. The mechanism itself is sound, and every key in the registry does correspond to a real package directory, tf2 included.
Three claims are wrong, though, and I have left them inline. Two are false negatives that would actively mislead a user, and one is an argument that lost its label entirely when the old constant was deleted.
The thing I would most like your view on is structural rather than any individual entry. This PR takes the file from six labels to 216, which turns a convention of "mark what is restricted" into "declare the full matrix for everything". That declared matrix duplicates information the code already carries in its registration decorators, and it now has to be maintained by hand against six backends -- while, as noted in the test comment below, nothing in CI can tell us when it drifts. The errors here are the predictable consequence of that: the mechanism is fine, but hand-declaring two hundred facts about other people's modules is difficult to keep true. Deriving the labels from the plugin registries, at least for descriptors and fittings where registration is the criterion, would make most of the matrix self-maintaining and reduce the hand-written part to the genuinely judgement-based cases like the training options.
That is a design question rather than a defect, and if you would rather land the corrections and keep the declarative form, that is a reasonable call -- but it is worth making deliberately, because the maintenance burden lands on whoever adds the next backend.
One note on the criterion itself. Excluding jax from the non-energy fittings is correct on a trainability reading, since the JAX trainer raises on any loss but ener -- I checked this because it looked like an inconsistency at first, and it is not one. But descriptors are labelled on a registration reading. Both are defensible; stating which one applies, in the registry comment, would make the matrix auditable by the next person instead of requiring them to re-derive your reasoning.
Not repeating the two points coderabbit already raised on spacing and on the property/DPA4 label; both look valid to me.
Correct the reviewed backend labels and make support-label regression tests independent of the formatter implementation. Coding-Agent: Codex Codex-Version: codex-cli 0.144.6 Model: gpt-5.6-sol Reasoning-Effort: xhigh
There was a problem hiding this comment.
🧹 Nitpick comments (1)
source/tests/common/test_argcheck_backend_docs.py (1)
13-21: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAdd a cache-reuse regression test.
The PR promises cached labels, but this suite would remain green if memoization were removed. Exercise the intended cache behavior (including cache isolation around registry patching) so the performance contract is protected.
🤖 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/test_argcheck_backend_docs.py` around lines 13 - 21, Add a regression test alongside test_registry_order_and_duplicate_keys that verifies supported_backends reuses cached labels on repeated calls, then confirms registry patching uses an isolated cache and does not leak modified labels into the original cached result. Use the existing supported_backends API and registry-patching mechanisms, preserving the current ordering and duplicate-key assertions.
🤖 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/test_argcheck_backend_docs.py`:
- Around line 13-21: Add a regression test alongside
test_registry_order_and_duplicate_keys that verifies supported_backends reuses
cached labels on repeated calls, then confirms registry patching uses an
isolated cache and does not leak modified labels into the original cached
result. Use the existing supported_backends API and registry-patching
mechanisms, preserving the current ordering and duplicate-key assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c6946270-8953-404d-a16e-9bd4d9cb42ce
📒 Files selected for processing (2)
deepmd/utils/argcheck.pysource/tests/common/test_argcheck_backend_docs.py
🚧 Files skipped from review as they are similar to previous changes (1)
- deepmd/utils/argcheck.py
Normalize embedded label spacing and narrow DPA4 property fitting documentation to its PyTorch-only support. Coding-Agent: Codex Codex-Version: codex-cli 0.144.6 Model: gpt-5.6-sol Reasoning-Effort: xhigh
|
Thanks for the mechanical audit and the design discussion. I kept the support declarations explicit in this PR rather than deriving them from backend registries. The criterion is user-configurable behavior, which is not uniform: descriptor variants generally follow registration, while fitting, loss, and training entries can be narrower when a registered component is inference-only or the trainer cannot consume an option. I documented that criterion next to the registry. The common tests now treat the declarations as change-detecting regression guards, not proof of backend correctness, and use literal expected labels. Importing every backend registry into this common documentation path would also make it dependency-heavy, so automated derivation is better considered as a separate follow-up with a deliberately scoped criterion. I also fixed the two CodeRabbit points endorsed in the review summary in af05ca3: embedded labels now render with exactly one separator, and DPA4 gets a fresh property fitting argument narrowed to PyTorch without changing the generic property fitting label. Validation: Coding agent: Codex |
Resolve the stat_file_mode documentation conflict by retaining the expanded backend support from master through the centralized support-label helper. Add a regression assertion for the merged backend matrix. Coding-Agent: Codex Codex-Version: codex-cli 0.144.6 Model: gpt-5.6-sol Reasoning-Effort: xhigh
wanghan-iapcm
left a comment
wanghan-iapcm
left a comment
There was a problem hiding this comment.
All five points check out against af05ca3d8, verified against the files rather than the replies.
I paid particular attention to the sub-argument labels, since those were asserted rather than shown. They are exactly right: on this branch pt_expt's Adam path is torch.optim.Adam(params, lr=initial_lr) with no betas and no weight decay, AdamW takes weight_decay but not the betas, and LKF/AdaMuon/HybridMuon raise. All six corresponding labels match. Likewise the dpa4_property_fitting.doc mutation in the follow-up commit is safe for the reason the comment gives -- fitting_property() returns a list, so get_argument builds a fresh wrapper each call and get_all_argument constructs its own independently, so nothing leaks into the generic property fitting.
Removing the cache and rewriting the registry comment to describe the actual import-time lifecycle is the right resolution, and the literal-string assertions plus the out-of-order, duplicate, all-hidden and unknown-key cases make the test file a real regression guard instead of a restatement of the source.
One forward-looking note, not a change request. #5913 rewrites this same pt_expt optimizer block so that both Adam and AdamW receive betas and weight_decay, and adds HybridMuon support. When that lands, Adam.weight_decay, both optimizers' adam_beta1/adam_beta2, and the HybridMuon type label all need pt_expt added. The literal tests here pin argcheck's own strings, so a change confined to the pt_expt side would leave them green while the labels go stale -- whichever of the two merges second will need to reconcile them by hand.
5 participants
Summary
supported_backends(...)labels while keeping explicit backend keys beside each documented argument and varianttf), PyTorch (pt), JAX (jax), PaddlePaddle (pd), PyTorch Exportable (pt_expt), and TensorFlow 2 (tf2)visible=FalseVerification
ruff format .ruff check .venv/bin/python -m pytest source/tests/common/test_argcheck_backend_docs.py source/tests/common/test_doc_train_input.py -q(10 passed)venv/bin/python -m py_compile deepmd/utils/argcheck.py source/tests/common/test_argcheck_backend_docs.pygit diff --checkdp doc-train-inputand confirmed the six-backend labeldp --versionand importeddeepmd,deepmd.tf, anddeepmd.ptRefs #5755
Refs #5756
Refs #5757
Coding agent: Codex
Codex version: codex-cli 0.144.6
Model: gpt-5.6-sol
Reasoning effort: xhigh
Summary by CodeRabbit
Documentation
Tests