Skip to content

fix(muon): split the fully-gathered QKV tensor before orthogonalizing under GTP - #7117

Merged
fanshiqing merged 7 commits into
NVIDIA:mainfrom
fanshiqing:shiqingf/fix_qkv_muon_update_with_gtp
Sep 14, 2026
Merged

fanshiqing merged 7 commits into
NVIDIA:mainfrom
fanshiqing:shiqingf/fix_qkv_muon_update_with_gtp

Conversation

@fanshiqing

@fanshiqing fanshiqing commented Sep 8, 2026 •

Copy link
Copy Markdown
Member
  • I, the PR author, have personally reviewed every line of this PR.

What does this PR do?

Muon runs a different update rule on linear_qkv depending on whether GTP is
enabled, silently.
This makes the decision layout-invariant.

Problem

The split-QKV decision tests the local shard row count. Under GTP the weight is
row-sharded, so the same weight takes different paths:

layout param.shape[0] % 6528 path
TP1 6528 (whole) 0 ✓ split → 3 Newton–Schulz
GTP64 102 (shard) ✗ 1 whole-matrix NS

Three facts make it matter:

  • The rules are far apart — 26 % different overall, 95 % on k and v (they are
    2.9 % of the rows, so joint orthogonalization lets q dominate them).
  • It does not wash out — NS amplifies the ~1e-7 fp32 reduction-order difference
    between layouts by ~10⁴x, and that floor is flat across condition number 1e1–1e6.
  • It is invisible — the skip is a logging.DEBUG record that production never sees.

Measured at 256 GPUs (identical init on both arms), GTP64 vs matched TP1:
training loss +0.173 % — a near-constant 0.09–0.12 B token lag, 4.3x outside
the run-to-run noise floor. GTP64 + Adam on the identical topology shows none, as
expected: Adam is elementwise, amplification ~1x.

Fix

  1. Decide from the global shape — qkv_global_rows():
    param.shape[0] * gtp_size - pad_length.
  2. Split after the all-gather — at 6528 rows over 64 shards the q/k/v boundaries
    fall mid-shard, so no rank can split correctly before the gather.
  3. take the GTP degree from the optimizer's own group, gated on
    is_gtp_weight_remat -- the same signal the split path uses

Result

image
GTP64 vs its own TP1 baseline vs noise floor (−0.038 … +0.040 %)
before +0.1731 % 4.3x outside
after −0.0342 % ✅ inside

(4 h from scratch, matched window 4.0–16.8 B, n=31. The residual is negative — GTP
slightly ahead — i.e. an overshoot past zero, not a leftover deficit.)

  • Bitwise: TP1 and GTP now produce identical qkv updates (max |diff| 0.000e+00).
  • Throughput: 2,527 → 2,536 ms/it at GTP64 (+0.36 %, inside cross-job noise). The
    split partitions the same NS FLOPs rather than adding any, on 8 attention layers.
  • Control: --muon-no-split-qkv on both arms also lands inside the floor
    (+0.008 %), confirming the asymmetry was the mechanism. Not the right fix — turning
    the split off costs +0.216 % lm loss at TP1, so GTP should gain it, not TP1 lose it.
Supporting measurements

Split vs whole-matrix NS, production settings (16 steps, polar_express, spectral,
extra_scale_factor 0.2), same 6528×2688 gradient:

block rows relative difference
q 6144 0.116
k / v 192 0.947
whole 6528 0.255

NS amplification of a 1e-7 relative input perturbation:

cond 1e1 1e3 1e6
qkv 6528×2688 10,525x 9,720x 20,374x

Also eliminated with data before landing here: expert routing, grad-norm inflation,
data assignment, loss/MTP reduction scope, router BF16 quantization, vocab-sized
weights, initialization.


Pre-checks

  • I have added relevant unit tests
  • I have added relevant functional tests
  • I have added proper typing to my code
  • I have added relevant documentation
  • I have run tools/autoformat.sh on my PR

