Skip to content

[BugFix] Treat negative expert ids as empty routing slots in GGUF MoE - #133

Closed
KaigeGao1110 wants to merge 1 commit into
vllm-project:mainfrom
KaigeGao1110:pr2-moe-negative-expert-ids
Closed

KaigeGao1110 wants to merge 1 commit into
vllm-project:mainfrom
KaigeGao1110:pr2-moe-negative-expert-ids

Conversation

@KaigeGao1110

Copy link
Copy Markdown

The bug

vLLM's top-k router writes expert id -1 for padding slots (VLLM_MOE_SKIP_PADDING, on by
default) and its own fused-MoE kernels skip those slots. The GGUF kernels index expert
weights directly by id, so -1 reads the bytes immediately before the expert tensor.

This is an out-of-bounds read on the device. In vLLM's memory-profiling run every token is
padding, so it is an illegal memory access during engine startup, before the model has
served anything. We hit it on the first full load of unsloth/Qwen3.8-Flash-Next-GGUF
UD-IQ4_XS.

It reaches every path below _fused_moe_gguf: the ggml_moe_a8 tile GEMM (through
moe_align_block_size), the ggml_moe_a8_vec per-row kernel, and the dequantize fallback
loop, since all three consume topk_ids as given.

The fix

Map empty slots onto expert 0 with weight 0, so they contribute nothing to the sum:

empty_slots = topk_ids < 0
topk_ids = topk_ids.clamp(min=0)
topk_weights = topk_weights.masked_fill(empty_slots, 0)

Done out of place, so the caller's tensors are untouched, and with no host sync, so it is
safe under CUDA graph capture. Three elementwise ops on a (tokens, top_k) tensor; the
cost is not measurable next to the GEMMs that follow.

Expert 0 is an arbitrary in-range choice — any valid id works, because the weight is zero.

Test

tests/test_moe_padding_ids.py builds a small Q8_0 expert set and runs _fused_moe_gguf
with two kinds of padding at once: a block of fully-padded tokens (every slot -1) and one
token with a single empty slot. It then asserts, in order:

  • the fully-padded tokens produce exactly zero output (count_nonzero == 0), not merely a
    finite value;
  • the partially-padded token matches a reference run with valid ids and that slot's routing
    weight set to zero, so the empty slot contributed nothing rather than something small;
  • topk_ids and topk_weights come back unmodified, because vLLM records those tensors for
    replay and this fix must not write through them.

Parametrised at 16 and 128 tokens so both the per-row path and the x.shape[0] > 64 tile
path are exercised.

Evidence

Run on 1x RTX PRO 6000 Blackwell WE, torch 2.9, vLLM 0.28.1rc1.dev628+g2a02f6efe, with this
branch's fused_moe.py and then with main's, everything else identical:

with this patch:            2 passed
with main's fused_moe.py:   2 failed
    FAILED test_negative_expert_ids_contribute_nothing[16]
    FAILED test_negative_expert_ids_contribute_nothing[128]

Scope

Eight lines in one function, plus the test. No kernel, no build, no ABI change. When the
router emits no negative ids the mask is all-false and the result is unchanged.

🤖 Generated with Claude Code

vLLM's top-k router writes id -1 for padding tokens (VLLM_MOE_SKIP_PADDING is
on by default) and its own MoE kernels skip those slots. The GGUF kernels index
expert weights directly by id, so -1 reads the bytes before the expert tensor;
in the engine's profile run, where every token is padding, that is an illegal
memory access at startup.

Send empty slots to expert 0 with weight 0 instead, so they contribute nothing.
Done out of place and without a host sync, leaving the caller's ids untouched.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@KaigeGao1110

Copy link
Copy Markdown
Author

Closing this as a duplicate of #130, which I opened earlier from the same account with the same fix and the same test file, and which carries a fuller before/after test-suite report. Sorry for the noise — I opened this one without first listing the open PRs on this repo. #130 is the one to review.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant