Skip to content

Conversation

@ltdo111
Copy link

@ltdo111 ltdo111 commented Nov 18, 2025

…use of the torch_npu.nup_mrope operator.

What this PR does / why we need it?

Does this PR introduce any user-facing change?

How was this patch tested?

@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 removes a conditional check in AscendMRotaryEmbedding.forward_oot to enforce the usage of the torch_npu.npu_mrope operator. My review focuses on improving code maintainability by suggesting the complete removal of the now-obsolete code block instead of commenting it out.

Comment on lines +408 to +409
# if self.mrope_section != [16, 24, 24]:
# return super().forward_oot(positions, query, key)
Copy link
Contributor

Choose a reason for hiding this comment

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

high

Instead of commenting out this conditional block, it should be removed entirely. Commented-out code, also known as dead code, can be confusing for future maintainers and adds clutter to the codebase.

@github-actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant