Skip to content

Conversation

@shaopeng-666
Copy link
Contributor

@shaopeng-666 shaopeng-666 commented Oct 23, 2025

What this PR does / why we need it?

Add mrope fusion op for qwen2.5-vl. This mrope operator dosen't support Qwen3-VL currently. Thus could only take affect in qwen2.5-vl

Does this PR introduce any user-facing change?

How was this patch tested?

Signed-off-by: shaopeng666 <[email protected]>
@github-actions
Copy link

👋 Hi! Thank you for contributing to the vLLM Ascend project. The following points will speed up your PR merge:‌‌

  • A PR should do only one thing, smaller PRs enable faster reviews.
  • Every PR should include unit tests and end-to-end tests ‌to ensure it works and is not broken by other future PRs.
  • Write the commit message by fulfilling the PR description to help reviewer and future developers understand.

If CI fails, you can run linting and testing checks locally according Contributing and Testing.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a 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 introduces the MRotaryEmbedding to the vllm-ascend project, along with corresponding tests. The MRotaryEmbedding is integrated into the rotary embedding operations and registered as a custom operator. The changes include modifications to tests/ut/ops/test_rotary_embedding.py, vllm_ascend/ops/rotary_embedding.py, and vllm_ascend/utils.py. I have identified a critical issue related to the instantiation of MRotaryEmbedding in the test suite, where the incorrect class is being used, potentially leading to incorrect behavior during testing.

Comment on lines +406 to +412
self.layer = MRotaryEmbedding(self.head_size,
self.head_size,
self.max_position_embeddings,
base=self.rope_theta,
is_neox_style=self.is_neox_style,
dtype=torch.bfloat16,
mrope_section=self.mrope_section)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

critical: The test case is instantiating MRotaryEmbedding from vllm.model_executor.layers.rotary_embedding instead of vllm_ascend.ops.rotary_embedding.AscendMRotaryEmbedding. This will lead to incorrect behavior during testing, as the test will not be using the Ascend-specific implementation.

To fix this, import AscendMRotaryEmbedding from vllm_ascend.ops.rotary_embedding and use that class to instantiate self.layer.

Suggested change
self.layer = MRotaryEmbedding(self.head_size,
self.head_size,
self.max_position_embeddings,
base=self.rope_theta,
is_neox_style=self.is_neox_style,
dtype=torch.bfloat16,
mrope_section=self.mrope_section)
from vllm_ascend.ops.rotary_embedding import AscendMRotaryEmbedding
self.layer = AscendMRotaryEmbedding(self.head_size,
self.head_size,
self.max_position_embeddings,
base=self.rope_theta,
is_neox_style=self.is_neox_style,
dtype=torch.bfloat16,
mrope_section=self.mrope_section)

@Potabk Potabk added ready read for review ready-for-test start test by label for PR labels Oct 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants