Conversation
a86c066 to
43674e9
Compare
HF format conversions can change DTensor placements. Restore the declared model layout before extracting each local tensor, rather than loading a shard with the wrong shape or ownership.
43674e9 to
e4535af
Compare
|
@claude review this PR Is the issue caused by such code?
These code exists because
We are going to change this soon, so that both are defined in the fused way, i.e. fusion becomes native in torchtitan (but not in HF checkpoints). Do you agree that after the change, we no longer need the fix in this PR? cc @anijain2305 |
|
Claude finished @tianyu-l's task in 3m 9s —— View job ReviewI read the fix, the new regression test, and traced the resharding through Short answer to your questionNo — I don't think making fusion native in TorchTitan removes the need for this fix. The two locations you cite are real fused/unfused glue, but they're a different fused/unfused boundary than the one that actually reshards here. Where the resharding actually comes fromIn this RL path the checkpoint is loaded as DTensors, and the resharding happens inside
Splitting/chunking a This matches the PR's own example and the regression test, which exercises Why the referenced code isn't the cause
Making TT store QKV/FFN fused natively would let you delete those hooks + glue. But the HF checkpoint stays unfused, so So the fix is best understood as a general invariant at the DTensor -> local boundary: whatever a format conversion does to placements, restore the model's declared layout before discarding DTensor metadata. That's not a speculative defensive check — it enforces a concrete, demonstrated contract. Code review of the diff itselfThe fix is correct and minimal. Two small notes:
SuggestionSince the real trigger is the HF adapter's split/chunk/reshape (not the QKV fusion glue), it'd be worth tightening the code comment at cc @tianyu-l @anijain2305 — happy to dig further if you have a specific checkpoint/parallelism combo in mind. |
|
Question: Why another callsite of Also, could you give a simple and concrete example of the issue? |
|
Claude finished @tianyu-l's task in 2m 11s —— View job Answers1. Why doesn't
|
There was a problem hiding this comment.
I think we won't have this problem if let FSDP wrap the generator model.
Please add a TODO for @andrewor14 to resolve
Summary
Restore the declared model sharding after Hugging Face format conversion, before extracting plain local tensors.
Qwen3.5's QKV fuse/split operations can change DTensor placements: a TP2 projection expected as
Shard(0)becameShard(1), producing local[2048, 512]instead of[1024, 1024]. Convolution and vision weights were affected too. The RL checkpoint adapter now redistributes to each parameter's existing layout beforeto_local(); it does not reshape incorrect shards or change CUDA-graph behavior.Adds a two-rank CPU/Gloo regression with independent expected local slices and runs it in RL CI. Naming-only follow-up #4786 is the lower stack layer.
Validation
4ac49088a, not a clean release-pin install.