[Dev] Add global-batch quantile balancing - #6637
Conversation
3c4e4ea to
6155ee0
Compare
|
/claude review |
There was a problem hiding this comment.
Light review — one blocking issue plus a few smaller notes.
Blocking: duplicate CLI flags will break parse_args()
--moe-router-quantile-balancing-estimation-scope and --moe-router-qb-num-bins are added manually in _add_moe_args, but the corresponding TransformerConfig fields are not in the exclude list at megatron/training/arguments.py:2729-2824. ArgumentGroupFactory therefore already generates both flags in _add_network_size_args (which runs first), so argparse hits a duplicate option string at parser-construction time. See the inline comment for the fix.
This is presumably why the CLI test in the PR builds a bare parser with only _add_moe_args rather than the full Megatron parser — that test passes while real training would fail at startup. Worth switching test_qb_cli_exposes_global_batch_scope_and_histogram_bins to use the full add_megatron_arguments(parser) so a regression like this is caught.
Documentation
megatron/core/transformer/moe/README.md documents the load-balancing strategies but was not updated. Two spots:
Load Balancing Strategies table (around line 300) — add a row such as:
| **quantile_balancing** | Kimi K3 aux-loss-free global-batch histogram quantile bias updates | `--moe-router-load-balancing-type quantile_balancing --moe-aux-loss-coeff 0` |
Router Arguments table (around line 531): the --moe-router-load-balancing-type row still reads "aux_loss, sinkhorn, seq_aux_loss, none" — it already omits global_aux_loss, and now quantile_balancing too. Adding rows for the two new flags would help as well.
Test coverage
The unit tests are thorough for the histogram math and the finalizer, but three of the six substantive tests are gated behind fused_topk_with_score_function_supports_qb, which is False against released TE. In CI today, effectively only the pure-math and non-fused paths run. Two gaps worth closing with tests that do not need the TE patch:
- A real-router non-fused (
moe_router_fusion=False) forward/backward plus finalize test.test_qb_mcore_router_accumulates_microbatches_and_finalizesis the only test that constructs an actualMoELayer, and it requires the TE QB API — so theRouter.__init__buffer registration androuting()wiring are currently untested in CI. - The
accumulate_qb_histogramgating inRouter.routing()— specifically that eval mode andfrozen_expert_bias=Truedo not accumulate.
Also, no functional test was added (the PR checklist notes this). Given this changes the router bias-update path, at least one tests/functional_tests/test_cases/moe/ case would be valuable before this leaves draft.
Minor
_update_router_expert_bias_with_quantile(model, config, tp_dp_cp_group=tp_dp_cp_group) at finalize_model_grads.py:601-603 fits on one line under the 100-char limit; tools/autoformat.sh (black) will collapse it.
The unrelated blank-line removal at router.py:199 touches code outside the PR scope — per docs/developer/contribute.md, worth reverting.
One thing I could not verify locally: whether torch.is_grad_enabled() is sufficient to exclude activation recomputation from histogram accumulation. Raised inline.
6155ee0 to
2d6521b
Compare
|
Addressed the latest review on the paired dev/main changes (main counterpart: #6638). Changes:
Validation on NVIDIA B200 with PyTorch 2.13 nightly and Transformer Engine 2.19 development:
I kept the coverage focused in the QB unit/distributed suite rather than adding a separate golden-value functional recipe: the updated tests now execute the production |
|
/claude review |
FDecaYed
left a comment
There was a problem hiding this comment.
A few non-blocking comments from the current head.
|
/ok to test 2d6521b |
|
Light review — the implementation looks coherent and the unit tests are thorough. Two things worth addressing before this leaves draft, plus one inline nit. 1. The distributed and fused tests are gated on an unmerged TE PR, so neither runs in CI
@pytest.mark.skipif(
not torch.cuda.is_available() or not fused_topk_with_score_function_supports_qb,
...
)and configures @pytest.mark.skipif(not torch.cuda.is_available(), reason="requires CUDA")
@pytest.mark.internal
@pytest.mark.parametrize(
"fused",
[
False,
pytest.param(
True,
marks=pytest.mark.skipif(
not fused_topk_with_score_function_supports_qb,
reason="requires the Transformer Engine QB fused-router API",
),
),
],
)
@pytest.mark.parametrize(
"tp_size,ep_size,dense_dp_size,expert_dp_size", [(1, 4, 8, 2), (1, 8, 8, 1), (4, 2, 2, 1)]
)
def test_qb_world8_ep_topologies_finalize_model_grads(
fused, tp_size, ep_size, dense_dp_size, expert_dp_size
):
...
config = _config(..., moe_router_fusion=fused, ...)That way the reduction-group, in-place-reset, and cross-rank-agreement assertions all become live CI coverage instead of skips. 2.
|
Signed-off-by: Harry Zhou <hhanyu@nvidia.com>
2d6521b to
eeb5227
Compare
|
Addressed the current QB review findings on both target branches in
QB continues to reuse the router's Final NVIDIA B300 results on each branch: 18 passed in the single-GPU suite (six world-size-gated skips), then all 6 EP4/EP8/TP4+EP2 fused/unfused cases passed on every rank. |
|
/ok to test eeb5227 |
What does this PR do?
Add Kimi K3 global-batch Quantile Balancing (QB) as an auxiliary-loss-free MoE routing mode on
dev.Paired main-target PR: #6638
Quantile estimator
For token
iand expertj, lets[i,j]be the raw sigmoid router score and letalpha[i]be the(k+1)-th largest value ofs[i,:] + b[:]. K3 defines:The histogram range for the next global batch is
[min(b_next)-1, max(b_next)+1]. The default is 1000 uniform bins per expert, matching the report.Reference: Kimi K3 technical report, Quantile Balancing.
Implementation
quantile_balancingas a sole router load-balancing mode and require--moe-aux-loss-coeff 0.--moe-router-quantile-balancing-estimation-scope global_batchand--moe-router-qb-num-bins.[num_experts, num_bins]per MoE router across all gradient-accumulation microbatches.expert_biasbuffer without enabling--moe-router-enable-expert-bias; that flag continues to select the independent signed-count updater.fused_atomicpath;(k+1)and histogram accumulation with PyTorch operations.No full score tensor is retained. The fused path adds shared-memory bin classification and histogram atomics to the existing router kernel. The global-batch finalizer communicates
num_experts * num_binsint32 counters per router; the non-fused fallback additionally launches PyTorch top-(k+1)and scatter operations.Validation
Validated on NVIDIA B300 with NVIDIA/TransformerEngine#3395 installed:
qb_histogram_mode="fused_atomic"and compares its histogram exactly with the non-fused implementation.Additional eight-rank validation on NVIDIA B300 exercised the production
finalize_model_gradspath for:For every topology, the test verifies model-parallel group sizes, per-microbatch accumulation, non-identical pre-reduction rank-local histograms, the all-reduced histogram against an independent quantile-update oracle, identical bias and bounds on all ranks, in-place reset, stable CUDA-graph-visible addresses, and exactly one gradient synchronization. All six fused/unfused parameterizations passed on all eight ranks. The focused Transformer Engine router suite passed 38 tests with 2 skips.
The commits are SSH-signed and signed off.
git diff --checkand the repositorytools/autoformat.shpass; pylint rates the changed files 10/10 and Ruff reports no errors.Scope and dependencies
The non-fused QB path has no dependency on NVIDIA/TransformerEngine#3395. If
--moe-router-fusionis enabled, MCore checks the installed TE signature and reports a targeted error when the histogram API is unavailable.This PR does not depend on SiTU-GLU or #6673. It has no SiTU activation imports or configuration coupling; the paired integration checkout was used only to verify compatibility between the independent branches.
Issue tracking
For PRs from open-source community contributors:
Linked issue: TODO — link a Megatron-LM feature-request issue before marking this draft ready for review.
Contribution process
Pre-checks
Code review
Feel free to message or comment @NVIDIA/mcore-oncall to help accelerate your merge into main. The less complex your PR is, the faster it will be approved and merged!
All PRs start as draft. If you open a non-draft PR, it will be automatically converted to draft.
Step 1: Mark PR as "Ready for Review"
.github/CODEOWNERS.Final Review might get declined if these requirements are not fulfilled.
Step 2: Final Review
For PRs that change
megatron/core, once all expert reviewers have approved, theFinal Reviewlabel is applied automatically and final reviewers are assigned.For PRs outside
megatron/core, this step is skipped.Step 3: Approved
Once all required reviewers have approved, the
Approvedlabel is applied automatically.Merge
Any member of mcore-engineers will be able to merge your PR.