Revert "[ROCm][AITER] Support AITER Rope ops in RotaryEmbedding Module." #22956
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request reverts the support for AITER Rope ops in the RotaryEmbedding Module. The changes mostly involve removing code related to this feature. The revert appears correct in its goal, but it introduces code duplication for the yarn_get_mscale function. This function is modified in common.py while its previous version is copied into deepseek_scaling_rope.py to maintain functionality. This duplication is a potential maintenance issue and should be addressed.
| def yarn_get_mscale(scale: float = 1, mscale: float = 1) -> float: | ||
| if scale <= 1: | ||
| return 1.0 | ||
| return 0.1 * mscale * math.log(scale) + 1.0 |
There was a problem hiding this comment.
This PR introduces a duplicate definition of yarn_get_mscale, which was previously defined in vllm/model_executor/layers/rotary_embedding/common.py. The version in common.py is being changed to remove the mscale parameter, while this file requires it.
Code duplication can lead to maintenance issues where one instance is updated but the other is missed. If this function is specific to Deepseek, it should be renamed to indicate its local scope (e.g., _yarn_get_mscale_for_deepseek).
A better approach would be to avoid the duplication altogether. The yarn_get_mscale function in common.py should probably retain the mscale parameter to support this use case. It would be cleaner to revert the changes in common.py to keep the version with mscale and continue importing it here, rather than duplicating the code.
github-actions
bot
added
the
ready
|
👋 Hi! Thank you for contributing to the vLLM project. 💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in #pr-reviews, coordinate on features in #feat- channels, or join special interest groups in #sig- channels. Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run Once the PR is approved and ready to go, your PR reviewer(s) can run CI to test the changes comprehensively before merging. To run CI, PR reviewers can either: Add 🚀 |
…e." (vllm-project#22956) Signed-off-by: vllmellm <[email protected]> Co-authored-by: vllmellm <[email protected]>
…e." (vllm-project#22956) Signed-off-by: vllmellm <[email protected]> Co-authored-by: vllmellm <[email protected]>
…e." (vllm-project#22956) Signed-off-by: vllmellm <[email protected]> Co-authored-by: vllmellm <[email protected]> Signed-off-by: Duncan Moss <[email protected]>
…e." (vllm-project#22956) Signed-off-by: vllmellm <[email protected]> Co-authored-by: vllmellm <[email protected]>
…e." (vllm-project#22956) Signed-off-by: vllmellm <[email protected]> Co-authored-by: vllmellm <[email protected]> Signed-off-by: Xiao Yu <[email protected]>
…e." (vllm-project#22956) Signed-off-by: vllmellm <[email protected]> Co-authored-by: vllmellm <[email protected]>
3 participants
Purpose
Revert "[ROCm][AITER] Support AITER Rope ops in RotaryEmbedding Module.".
Requires more investigation.
Test Plan
Test Result
(Optional) Documentation Update
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.