-
Notifications
You must be signed in to change notification settings - Fork 461
[6/N][refactor]delete torchair in rotary ops #2581
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
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 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.
return rope_forward_oot(self, positions, query, key, offsets, | ||
is_neox_style_override, | ||
is_qwen_torchair) # type: ignore |
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.
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 inrope_forward_oot
after the refactoring. It should be removed from this call and from the function definition ofrope_forward_oot
. - The parameters
max_seq_len
andis_prefill
in the signature offorward_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
74ff6ae
to
65891d0
Compare
65891d0
to
372020d
Compare
78077be
to
82c7426
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. 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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
4db44b2
to
0d50bcd
Compare
Signed-off-by: hust17yixuan <303660421@qq.com>
0d50bcd
to
4c2da15
Compare
### 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>
### 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>
### 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>
### 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>
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