Skip to content

[Bugfix][MoE] Remap invalid topk_ids in moe_permute without expert_map - #59717

Open
jlqibm wants to merge 3 commits into
vllm-project:mainfrom
jlqibm:fix/moe-permute-neg1-offsets
Open

jlqibm wants to merge 3 commits into
vllm-project:mainfrom
jlqibm:fix/moe-permute-neg1-offsets

Conversation

@jlqibm

@jlqibm jlqibm commented Oct 2, 2026 •

Copy link
Copy Markdown

Overview

Fixes #59686. moe_permute returns wrong expert_first_token_offset when topk_ids contains -1 (the id the router writes for CUDA-graph padding tokens with VLLM_MOE_SKIP_PADDING=1, the default) and no expert_map is given. The CUTLASS fp8 MoE path then runs some valid tokens through the wrong expert. This always applies the existing invalid-id remap in moe_sort_routing.

Claims

  • Valid tokens' MoE outputs no longer depend on CUDA-graph padding when CutlassExpertsFp8 (and the other moe_permute callers: cutlass_moe.py W4A8 path, Humming non-scatter path) run without an expert_map.
  • Fixes single-request serving quality of fp8 MoE checkpoints on the auto-selected VLLM_CUTLASS backend (first-token collapse to '\n', off-topic sampled output).
  • No change when all ids are valid or when an expert_map is given (the remap already ran there).

Validation

H100 (SM 9.0), vLLM main 47de9d4d04, _moe_C_stable_libtorch rebuilt for sm_90 with CUDA 13.1; unpatched = the precompiled nightly extension for the same source.

check unpatched patched
repro from #59686: batch sizes (1..48, padded to a multiple of 8) with wrong offsets 42 / 48 0 / 48
CutlassExpertsFp8 (E=128, top-4, K=3072, N=1536, per-token act / per-channel weight fp8): sizes where valid rows differ from an unpadded call 25 / 48 0 / 48
new test_moe_permute_invalid_expert_ids_without_expert_map 8 failed (offset assertion) 8 passed
tests/kernels/moe/test_moe_permute_unpermute.py 462 passed

Model evaluation, 128-expert top-4 fp8 MoE checkpoint (compressed-tensors FP8_DYNAMIC), TP=2, default backends (VLLM_CUTLASS MoE, CUDA graphs on). Next-token distributions vs. the bf16 checkpoint on 90 short chat prompts x thinking on/off; "foreign mass" = probability the fp8 model puts on its top-10 tokens that are outside the bf16 top-100 (0 for bf16 vs. itself).

regime (first 8 response tokens) unpatched: foreign mass / p99 / top-1 agreement patched
greedy decode, one request at a time 0.0081 / 0.39 / 0.951 0.0000 / 0.0000 / 0.988
teacher-forced, one request at a time 0.0028 / 0.0000 / 0.979 0.0000 / 0.0000 / 0.989
teacher-forced, all requests in one batch 0.0000 / 0.0000 / 0.990 0.0000 / 0.0000 / 0.990

Commands:

pytest -q tests/kernels/moe/test_moe_permute_unpermute.py

Details

Root cause: CubKeyValueSorter radix-sorts only the low floor(log2(2E-1)) + 1 bits, so a raw -1 (all ones) sorts after every real expert, but findTotalEltsLessThanTarget binary-searches the keys as signed ascending ints. Probes landing in the trailing -1s push offsets too high, so an expert's grouped-GEMM segment can cover the next expert's rows; when padding outnumbers valid tokens every offset equals the total route count and the valid MoE output is zero. preprocessTopkIdKernel already maps ids outside [0, n_expert) to the n_expert sentinel the sort expects, but it only ran with an expert_map or build_inverse; moe_permute / moe_permute_with_scratch pass neither on a single EP rank.

Cost: one n_token * topk int32 copy and one elementwise kernel per moe_permute call, no host sync, CUDA-graph safe.

preprocessTopkIdLauncher now returns early for an empty batch, since the unconditional call would otherwise compute block = 0. This overlaps #58790 (empty routing in moe_permute, open); whichever lands second can drop its guard.

