[eagle3][pcp] fix acceptance rate for eagle3 and pcp enabled - #7549
Conversation
Signed-off-by: lilinsiman <lilinsiman@gmail.com>
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 resolves a critical bug affecting the acceptance rate of the Eagle3 speculative decoding mechanism when used in conjunction with PCP. The fix ensures that the attention metadata, which is crucial for correct token generation and validation, is properly isolated for each processing step by cloning relevant tensors. This prevents data corruption due to shared mutable state, leading to accurate speculative decoding and improved model performance under these specific configurations. Highlights
🧠 New Feature in Public Preview: You can now enable Memory 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 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 counter productive. 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
|
There was a problem hiding this comment.
Code Review
This pull request addresses a data corruption bug in the speculative decoding logic for the Eagle proposer, specifically when Prefill Context Parallelism (PCP) is enabled. The issue stems from in-place modifications of tensors within the attention metadata across different speculative decoding steps, which could lead to incorrect behavior and a reduced token acceptance rate. The fix involves cloning several tensors (block_table_tensor, seq_lens, seq_lens_cpu, num_computed_tokens_cpu, and positions) before they are modified within the proposal and attention update logic. This ensures that each step of the speculative decoding process operates on an independent copy of the metadata, preventing side effects and ensuring correctness. The changes are well-targeted and correctly resolve the potential for data corruption.
|
👋 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. |
yiz-liu
left a comment
There was a problem hiding this comment.
This should be fine in eager mode, but it will definitely cause issues in ACL Graph.
- Put the new block tables in a persistent buffer, maybe as a
selfattribute in the proposer. - Make sure both
dummy_runandproposeread from that same buffer.
Signed-off-by: lilinsiman <lilinsiman@gmail.com>
…to qwen3next_graph * 'main' of https://github.com/vllm-project/vllm-ascend: (94 commits) [bugfix] Fixed the error issue when overlaying MTP and full decode on DSV3.1 C8. (vllm-project#7571) [eagle3][pcp] fix acceptance rate for eagle3 and pcp enabled (vllm-project#7549) [bugfix][CI] fix '_OpNamespace' 'vllm' object has no attribute 'qkv_rmsnorm_rope' (vllm-project#7620) [Nightly] Nightly pre-build image (vllm-project#7388) [Bugfix]Fix deepseek 3.2 C8 precision by rotary tensor (vllm-project#7537) adapt to main2main for model runner v2 (vllm-project#7578) [Patch] Fix balance scheduling (vllm-project#7611) [310P]fused recurrent gated delta rule pytorch core and ut (vllm-project#7398) [CI] refine issue triage rules, wan regex and update stale setting (vllm-project#7531) [Lint]Add lint hooks for clang-format, shellcheck, forbidden imports, and boolean context manager checks (vllm-project#7511) [doc] add enable_sparse_c8 option in configuration options (vllm-project#7600) lower log level in PD Disaggregation (vllm-project#7589) [model_runner_v2]:optimize the performance of the _compute_slot_mappings_kernel (vllm-project#7575) [Feat][SP] Suport SP for VL MoE models (vllm-project#7044) Fix Qwen3Next CI Config (vllm-project#7561) [Feat] Add npugraph_ex enablement logging (vllm-project#7574) [UT] Align input arguments with Ascend(Yarn)RotaryEmbedding with vLLM and add ut (vllm-project#7358) [P/D] Check wildcard address for layerwise connector (vllm-project#7389) [P/D] [Bugfix] fix mooncake layerconnector dead when update_decoder_info fail (vllm-project#7514) [BugFix][P/D] fix padding error on FullGraph mode && fix layerwise connector mamba accuracy (vllm-project#7506) ...
…oject#7549) ### What this PR does / why we need it? fix the position 3 acceptance rate for eagle3 and pcp enabled detail: In the merged graph of eagle_proposer, the code logic was changed from updating the code once before the forward pass of the draft model to updating all three positions of common_attn_metadata in the merged graph before performing the forward pass of the model. As a result, the update of position 2 and position 3 affected the update of position 1. For example, in the following field: common_attn_metadata.block_table_tensor[:batch_size] = common_attn_metadata.block_table_tensor[block_indices] When updating the block_table_tensor at position 2, the modification of this field occurred at the original address of common_attn_metadata. As a result, the parameter at position 1 was also modified, but the forward pass at position 1 had not been performed. Therefore, a copy of the address of block_table_tensor needs to be made, and the modification needs to be performed on the new address to ensure complete isolation between positions. ### Does this PR introduce _any_ user-facing change? no ### How was this patch tested? tests and ut - vLLM version: v0.18.0 - vLLM main: vllm-project/vllm@8b63257 --------- Signed-off-by: lilinsiman <lilinsiman@gmail.com>
…oject#7549) ### What this PR does / why we need it? fix the position 3 acceptance rate for eagle3 and pcp enabled detail: In the merged graph of eagle_proposer, the code logic was changed from updating the code once before the forward pass of the draft model to updating all three positions of common_attn_metadata in the merged graph before performing the forward pass of the model. As a result, the update of position 2 and position 3 affected the update of position 1. For example, in the following field: common_attn_metadata.block_table_tensor[:batch_size] = common_attn_metadata.block_table_tensor[block_indices] When updating the block_table_tensor at position 2, the modification of this field occurred at the original address of common_attn_metadata. As a result, the parameter at position 1 was also modified, but the forward pass at position 1 had not been performed. Therefore, a copy of the address of block_table_tensor needs to be made, and the modification needs to be performed on the new address to ensure complete isolation between positions. ### Does this PR introduce _any_ user-facing change? no ### How was this patch tested? tests and ut - vLLM version: v0.18.0 - vLLM main: vllm-project/vllm@8b63257 --------- Signed-off-by: lilinsiman <lilinsiman@gmail.com>
…oject#7549) ### What this PR does / why we need it? fix the position 3 acceptance rate for eagle3 and pcp enabled detail: In the merged graph of eagle_proposer, the code logic was changed from updating the code once before the forward pass of the draft model to updating all three positions of common_attn_metadata in the merged graph before performing the forward pass of the model. As a result, the update of position 2 and position 3 affected the update of position 1. For example, in the following field: common_attn_metadata.block_table_tensor[:batch_size] = common_attn_metadata.block_table_tensor[block_indices] When updating the block_table_tensor at position 2, the modification of this field occurred at the original address of common_attn_metadata. As a result, the parameter at position 1 was also modified, but the forward pass at position 1 had not been performed. Therefore, a copy of the address of block_table_tensor needs to be made, and the modification needs to be performed on the new address to ensure complete isolation between positions. ### Does this PR introduce _any_ user-facing change? no ### How was this patch tested? tests and ut - vLLM version: v0.18.0 - vLLM main: vllm-project/vllm@8b63257 --------- Signed-off-by: lilinsiman <lilinsiman@gmail.com> Signed-off-by: zouyida2052 <zouyida2002@gmail.com>
…oject#7549) ### What this PR does / why we need it? fix the position 3 acceptance rate for eagle3 and pcp enabled detail: In the merged graph of eagle_proposer, the code logic was changed from updating the code once before the forward pass of the draft model to updating all three positions of common_attn_metadata in the merged graph before performing the forward pass of the model. As a result, the update of position 2 and position 3 affected the update of position 1. For example, in the following field: common_attn_metadata.block_table_tensor[:batch_size] = common_attn_metadata.block_table_tensor[block_indices] When updating the block_table_tensor at position 2, the modification of this field occurred at the original address of common_attn_metadata. As a result, the parameter at position 1 was also modified, but the forward pass at position 1 had not been performed. Therefore, a copy of the address of block_table_tensor needs to be made, and the modification needs to be performed on the new address to ensure complete isolation between positions. ### Does this PR introduce _any_ user-facing change? no ### How was this patch tested? tests and ut - vLLM version: v0.18.0 - vLLM main: vllm-project/vllm@8b63257 --------- Signed-off-by: lilinsiman <lilinsiman@gmail.com>
…oject#7549) ### What this PR does / why we need it? fix the position 3 acceptance rate for eagle3 and pcp enabled detail: In the merged graph of eagle_proposer, the code logic was changed from updating the code once before the forward pass of the draft model to updating all three positions of common_attn_metadata in the merged graph before performing the forward pass of the model. As a result, the update of position 2 and position 3 affected the update of position 1. For example, in the following field: common_attn_metadata.block_table_tensor[:batch_size] = common_attn_metadata.block_table_tensor[block_indices] When updating the block_table_tensor at position 2, the modification of this field occurred at the original address of common_attn_metadata. As a result, the parameter at position 1 was also modified, but the forward pass at position 1 had not been performed. Therefore, a copy of the address of block_table_tensor needs to be made, and the modification needs to be performed on the new address to ensure complete isolation between positions. ### Does this PR introduce _any_ user-facing change? no ### How was this patch tested? tests and ut - vLLM version: v0.18.0 - vLLM main: vllm-project/vllm@8b63257 --------- Signed-off-by: lilinsiman <lilinsiman@gmail.com> Signed-off-by: nanxing <1014662416@qq.com>
…oject#7549) ### What this PR does / why we need it? fix the position 3 acceptance rate for eagle3 and pcp enabled detail: In the merged graph of eagle_proposer, the code logic was changed from updating the code once before the forward pass of the draft model to updating all three positions of common_attn_metadata in the merged graph before performing the forward pass of the model. As a result, the update of position 2 and position 3 affected the update of position 1. For example, in the following field: common_attn_metadata.block_table_tensor[:batch_size] = common_attn_metadata.block_table_tensor[block_indices] When updating the block_table_tensor at position 2, the modification of this field occurred at the original address of common_attn_metadata. As a result, the parameter at position 1 was also modified, but the forward pass at position 1 had not been performed. Therefore, a copy of the address of block_table_tensor needs to be made, and the modification needs to be performed on the new address to ensure complete isolation between positions. ### Does this PR introduce _any_ user-facing change? no ### How was this patch tested? tests and ut - vLLM version: v0.18.0 - vLLM main: vllm-project/vllm@8b63257 --------- Signed-off-by: lilinsiman <lilinsiman@gmail.com>
…oject#7549) ### What this PR does / why we need it? fix the position 3 acceptance rate for eagle3 and pcp enabled detail: In the merged graph of eagle_proposer, the code logic was changed from updating the code once before the forward pass of the draft model to updating all three positions of common_attn_metadata in the merged graph before performing the forward pass of the model. As a result, the update of position 2 and position 3 affected the update of position 1. For example, in the following field: common_attn_metadata.block_table_tensor[:batch_size] = common_attn_metadata.block_table_tensor[block_indices] When updating the block_table_tensor at position 2, the modification of this field occurred at the original address of common_attn_metadata. As a result, the parameter at position 1 was also modified, but the forward pass at position 1 had not been performed. Therefore, a copy of the address of block_table_tensor needs to be made, and the modification needs to be performed on the new address to ensure complete isolation between positions. ### Does this PR introduce _any_ user-facing change? no ### How was this patch tested? tests and ut - vLLM version: v0.18.0 - vLLM main: vllm-project/vllm@8b63257 --------- Signed-off-by: lilinsiman <lilinsiman@gmail.com>
What this PR does / why we need it?
fix the position 3 acceptance rate for eagle3 and pcp enabled
detail:
In the merged graph of eagle_proposer, the code logic was changed from updating the code once before the forward pass of the draft model to updating all three positions of common_attn_metadata in the merged graph before performing the forward pass of the model. As a result, the update of position 2 and position 3 affected the update of position 1.
For example, in the following field:
common_attn_metadata.block_table_tensor[:batch_size] = common_attn_metadata.block_table_tensor[block_indices]
When updating the block_table_tensor at position 2, the modification of this field occurred at the original address of common_attn_metadata. As a result, the parameter at position 1 was also modified, but the forward pass at position 1 had not been performed. Therefore, a copy of the address of block_table_tensor needs to be made, and the modification needs to be performed on the new address to ensure complete isolation between positions.
Does this PR introduce any user-facing change?
no
How was this patch tested?
tests and ut