Skip to content

[Bugfix][Kimi-K3] Declare max_tp_shards on the DSpark MLA KV cache spec - #59733

Merged
ywang96 merged 6 commits into
vllm-project:mainfrom
hyukjlee:fix/k3-dspark-mla-max-tp-shards
Oct 8, 2026
Merged

ywang96 merged 6 commits into
vllm-project:mainfrom
hyukjlee:fix/k3-dspark-mla-max-tp-shards

Conversation

@hyukjlee

@hyukjlee hyukjlee commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

Overview

Fixes a Kimi-K3 + DSpark perf regression from #57652: the K3 DSpark draft's MLAAttentionSpec does not declare max_tp_shards, so its MLA layers stop merging with the target's and the KV cache splits from 4 to 20 groups. One-line fix: declare max_tp_shards=1 on the draft spec, matching MLAAttention.

Claims

Validation

Unit test (tests/models/test_dspark_mla.py::test_k3_dspark_mla_kv_cache_spec_groups_with_target_mla): builds the target spec via MLAAttention.get_kv_cache_spec and the draft spec via the K3 MultiHeadLatentAttention.get_kv_cache_spec, then groups 24 target + 5 draft layers with _get_kv_cache_groups_uniform_page_size.

  • Without the fix: AssertionError: assert 6 == 1
  • With the fix: passes (1 group, 29 layers)

Run inside vllm/vllm-openai-rocm:nightly-rocm100-ac9126e5… with python -m pytest -q tests/models/test_dspark_mla.py -k k3_dspark_mla_kv_cache_spec. pre-commit passes on both files.

End-to-end: Kimi-K3 (MXFP4) + Kimi-K3-DSpark, MI355X ×8, TP8, exact InferenceX CI serve command (DSpark k=7 synthetic AL 3.84, fp8 KV, SimpleCPUOffloadConnector, FULL cudagraph, --max-num-seqs 16). Client: vllm bench serve --dataset-name random --random-input-len 8192 --random-output-len 1024 --ignore-eos --max-concurrency 8 --num-prompts 80, 3 seeds per arm after a 16-prompt warmup, same host, arms run back to back.

