Fix TE grouped MLP fused main-grad setup#5209
Conversation
242c0d2 to
20fc008
Compare
20fc008 to
d53396b
Compare
5b5ec96 to
05236b6
Compare
|
/ok to test 05236b6 |
|
/claude strict-review |
There was a problem hiding this comment.
Strict review passed — no significant issues found. LGTM
Traced the full data/dtype flow for this fix:
Correctness (verified): The change fills a genuine gap. Megatron-FSDP's _bucket_forward sets overwrite_main_grad=True but does not pre-populate param.main_grad; the non-fused wgrad paths (tensor_parallel/layers.py:586, core/utils.py:1195) set it lazily inside wgrad_compute, but the TE op-fuser path has no equivalent hook, so param.main_grad would be None when TE's fused wgrad accumulation writes into it. Exposing it in the pre-forward hook mirrors the reference FSDP pattern (megatron_fsdp.py:652,670).
Backward compatibility (verified safe): get_main_grad/overwrite_main_grad exist only on FSDP-managed params, so the getattr(..., None) is not None and hasattr(...) guards leave the DDP + fuse_wgrad_accumulation path completely untouched — no default or behavior change for existing users.
Details that are correct:
is Noneidentity check (not truthiness) avoids the ambiguous-tensor-bool error.- Under gradient accumulation,
main_gradis reset toNoneonly at grad-reduce time (reset_param_main_grad), so the guard is a no-op on later microbatches and the same bucket is reused — consistent. parameters(recurse=False)is correct: TEGroupedLinear registers its weights/biases directly on the module.- No new direct
parallel_state.get_*_group()reads; all new identifiers have live use paths.
The unit test cleanly validates the guard logic (uses pytest.fail to assert get_main_grad is skipped when main_grad is already set, and that non-fuse_wgrad params are left alone).
Risk level: low.
|
@fanshiqing can you please take a look at these changes? |
05236b6 to
d000dad
Compare
|
@aklife97 @yashaswikarnati Could you please help review these changes? |
|
/ok to test d000dad |
|
@kvareddy @lmcafee-nvidia Could you please help review these changes? |
Head branch was pushed to by a user without write access
d000dad to
0ac5c6f
Compare
Signed-off-by: Shijie Wang <jaywan@nvidia.com>
0ac5c6f to
fba3816
Compare
|
/ok to test fba3816 |
|
🔄 Merge queue validation started! You can track the progress here: https://github.com/NVIDIA/Megatron-LM/actions/runs/29988911330 |
|
🔄 Merge queue validation started! You can track the progress here: https://github.com/NVIDIA/Megatron-LM/actions/runs/30002442211 |
What does this PR do ?
This PR exposes Megatron-FSDP main-grad buffers to the TE grouped MLP op-fuser path before fused execution.
Issue tracking
Linked issue: N/A. This is a small compatibility bug fix for Megatron-FSDP with the TE grouped MLP op-fuser path.
Changes
param.main_gradfromparam.get_main_grad()for grouped MLP fused parameters when needed.overwrite_main_gradenabled for the fused path.Validation
python3 -m compileall -q megatron/core/transformer/moe/experts.py tests/unit_tests/transformer/moe/test_grouped_mlp.pyCHECK_ONLY=true tools/autoformat.shpassed for this PR branchContribution process
Pre-checks