Skip to content

[Bugfix][ROCm] Give KV-first attention blocks their own page in hybrid models - #51837

Merged
yewentao256 merged 1 commit into
vllm-project:mainfrom
stefankoncarevic:fix/rocm-kv-first-blocks-alias-mamba-pages
Aug 11, 2026
Merged

yewentao256 merged 1 commit into
vllm-project:mainfrom
stefankoncarevic:fix/rocm-kv-first-blocks-alias-mamba-pages

Conversation

@stefankoncarevic

@stefankoncarevic stefankoncarevic commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Purpose

On MI300, DSpark speculative decoding on RedHatAI/Qwen3.6-35B-A3B-NVFP4 produces garbage for most requests in a batch. GSM8K accuracy drops to 0.20 against 0.95 for the same target model without speculation, and test_dspark_correctness_and_acceptance_rate[qwen3.6-speculators] fails. The same test passes on H200.

Hybrid models share a single page pool between the full-attention layers, the Mamba/GDN state layers, and the draft model's layers. That sharing is only safe because every group agrees that block id b occupies page b, i.e. the bytes [b * page_size, (b + 1) * page_size).

ROCm's KV cache layout breaks that agreement. Its shape is (2, num_blocks, block_size, num_kv_heads, head_size), so K and V live in two far-apart halves of the allocation and block b actually covers half of page b / 2 plus half of page num_blocks / 2 + b / 2. Attention blocks and Mamba state pages therefore resolve to overlapping bytes and scribble over each other. Backends whose block dimension comes first, such as TRITON_ATTN and the NVIDIA paths, keep the agreement and are unaffected, which is why this is ROCm-only.

The corruption is silent. Instrumenting the first full-attention layer showed clean inputs and NaN outputs for two of four sequences, with NaN values sitting inside the valid context range of those sequences' KV cache blocks. The affected sequences were exactly the ones holding an even block id, matching the factor of two between the layout and the page size. A single NaN key poisons the whole softmax row, so the target returns degenerate logits and the decoded text collapses.

The fix builds the view page-first, then swaps the dimensions back, so each block owns exactly its own page. It applies only to KV-first attention layers whose allocation is also used by Mamba state; blocks-first backends and attention-only allocations keep their current layout. The logical shape is unchanged, so kernels see exactly what they saw before.

Test Plan

Unless noted, everything below ran on a single MI300 (gfx942) with RedHatAI/Qwen3.6-35B-A3B-NVFP4 plus RedHatAI/Qwen3.6-35B-A3B-speculator.dspark.

  1. The originally failing case:
    pytest "tests/v1/e2e/spec_decode/acceptance_rates/dspark/test_dspark.py::test_dspark_correctness_and_acceptance_rate[qwen3.6-speculators]"
  2. The whole file, to cover the other speculator/model combinations:
    pytest tests/v1/e2e/spec_decode/acceptance_rates/dspark/test_dspark.py
  3. KV cache layout unit tests:
    pytest tests/v1/worker/test_attn_utils.py tests/v1/core/test_kv_cache_utils.py tests/v1/core/test_contiguous_kv_packing.py
  4. A 40-question greedy GSM8K run with speculation on the default ROCm backend, before and after the change.
  5. Two new unit tests in tests/v1/worker/test_attn_utils.py: one asserts that a KV-first layer sharing its allocation with Mamba gives every block its own page, the other asserts that an attention-only allocation keeps the existing layout.

Test Result

  1. Passes. It failed before the change.
  2. All three cases pass.
  3. 94 passed.
  4. GSM8K accuracy goes from 0.200 to 0.900 with no invalid answers, and the mean accepted length goes from about 1.0 to 5.36 tokens, so speculation is doing real work instead of having every draft rejected. For reference, the same model without speculation scores 0.950, and forcing TRITON_ATTN on both target and draft scored 0.925.
  5. Both pass with the change. The first one fails without it, the control passes either way.

Essential Elements of an Effective PR Description Checklist
  • The purpose of the PR, such as "Fix some issue (link existing issues this PR will resolve)".
  • The test plan, such as providing test command.
  • The test results, such as pasting the results comparison before and after, or e2e results
  • (Optional) The necessary documentation update, such as updating supported_models.md and examples for a new model.

…d models

Hybrid models share one page pool between attention and Mamba layers, which
relies on block id b addressing page b. ROCm's (2, num_blocks, ...) layout
splits a block into two far-apart halves, so attention blocks and Mamba state
pages resolve to overlapping bytes and silently corrupt each other. This shows
up as NaN inside the attention KV cache and garbage output: Qwen3.6-35B DSpark
speculative decoding on MI300 scores 0.20 on GSM8K instead of 0.95.

Lay KV-first views out page-first when the allocation is shared with Mamba
state, so every block owns exactly its own page.

Signed-off-by: Stefan Koncarevic <stefan.koncarevic@amd.com>

@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.

@mergify mergify Bot added rocm Related to AMD ROCm mrv2 Model Runner V2 specific bug Something isn't working labels Aug 11, 2026
@github-project-automation github-project-automation Bot moved this to Todo in AMD Aug 11, 2026
@AndreasKaratzas

Copy link
Copy Markdown
Member

/ci run

@github-actions

Copy link
Copy Markdown

✅ Triggered Buildkite CI #83379 for commit 12ae63de227c.

@AndreasKaratzas AndreasKaratzas added the ready-run-all-tests Trigger CI with all tests for wide-ranging PRs label Aug 11, 2026
@github-actions

Copy link
Copy Markdown

@stefankoncarevic, CI is now available for this PR.

  • /ci run starts a CI build.
  • /ci retry retries failed jobs in the CI build for the current PR head. If the current head has no CI build, it starts a new CI build for the current head containing only jobs that failed in the latest earlier CI build for this PR.
  • /ci cancel cancels scheduled or running CI builds for this PR branch.