image KV cache groups total tput/GPU (tok/s) output tput (tok/s) median TPOT (ms) median ITL (ms)
nightly 36768d1 (before #57652) 4 729 648 11.13 33.4
nightly ac9126e (includes #57652) 20 701 623 11.47 34.4
ac9126e + revert #57652 4 726 646 11.15 33.2
ac9126e + this PR 4 727 647 11.09 33.1

Per-seed median TPOT for this PR: 11.10 / 10.99 / 11.18 ms (vs 11.43 / 11.44 / 11.54 without it). The regression was first seen in InferenceX CI on the K3 agentic c8 cell (kimik3 fp4 mi355x vllm TP8 mtp dram KV offload vllm-simple c8): −6.1% total throughput/GPU and −16.6% p90 interactivity between the two nightlies.

Accuracy (gsm8k): same server config but with real DSpark verification ("rejection_sample_method": "block", no synthetic acceptance length), evaluated with the InferenceX CI lm-eval setup: lm_eval --model local-chat-completions --apply_chat_template --tasks gsm8k (5-shot, CI task YAML), full 1319-question test set, temperature=0, max_tokens=16384, num_concurrent=16.

image KV cache groups gsm8k exact_match (strict / flexible)
ac9126e + this PR 4 0.9636 ± 0.0052 / 0.9636 ± 0.0052

Mean DSpark acceptance length during the eval was ~3.0, confirming real verification.

Details

Root cause. #57652 added AttentionSpec.max_tp_shards and set it to 1 in MLAAttention.get_kv_cache_spec. vllm/models/kimi_k3/nvidia/mla.py (used for the K3 target on NVIDIA and for the DSpark draft on both vendors) builds its own MLAAttentionSpec and leaves it None. On ROCm the K3 target uses MLAAttention (1) while the draft uses this class (None), so MLAAttentionSpec.merge's per-field AttentionSpec equality check rejects them. _get_kv_cache_groups_uniform_page_size then sees buckets of 69 KDA / 24 target MLA / 5 draft MLA layers instead of 69 / 29, the group size drops from 29 to 5, and the group count goes from 4 (3 KDA + 1 MLA) to 20 (14 + 5 + 1). The server log shows kv cache group sizes growing from 4 to 20 entries and Add 1 padding layers twice instead of Add 18 padding layers.

Why 20 groups is slower. _build_attention_metadata builds per KV cache group each step, and neither the ROCm K3 KDA builder nor the AITER MLA builder supports update_block_table reuse, so every extra group is a full builder.build() plus its own block-table/slot-mapping work.

Why 1 is correct. The draft's MLA latent KV has a single head and is replicated across TP ranks, same as MLAAttention. On NVIDIA the target and draft both go from None to 1, so they still merge and grouping does not change. kv_tp_replicas remains 1 for K3 because its KDA (Mamba) layers contribute 1 to the gcd, so offloading behavior is unchanged.

Not duplicate. No open PR sets max_tp_shards on the K3 draft spec. #57942 (draft) adds a separate AMD DSpark MLA module, and #58207 changes the group-size heuristic. Both are broader and neither fixes this mismatch on main. This PR is the minimal fix for the regression.


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.

AI assistance (Claude Code) was used to root-cause the regression, write the fix and test, and run the benchmarks and accuracy eval.

🤖 Generated with Claude Code

vllm-project#57652 sets max_tp_shards=1 on MLAAttention's KV cache spec, but the
Kimi-K3 DSpark draft builds its own MLAAttentionSpec without it. The
mismatch makes MLAAttentionSpec.merge reject the draft layers, so they
no longer share a group with the target's MLA layers. The group size
drops from 29 to 5 and Kimi-K3 + DSpark goes from 4 to 20 KV cache
groups, adding per-step metadata/block-table work.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Signed-off-by: hyukjlee <hyukjlee@amd.com>
@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
@hyukjlee
hyukjlee marked this pull request as ready for review October 2, 2026 04:58

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

@seungrokj

Copy link
Copy Markdown
Contributor

Some notes on scope: why only Kimi-K3 + DSpark on ROCm regresses after #57652, and why other target + MTP/draft pairs don't.

When #57652 changes grouping. MLAAttentionSpec.merge compares every AttentionSpec field, and max_tp_shards is now one of them. _get_kv_cache_groups_uniform_page_size puts layers in one bucket only if merge() succeeds. So grouping changes only when a model has target and draft layers that used to merge and now differ in max_tp_shards alone. That happens only when the target and the draft build their specs in different code, and one of those places sets the field while the other doesn't.

Kimi-K3 + DSpark on ROCm meets that condition. The ROCm target MLA goes through the generic MLAAttention (max_tp_shards=1). The registry maps K3DSparkForCausalLM to kimi_k3/nvidia/dspark_mla.py on both vendors, and that file uses the spec from kimi_k3/nvidia/mla.py, which leaves the field None. K3's layer counts make the effect large: 69 KDA, 24 target MLA and 5 draft MLA layers. The smallest bucket, now the 5 draft layers, sets the group size, so it falls from 29 to 5 and the group count rises from 4 to 20.

Pairs that are not affected (checked on main):

model + draft target MLA spec draft MLA spec effect
Kimi-K3 + DSpark, ROCm MLAAttention (1) kimi_k3/nvidia/mla.py (None) 4 → 20 groups
Kimi-K3 + DSpark, NVIDIA kimi_k3/nvidia/mla.py (None) same class (None) equal, still merges
Kimi-K3 + MTP, ROCm / NVIDIA platform KimiDecoderLayer MLA same KimiDecoderLayer (amd/mtp.py, nvidia/mtp.py) equal
DeepSeek V3 / V3.2 + MTP MLAAttention (1); V3.2 indexer DeepseekV32IndexerCache (1) same classes equal; #57652 set 1 on both
DeepSeek V4 deepseek_v4/* (1) same classes equal
DeepSeek V4.1 + DSpark deepseek_v41/attention.py DeepseekV4Attention / DeepseekV4IndexerCache (None) deepseek_v41/{amd,nvidia}/dspark.py use the same class equal (both None)
MiniMax-M3 + MTP indexer minimax_m3/common/indexer.py (None, cache_role=INDEXER) MTP has no indexer nothing to merge with
Qwen4-exp + MTP QSACompressedKeyCache (None, tokens_per_state>1) MTP uses standard attention (Qwen3_5Model) different spec type, never merged
EAGLE3 / Qwen3-DSpark drafts on MLA targets MLA standard attention (FullAttentionSpec) different spec type, never merged

So this one-line fix covers every grouping regression from #57652 on main.

Remaining None builders. The builders that still leave the field None (deepseek_v41/attention.py ×2, minimax_m3/common/indexer.py, qwen4_exp/common/qsa_cache.py) don't change grouping. However, kv_cache_groups_tp_replicas treats None as "not replicated", so they keep kv_tp_replicas at 1. That only matters for #57652's replicated-layout KV offload path. Could they get max_tp_shards=1 in a follow-up? For QSA, someone should first confirm the compressed key cache is identical on every TP rank. Also, merge() keys grouping on max_tp_shards, a field that only describes TP sharding. That makes this kind of split easy to reintroduce whenever a new builder forgets the field. Comparing it outside the grouping key, or defaulting it from num_kv_heads == 1, would remove that trap.

Independent measurement. We saw the same effect on MI355X TP8, Kimi-K3 + DSpark k=7, conc 4, no KV offload, 3 seeds per arm:

image KV cache groups output tok/s mean TPOT (ms)
nightly 36768d1 4 452.1 7.77
nightly ac9126e 20 438.4 7.95
ac9126e + revert #57652 4 458.1 7.65

(Analysis prepared with Claude Code.)

@github-actions

github-actions Bot commented Oct 3, 2026

Copy link
Copy Markdown

❌ @seungrokj, Only reviewers with write access can use CI commands before CI is delegated to the PR author.

@ChuanLi1101 ChuanLi1101 added the ready ONLY add when PR is ready to merge/full CI is needed label Oct 5, 2026
@github-actions

github-actions Bot commented Oct 5, 2026

Copy link
Copy Markdown

✅ @hyukjlee, CI is now available for this PR.

  • /ci run starts upstream CI; /amd-ci run starts AMD CI only.
  • Your branch must contain every commit currently on its upstream target branch. Merge or rebase onto the latest target branch, then rerun the command. Append --allow-stale to a run command to test an outdated branch at your own risk.
  • /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.
  • /amd-ci retry retries failed jobs in AMD CI for the current PR head. Use /amd-ci run when the current head has no AMD CI build.
  • /ci cancel cancels scheduled or running CI builds for this PR branch; /amd-ci cancel does the same for AMD CI only.

@seungrokj

Copy link
Copy Markdown
Contributor

/ci run

@seungrokj

Copy link
Copy Markdown
Contributor

/ci run

@github-actions

github-actions Bot commented Oct 6, 2026

Copy link
Copy Markdown

❌ @seungrokj, Only reviewers with write access can use CI commands before CI is delegated to the PR author.

@hyukjlee

hyukjlee commented Oct 6, 2026

Copy link
Copy Markdown
Contributor Author

/ci run

@github-actions

github-actions Bot commented Oct 6, 2026

Copy link
Copy Markdown

❌ This PR is 1 commit behind upstream main. Your branch must contain every commit currently on upstream main. No new CI build was started. Merge or rebase onto the latest main, then rerun /ci run. To test this branch at your own risk, use /ci run --allow-stale.

@hyukjlee

hyukjlee commented Oct 6, 2026

Copy link
Copy Markdown
Contributor Author

/ci run

@github-actions

github-actions Bot commented Oct 6, 2026

Copy link
Copy Markdown

✅ Triggered Buildkite CI #92996 for commit 09a1d294678e.

@vllm-agent

vllm-agent commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

CI selector (shadow): 6 test steps (14 jobs) instead of 36 (52 jobs)

Shadow mode: this changes nothing about what CI runs. It shows what the evidence-based selector would pick for this PR, next to today's rules. How it works.

Feedback welcome: reply here if it would skip a step this change needs, or runs something unrelated.

steps (jobs) Today's rules Selector Would skip Would add
NVIDIA, CPU and others 36 (52) 6 (14) 33 (46) 3 (8)
AMD mirrors 33 (43) 0 (0) 33 (43) 0 (0)
Selector would run (6)
  • arm-cpu-test ×3
  • basic-models-test-other-cpu
  • cpu-language-generation-and-pooling-model-tests ×3
  • cpu-multi-modal-model-tests-n ×4
  • cpu-spec-decode-tests
  • kimi-k3-prefix-cache-4xb200 ×2
Would skip (today's rules run them) (33)
  • ascend-npu-test
  • basic-correctness ×2
  • basic-correctness-cpu-offload
  • basic-correctness-cumem
  • basic-correctness-prefetch-offload
  • basic-correctness-sleep-mode
  • basic-models-tests-initialization
  • basic-models-tests-other
  • benchmarks-cli-test
  • distributed-model-tests-2-gpus ×3
  • entrypoints-integration-api-server ×4
  • entrypoints-integration-api-server-generate
  • entrypoints-integration-api-server-openai-chat_completion
  • entrypoints-integration-api-server-openai-completion
  • entrypoints-integration-llm
  • entrypoints-integration-multimodal
  • entrypoints-integration-pooling
  • entrypoints-integration-responses-api
  • entrypoints-integration-speech_to_text
  • kernels-fla-ops-test-b200
  • kernels-mhc-test-b200
  • kernels-root-misc-test-b200
  • kimi-k3-unit-tests-b200
  • language-models-tests-granite-l4-compatibility
  • language-models-tests-hybrid ×2
  • language-models-tests-standard
  • multi-modal-models-standard-1-qwen2
  • multi-modal-models-standard-2-qwen3-gemma
  • multi-modal-models-standard-3-llava-qwen2-vl
  • multi-modal-models-standard-4-other-whisper
  • multi-modal-processor ×4
  • multi-modal-processor-cpu ×4
  • rl-entrypoints-tests
Would add (today's rules do not run them) (3)
  • arm-cpu-test ×3 (code map)
  • cpu-multi-modal-model-tests-n ×4 (code map)
  • cpu-spec-decode-tests (code map)
AMD mirrors: would skip (33)
  • basic-correctness ×2
  • basic-correctness-cpu-offload
  • basic-correctness-cumem
  • basic-correctness-prefetch-offload
  • basic-correctness-sleep-mode
  • basic-models-tests-initialization
  • basic-models-tests-other
  • benchmarks-cli-test
  • distributed-model-tests-2-gpus ×3
  • entrypoints-integration-api-server ×4
  • entrypoints-integration-api-server-generate
  • entrypoints-integration-api-server-openai-chat_completion
  • entrypoints-integration-api-server-openai-completion
  • entrypoints-integration-llm
  • entrypoints-integration-multimodal
  • entrypoints-integration-pooling
  • entrypoints-integration-responses-api
  • entrypoints-integration-speech_to_text
  • kernels-fla-ops-test-b200
  • kernels-mhc-test-b200
  • kernels-root-misc-test-b200
  • kimi-k3-unit-tests-b200
  • language-models-tests-granite-l4-compatibility
  • language-models-tests-hybrid ×2
  • language-models-tests-standard
  • multi-modal-models-standard-1-qwen2
  • multi-modal-models-standard-2-qwen3-gemma
  • multi-modal-models-standard-3-llava-qwen2-vl
  • multi-modal-models-standard-4-other-whisper
  • multi-modal-processor ×4
  • platform-tests
  • pytorch-compilation-passes-unit-tests
  • rl-entrypoints-tests
AMD mirrors: would add (0)

none

2 changed files · base d6fe5dca68 · head ceaf8c9bf1 · Python record: build 93039 at 1e5d0ea888 · kernel record: table 43b4aae (build 93244), map 43b4aae · not counted: 10 build steps, 5 A100 steps the generator no longer emits, 10 optional steps the selector would also run

@GirasoleY GirasoleY left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the point fix.

cc. @varun-sundar-rabindranath @LucasWilkinson for a second eye.

@GirasoleY

Copy link
Copy Markdown
Contributor

/ci run

@ChuanLi1101

Copy link
Copy Markdown
Collaborator

The DSpark draft was missing max_tp_shards=1, so it stopped sharing a KV group with the target. K3 went from 4 groups to 20. This one line puts them back together.

MI355X TP8: 701 to 727 tok/s/GPU, TPOT 11.47 to 11.09 ms, back to the nightly before the regression. GSM8K 0.9636. CI passed. Good to land after a rebase onto main.

@github-actions

github-actions Bot commented Oct 7, 2026

Copy link
Copy Markdown

✅ Triggered Buildkite CI #93413 for commit 6451effed455.

@seungrokj

Copy link
Copy Markdown
Contributor

@hyukjlee can you
merge or rebase onto current main and run ci again ?

@hyukjlee

hyukjlee commented Oct 7, 2026

Copy link
Copy Markdown
Contributor Author

/ci run

@github-actions

github-actions Bot commented Oct 7, 2026

Copy link
Copy Markdown

✅ Triggered Buildkite CI #93456 for commit ceaf8c9bf13e.

@GirasoleY

Copy link
Copy Markdown
Contributor

/ci run

@ywang96
ywang96 merged commit 1fbbfa1 into vllm-project:main Oct 8, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working dflash k3 kimi ready ONLY add when PR is ready to merge/full CI is needed

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

6 participants