Not duplicate: open PRs on the same -1 padding sentinel target other backends (AITER #57036, FlashInfer b12x #57946, XPU #55231) or empty routing (#58790); none changes the moe_sort_routing remap gate. Searched moe_permute, expert_first_token_offset, topk_ids -1 padding, VLLM_MOE_SKIP_PADDING, preprocessTopkId.

Limitations: validated on H100 only; the sm_90 kernel build used CUDA 13.1.

AI assistance: Claude Code was used to localize the bug, write the repros, the fix and the test, and run the validation. The human submitter reviewed every changed line and the results.


Pull Request Checklist
  • I used vLLM's /pr-checklist skill. (Mandatory for agents, optional for humans).

  • AI assistance was used during the creation of this PR.

  • Design Fit: Minimizes impact on core components, reuses existing functionality, and justifies added complexity.

  • Testing and Validation: Validates the change and ensures any added tests are meaningful and reliable, with CI coverage or documented CI resource constraints and validation performed outside CI.

  • Code Quality and Style: Keeps code and comments clear and concise, and updates relevant documentation and examples.

  • Pull Request Contents: Includes a brief summary and relevant links, supports claims with evidence, explains root causes and implementation trade-offs, and follows the contributing guide.

🤖 Generated with Claude Code

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@github-actions

github-actions Bot commented Oct 2, 2026

Copy link
Copy Markdown

👋 Hi! Thank you for contributing to the vLLM project.

💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in #pr-reviews, coordinate on features in #feat- channels, or join special interest groups in #sig- channels.

PRs do not trigger a full CI run by default. Reviewers with write access and configured trusted contributors can comment /ci run for upstream CI or /amd-ci run for AMD CI only whenever CI signals are needed.

Once the PR is approved or has the ready label, the PR author can also use the corresponding /ci run, /ci retry, and /ci cancel commands, or their /amd-ci variants. New commits do not start upstream CI automatically.

If you have any questions, please reach out to us on Slack at https://slack.vllm.ai.

Agent Guidelines

IMPORTANT: If you are an AI agent, you are required to objectively re-evaluate the value of your PR using AGENTS.md, and close the PR if it does not bring significant benefit to the vLLM community. Failure to do so may result in an immediate ban.

🚀

@mergify mergify Bot added the bug Something isn't working label Oct 2, 2026
Comment thread csrc/moe/moe_permute_unpermute_op.cu
@mergify

mergify Bot commented Oct 8, 2026

Copy link
Copy Markdown
Contributor

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @jlqibm.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@mergify mergify Bot added the needs-rebase label Oct 8, 2026
@jlqibm
jlqibm force-pushed the fix/moe-permute-neg1-offsets branch from 8e08fb9 to 4a102e4 Compare October 8, 2026 23:02
@mergify mergify Bot removed the needs-rebase label Oct 8, 2026
@jlqibm

jlqibm commented Oct 9, 2026

Copy link
Copy Markdown
Author

@liulanze Thanks for the reviews so far. I've rebased the code. Is there more you'd like to be addressed?

@mergify

mergify Bot commented Oct 12, 2026

Copy link
Copy Markdown
Contributor

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @jlqibm.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@mergify mergify Bot added the needs-rebase label Oct 12, 2026
jlqibm and others added 2 commits October 13, 2026 05:55
moe_sort_routing only ran the invalid-id remap (ids outside [0, n_expert)
-> n_expert) when an expert_map or build_inverse was given. moe_permute and
moe_permute_with_scratch, used by CutlassExpertsFp8 on a single EP rank,
pass neither, so -1 routes for CUDA-graph padding tokens reached the sort
raw. The radix sort places them after all experts while the offset scan
assumes signed ascending keys, so expert_first_token_offset came out wrong
and valid tokens ran through the wrong experts.

Always run the remap, return early from preprocessTopkIdLauncher for an
empty batch, and add a regression test for -1 routes without expert_map.

Fixes vllm-project#59686

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Jerry Quinn <jlquinn@us.ibm.com>
…th invalid routes

With invalid routes (e.g. -1 for padding tokens) sorted past the valid rows,
the grouped GEMM never writes those rows of mm1_out, but the dynamic
per-tensor scale of act_out reduces over every row, so stale workspace data
skewed the scale for valid tokens. run_cutlass_moe_fp8 only zeroed the
workspace with an expert_map or the batched format; zero it whenever the
activation scale is per-tensor. Apply the same guard to
run_cutlass_moe_w4a8_fp8, and always bound the Humming scatter path's input
quantization to the valid routes.

Add a regression test running per-tensor CUTLASS fp8 MoE with -1 padding
routes and a garbage-filled workspace against an unpadded reference.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Jerry Quinn <jlquinn@us.ibm.com>
@jlqibm
jlqibm force-pushed the fix/moe-permute-neg1-offsets branch from 4a102e4 to b5b1095 Compare October 13, 2026 21:56
@mergify mergify Bot removed the needs-rebase label Oct 13, 2026
Comment thread vllm/model_executor/layers/fused_moe/experts/cutlass_moe.py Outdated
@github-project-automation github-project-automation Bot moved this to Ready in NVIDIA Oct 16, 2026
… MoE path

run_cutlass_moe_w4a8_fp8 asserts per_act_token, so the `if not per_act_token`
fill added there could never run. With per-token activation scales, rows the
grouped GEMM leaves unwritten can't affect the valid rows' scales anyway. The
fill that matters stays in run_cutlass_moe_fp8, which allows per-tensor scales.

Signed-off-by: Jerry Quinn <jlquinn@us.ibm.com>
@jlqibm

jlqibm commented Oct 16, 2026

Copy link
Copy Markdown
Author

@bnellnm Thanks. Can you add the ready label?

This branch has not been deployed

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

Labels

bug Something isn't working nvidia

Projects

Status: Ready

Development

Successfully merging this pull request may close these issues.

[Bug]: CUTLASS fp8 MoE corrupts valid tokens when CUDA-graph padding routes are -1 (wrong expert_first_token_offset from moe_permute)

3 participants