Pcp dcp refactor #5001
Pcp dcp refactor #5001
Conversation
Signed-off-by: zhenwenqi2024 <[email protected]>
Signed-off-by: zhenwenqi2024 <[email protected]>
Signed-off-by: zhenwenqi2024 <[email protected]>
Signed-off-by: zhenwenqi2024 <[email protected]>
Signed-off-by: zhenwenqi2024 <[email protected]>
Signed-off-by: zhenwenqi2024 <[email protected]>
Signed-off-by: zhenwenqi2024 <[email protected]>
Signed-off-by: zhenwenqi2024 <[email protected]>
Signed-off-by: zhenwenqi2024 <[email protected]>
Signed-off-by: zhenwenqi2024 <[email protected]>
|
👋 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. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the Prefill Context Parallelism (PCP) and Decode Context Parallelism (DCP) logic by moving it from NPUModelRunner into a new PCPManager class. While this is a good structural improvement, the refactoring has introduced several critical bugs, including typos, incorrect method calls with missing or wrong arguments, and usage of undefined attributes. These issues will lead to runtime errors and must be addressed.
| self.dcp_rank = 0 | ||
| self.pcp_size = 1 | ||
| self.pcp_rank = 0 | ||
| max_buffer_num_tokens = self.max_num_token |
There was a problem hiding this comment.
There's a typo in the attribute name. self.max_num_token should be self.max_num_tokens. This will cause an AttributeError at runtime as max_num_token is not defined.
| max_buffer_num_tokens = self.max_num_token | |
| max_buffer_num_tokens = self.max_num_tokens |
| self.pcp_manager = PCPManager( | ||
| self.pcp_size, | ||
| self.pcp_rank, | ||
| self.dcp_size, | ||
| self.dcp_rank, | ||
| max_buffer_num_tokens, | ||
| self.max_num_reqs, | ||
| self.device, | ||
| self.pin_memory, | ||
| ) |
There was a problem hiding this comment.
The constructor for PCPManager is called with incorrect arguments. The arguments decode_threshold and vllm_config are missing, which will lead to a TypeError at runtime.
self.pcp_manager = PCPManager(
self.pcp_size,
self.pcp_rank,
self.dcp_size,
self.dcp_rank,
max_buffer_num_tokens,
self.max_num_reqs,
self.decode_threshold,
self.device,
self.vllm_config,
self.pin_memory,
)|
|
||
| total_num_pcp_pads = sum(self.num_pcp_pads) | ||
| max_num_scheduled_tokens = max(tokens) | ||
| if self.pcp__world_szie > 1: |
There was a problem hiding this comment.
There is a typo in the attribute name. self.pcp__world_szie should be self.pcp_size. This will cause an AttributeError at runtime.
if self.pcp_size > 1:| discard_request_indices = np.nonzero( | ||
| discard_requests_mask.np[:num_reqs])[0] |
There was a problem hiding this comment.
The get_discard_request_mask method returns a numpy array, which does not have a .np attribute. Accessing discard_requests_mask.np will raise an AttributeError. You should use the returned numpy array directly.
discard_request_indices = np.nonzero(discard_requests_mask)[0]| slot_mapping = self.pcp_manager.get_padded_slot_mapping( | ||
| num_tokens, | ||
| slot_mapping, | ||
| ) |
There was a problem hiding this comment.
The variable num_tokens is not defined in this scope, which will cause a NameError at runtime. Based on the context, it seems you intended to use total_num_scheduled_tokens.
slot_mapping = self.pcp_manager.get_padded_slot_mapping(
total_num_scheduled_tokens,
slot_mapping,
)
vllm_ascend/worker/pcp_utils.py
Outdated
| return dcp_local_seq_lens | ||
|
|
||
| def generate_kv_idx(self, scheduler_output, input_batch): | ||
| if not self.pcp_size > 1: |
There was a problem hiding this comment.
The attribute self.pcp_size is not defined in this class. This will cause an AttributeError. You probably meant to use self.pcp_world_size.
| if not self.pcp_size > 1: | |
| if not self.pcp_world_size > 1: |
vllm_ascend/worker/pcp_utils.py
Outdated
| def generate_kv_idx(self, scheduler_output, input_batch): | ||
| if not self.pcp_size > 1: | ||
| return | ||
| self.cp_kv_recover_idx_for_chunk = [[] for _ in range(self.pcp_size)] |
There was a problem hiding this comment.
The attribute self.pcp_size is not defined in this class. This will cause an AttributeError. You probably meant to use self.pcp_world_size.
| self.cp_kv_recover_idx_for_chunk = [[] for _ in range(self.pcp_size)] | |
| self.cp_kv_recover_idx_for_chunk = [[] for _ in range(self.pcp_world_size)] |
vllm_ascend/worker/pcp_utils.py
Outdated
| is_prefill = input_batch.num_computed_tokens_cpu[ | ||
| i] < self.input_batch.num_prompt_tokens[i] |
There was a problem hiding this comment.
The attribute self.input_batch is not defined in PCPManager. You should use the input_batch argument passed to the method.
| is_prefill = input_batch.num_computed_tokens_cpu[ | |
| i] < self.input_batch.num_prompt_tokens[i] | |
| is_prefill = input_batch.num_computed_tokens_cpu[ | |
| i] < input_batch.num_prompt_tokens[i] |
vllm_ascend/worker/pcp_utils.py
Outdated
| kv_req_offset = 0 | ||
| q_head_chunk_id = self.pcp_world_rank | ||
| q_tail_chunk_id = self.pcp_world_size * 2 - 1 - self.pcp_world_rank | ||
| for i, seq_len in enumerate(self.query_lens): |
There was a problem hiding this comment.
The attribute self.query_lens is not defined in PCPManager. It needs to be passed as an argument to this method.
vllm_ascend/worker/pcp_utils.py
Outdated
| tail_attn_nomask_seqlens = torch.tensor( | ||
| [chunk_seqlens, kv_with_q_tail_nomask_seqlens], | ||
| dtype=torch.int32) | ||
| pcp_prefill_mask = self.attn_mask |
There was a problem hiding this comment.
The attribute self.attn_mask is not defined in PCPManager. It needs to be passed as an argument to this method.
Signed-off-by: zhenwenqi2024 <[email protected]>
Signed-off-by: zhenwenqi2024 <[email protected]>
vllm_ascend/worker/pcp_utils.py
Outdated
| @@ -0,0 +1,592 @@ | |||
| # SPDX-License-Identifier: Apache-2.0 | |||
| # SPDX-FileCopyrightText: Copyright contributors to the vLLM project | |||
| from typing import List | |||
There was a problem hiding this comment.
不单独创建新的pcp_utils文件，采用和GPU的commn_utils方式，创建公共的工具文件
There was a problem hiding this comment.
ok,i will chanage it
Signed-off-by: zhenwenqi2024 <[email protected]>
|
Is it possible to refactor the logic of pcp_dcp in attention so that both pcp and non-pcp paths share as much common processing as possible, minimizing major branching differences? |
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
1 similar comment
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
3 participants
What this PR does / why we need it?
Does this PR introduce any user-facing change?
How was this patch tested?