Repository navigation
[Fix] Reduce the Sarvam dense MLP through its row-parallel projection - #41752
Merged
Merged
Conversation
This was referenced Sep 29, 2026
ch-wan
force-pushed
the
cheng/refactor/final-norm-skip-empty
branch
from
September 29, 2026 20:00
f41ba93 to
d8faaaf
Compare
ch-wan
force-pushed
the
cheng/hot-fix/sarvam-dense-mlp-reduction
branch
from
September 29, 2026 20:00
ed9d1d9 to
565b44a
Compare
ch-wan
force-pushed
the
cheng/refactor/final-norm-skip-empty
branch
from
September 29, 2026 20:29
d8faaaf to
7916f5b
Compare
ch-wan
requested review from
1am9trash,
BBuf,
Edwardf0t1,
Fridge003,
HaiShaw,
Jiminator,
JustinTong0323,
YAMY1234,
Ying1123,
fzyzcjy,
hnyls2002,
hubertlu-tw,
iforgetmyname,
ispobock,
kkHuang-amd,
merrymercy,
ping1jing2,
rainj-me,
sogalin,
whybeyoung,
wisclmy0611 and
zijiexia
as code owners
September 29, 2026 20:29
ch-wan
force-pushed
the
cheng/hot-fix/sarvam-dense-mlp-reduction
branch
from
September 29, 2026 20:29
565b44a to
fa7b7f2
Compare
ch-wan
force-pushed
the
cheng/refactor/final-norm-skip-empty
branch
from
September 29, 2026 23:47
7916f5b to
c1e1041
Compare
ch-wan
force-pushed
the
cheng/hot-fix/sarvam-dense-mlp-reduction
branch
from
September 29, 2026 23:47
fa7b7f2 to
e1b4a77
Compare
ch-wan
force-pushed
the
cheng/refactor/final-norm-skip-empty
branch
from
September 29, 2026 23:56
c1e1041 to
8d3cb4e
Compare
ch-wan
force-pushed
the
cheng/hot-fix/sarvam-dense-mlp-reduction
branch
from
September 29, 2026 23:56
e1b4a77 to
9c1ad64
Compare
This was referenced Sep 29, 2026
ch-wan
force-pushed
the
cheng/refactor/final-norm-skip-empty
branch
from
September 30, 2026 00:33
8d3cb4e to
40f0308
Compare
ch-wan
force-pushed
the
cheng/hot-fix/sarvam-dense-mlp-reduction
branch
from
September 30, 2026 00:33
9c1ad64 to
c83bf2c
Compare
ch-wan
force-pushed
the
cheng/refactor/final-norm-skip-empty
branch
from
September 30, 2026 01:01
40f0308 to
ba89c53
Compare
ch-wan
force-pushed
the
cheng/hot-fix/sarvam-dense-mlp-reduction
branch
from
September 30, 2026 01:01
c83bf2c to
53e896c
Compare
The dense MLP of SarvamMoEMLADecoderLayer was built with reduce_results=False, and the decoder then all-reduced its output over the TP group whenever attention TP was larger than one and the FFN exit had published neither skip flag. That gate tests the wrong group: - under attention DP with attention TP 1 the dense MLP still spans the full TP group, so an output the exit completes in this layer (for example with EAGLE, or a padding mode without reduce-scatter) was never summed; - with moe_dense_tp_size=1 the dense MLP runs on each rank's own rows and owes no sum, but attention TP > 1 summed rows of different tokens. Build the projection with the default reduce_results=True, as BailingMoE does: RowParallelLinear reduces over the MLP's own group and skips the reduction exactly when the exit publishes a flag. The decoder no longer runs a collective of its own. Both cases are read from the code; no Sarvam checkpoint was available to reproduce them.
ch-wan
changed the base branch from
cheng/refactor/final-norm-skip-empty
to
main
September 30, 2026 04:09
Contributor
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Automations to automatically generate PRs for you. |
ch-wan
force-pushed
the
cheng/hot-fix/sarvam-dense-mlp-reduction
branch
from
September 30, 2026 04:10
53e896c to
cfdddf4
Compare
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
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.
This PR is part of a stack (oldest at bottom):
Motivation
The dense MLP of
SarvamMoEMLADecoderLayeris built withreduce_results=False, and the decoder then all-reduces its output over the TP group whenever attention TP is larger than one and the FFN exit published neither skip flag. That gate tests the wrong group:moe_dense_tp_size=1, the dense MLP runs on each rank's own rows and owes no sum, but with attention TP > 1 the decoder sums rows that belong to different tokens.Modifications
reduce_results=True, as BailingMoE does.RowParallelLinearreduces over the MLP's own group and skips the reduction exactly when the exit publishes a flag.Accuracy Tests
B200,
sarvamai/sarvam-105bcut to its first four layers (layer 0 dense, layers 1–3 MoE) in bf16,python -m sglang.benchmark.one_batch --correctness-test. Four layers do not produce meaningful text, so logits and tokens are compared. For scale, the TP2 and TP4 references differ by at most 0.09 in the printed logits.--tp-size 2and--tp-size 4: identical to the parent commit.--tp-size 4 --dp-size 4 --enable-dp-attention --ep-size 2, eager prefill, triton attention): the parent never reduces the dense MLP output. Against the--tp-size 4 --ep-size 2reference, the parent differs by up to 4.25 in the logits and generates different tokens; this PR differs by 0.03 and generates the same tokens.--tp-size 4 --dp-size 2 --enable-dp-attention --moe-dense-tp-size 1(batch size 2): the parent all-reduces the dense MLP output over the four-rank TP group, adding rows of different tokens. This PR runs no such reduction, and its output matches--dp-size 4with the same dense setting (within 0.05). This configuration still differs from the TP4 reference for another reason that this PR does not change: under--moe-dense-tp-size 1the MoE layers' shared experts are also built with TP size 1 and then summed over the TP group.--tp-size 2 --dp-size 2 --enable-dp-attentionand--tp-size 4 --dp-size 2 --enable-dp-attention: identical to the parent, and close to the reference. There the sum is left to a later step.97 affected unit test files pass.
Speed Tests and Profiling
Not applicable: in the configurations that were already correct, the same all-reduce runs.
Checklist
Review and Merge Process
/tag-and-rerun-ci,/tag-run-ci-label,/rerun-failed-ciCI States
Latest PR Test (Base): 🚫 Run #36667620431
Latest PR Test (Extra): 🚫 Run #36667620347
Latest PR Test (AMD ROCm 10): ❌ Run #36667620499