Repository navigation
fix(finetune): ignore runtime-only descriptor options in warnings - #6088
SiyuanChenToDo wants to merge 2 commits into
Conversation
`descriptor.seed` only controls the RNG used when a module is first constructed; it is not loaded from the pretrained checkpoint. Reporting it as an incompatible descriptor change is misleading. Add it to the ignored descriptor keys and cover the warning path with a regression test. Fixes deepmodeling#6038 Co-Authored-By: Claude Code <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
njzjz-bot
left a comment
There was a problem hiding this comment.
Reviewed exact head a80bcf1. I found no blocking defect in the seed-only change: both warning entry points share the filtered comparison, structural differences still warn, and the PyTorch pretrained-tensor copy does not depend on this warning.
One nonblocking scope clarification: #6038 also explicitly requests ignoring use_amp (#6038 (comment)). An otherwise-identical DPA4C descriptor with use_amp=False versus True still emits the incompatibility warning at this head. The DPA4C serializer documents that this is execution policy rather than learned model state. Since this PR says “Fixes #6038”, please either cover that requested case with a regression test or keep the issue open to track it. This is a remaining pre-existing case, not a new regression introduced by this patch.
Validation: 5 repository tests run against AST-extracted comparison/warning functions, plus 25 focused probes, passed. Those cover None/integer/list/missing seed values, fallback after normalization failure, structural warning retention, unchanged inputs, both warning entry points, and a countercheck with the old ignored-key set. These are isolated tests; the full normalization and backend training suites were not run here. Test Python and the other normal GitHub Actions runs currently show action_required, so this is not a full CI pass.
Agent: dot
`seed` and `use_amp` are initialization and execution policies, not loaded parameters. Extend the ignored descriptor keys and cover both cases in the regression test. Fixes deepmodeling#6038 Co-Authored-By: Claude Code <noreply@anthropic.com>
njzjz-bot
left a comment
There was a problem hiding this comment.
Re-reviewed the delta from a80bcf1 to 0eab67c. This addresses the use_amp case from my earlier review: the shared descriptor comparison now ignores both runtime-only options, and the regression checks each one independently. The DPA4C serializer and PyTorch experimental wrapper confirm that use_amp controls autocast execution rather than checkpoint parameters.
No blocking findings in this delta. Validation: 5 selected repository tests against AST-extracted exact-source comparison/warning functions plus 40 focused probes passed (45 total). These cover both warning entry points, missing values, normalization-failure fallback, unchanged input dictionaries, retention of real shape/precision differences, and counterchecks showing the updated regression fails if either ignore entry is removed.
These are isolated tests; full normalization and backend training suites were not run here. Normal GitHub Actions workflows, including Test Python, still report action_required at this head, so this does not establish a full CI pass.
Agent: dot
Summary
descriptor.seedanddescriptor.use_ampare runtime-only options.seedonly selects the RNG used when a module is first constructed, and
use_amponly selects the autocast execution policy. Neither is a loaded parameter:
the DPA4C serializer deliberately omits
use_ampfrom the checkpoint, and thept / pt_expt modules store it as a Python bool that
load_state_dictdoes notrestore. Changing either value therefore does not change which pretrained
weights are reusable, but
warn_configuration_mismatch_during_finetunereported them as incompatible descriptor changes.
This adds
seedanduse_ampto_IGNORED_DESCRIPTOR_KEYSnext totrainable, and adds a regression test covering both runtime-onlydifferences.
Fixes #6038
Duplicate-work check
gh issue view 6038 --repo deepmodeling/deepmd-kit --comments: no linked PRor assignment.
gh pr list --repo deepmodeling/deepmd-kit --state open --search "6038":no results.
gh pr list --repo deepmodeling/deepmd-kit --state open --search "finetune seed":no results.
Verification
pytest source/tests/common/test_finetune_utils.py— 16 passed.ruff check deepmd/utils/finetune.py source/tests/common/test_finetune_utils.py— passed.
ruff format --check deepmd/utils/finetune.py source/tests/common/test_finetune_utils.py— passed.seedoruse_ampmakes the correspondingregression case fail, confirming the test exercises both cases.
AI assistance
This change was prepared with AI assistance (Claude Code). I reviewed the diff,
ran the commands above, and can explain and defend the change.
Summary by CodeRabbit
seedoruse_amp. These fields are treated liketrainablewhen comparing descriptor configurations.