-
Notifications
You must be signed in to change notification settings - Fork 559
qwen3-vl Vit module enable sp and mrope fusion op #4165
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request adapts the Qwen3-VL large model for sequence parallelism (SP) on Ascend NPUs. It introduces new distributed utility functions for all-to-all communication and modifies the vision transformer components to incorporate SP logic, including tensor padding, sharding, and gathering. While the overall approach to implementing sequence parallelism is sound, I've identified critical bugs in the new all-to-all communication primitives. These bugs will cause incorrect tensor reshaping, leading to corrupted data and incorrect model outputs. These issues must be addressed for the SP implementation to function correctly.
| output = output.reshape(hc, shard_seqlen, bs, | ||
| hs).transpose(0, 2).contiguous() | ||
| return output.reshape(bs, shard_seqlen, hc, hs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reshape operation on the output tensor is incorrect. The tensor has a shape of (seq_world_size, shard_hc, shard_seqlen, bs, hs), and the reshape attempts to merge the first two dimensions (seq_world_size and shard_hc). However, these dimensions are not contiguous in memory after the preceding transpose operations. A transpose(0, 1) is required to make them adjacent before reshaping. Failure to do so will result in a tensor with scrambled data.
Additionally, the reshape in the return statement is redundant as the tensor already has the correct shape after the preceding operations.
| output = output.reshape(hc, shard_seqlen, bs, | |
| hs).transpose(0, 2).contiguous() | |
| return output.reshape(bs, shard_seqlen, hc, hs) | |
| output = output.transpose(0, 1).contiguous().reshape( | |
| hc, shard_seqlen, bs, hs).transpose(0, 2).contiguous() | |
| return output |
| output = output.reshape(hc, shard_seqlen, | ||
| hs).transpose(0, 1).contiguous() | ||
| return output |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the issue in all_to_all_4d, the reshape operation on the output tensor here is incorrect. The output tensor has a shape of (seq_world_size, shard_hc, shard_seqlen, hs), and reshape(hc, ...) incorrectly attempts to merge the non-contiguous first two dimensions. This will lead to data corruption. You need to transpose the first two dimensions to make them contiguous before reshaping.
| output = output.reshape(hc, shard_seqlen, | |
| hs).transpose(0, 1).contiguous() | |
| return output | |
| output = output.transpose(0, 1).contiguous().reshape( | |
| hc, shard_seqlen, hs).transpose(0, 1).contiguous() | |
| return output |
What this PR does / why we need it?
Enable Qwen3-VL vit sp parallel and mrope npu fusion op
Does this PR introduce any user-facing change?
No
How was this patch tested?
Test Qwen3-VL 30B model accuracy on textVQA with aisbench