Skip to content

[refactor][model] Kimi-K3 DSpark MLA common base class - #60054

Draft
gronsti-amd wants to merge 7 commits into
vllm-project:mainfrom
gronsti-amd:sgronroo/refactor-kimi-k3-dspark
Draft

gronsti-amd wants to merge 7 commits into
vllm-project:mainfrom
gronsti-amd:sgronroo/refactor-kimi-k3-dspark

Conversation

@gronsti-amd

@gronsti-amd gronsti-amd commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

Purpose

Currently, the Kimi-K3 DSpark draft model has only one NVIDIA implementation,
and the dispatch is hard-wired to it on every platform. A ROCm module that copies that file drifts.
This PR moves the draft model into vllm/models/kimi_k3/common/dspark_mla.py and leaves NVIDIA as a thin binding.
NVIDIA behavior stays the same: the same MultiHeadLatentAttention, the same layer name, and the same kernels.
The registry still loads vllm.models.kimi_k3.nvidia.dspark_mla:
removing the hard-wired dispatch is left for a follow-up PR (#60857).

What changes

  • The decoder, context-KV insert, weight mapper, Markov head, and confidence head live once, in common/dspark_mla.py. That file imports neither amd/ nor nvidia/.
  • NVIDIA supplies build_self_attn and build_mlp. kv_cache_layer defaults to the attention module, so context-KV insert and get_draft_kv_cache_layer_names() still use self_attn.

Validation

  • pytest tests/models/test_dspark_mla.py passes.
  • Existing tests cover the registry class, the weight mapper, duplicated context-KV shards, the replicated Markov head, and the compile-free entry point. Construction monkeypatches target vllm.models.kimi_k3.common.dspark_mla.
  • New tests check that the NVIDIA binding's attention class is MultiHeadLatentAttention, that kv_cache_layer returns the attention module, that a wrapper subclass's mla_attn_cls is constructed with a prefix ending in .attn, and that the default hooks return their inputs.
  • The file is collected in Basic Models, including the CPU job. Main's newer DeepSeek and layer-registry entries in that job stay on the command line.

Details

AMD and NVIDIA attention and cache architecture for MLA differ: NVIDIA MultiHeadLatentAttention owns the KV cache, while the AMD stack is KimiK3MultiHeadLatentAttentionWrapper around a generic MLAAttention whose layer_name is prefix + ".attn". Geometry, norms, and context RoPE already sit on self_attn on both sides. This PR refactors so that the three reads that differ (cache, dtype and scale, layer name) go through kv_cache_layer.

  • common/ does not construct the NVIDIA attention class. A default there would import nvidia.mla, which is also the NVIDIA target K3 attention.
  • #58814 pointer refresh is in the shared path: _has_uniform_block_layout keys the cache on data_ptr() and drops _context_cache_ptrs when the allocation changes.
  • enable_confidence_head builds DSparkConfidenceHead, keeps those checkpoint weights, and compute_confidence serves adaptive verification. The head stays None when the flag is off.
  • MLAAttention stores positions as _rope_positions (assigned every forward, including None). The wrapper takes mla_attn_cls. Three hooks default to today's behavior: _prepare_kv_cache_update, _prepare_mha_inputs, and _form_decode_q. Custom-op schemas stay as they are. Positions are module state the ops read.
  • CUDA-graph replay sees new positions and slot mappings only while those tensors stay the persistent buffers the runner updates in place. Replacing either tensor freezes the captured pointer. Comments on both sites say so.

A planned fusion of QK-RoPE plus cache insert (fused_qk_rope_concat_and_cache_mla) requires an epilogue to replace query formation and the KV write.
The fusion cannot live only in the draft model: Absorb runs inside MLAAttention.forward_impl, and unified_mla_kv_cache_update cannot take positions.
This PR adds three defaulted hooks as an extension point for this decode epilogue, to avoid needing a second refactor immediately after this. The defaults do not read _rope_positions.

The Kimi-K3 DSpark MLA is under active development, causing a naive duplicate-and-modify approach to quickly drift, as shown by #57942:

Refactoring reduces the drift, allowing both platforms to gain new features in a single change.

Out of scope

Suggested review order

Review by commit. The aggregate diff looks like a rewrite of nvidia/dspark_mla.py because that path stays as the public binding, so Git does not record a rename.


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.

@gronsti-amd gronsti-amd changed the title Sgronroo/refactor kimi k3 dspark [refactor][model] Kimi-K3 DSpark MLA common base class Oct 5, 2026
@gronsti-amd gronsti-amd mentioned this pull request Oct 5, 2026
2 of 7 tasks
@mergify

mergify Bot commented Oct 7, 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, @gronsti-amd.

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 7, 2026
gronsti-amd and others added 7 commits October 9, 2026 15:06
Keep the NVIDIA module as a re-export so the registry path is unchanged.

Signed-off-by: Stig-Arne Grönroos <stig-arne.gronroos@amd.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
NVIDIA keeps MultiHeadLatentAttention as the cache owner. A platform
override can point context-KV insert and draft layer names at another module.

Signed-off-by: Stig-Arne Grönroos <stig-arne.gronroos@amd.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
The defaults keep today's concat and cache write. A subclass can replace
the decode query and the KV rows it writes without a new custom-op argument.

Signed-off-by: Stig-Arne Grönroos <stig-arne.gronroos@amd.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
The common module is a new file, so pre-commit lints it in full. The
(list, tuple) form was already on main and fails UP038 once the file moves.

Signed-off-by: Stig-Arne Grönroos <stig-arne.gronroos@amd.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
The file was not listed in Buildkite, so the refactor would not be collected on either platform.

Signed-off-by: Stig-Arne Grönroos <stig-arne.gronroos@amd.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
DeepseekV32Attention already assigns a concrete RotaryEmbedding to that name. Typing the base attribute as optional made mypy reject its cache reads.

Signed-off-by: Stig-Arne Grönroos <stig-arne.gronroos@amd.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Replay reads the captured positions and slot-mapping allocations, so a replacement tensor freezes the pointer. Lock the NVIDIA binding class and the default hook returns.

Signed-off-by: Stig-Arne Grönroos <stig-arne.gronroos@amd.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
@mergify

mergify Bot commented Oct 10, 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, @gronsti-amd.

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 10, 2026

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

Projects

Status: Backlog

Development

Successfully merging this pull request may close these issues.

1 participant