ci: validate PEFT reload suite from #3945 - #3965
Closed
yuhezhang-ai wants to merge 7 commits into
Closed
yuhezhang-ai wants to merge 7 commits into
yuhezhang-ai wants to merge 7 commits into
Conversation
First deliverable for #3867. Export defects keep recurring because the outer prefix, the model-owned renames, the fused expert layout and the target metadata are produced in separate places, and the existing tests mostly check that AutoModel can read back what AutoModel wrote. That passes even when the converter is wrong in both directions, which is how the artifact ends up unloadable in real PEFT. Each family is one entry in a registry: a tiny Transformers model plus the AutoModel adapter that owns its naming. Every entry saves through Checkpointer.save_model with is_peft, reloads with PeftModel.from_pretrained, and must match tensor for tensor and in its forward output. A second test requires the bulk and per-tensor exports to agree, since streaming consumers use the latter. The model comes from Transformers rather than the native AutoModel class because several native MoE classes build rope buffers on torch.cuda.current_device() and cannot be constructed on CPU. The adapter is the component under test either way. Covers llama (dense control, no adapter), nemotron_v3, qwen3_moe and minimax_m2. Reverting the Nemotron PEFT namespace selection, the AMINT-330 defect fixed in #3866, fails only that family's case. Signed-off-by: stanley1208 <stanley.mei08@gmail.com>
Adds qwen3_omni_moe. The family needs two things the text families do not: the source model and the reload target are different classes, and the comparison runs against the adapted submodule rather than the wrapper. AutoModel trains the Omni thinker on its own, so its keys carry no thinker. segment and the adapter adds one on export. The artifact therefore targets the full Omni model, and the suite now lets a family name a separate reload target plus the attribute that corresponds to the source. The case is marked xfail: the exported tensors carry the thinker. segment but target_modules does not, so PEFT's suffix match also adapts talker.model.* on the full model. Those adapters have no counterpart in the file and are initialized randomly, which silently adapts the talker. Tensor names and target metadata are produced by different code paths, which is the asymmetry #3867 is about. Signed-off-by: stanley1208 <stanley.mei08@gmail.com>
Found by the reload suite added in this branch. to_hf and convert_single_tensor_to_hf both put exported adapter tensors under thinker., but target_modules went out unchanged because the adapter never overrode map_peft_target_module_to_hf. PEFT suffix-matches that list against the receiving model, so on the full Omni model an entry like model.layers.0.self_attn.q_proj also matched talker.model.layers.0.self_attn.q_proj. The talker got adapter modules the checkpoint has no weights for, so they stayed randomly initialized: a model silently carrying untrained adapters on a tower the user never fine-tuned. Override the hook the same way kimi_k3 does, whose docstring already states the rule: target_modules entries need the same renames the state-dict keys get. Both export formats keep the namespace, since the omni checkpoint has no layout that addresses thinker modules without it. The suite's xfail machinery is now declarative rather than an imperative pytest.xfail() call, which aborted before the body ran and would have let a fixed export keep its marker unnoticed. Signed-off-by: stanley1208 <stanley.mei08@gmail.com>
map_peft_target_module_to_hf added the thinker. namespace every time. to_hf and convert_single_tensor_to_hf both check _uses_thinker_prefix first, so on a thinker-only base the config said thinker.model.layers... while the tensors stayed base_model.model.model.layers..., and peft refused the reload with "Target modules ... not found". The repo already relies on that layout: two existing tests assert the flag goes False for a thinker-less checkpoint. The full omni case is unchanged, since a fresh export and a full omni load both leave the flag True. Adds a standalone-thinker case to the reload suite and a unit test tying target_modules to the exported tensor names. Also raises the peft floor to 0.20.0. 0.19.1 passes distributed_operation into WeightConverter, which has never accepted it in any transformers v5, so any MoE lora reload dies with a TypeError before it reads a weight. It reproduces the same on transformers 5.12.1 and 5.15.1, and it already breaks three merged tests in test_moe_peft_v5_state_dict_adapters.py. 0.20.0 assigns those attributes after construction instead. Signed-off-by: stanley1208 <stanley.mei08@gmail.com>
Signed-off-by: Yuhe Zhang <yuhez@nvidia.com>
Signed-off-by: Yuhe Zhang <yuhez@nvidia.com>
Contributor
Author
|
/ok to test e7a0008 |
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do ?
CI proxy for #3945, testing the shared HF PEFT reload suite and Qwen3 Omni target-module namespace fix through the maintainer CI queue. The source PR remains the merge target; this proxy is for validation only.
The source branch includes current main (
44cf34834b679ff0cf0df4ff56cc37ddf5273a02) and the generated dependency refresh for PEFT 0.20.0. This proxy preserves the author's commits and starts at the exact same source head:e7a0008c03ea64121e0ea81fe93202cbbde3b264.Changelog
Before your PR is "Ready for review"
Additional Information
Original contribution by @stanley1208. Merge #3945 after validation, then close this proxy.