fix(optimizer): use expert TP group for gradient stats - #80
Merged
Conversation
Expert optimizers filtered replicated parameters with the dense tensor- parallel group. When ETP differed from TP, this omitted distinct expert gradients from norm and zero-count calculations. Pass the expert tensor-parallel group to the split expert optimizer while preserving dense and custom process-group fallbacks. Add distributed coverage for legacy, explicit, and incomplete custom collections.
yueming-yuan
approved these changes
Aug 12, 2026
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
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.
Summary
Count expert gradients with expert TP ownership in standard split optimizers.
Symptom & Reproduction
allreduce=Falseexpert parameter per rank under TP=EP=world size and ETP1. Before the fix, the expert child uses dense TP ownership, so only dense TP rank 0 retains its expert gradient.In a DSv4 training step, the reported norm was
11.5135000849versus the logical FP64 norm12.2754343246. Withclip_grad=1, this made the clipping coefficient6.62%too large.The trigger is the standard Adam/SGD non-FSDP path with separate dense and expert child optimizers when dense TP ownership differs from ETP ownership. Backward still computes the omitted expert gradients, and the optimizer still updates those parameters; only gradient statistics and clipping use the wrong ownership set.
Root Cause
get_megatron_optimizercreates separate dense and expert child optimizers._get_megatron_optimizer_based_on_param_groupsassigns dense TP ownership to both children.param_is_not_tensor_parallel_duplicatekeeps replicated parameters only on that group's rank 0.TP4/EP4/ETP1places distinct expert shards on ranks filtered as dense replicas.get_main_grads_for_grad_normandcount_zerosboth consume the child optimizer'stp_group, so the same ownership error affects norm computation, clipping, and zero-count logging.Fix
Add a child-specific
tp_groupoverride to_get_megatron_optimizer_based_on_param_groupsand pass the expert tensor-parallel group when constructing the split expert optimizer. Dense children retain the dense TP group. The legacy path uses the global ETP group, an explicitProcessGroupCollectionusesexpt_tp, and a custom collection withoutexpt_tpretains its existing dense-TP fallback.NVIDIA/Megatron-LM#5916 is the comprehensive upstream root fix, merged into NVIDIA
mainascd4afffa. PR #80's based075c1edoes not contain that commit. This PR adapts only the split-optimizer ownership correction required by this fork; it does not backport NVIDIA#5916's gradient-synchronization, TE/native parameter-tagging, or parameter-norm changes. FSDP and Muon also remain outside this PR.Verification
test_expert_optimizer_uses_expert_tp_group_for_grad_normon eight ranks:legacyandexplicitmodes bind expert gradients to ETP1 on every rank;custom_without_expt_tppreserves the prior dense-TP fallback.12.2851015996versus FP64 logical12.2851021737(4.67e-8relative error), with zero ownership mismatches instead of 512 missing expert tensors on each of ranks 1–3.1.28e-7relative error, while the old selector was3.97%to5.49%low across run 2.Review Focus
_get_megatron_optimizer_based_on_param_groups: the optional override changes only the selected child's duplicate-filter ownership.get_megatron_optimizer: legacy and explicit process-group paths select ETP for the expert child without changing dense ownership.custom_without_expt_tp: the compatibility fallback is intentionally unchanged; broader expert-topology fixes from Fix gradient-norm undercounting when using EP and TP NVIDIA/Megatron-LM#5916 remain outside this PR.