correct bug to fix the value of max_num_tokens #3933
Conversation
Signed-off-by: zouyida2052 <zouyida2002@gmail.com>
|
👋 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 fixes a critical bug in the calculation of max_num_tokens within the NPUTorchairModelRunner. The original code incorrectly used the tensor parallel size, which resulted in a wrong capacity calculation for mc2_tokens_capacity. This could lead to incorrect behavior or performance issues, especially for MoE models. The proposed change correctly computes max_num_tokens using the maximum number of requests and the decode query length, aligning with the intended logic and fixing the bug. The change is correct and necessary.
| # NOTE: To be clear, we need to make sure that during graph capture, the number of | ||
| # tokens is less than or equal to mc2_tokens_capacity. According to _set_cudagraph_sizes, | ||
| # the max number of tokens in graph is min(max_num_seqs * uniform_decode_query_len, 512). | ||
| max_num_tokens = self.parallel_config.tensor_parallel_size |
There was a problem hiding this comment.
The calculation for max_num_tokens was incorrect. It was set to the tensor parallel size, which is not related to the number of tokens. This leads to an incorrect mc2_tokens_capacity, which can cause issues with MoE communication strategy selection and other capacity-dependent logic. The fix correctly calculates it based on the maximum number of requests and the uniform decode query length.
| max_num_tokens = self.parallel_config.tensor_parallel_size | |
| max_num_tokens = self.max_num_reqs * self.uniform_decode_query_len |
wangxiyuan
added
ready
3 participants
What this PR does / why we need it?
correct bug to fix the value of max_num_tokens
Does this PR introduce any user-facing change?
no
How was this patch tested?