tests/unit_tests/generalized_tensor_parallel/test_gtp_muon_qkv.py — 14 cases, all
passing at world 2 and 4. Nothing existing could have caught this:
test_muon_optimizer_qkv_split sets param.is_qkv by hand on one rank, and
test_gtp_muon.py's parity cases carry no qkv weight.

  • test_local_shard_would_have_disabled — the regression test proper; pins the shape
    that used to fail, 102 × 64 = 6528.
  • test_split_after_gather_matches_tp1 — GTP result == TP1 result, per shard.
  • test_gtp_split_differs_from_whole_matrix — negative control, so a regression to
    the old behaviour cannot pass trivially.
  • test_row_sharded_modes_raise, test_padding_is_excluded,
    test_decision_is_layout_invariant, test_non_qkv_shape_is_rejected.

test_gtp_muon.py passes identically before and after (6 passed / 8 skipped both
sides; the 8 need an 8-GPU host).

Reviewer notes

  • Behaviour change: blockwise/distributed with muon_split_qkv=True now
    raises. Those modes keep the weight row-sharded so no rank holds a q/k/v boundary —
    they were silently getting a different update rule. --muon-no-split-qkv is the
    opt-out.
  • qkv_global_rows reads is_qkv/qkv_split_shapes (tensor-parallel attrs) and
    group/pad_length (GTP attrs) off the same param. Both are copied at the same
    sites today; a path copying only one set would compute global_rows wrongly.
  • Same pattern, not fixed here: mixer.in_proj [12352, 2688] is also fused
    ([z,x,B,C,dt]) and GTP-sharded, escaping only via use_muon=False.
  • MLA still unhandled (pre-existing TODO(deyuf)).

@fanshiqing
fanshiqing requested review from a team as code owners September 8, 2026 03:14
@copy-pr-bot

copy-pr-bot Bot commented Sep 8, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@svcnvidia-nemo-ci
svcnvidia-nemo-ci marked this pull request as draft September 8, 2026 03:14
@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

This PR has been automatically converted to draft because all PRs must start as drafts.

When you are ready for review, click Ready for Review to begin the review process. This will:

  1. Add the oncall reviewer (optional reviewer)
  2. Add required review teams based on your changes

See the contribution guide for more details.

@fanshiqing fanshiqing changed the title fix(muon): decide split-QKV from the gathered shape, not this rank's … fix(muon): decide split-QKV from the gathered shape, not this rank's shard Sep 8, 2026
@fanshiqing
fanshiqing marked this pull request as ready for review September 8, 2026 03:15
@fanshiqing

Copy link
Copy Markdown
Member Author

/claude review

@fanshiqing

Copy link
Copy Markdown
Member Author

/ok to test b142e6a

claude[bot]

This comment was marked as resolved.

@fanshiqing fanshiqing linked an issue Sep 8, 2026 that may be closed by this pull request
@fanshiqing fanshiqing self-assigned this Sep 8, 2026
…shard

Whether split-QKV ran depended on the parallel layout, so one weight could get a
different Muon update at TP1 and under GTP. The two rules differ by 26% (95% on k
and v), and Newton-Schulz amplifies a 1e-7 input difference ~10^4x, so it does not
wash out: on a 30B MoE run GTP64 trailed its TP1 baseline by a constant 0.09-0.12B
token lag, 22x the measured run-to-run floor. This removes 82% of it.

Cause: `is_qkv` tested `param.shape[0] % sum(qkv_split_shapes)` -- this rank's
shard, not the weight. GTP row-shards dim 0, so the test fails once the GTP degree
stops dividing the query-group count, and the split is dropped on every GTP rank
while TP1, holding all the rows, keeps it. Silent and configuration-dependent,
with only a DEBUG log that never reached a production logger.

Fix:
  - decide from the row count across GTP shards (local x gtp_size - pad)
  - split after the all-gather; a row shard cuts q/k/v mid-boundary
  - take the GTP degree from the optimizer's own group, gated on
    is_gtp_weight_remat -- the same signal the split path uses

blockwise and distributed keep the rows sharded, so the split cannot run there.
They fall back to whole-matrix Newton-Schulz -- what they already did before this
fix -- and warn once, so existing jobs on those modes are unchanged. duplicated,
the default, gets the fix.

