Repository navigation
Conversation
b11d486 to
46a252e
Compare
GatedDeltaNet gathers the sequence before in_proj and disables sequence parallelism on the projections for that call, but only on the modules. The gradient finalizer reads param.sequence_parallel, so the fused input RMSNorm gamma gradient, already complete on every tensor parallel rank, was summed once more and came out multiplied by the tensor parallel size. Fixes modelscope#241 Signed-off-by: li-lizhe <147392333@qq.com>
46a252e to
6593a25
Compare
| def _finalize_layernorm_grad(param, tp_size): | ||
| """Megatron's branch: a sequence-parallel parameter has its complete gradient summed over the TP group.""" | ||
| if getattr(param, 'sequence_parallel', False): | ||
| return param.main_grad * tp_size | ||
| return param.main_grad |
There was a problem hiding this comment.
Can this be replaced with the Megatron API?
…its rule Addresses the review question on the test: drop the hand-written _finalize_layernorm_grad stub and call Megatron's real _allreduce_non_tensor_model_parallel_grads (megatron.core.distributed. finalize_model_grads) on two gloo ranks, reading the reduction off main_grad. The pre-fix state is kept as a control case and has to come out at tp_size times, so the pin still fails if the parameter half of the toggle goes missing. The toggle itself is still exercised directly with the Transformer Engine shaped LayerNormLinear stand-in, and the test skips cleanly where Transformer Engine is absent. Signed-off-by: li-lizhe <147392333@qq.com>
|
Thanks for the review — good point, and it is done in The test no longer re-implements Megatron's rule. It now calls the real To keep the pin meaningful I kept the pre-fix state as a control case: Verification (local, CPU, no GPU): megatron-core 0.16.1 on Python 3.11 with torch 2.7.1+cpu — One caveat to be explicit about: this is a CPU/gloo run of the reduction, not the real NCCL TP4 training job from the issue, so it pins the flag semantics and the reduction, not the end-to-end gamma magnitude on a GPU cluster. |
What
GatedDeltaNetgathers the sequence beforein_projand turns sequence parallelism off on the projections forthat call, because Transformer Engine then all-reduces the projection gradient itself. Megatron's gradient
finalizer decides from
param.sequence_parallel(seefinalize_model_grads) whether a gradient still has tobe summed over the tensor parallel group, and Transformer Engine marks the parameters it wants reduced with the
same flag (
layer_norm_weightof aLayerNormLinear). Only the module attribute was toggled, so on everytensor parallel rank the gamma gradient was complete and still summed: with TP4 the gradient of
self_attention.in_proj.layer_norm_weightcame out 4x the unsharded reference and reached the optimizer andthe global gradient clipping that way.
_set_linear_sequence_parallelnow toggles the flag on the submodules and on their parameters, and_restore_linear_sequence_parallelrestores the modules while leaving the parameters atFalse. Theparameters deliberately stay off: the finalizer reads them after the backward, long after
forwardrestoredthe modules, and since the projections run without sequence parallelism every rank already holds the complete
gradient for them.
align_corners-style behaviour of the projections themselves is untouched — the moduleattribute is saved and restored exactly as before.
Fixes #241.
Tests
tests/test_gdn_linear_sequence_parallel_params.py(CPU, no GPU, no model weights) stands in for aTransformer Engine
LayerNormLinear(a module and alayer_norm_weightparameter that both carry the flag,plus an unmarked weight) and calls the two helpers on it:
off;
param.main_grad * tp_sizefor a marked parameter) leaves thegamma gradient unscaled, which is the behaviour the issue asks for;
main(23d95fd) + this test fileassert True is Falseon the parameter marker, and the finalizer scales the gamma gradient by 4Verification
Run on a Linux CPU host, Python 3.14.6, torch
2.15.0.dev20260917+cpu, pytest 9.1.1,megatron-corefromPyPI, with the
transformer_engine,tritonanddatasetsimports stubbed (this machine has no GPU and noTransformer Engine build) and
mcore_bridgeloaded as namespace packages so the package__init__patchchain is skipped. The helper functions under test are the real ones, imported from the patched file.
Checked before opening: no other open PR references #241 (
/search/issues?q=repo:modelscope/mcore-bridge+is:pr+241and the issue timeline).
Not tested: the TP4/CP2 reproduction from the issue and the full Qwen3.8-27B training runs (no GPU here),
and the repository's own test suite beyond this file (it needs the full Megatron + Transformer Engine
environment). The multiplier is a property of the flag the finalizer reads, which the test asserts directly.