[TRTLLM-15284][feat] add Kimi K3 SiTU MegaMoE support - #17063
Conversation
71e9990 to
086ab73
Compare
brnguyen2
left a comment
There was a problem hiding this comment.
The overall structure is good — AUTO resolution keeps the historical default, and the isinstance guard turning create_moe's silent Cutlass fallback into a hard error for explicit MEGAMOE_DEEPGEMM is exactly right. Two correctness gaps and some process items before this merges:
- The two inline comments on silent SwiGLU selection (explicit
CUTLASSpassthrough, and the nestedtext_configmiss in_resolve_activation_config) are the important ones — both produce wrong numerics with no error. - The description says a fresh native build and B200/B300 runtime validation against the 2.6.1-based pin are still pending. Since the DG kernel call now passes new kwargs (
situ_beta/situ_linear_beta) whose acceptance can't be verified from this repo, at least one clean build + parity run against the pinned commit should land in the PR before merge. - The dependency now points at a personal GitHub fork (
longlee0622/DeepGEMM). The description acknowledges this; please record the concrete plan (upstream PR link or NVIDIA-org fork) so it doesn't linger — a personal fork is a single-owner risk for everyone building this branch. examples/kimi_k3/eval_extra_llm_options.yamlraisingmoe_config.max_num_tokens33024→131072 isn't mentioned in the description and also affects the default TRTLLM path (less MoE chunking, higher peak activation memory). Worth a sentence on why.- This is a nontrivial feature carrying
[None]— it should reference a TRTLLM JIRA ticket per repo convention.
Signed-off-by: Jonas Li <6110159+longlee0622@users.noreply.github.com>
Signed-off-by: Jonas Li <6110159+longlee0622@users.noreply.github.com>
Port the packed-expert streaming adapter and MegaMoE capacity fix from xguannv/TensorRT-LLM commit d39eb40. Signed-off-by: Jonas Li <6110159+longlee0622@users.noreply.github.com>
Signed-off-by: Jonas Li <6110159+longlee0622@users.noreply.github.com>
Signed-off-by: Jonas Li <6110159+longlee0622@users.noreply.github.com>
Signed-off-by: Jonas Li <6110159+longlee0622@users.noreply.github.com>
a6f479e to
fb3a9d7
Compare
Signed-off-by: Jonas Li <6110159+longlee0622@users.noreply.github.com>
Signed-off-by: Jonas Li <6110159+longlee0622@users.noreply.github.com>
Signed-off-by: Jonas Li <6110159+longlee0622@users.noreply.github.com>
Signed-off-by: Jonas Li <6110159+longlee0622@users.noreply.github.com>
Signed-off-by: Jonas Li <6110159+longlee0622@users.noreply.github.com>
Signed-off-by: Jonas Li <6110159+longlee0622@users.noreply.github.com>
Signed-off-by: Jonas Li <6110159+longlee0622@users.noreply.github.com>
Signed-off-by: Jonas Li <6110159+longlee0622@users.noreply.github.com>
brnguyen2
left a comment
There was a problem hiding this comment.
Approving — the comments below are optional touch-ups, not blockers.
All previous review rounds are addressed and verified: backend rejection happens in _routed_moe_model_config before construction, SiTU config is passed explicitly to MegaMoE (with a text_config fallback in the generic resolver), the fused latent-down gate is an explicit isinstance, the NCCL fixture uses a free port and destroys the group, and the example YAML is back to 33024 with the capacity bump scoped to the private routed config.
One residual note on the new parity test (inline): the process-global _MEGA_MOE_SYMM_BUFFER_CACHE is keyed on id(ep_pg) and outlives the per-test process group, so the parametrized runs accumulate GPU-resident SymmBuffers and can theoretically hit a stale entry after address reuse.
Per the description, a fresh native build and B200/B300 runtime validation against the upstream DeepGEMM pin are still pending — fine for feat/kimi_k3, but please close that loop before this branch heads to main.
Signed-off-by: Jonas Li <6110159+longlee0622@users.noreply.github.com>
Cherry-picked from NVIDIA#17063. Signed-off-by: Jonas Li <6110159+longlee0622@users.noreply.github.com>
Cherry-picked from NVIDIA#17063. Signed-off-by: Jonas Li <6110159+longlee0622@users.noreply.github.com>
What changed
deepseek-ai/DeepGEMMrepository to the upstream SiTU merge commit from feat: add SiTU support to FP8/FP4 Mega MoE deepseek-ai/DeepGEMM#396 and update the attribution metadata.Upstream DeepGEMM PR: deepseek-ai/DeepGEMM#396
Pinned commit:
8b1392b978f5a03c828dd1711090d7fb50958b8aWhy
Kimi K3 uses SiTU rather than SwiGLU in its routed experts. The existing MegaMoE backend only exposed the SwiGLU path, and the Kimi model forced routed experts back to TRTLLM-Gen even when
MEGAMOE_DEEPGEMMwas explicitly requested. This change adds the required SiTU specialization and lets an explicit MegaMoE selection reach the fused DeepGEMM dispatch, GEMM, activation, and combine path without changing the historical default.Impact and constraints
W4A8_MXFP4_MXFP8, expert-parallel MegaMoE path.Validation
The branch records the following validation of the original Kimi SiTU MegaMoE implementation:
0.99936855, relative L23.575343%.For the official upstream DeepGEMM pin in the latest commit: