[BugFix][CI][310p] Fix CI error for 310p caused by DSV4 - #9402
Conversation
Signed-off-by: Tflowers-0129 <2906339855@qq.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 addresses CI errors encountered in the 310p environment by improving the handling of KV cache groups within the block table and model runner. The changes introduce support for multi-group configurations, ensuring that block tables and slot mapping logic correctly account for diverse KV cache specifications. These updates provide better compatibility with complex model architectures and resolve issues arising from previous assumptions about single-group KV cache structures. 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. |
There was a problem hiding this comment.
Code Review
This pull request updates the MultiGroupBlockTable and NPUInputBatch for the Ascend 310P platform to support multiple KV cache groups and compressed slot mapping inputs. Key changes include the propagation of kv_cache_groups during initialization and the implementation of logic to handle per-group compressed positions and request indices. Feedback highlights a critical IndexError in model_runner_310p.py where iterating over all KV cache groups causes an index mismatch with the filtered block_sizes list. Additionally, it is recommended to simplify the MultiGroupBlockTable initialization to reduce code duplication and to update the PR title and summary to align with the repository's style guide.
| max_num_blocks = [] | ||
| max_model_len = max(self.max_model_len, self.max_encoder_len) | ||
| for i, kv_cache_group in enumerate(kv_cache_config.kv_cache_groups): | ||
| if isinstance(kv_cache_group.kv_cache_spec, EncoderOnlyAttentionSpec): | ||
| continue | ||
| max_num_blocks_per_req = cdiv(max_model_len, block_sizes[i] * get_total_cp_world_size()) | ||
| if isinstance(kv_cache_group.kv_cache_spec, MambaSpec): | ||
| mamba_blocks_per_req = ( | ||
| max_num_blocks_per_req if self.cache_config.enable_prefix_caching else 1 | ||
| ) + kv_cache_group.kv_cache_spec.num_speculative_blocks | ||
| max_num_blocks_per_req = max(max_num_blocks_per_req, mamba_blocks_per_req) | ||
| max_num_blocks.append(max_num_blocks_per_req) |
There was a problem hiding this comment.
There is a potential IndexError or logic error here. block_sizes (defined at line 812) is a filtered list that excludes EncoderOnlyAttentionSpec groups. However, the loop at line 848 iterates over all kv_cache_config.kv_cache_groups using enumerate. If any group is an EncoderOnlyAttentionSpec, the index i will no longer align with the indices in the block_sizes list.
For example, if the first group is EncoderOnlyAttentionSpec, block_sizes[0] would refer to the second group's block size, but the loop would skip i=0 and then try to access block_sizes[1] for the second group, which might be out of bounds or incorrect.
max_num_blocks = []
max_model_len = max(self.max_model_len, self.max_encoder_len)
filtered_groups = [
g for g in kv_cache_config.kv_cache_groups
if not isinstance(g.kv_cache_spec, EncoderOnlyAttentionSpec)
]
for block_size, kv_cache_group in zip(block_sizes, filtered_groups):
max_num_blocks_per_req = cdiv(max_model_len, block_size * get_total_cp_world_size())
if isinstance(kv_cache_group.kv_cache_spec, MambaSpec):
mamba_blocks_per_req = (
max_num_blocks_per_req
if self.cache_config.enable_prefix_caching else 1
) + kv_cache_group.kv_cache_spec.num_speculative_blocks
max_num_blocks_per_req = max(max_num_blocks_per_req, mamba_blocks_per_req)
max_num_blocks.append(max_num_blocks_per_req)| if kv_cache_groups is not None: | ||
| self.block_tables = [ | ||
| BlockTable( | ||
| block_size, | ||
| max_num_reqs, | ||
| max_num_blocks_per_req, | ||
| max_num_batched_tokens, | ||
| pin_memory, | ||
| device, | ||
| kernel_size_list, | ||
| cp_kv_cache_interleave_size, | ||
| num_speculative_tokens, | ||
| kv_cache_group, | ||
| ) | ||
| for block_size, kernel_size_list, max_num_blocks_per_req, kv_cache_group in zip( | ||
| block_sizes, kernel_sizes, max_num_blocks, kv_cache_groups | ||
| ) | ||
| ] | ||
| else: | ||
| self.block_tables = [ | ||
| BlockTable( | ||
| block_size, | ||
| max_num_reqs, | ||
| max_num_blocks_per_req, | ||
| max_num_batched_tokens, | ||
| pin_memory, | ||
| device, | ||
| kernel_size_list, | ||
| cp_kv_cache_interleave_size, | ||
| num_speculative_tokens, | ||
| ) | ||
| for block_size, kernel_size_list, max_num_blocks_per_req in zip( | ||
| block_sizes, kernel_sizes, max_num_blocks | ||
| ) | ||
| ] |
There was a problem hiding this comment.
Suggested PR Title:
[CI][BugFix] Fix CI error for 310p caused by DSV4Suggested PR Summary:
### What this PR does / why we need it?
This PR fixes CI errors on Ascend 310P platforms by updating the `MultiGroupBlockTable` and `NPUInputBatch` to correctly handle multiple KV cache groups and compressed slot mapping inputs. It ensures that `kv_cache_groups` are properly propagated and adds support for per-group compressed positions and request indices in the 310P-specific slot mapping implementation.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Unit tests were added in `tests/ut/_310p/test_block_table_310p.py` covering multi-group slot mapping with standard and compressed inputs.Feedback
The initialization logic for self.block_tables is duplicated. Since the BlockTable constructor handles kv_cache_group=None by default, you can simplify this by ensuring kv_cache_groups is a list of the correct length (padded with None if necessary) and using a single list comprehension. This improves maintainability and reduces code duplication.
kv_cache_groups = kv_cache_groups or [None] * len(block_sizes)
self.block_tables = [
BlockTable(
block_size,
max_num_reqs,
max_num_blocks_per_req,
max_num_batched_tokens,
pin_memory,
device,
kernel_size_list,
cp_kv_cache_interleave_size,
num_speculative_tokens,
kv_cache_group,
)
for block_size, kernel_size_list, max_num_blocks_per_req, kv_cache_group in zip(
block_sizes, kernel_sizes, max_num_blocks, kv_cache_groups
)
]References
- The repository style guide requires PR reviews to include a suggested title and summary in markdown code blocks. (link)
…#9402) ### What this PR does / why we need it? Fix 310P online CI issue caused by an extra argument added to blocktable.py in DeepSeek v4. ### Does this PR introduce _any_ user-facing change? NA ### How was this patch tested? CI - vLLM version: v0.20.2 - vLLM main: vllm-project/vllm@0d4d334 --------- Signed-off-by: Tflowers-0129 <2906339855@qq.com> Signed-off-by: JunhaoWang <1594260677@qq.com>
…#9402) ### What this PR does / why we need it? Fix 310P online CI issue caused by an extra argument added to blocktable.py in DeepSeek v4. ### Does this PR introduce _any_ user-facing change? NA ### How was this patch tested? CI - vLLM version: v0.20.2 - vLLM main: vllm-project/vllm@0d4d334 --------- Signed-off-by: Tflowers-0129 <2906339855@qq.com>
…#9402) ### What this PR does / why we need it? Fix 310P online CI issue caused by an extra argument added to blocktable.py in DeepSeek v4. ### Does this PR introduce _any_ user-facing change? NA ### How was this patch tested? CI - vLLM version: v0.20.2 - vLLM main: vllm-project/vllm@0d4d334 --------- Signed-off-by: Tflowers-0129 <2906339855@qq.com>
…#9402) ### What this PR does / why we need it? Fix 310P online CI issue caused by an extra argument added to blocktable.py in DeepSeek v4. ### Does this PR introduce _any_ user-facing change? NA ### How was this patch tested? CI - vLLM version: v0.20.2 - vLLM main: vllm-project/vllm@0d4d334 --------- Signed-off-by: Tflowers-0129 <2906339855@qq.com>
…#9402) ### What this PR does / why we need it? Fix 310P online CI issue caused by an extra argument added to blocktable.py in DeepSeek v4. ### Does this PR introduce _any_ user-facing change? NA ### How was this patch tested? CI - vLLM version: v0.20.2 - vLLM main: vllm-project/vllm@0d4d334 --------- Signed-off-by: Tflowers-0129 <2906339855@qq.com>
…#9402) ### What this PR does / why we need it? Fix 310P online CI issue caused by an extra argument added to blocktable.py in DeepSeek v4. ### Does this PR introduce _any_ user-facing change? NA ### How was this patch tested? CI - vLLM version: v0.20.2 - vLLM main: vllm-project/vllm@0d4d334 --------- Signed-off-by: Tflowers-0129 <2906339855@qq.com>
…#9402) ### What this PR does / why we need it? Fix 310P online CI issue caused by an extra argument added to blocktable.py in DeepSeek v4. ### Does this PR introduce _any_ user-facing change? NA ### How was this patch tested? CI - vLLM version: v0.20.2 - vLLM main: vllm-project/vllm@0d4d334 --------- Signed-off-by: Tflowers-0129 <2906339855@qq.com>
What this PR does / why we need it?
Fix 310P online CI issue caused by an extra argument added to blocktable.py in DeepSeek v4.
Does this PR introduce any user-facing change?
NA
How was this patch tested?
CI