Repository navigation
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
mathemakitten
left a comment
There was a problem hiding this comment.
Right now the temporary parts of this PR are mixed with the permanent ones, and unwinding them after the FlashInfer PRs land would be another review cycle. Is it possible to restructure this in a way to minimize the changes and make unwinding a matter of just deleting a temp module?
I would also like to understand the speedup in decode vs main with inference_flashinfer_mxfp8_token_capacity set (for MXFP8). The comparison to make is not actually main with unbounded rows but main with bounded rows (which is only available in MXPF8 right now, but there is no real reason it can't be made available to BF16). Can we run that comparison?
d6bdd10 to
9dd8b1e
Compare
Done; how does this look? |
There's a few problems I see with the bound solution. One problem is that it only runs on decode steps by design, which means that when we have frequent prefills we do not see a benefit. Since EP > DP, if any of the DP replicas are in prefill, none of them will use the bound. I'm working to get that kind of data out, just hitting various issues with |
|
/ok to test b21432e |
mathemakitten
left a comment
There was a problem hiding this comment.
Some cleanup items, but otherwise the restructure is looking great.
|
|
||
|
|
||
| @requires_cuda_triton | ||
| def test_routed_emulation_finalizes_the_live_rows(monkeypatch): |
There was a problem hiding this comment.
These emulation tests stub the kernel, so they check the Triton finalize/combine arithmetic but not the assumption underneath it: that do_finalize=False returns unweighted FC2 rows (routed) and that enable_alltoall=True writes exactly the locally-routed rows (cutlass). A double- or missing-weight bug would pass these and the replay tests.
Could we add a Blackwell-only test per kernel that runs the real FlashInfer kernel both ways on the same inputs and compares rows [0, num_active_tokens): the shim's emulated path vs. the kernel's own finalize (do_finalize=True for routed, default enable_alltoall=False for cutlass) over all rows?
There was a problem hiding this comment.
Yeah, thanks for pointing this out. Should be fixed now.
|
|
||
| # UNWIND(flashinfer num_active_tokens): once flashinfer-ai/flashinfer#6783 ships, | ||
| # import cutlass_fused_moe from flashinfer.fused_moe instead. | ||
| from megatron.core.inference.moe.flashinfer_active_tokens import cutlass_fused_moe |
There was a problem hiding this comment.
Can we guard this in a try block like the rest of the FlashInfer imports? Lots of containers (incl likely training containers) are still missing FlashInfer and we cannot break on importing experts.py.
There was a problem hiding this comment.
The cutlass_fused_moe file already has a try/catch around FlashInfer importing; experts.py is safe to import without FlashInfer.
| do_finalize: bool = True, | ||
| output: torch.Tensor | None = None, | ||
| num_active_tokens: torch.Tensor | None = None, | ||
| **kwargs, |
There was a problem hiding this comment.
We do not accept **kwargs in /core, please pass explicit args.
| torch.float32, | ||
| quant_scales=None, | ||
| activation_type=self._flashinfer_activation_type, | ||
| **( |
There was a problem hiding this comment.
We do not accept **kwargs in core, can we pass args unconditionally? uv.lock pins FlashInfer 0.7.2.post1 so this shouldn't be a problem, though you might have to update pyproject.toml.
Signed-off-by: Teodor-Dumitru Ene <teodord.ene@gmail.com>
Signed-off-by: Teodor-Dumitru Ene <teodord.ene@gmail.com>
b21432e to
16dc3f7
Compare
|
@sidsingh-nvidia @wdykas can you please take a look at this MR? |
What does this PR do?
This makes the FlashInfer MoE kernels only do work proportional to the live tokens on a step, instead of proportional to the overall buffer. This in the same spirit of changes started by #4258.
As a result, my testing shows that a decode step on Nano 3.5 drops from 25ms to 9ms with a 65,536-row buffer (EP4), and from 76ms to 12ms with a 262,144-row buffer (EP16), with no change at all in outputs.
Problem
The FlashInfer MoE kernels currently assume that the token count can be provided as a host-side parameter. This is false. The host only provides this parameter at CUDA graph record, and it has no way of knowing what the token count will ever be, because the token count is only known after the all-to-all expert collectives run on every layer.
A secondary problem is that the FlashInfer MoE kernels currently downcast their accumulated results from FP32 to BF16 and only return BF16, forcing the user (Megatron Inference) to manually upcast the BF16 out back to FP32. This is a loss of precision and slowdown with no benefit.
Solution
Monkey-patch the FlashInfer kernels until they are fixed at a future date.
Perf numbers
One GB300 node, Nemotron 3.5 Nano, TP 2 EP 2 DP 2, 256 requests of 128-token prompts, 1,024 greedy tokens, three
trials per run averaged out:
mainwith FlashInfermainwith FlashInferPlease see the Slack thread for a wide list of perf numbers. More are available upon request.
Related PRs
flashinfer-ai/flashinfer#6783 flashinfer-ai/flashinfer#6784 flashinfer-ai/flashinfer#6785
Issue tracking
For PRs from open-source community contributors:
Linked issue:
Contribution process
Pre-checks
jit_fuser/torch.compile, CUDA extension, TE or external-library dispatch, or a scatter/index accumulation), I have added or updated its bit-exact determinism test and registered it intests/unit_tests/determinism/kernels/manifest.py(guide)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.