[0.11.0][ops] npu_top_k_top_p supports k and p only #4153
Conversation
Signed-off-by: linfeng-yuan <[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 aims to extend the usage of the npu_top_k_top_p kernel to handle cases where only top_k or only top_p is specified, which is an improvement over the previous implementation that required both. However, the changes introduce a critical contradiction between a code comment and the logic. The code allows k to be None when calling the NPU kernel, while the comment explicitly states k must be within a specific range, implying it cannot be None. This could lead to a runtime error. My review highlights this critical issue and provides a suggested fix.
| # npu_top_k_top_p uses the operator aclnnApplyTopKTopP, but aclnnApplyTopKTopP currently does not support 310P | ||
| if not is_310p(): | ||
| # npu_top_k_top_p requires parameter k ranged from 1 to 1024 | ||
| if k is None or 1 <= int(k.max()) <= 1024: |
There was a problem hiding this comment.
There is a contradiction between the comment on the preceding line and this condition. The comment states that npu_top_k_top_p requires k to be in the range [1, 1024], which implies k cannot be None. However, this condition allows k to be None, which would then be passed to torch_npu.npu_top_k_top_p.
If npu_top_k_top_p does not support k=None, this will cause a runtime error. Assuming the comment is correct, the condition should be changed to ensure k is not None.
If npu_top_k_top_p does support k=None (e.g., for a top-p only operation), then the code is correct, but the comment on line 31 is dangerously misleading and should be updated to reflect that k can be None.
| if k is None or 1 <= int(k.max()) <= 1024: | |
| if k is not None and 1 <= int(k.max()) <= 1024: |
1 participant
What this PR does / why we need it?
Does this PR introduce any user-facing change?
How was this patch tested?