@AndreasKaratzas AndreasKaratzas left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@yewentao256 yewentao256 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the work

Please provide with full reproduce command lm_eval .. line with full logs, also please add metrics for e2e performance using vllm bench ...

@AndreasKaratzas

Copy link
Copy Markdown
Member

I ran 40 GSM8K questions with 5-shot prompting and greedy decoding.

LMEVAL_LOG_LEVEL=DEBUG .venv/bin/lm_eval run \
  --model vllm \
  --model_args \
    pretrained=RedHatAI/Qwen3.6-35B-A3B-NVFP4 \
    trust_remote_code=True \
    max_model_len=4096 \
    max_num_seqs=32 \
    gpu_memory_utilization=0.85 \
    language_model_only=True \
    enable_prefix_caching=False \
    disable_log_stats=False \
    spec_method=dspark \
    spec_model=RedHatAI/Qwen3.6-35B-A3B-speculator.dspark \
    spec_tokens=8 \
    enable_thinking=False \
    seed=0 \
  --tasks gsm8k \
  --num_fewshot 5 \
  --limit 40 \
  --batch_size auto \
  --apply_chat_template \
  --fewshot_as_multiturn false \
  --gen_kwargs temperature=0 max_gen_toks=256 \
  --log_samples \
  --output_path ./lm-eval-results \
  --confirm_run_unsafe_code \
  --write_out \
  --show_config \
  2>&1 | tee lm-eval.log
Revision Exact match
Parent 2.5% (1/40)
PR head 80.0% (32/40)

Serving benchmark

For the E2E benchmark, I used 512 random requests with 512 input tokens and 128 output tokens, unlimited request rate, and a maximum concurrency of 32. I ran the same workload three times on each revision.

.venv/bin/python -m vllm.entrypoints.cli.main serve \
  RedHatAI/Qwen3.6-35B-A3B-NVFP4 \
  --trust-remote-code \
  --max-model-len 4096 \
  --max-num-seqs 32 \
  --gpu-memory-utilization 0.85 \
  --language-model-only \
  --no-enable-prefix-caching \
  --speculative-config '{"method":"dspark","model":"RedHatAI/Qwen3.6-35B-A3B-speculator.dspark","num_speculative_tokens":8,"draft_sample_method":"probabilistic"}' \
  --default-chat-template-kwargs '{"enable_thinking":false}' \
  --port 8000
.venv/bin/python -m vllm.entrypoints.cli.main bench serve \
  --backend vllm \
  --base-url http://127.0.0.1:8000 \
  --endpoint /v1/completions \
  --model RedHatAI/Qwen3.6-35B-A3B-NVFP4 \
  --dataset-name random \
  --random-input-len 512 \
  --random-output-len 128 \
  --random-range-ratio 0 \
  --num-prompts 512 \
  --num-warmups 32 \
  --request-rate inf \
  --max-concurrency 32 \
  --ignore-eos \
  --temperature 0 \
  --seed 42 \
  --percentile-metrics ttft,tpot,itl,e2el \
  --metric-percentiles 50,90,99 \
  --save-result

These are the averages across the three runs:

Metric Parent PR head Change
Successful requests 512/512 512/512 no failures
Request throughput 7.673 req/s 9.343 req/s +21.8%
Output throughput 982.1 tok/s 1195.9 tok/s +21.8%
Median E2E latency 4174 ms 2805 ms -32.8%
P99 E2E latency 4500 ms 7006 ms +55.7%
Mean acceptance length 1.011 2.068 +1.057

All requests completed successfully. Throughput and median latency improved, although P99 E2E latency was worse on this particular random workload.

@yewentao256 yewentao256 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, thanks for the work!

@yewentao256
yewentao256 merged commit 3e372c5 into vllm-project:main Aug 11, 2026
106 checks passed
@github-project-automation github-project-automation Bot moved this from Todo to Done in AMD Aug 11, 2026
@njhill

njhill commented Aug 11, 2026

Copy link
Copy Markdown
Member

We are trying to get #51718 merged asap, and I think this one will need to be reconsidered with that.

Can TRITON_ATTN not be used for this?

@AndreasKaratzas

Copy link
Copy Markdown
Member

Probably TRITON_ATTN is fine. I'd like to get a second opinion on that by @Rohan138
Generally the context here is the KV layout refactor PRs by @LucasWilkinson . And the question is, is AITER ready for this migration? Cause TRITON_ATTN will be a temporary solution until we default AITER at some point and likely deprecate ROCM_ATTN.

@Rohan138

Rohan138 commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator

Yeah TRITON_ATTN is fine for CI, for perf for e.g. upcoming Qwen 3.8 we'll probably use e.g. ROCM_AITER_FA or ROCM_AITER_UNIFIED_ATTN. We will eventually work on consolidating these, in the meantime I'm fine marking off these paths in the attention backend selector so that e.g. ROCM_ATTN is never available for hybrid models after the layout refactor.

@AndreasKaratzas

AndreasKaratzas commented Aug 11, 2026

Copy link
Copy Markdown
Member

Let's go with ROCM_AITER_FA or ROCM_AITER_UNIFIED_ATTN. I want to stay as close as possible to perf pipeline.
cc @njhill

zyp2014 pushed a commit to zyp2014/vllm that referenced this pull request Aug 21, 2026
…d models (vllm-project#51837)

Signed-off-by: Stefan Koncarevic <stefan.koncarevic@amd.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working mrv2 Model Runner V2 specific ready-run-all-tests Trigger CI with all tests for wide-ranging PRs rocm Related to AMD ROCm

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

5 participants