[Bugfix]:replace npu_incre_flash_attention with npu_fused_infer_atten… #2901
Conversation
|
👋 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 replaces npu_incre_flash_attention with npu_fused_infer_attention_score as a bugfix. The core change in the attention implementation appears correct. However, an associated end-to-end test has been weakened by the removal of assertions, which compromises its ability to catch regressions. I have provided a comment to restore a basic check to maintain test integrity.
| for i in range(len(vllm_output)): | ||
| print(f"Generated text: {vllm_output[i][1]!r}") |
There was a problem hiding this comment.
This test has been weakened by removing all assertions. It now only runs the model and prints the output, but does not verify anything about the output. If vllm_model.generate_greedy returns an empty list, this test will still pass. At a minimum, it should assert that the number of outputs matches the number of input prompts to ensure it's still a meaningful test.
| for i in range(len(vllm_output)): | |
| print(f"Generated text: {vllm_output[i][1]!r}") | |
| assert len(vllm_output) == len(example_prompts) | |
| for i in range(len(vllm_output)): | |
| print(f"Generated text: {vllm_output[i][1]!r}") |
panchao-hub
force-pushed
the
bugfix0912
branch
5 times, most recently
from
36be2b6 to
be3f6e5
Compare
|
Please describe why this replacement was made. |
The npu_incre_flash_attention interface cannot update the tiling. In addition, npu_incre_flash_attention will not be maintained in the future. |
| def stubbed_get_state(ep_size, with_prefill, is_deepseek_v3_r1): | ||
| return _get_fused_moe_state(16, with_prefill, is_deepseek_v3_r1) | ||
|
|
||
| with patch("vllm_ascend.ascend_forward_context._get_fused_moe_state", |
There was a problem hiding this comment.
why using patch in e2e test, it's not corrct IMO.
There was a problem hiding this comment.
torchair graph only support mc2, and mc2 can be enabled only when the ep is greater than or equal to 16.
panchao-hub
force-pushed
the
bugfix0912
branch
from
be3f6e5 to
c95366c
Compare
…tion_score Signed-off-by: p00465316 <panchao13@huawei.com>
wangxiyuan
added
ready
What this PR does / why we need it?
[Bugfix]:replace npu_incre_flash_attention with npu_fused_infer_attention_score in order to be able to tiling update
Does this PR introduce any user-facing change?
No
How was this patch tested?