[Kimi-K3][AMD] Return KDA and MLA projection outputs directly - #50592
LiuYinfeng01 wants to merge 6 commits into
Conversation
|
Hi @tjtanaa, Could you please take a look at this PR when you have a chance? Your feedback would be greatly appreciated. Thank you! |
|
@hongxiayang I completed a full Kimi-K3 GSM8K A/B for PR #50592 on 8x MI355X using Neither baseline nor the KDA direct-return variant produced garbled text. Results: vLLM tests/evals/gsm8k/gsm8k_eval.py, 1,319 questions, 5-shot, baseline: 93.8590% (1,238/1,319), invalid rate 0.0758% KDA direct return: 95.5269% (1,260/1,319), invalid rate 0.0758% delta: +1.6679 percentage points lm_eval 0.4.12, gsm8k, 5-shot, baseline: 96.7400% (1,276/1,319) KDA direct return: 96.0576% (1,267/1,319) delta: -0.6823 percentage points The paired lm_eval breakdown was 1,259 both correct, 17 baseline-only Environment qualification: PR #50592 head is All aggregate JSON, per-sample logs, raw smoke responses, server logs, |
|
@hongxiayang #!/usr/bin/env bash VARIANT=${1:?usage: run_server.sh baseline|optimized} case "$VARIANT" in "${PODMAN[@]}" rm -f "$NAME" 2>/dev/null || true |
|
Thanks for this optimization. I mapped this PR's copy site against a new fixed-cohort pure-decode trace so we have a concurrency-relevant estimate in addition to the batch-one result in the description. Profile setup: 8 x MI355X (gfx950), TP8, BF16/MXFP4, non-speculative decode, prefix caching, 63,911-token shared prefix + 4,089-token suffix, full HIP-graph replay, and The trace contains 93 post-attention-projection
I would therefore expect an isolated fixed-cohort gain around 0.35-0.42 ms/step for this PR at C16/C24. This is a trace-derived removable-cost estimate, not a candidate A/B result; allocator/lifetime effects can make end-to-end behavior differ. For scope clarity, the 24 MLA projection copies are covered by #50847, while the separate 69 KDA recurrent-output placement copies are covered on the gfx950 pure-decode path by #50654. Those costs should not be attributed to this PR. |
|
This pull request has merge conflicts that must be resolved before it can be |
Hi. @Fangzhou-Ai Thanks for clarifying the scope. I will fold the functionality of #50847 into #50592, so a single PR will remove both the KDA and MLA post-projection copies. This does not overlap with #50654: #50654 removes the earlier KDA recurrent-output placement copy and fuses the decode operations, while #50592 removes the separate copy after the output projection. The two optimizations are complementary and can be applied together. |
bbe70a0 to
47af72b
Compare
|
✅ Triggered Buildkite CI #83945 for commit |
|
/ci retry |
|
❌ @LiuYinfeng01, A reviewer with write access must run |
|
@AndreasKaratzas seems this PR is ready to be merged? |
|
/ci run |
|
b200 batch invariance seems to be broken. I merged main to re eval it cause it was not failing in latest nightly. |
|
Hi @LiuYinfeng01, the pre-commit checks have failed. Please run: uv pip install pre-commit>=4.5.1
pre-commit install
pre-commit run --all-filesThen, commit the changes and push to your branch. For future commits, |
|
/ci retry |
|
✅ Triggered Buildkite CI #84061 for commit |
AndreasKaratzas
left a comment
There was a problem hiding this comment.
This also affects NVIDIA’s low-rank Kimi-Linear path. vllm/models/kimi_k3/nvidia/model.py imports this shared class when use_full_rank_gate=False, but still sets _self_attn_writes_output = True and calls it with output=output. Since this forward() no longer accepts output, that path will fail with an unexpected-keyword TypeError.
Could you update the NVIDIA caller to the new return-value contract too? The clean fix would be to make _run_self_attn() always return self.self_attn(...), since all three NVIDIA attention implementations now return tensors. vllm/models/kimi_k3/nvidia/kda.py itself does not need changing.
09e3915 to
32efaff
Compare
Avoid an extra full-hidden-state copy by returning the KDA output projection storage while retaining MLA's explicit output-buffer contract. Signed-off-by: Liuyinfeng01 <yinfeliu@amd.com>
Fold the MLA direct-return path into the KDA optimization so all Kimi-K3 attention layers avoid copying their projection output into a temporary buffer. Signed-off-by: Liuyinfeng01 <yinfeliu@amd.com>
Apply direct return to the dedicated full-rank Kimi-K3 KDA implementation selected by current main and cover its concrete projection ownership contract. Signed-off-by: Liuyinfeng01 <yinfeliu@amd.com>
Carry each MLP output into the next fused AttnRes call to avoid a standalone residual-add launch while preserving auxiliary-state and pipeline-boundary contracts. Signed-off-by: Liuyinfeng01 <yinfeliu@amd.com>
Apply Ruff's multiline union formatting to the decoder return annotation so pre-commit passes. Signed-off-by: Liuyinfeng01 <yinfeliu@amd.com>
Use the shared direct-return attention interface so NVIDIA's low-rank Kimi-Linear path no longer passes the removed output buffer. Signed-off-by: Liuyinfeng01 <yinfeliu@amd.com>
32efaff to
498a91b
Compare
Hi. Andreas. I’ve fixed the NVIDIA path and added a test for it. Thank you very much. |
|
/ci run |
|
✅ Triggered Buildkite CI #84163 for commit |
Purpose
Kimi-K3's AMD attention paths copied each output projection into a caller-owned buffer even though the projection already returns a tensor with the required shape, dtype, layout, and lifetime.
This PR removes that redundant post-projection allocation and copy from both attention families:
The functionality from #50847 is now folded into this PR. #50654 is separate and complementary: it optimizes the earlier KDA recurrent/conv/RMSNorm path, while this PR removes the later copy after the output projection.
Implementation
main.KimiDecoderLayerfor KDA and MLA.Validation
The tests cover KDA dispatch ownership, the concrete full-rank Kimi-K3 KDA forward path, and MLA dispatch ownership. In each case, the returned tensor is the exact projection tensor and has the same
data_ptr().Fixed-cohort decode A/B
Setup
Both variants used image
localhost/kimi-pp8-upstream-clean(vLLMbeca88e59, AITER99733dc00). The baseline was the unmodified image. The candidate mounted only this PR's AMD runtime files from43d81ba3e. The relevant AMD Kimi-K3 files in the image are identical to the rebasedmainbaseline before this PR.Results
Means over three measured runs. TPOT and ITL are end-to-end serving metrics, not the reviewer's pure-GPU trace period.
Mean-TPOT standard deviation across the three runs was 0.184 ms / 0.348 ms for baseline C16/C24 and 0.019 ms / 0.122 ms for candidate C16/C24.
Interpretation
The reviewer's trace found that the complete 93-copy group costs approximately 0.495 ms at C16 and 0.563 ms at C24. The combined A/B removes all 93 copies and measures a 0.713 ms C16 and 0.501 ms C24 TPOT reduction. C24 closely matches the trace-derived removable cost. C16 is modestly larger, which is plausible because returning projection storage directly also changes allocator and lifetime behavior; the baseline's run-to-run variance is also higher.
The absolute TPOT values should not be compared directly with the reviewer's 32.82/35.98 ms pure-GPU periods because TPOT includes serving and scheduling overhead. The A/B delta is the relevant same-harness comparison.
Scope summary