Skip to content

Conversation

hust17yixuan
Copy link
Contributor

@hust17yixuan hust17yixuan commented Aug 27, 2025

What this PR does / why we need it?

After moved torchair related rope ops into torchair_ops, split the torchair from the origin rope ops to make the code clean.

Does this PR introduce any user-facing change?

No

How was this patch tested?

vLLM version: main
vLLM main: vllm-project/vllm@ab9f2cf

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 refactors the rotary embedding operations to remove torchair-specific logic, aiming for cleaner code. The changes correctly remove conditional blocks related to torchair from rope_forward_oot, AscendRotaryEmbedding.__init__, and AscendRotaryEmbedding.forward_oot. However, this refactoring has left some function parameters unused, which should be removed to complete the cleanup.

Comment on lines +126 to +127
return rope_forward_oot(self, positions, query, key, offsets,
is_neox_style_override,
is_qwen_torchair) # type: ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

high

As part of the torchair code removal, several parameters have become unused. To align with the PR's goal of cleaning up the code, it's best to remove these dead parameters to improve clarity and maintainability.

  • The parameter is_qwen_torchair is passed here but is no longer used in rope_forward_oot after the refactoring. It should be removed from this call and from the function definition of rope_forward_oot.
  • The parameters max_seq_len and is_prefill in the signature of forward_oot are also no longer used and should be removed.

Removing these unused parameters will make the code cleaner and prevent potential confusion for future developers.

        return rope_forward_oot(self, positions, query, key, offsets,
                                is_neox_style_override)  # type: ignore

@hust17yixuan hust17yixuan force-pushed the split_rope_ops branch 2 times, most recently from 74ff6ae to 65891d0 Compare August 28, 2025 01:06
@wangxiyuan wangxiyuan added the ready read for review label Aug 28, 2025
@hust17yixuan hust17yixuan force-pushed the split_rope_ops branch 2 times, most recently from 78077be to 82c7426 Compare August 30, 2025 07:09
Copy link

codecov bot commented Aug 30, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.06%. Comparing base (3a5fc5e) to head (4c2da15).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2581      +/-   ##
==========================================
+ Coverage   73.03%   73.06%   +0.03%     
==========================================
  Files         149      149              
  Lines       21515    21474      -41     
==========================================
- Hits        15714    15691      -23     
+ Misses       5801     5783      -18     
Flag Coverage Δ
unittests 73.06% <100.00%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@hust17yixuan hust17yixuan force-pushed the split_rope_ops branch 2 times, most recently from 4db44b2 to 0d50bcd Compare August 31, 2025 15:07
Signed-off-by: hust17yixuan <303660421@qq.com>
@wangxiyuan wangxiyuan merged commit ad13964 into vllm-project:main Sep 1, 2025
25 of 28 checks passed
wenba0 pushed a commit to wenba0/vllm-ascend that referenced this pull request Sep 5, 2025
### What this PR does / why we need it?
After moved torchair related rope ops into torchair_ops, split the
torchair from the origin rope ops to make the code clean.

### Does this PR introduce _any_ user-facing change?
No

### How was this patch tested?
vLLM version: main
vLLM main:
vllm-project/vllm@ab9f2cf

- vLLM version: v0.10.1.1
- vLLM main:
vllm-project/vllm@81eea3d

Signed-off-by: hust17yixuan <303660421@qq.com>
Signed-off-by: lijiaojiao <lijiaojiao990304@163.com>
offline893 pushed a commit to offline893/vllm-ascend that referenced this pull request Sep 16, 2025
### What this PR does / why we need it?
After moved torchair related rope ops into torchair_ops, split the
torchair from the origin rope ops to make the code clean.

### Does this PR introduce _any_ user-facing change?
No

### How was this patch tested?
vLLM version: main
vLLM main:
vllm-project/vllm@ab9f2cf

- vLLM version: v0.10.1.1
- vLLM main:
vllm-project/vllm@81eea3d

Signed-off-by: hust17yixuan <303660421@qq.com>
Signed-off-by: offline0806 <z00858301@china.huawei.com>
wangxiaoteng888 pushed a commit to LCAIZJ/vllm-ascend that referenced this pull request Sep 25, 2025
### What this PR does / why we need it?
After moved torchair related rope ops into torchair_ops, split the
torchair from the origin rope ops to make the code clean.

### Does this PR introduce _any_ user-facing change?
No

### How was this patch tested?
vLLM version: main
vLLM main:
vllm-project/vllm@ab9f2cf


- vLLM version: v0.10.1.1
- vLLM main:
vllm-project/vllm@81eea3d

Signed-off-by: hust17yixuan <303660421@qq.com>
chopper0126 pushed a commit to chopper0126/vllm-ascend that referenced this pull request Sep 26, 2025
### What this PR does / why we need it?
After moved torchair related rope ops into torchair_ops, split the
torchair from the origin rope ops to make the code clean.

### Does this PR introduce _any_ user-facing change?
No

### How was this patch tested?
vLLM version: main
vLLM main:
vllm-project/vllm@ab9f2cf


- vLLM version: v0.10.1.1
- vLLM main:
vllm-project/vllm@81eea3d

Signed-off-by: hust17yixuan <303660421@qq.com>
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.

2 participants