[Feature][Model] Fix per-request metadata buffer sizing for DSA-CP under full cudagraph padding - #16168
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical bug in the DSA-CP metadata builder where per-request buffers were sized based on the standard maximum sequence count rather than the padded count required during full cudagraph execution. By unifying the sizing logic to account for cudagraph capture sizes and FIA dummy requests, the changes ensure that memory allocations are robust against the larger request counts encountered in specific deployment configurations. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
|
👋 Hi! Thank you for contributing to the vLLM Ascend project. The following points will speed up your PR merge:
If CI fails, you can run linting and testing checks locally according Contributing and Testing. Tip 💡 Consider Linking a Related Issue or RFCYour PR title contains the [BugFix] tag, indicating a bug fix or new feature. Linking a related issue or RFC in the PR description is strongly encouraged — it gives reviewers helpful context and speeds up the review. You can use any of these keywords:
🙏 Thanks for helping us keep the project well-organized! |
There was a problem hiding this comment.
Code Review
Suggested PR Title:
[Attention][BugFix] Size all per-request buffers for graph-mode maximum to prevent memory corruptionSuggested PR Summary:
### What this PR does / why we need it?
This PR sizes all per-request buffers (such as `start_pos_prefill`, `qli_seqused_k`, `qli_cmp_residual_k`, `local_query_start_loc`, and `local_seq_lens`) for the graph-mode maximum (`max_padded_reqs`) instead of `scheduler_config.max_num_seqs`. This prevents issues where views get truncated, `copy_()` operations fail due to shape mismatches, or Triton kernels write past the buffer end and corrupt adjacent device memory.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
No tests were explicitly added, but existing tests should pass with these updated buffer sizes.I have no further feedback to provide as there are no review comments.
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
… sizing main reverted vllm-project#12599 (5d4294f); this re-applies the DSA-CP aclgraph feature on top of current main, keeping the graph-mode-aware per-request buffer sizing (max_padded_reqs) that fixes EZ1007 copy_ shape mismatches and out-of-bounds triton writes when num_reqs is padded beyond max_num_seqs during FULL_DECODE_ONLY aclgraph replay. Signed-off-by: frankie <wangyongsheng686@gmail.com>
…cp graph Re-applies the MTP speculative config removed by the vllm-project#12599 revert and adds a concurrent-request accuracy guard: max_num_seqs smaller than the cudagraph capture bucket with 4 concurrent MTP requests exercises the padded draft path. Signed-off-by: frankie <wangyongsheng686@gmail.com>
5c6021c to
56b9dad
Compare
|
The PR #16181 was reverted because it caused a nightly test failure. I will trigger the failed test case from that run in the comments, please keep an eye on the execution results. |
|
/nightly glm-5.2-w4a8c8-sfa-dcp
|
Thanks,I see this test has passed. |
vllm-project#16544) Reverts commit 200309d on main_verify. Clean revert: 41 of 43 surviving files are byte-identical to the pre-PR state. dsa_cp.py and test_model_runner_v2.py keep later upstream changes (vllm-project#16168 per-request metadata buffer sizing fix, vllm-project#15747 spec-pp protocol rename) as intended. Committed with --no-verify: pre-commit gitleaks/check-logger hooks fail on this Windows host (/bin/bash path + GBK locale); equivalent content checks (ruff, codespell, typos, check-logger, check-long-functions) all pass when run manually. Signed-off-by: chenzeyu <2978509328@qq.com>
vllm-project#16544) Reverts commit 200309d on main (cherry-picked from main_verify 82145c7). Conflict resolution: ascend_forward_context.py, patch/__init__.py, worker.py and test_ascend_forward_context.py were entangled with vllm-project#16626 (merged before vllm-project#16544, already reverted on this branch); they are restored to the pre-vllm-project#16626 state (c7ca0b6~1), the correct composition of both reverts. dsa_cp.py and test_model_runner_v2.py keep later upstream changes (vllm-project#16168 metadata buffer sizing fix, vllm-project#15747 spec-pp protocol rename). All other surviving files match the pre-PR state. Signed-off-by: chenzeyu <2978509328@qq.com>
What this PR does / why we need it?
This PR re-applies the DSA-CP + MTP aclgraph support for DeepSeek V4 that was reverted from main in #16181 (revert of #12599), together with a fix for the crash that motivated scrutiny of the original feature: serving concurrent requests with
cudagraph_mode=FULL_DECODE_ONLYfailed withaclnnInplaceCopyshape-mismatch errors (EZ1007) or illegal device memory accesses (EZ9999).Root cause of the crash
In FULL-decode graph mode, the DSA-CP draft path receives
num_reqsas the padded request count — the cudagraph capture bucket size plus the FIA dummy request from mixed-batch padding — which can exceedscheduler_config.max_num_seqs. The per-request metadata buffers were sized bymax_num_seqs, so:[:num_reqs]views of these buffers get silently truncated andcopy_()into them fails with shape mismatches — e.g.--max-num-seqs 6with 3 concurrent MTP requests pads 12 tokens to bucket 16, producingShape [16] vs [6] do not meet the broadcast condition (EZ1007);EZ9999: MTE accesses an invalid GM address).Fix
Compute one graph-mode-aware capacity up front and size all per-request buffers with it:
This covers the step-0 buffers (
start_pos_prefill,local_query_start_loc,local_seq_lens), the draft buffers (spec_local_query_start_loc,spec_local_seq_lens,spec_start_pos), and the QLI buffers (qli_seqused_k,qli_cmp_residual_k) with a single capacity source. All usage sites are[:num_reqs]slices or whole-buffer kernel inputs, so the change is a pure capacity gain with negligible (int32-level) memory overhead.The re-applied feature is fully adapted to current main: it fuses with the sequence-parallel prefill rework (#15549), the dynamic DSA indexer quant_mode (#16224) and the DSA PCP + DSpark support (#15958) already on main.
The feature enables aclgraph capture/replay for DSA-CP with MTP=1 and MTP=3, including stable per-draft-index metadata buffers (
spec_sas_metadata,spec_start_pos,spec_local_*, per-draft RoPE cache) so tensor addresses stay fixed across graph capture and replay.Does this PR introduce any user-facing change?
Yes, it re-enables DSA-CP + MTP with FULL_DECODE_ONLY graphs for DeepSeek V4 (
enable_dsa_cp: true+speculative_config+cudagraph_mode: FULL_DECODE_ONLY), which was available before the revert. Users withmax_num_seqssmaller than the max cudagraph capture size no longer hit EZ1007 / EZ9999 errors under concurrent load.The gain show below:
when use dsv4+mtp 3+eager:
GMS8K accuracy evaluation for MTP+eager and MTP+graph is attached below.