Tests: tests/unit_tests/generalized_tensor_parallel/test_gtp_muon_qkv.py
Signed-off-by: Shiqing Fan <shiqingf@nvidia.com>
@fanshiqing
fanshiqing force-pushed the shiqingf/fix_qkv_muon_update_with_gtp branch from b142e6a to 886b565 Compare September 8, 2026 04:42
@fanshiqing

Copy link
Copy Markdown
Member Author

/ok to test 886b565

@fanshiqing

Copy link
Copy Markdown
Member Author

/claude review

Signed-off-by: Shiqing Fan <shiqingf@nvidia.com>
@fanshiqing

Copy link
Copy Markdown
Member Author

/ok to test a6eb95e

@fanshiqing

Copy link
Copy Markdown
Member Author

/ok to test 1c72d57

@svcnvidia-nemo-ci svcnvidia-nemo-ci added the Approved All necessary approvals have been made label Sep 14, 2026
@fanshiqing
fanshiqing added this pull request to the merge queue Sep 14, 2026
@nemo-automation-bot

Copy link
Copy Markdown

🔄 Merge queue validation started!

You can track the progress here: https://github.com/NVIDIA/Megatron-LM/actions/runs/34833388075

Merged via the queue into NVIDIA:main with commit 312f867 Sep 14, 2026
129 of 133 checks passed
@fanshiqing
fanshiqing deleted the shiqingf/fix_qkv_muon_update_with_gtp branch September 14, 2026 11:38
xuwchen pushed a commit to xuwchen/Megatron-LM that referenced this pull request Sep 22, 2026
… under GTP (NVIDIA#7117)

Signed-off-by: Shiqing Fan <shiqingf@nvidia.com>
(cherry picked from commit 312f867)
xuwchen pushed a commit to xuwchen/Megatron-LM that referenced this pull request Sep 22, 2026
… under GTP (NVIDIA#7117)

Signed-off-by: Shiqing Fan <shiqingf@nvidia.com>
(cherry picked from commit 312f867)
nemo-autobot-origin Bot pushed a commit to svcnemo-autobot/Megatron-LM that referenced this pull request Oct 9, 2026
Repairs for failures found by local validation of main slice ab20a60
merged into dev f80139c:

- tests/unit_tests/inference: give the prefix-caching _StubEngine the
  cuda_graph_all_prefills attribute the merged DynamicInferenceEngine
  reads.
- megatron/core/optimizer/layer_wise_optimizer.py: tolerate a
  pg_collection without a tp group (getattr(..., 'tp', None)).
- tests/unit_tests/distributed/mfsdp_v2/profiler_utils.py: recover
  kernel correlation ids without FunctionEvent.linked_correlation_id,
  which the dev image's torch does not provide.
- megatron/core/parallel_state.py: size dynamic data/context-parallel
  groups against the replicate DP-CP world (with_gtp_remat=False).
- gpt3_mcore_te_tp2_pp2_ep4_mhc_overlap_attn_cudagraph{,_nosplit}:
  refresh dev H100 golden values after main cd4afff (NVIDIA#5916); lm loss
  and num-zeros match the values main recorded for this change.
- megatron/core/inference/contexts/dynamic_context.py: compute Mamba
  intermediate-state offsets for every prefill chunk, as main does, so
  the last block boundary of a multi-chunk prompt is cached (required by
  main dfe9d04 NVIDIA#5952).
- tests/unit_tests/test_training.py: give the dev-only training_log
  fixtures gtp_weight_remat_size=1, the argument main c5ff22b (NVIDIA#4967)
  made training_log read when computing the global batch size.
- megatron/training/training.py: read args.freeze_all_layers with
  getattr(..., False) in the config-container model-builder path that
  main ff6b92a (NVIDIA#5926) added, as the same change already does in
  _forward_backward_grad_context; dev-only tests pass minimal args.
- megatron/core/parallel_state.py: with weight rematerialization off
  (gtp_remat_size == expert_gtp_remat_size == 1), reuse the existing
  groups of identical membership for the expert GTP weight-remat group
  and the EGTP-merged expert tp-ep-pp group instead of creating
  duplicate NCCL process groups. main c5ff22b (NVIDIA#4967) creates them
  unconditionally; every NCCL group keeps a watchdog thread until
  process exit, and the extra groups exhausted the thread limit in the
  transformer unit-test bucket.
- tests/functional_tests golden values: drop non-finite iteration-time
  samples from the 10 golden-value files this sync changes and set
  start_step/end_step/step_interval to the remaining steps, the layout
  main uses (cd4afff NVIDIA#5916, 7e6a045 NVIDIA#5997). tools/check_golden_values.py
  from main bb5647a (NVIDIA#5989) rejects NaN/inf in changed golden values.
  The H100 files lose only step 1, which lies outside the iteration-time
  steady window; the two gpt3_7b memory_speed GB200 files keep their even
  steps 2-24 at step_interval 2.
- tests/test_utils/recipes/gb200/gpt-dynamic-inference.yaml: append the
  read-only GitLab credential to /root/.netrc with `>>` instead of
  `| tee -a`, so the token is not echoed to stdout and CI logs. The
  recipe arrives in dev with main 354b5d2 (NVIDIA#6104); this keeps the file
  otherwise identical to main.
- tests/test_utils/python_scripts/generate_jet_trigger_job.py and
  .gitlab/stages/0{2,3,4}*.yml: restore main's opt-in error extraction
  (build_test_script, --enable-error-extraction and the four stage
  flags from main 06bb399, NVIDIA#5831). The earlier dev sync NVIDIA#7055 dropped
  them without a recorded reason, so this merge kept dev's copies while
  importing main's test_ci_triage.py, linear_ci.py and 06.triage.yml
  (e443bd3 NVIDIA#5881, b842f59 NVIDIA#5957), which depend on them. All four
  files now match main; 01.build.yml keeps dev's base-image pin.
- moe/gpt3_mcore_te_tp2_pp1_te_8experts2parallel_{overlap_grad_reduce_param_gather_groupedGEMM,ddp_average_in_collective}_1node
  and moe/gpt3_moe_mcore_te_tp4_ep2_etp2_pp2_resume_torch_dist_dist_optimizer_1node:
  refresh the dev GB200 golden values after main cd4afff (NVIDIA#5916). This
  merge imports the grad-norm/num-zeros fix but kept dev's pre-fix GB200
  goldens, so lm loss (exact) and num-zeros failed on dgx_gb200. The values
  come from the dgx_gb200 golden-value artifacts of CICD run 37688059691 on
  the previous head 29cf2c4. lm loss, num-zeros and both memory metrics are
  bit-identical to the goldens the reverted dev sync 5ccdc55 (NVIDIA#7094)
  regenerated. iteration-time drops its step-1 NaN (start_step 2), as above.
- megatron/core/models/gpt/fine_grained_callables.py: forward
  config.moe_ncclep_zero_copy from the GPT schedule-plan
  TransformerLayerNode to ScheduleNode, the contract main NVIDIA#5735 added,
  so free_input never empties NCCL-EP symmetric-memory inputs.
  megatron/core/transformer/transformer_config.py: reject
  moe_ncclep_zero_copy with dev's ep_overlap_use_scheduled_tensor_release
  (NVIDIA#7062), whose scheduled release would own and empty those persistent
  shared buffers. Addresses review 5470751474 (finding 1).
- megatron/core/optimizer/__init__.py and emerging_optimizers.py: with
  GTP_remat (main NVIDIA#4967), tag fused QKV weights from their TP-local rows
  before GTP sharding (param._unsharded_shape) instead of the local
  shard, which silently disabled dev's split-QKV Muon (NVIDIA#7085); rebuild
  the GTP_remat row shards and strip their alignment padding before the
  projection/per-head/global QKV split, then restore this rank's shard.
  Mirrors main's later fix (NVIDIA#7117), which is outside this slice.
  Addresses review 5470751474 (finding 2).

Signed-off-by: svcnemo-autobot <svcnemo-autobot@nvidia.com>
xuwchen pushed a commit to xuwchen/Megatron-LM that referenced this pull request Oct 10, 2026
… under GTP (NVIDIA#7117)

Signed-off-by: Shiqing Fan <shiqingf@nvidia.com>
(cherry picked from commit 312f867)

This branch was successfully deployed

2 active deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[feat] GTP+Muon

7 participants