[feat]: Create interface for model-specific M-RoPE #24194

AzizCode92 · 2025-09-03T19:44:44Z

Purpose

Introduces a support_mrope interface on the model class to delegate M-RoPE implementation details to each model, rather than using a centralized helper.

The Qwen2-VL model is updated to use this new interface as the first implementation.

Related to: #24165

#24172 also is working on the same issue. Please feel free to close mine if the prior PR does the job.

Test Plan

Test Result

Essential Elements of an Effective PR Description Checklist

The purpose of the PR, such as "Fix some issue (link existing issues this PR will resolve)".
The test plan, such as providing test command.
The test results, such as pasting the results comparison before and after, or e2e results
(Optional) The necessary documentation update, such as updating supported_models.md and examples for a new model.
(Optional) Release notes update. If your change is user facing, please update the release notes draft in the Google Doc.

Signed-off-by: AzizCode92 <azizbenothman76@gmail.com>

gemini-code-assist

Code Review

This pull request introduces a new SupportsMRoPE interface to delegate model-specific M-RoPE logic, which is a positive step towards improving modularity. My review focuses on ensuring the new interface is correctly defined and implemented, following existing patterns in the codebase. I've identified some issues in the interface definition in interfaces.py regarding consistency and correctness. Additionally, I've suggested an improvement to the Qwen2VLForConditionalGeneration implementation to make it more self-contained and robust, which aligns better with the goals of this PR.

vllm/model_executor/models/interfaces.py

vllm/model_executor/models/qwen2_vl.py

pratapyash · 2025-09-03T19:57:05Z

Can we also expect an update to Qwen-2.5-Omni to use this new interface as well?

Currently, it emits this warning at model init:

Unrecognized keys in `rope_scaling` for 'rope_type'='default': {'mrope_section'}

Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Signed-off-by: Aziz <azizbenothman76@gmail.com>

Signed-off-by: AzizCode92 <azizbenothman76@gmail.com>

AzizCode92 · 2025-09-03T20:05:13Z

Can we also expect an update to Qwen-2.5-Omni to use this new interface as well?

Currently, it emits this warning at model init:
Unrecognized keys in `rope_scaling` for 'rope_type'='default': {'mrope_section'}

We will incrementally adjust all other models to adopt the new interface design. This PR focuses as first step on adapting qwen2_vl. Once approved we can move to the other models.

wwl2755 · 2025-09-03T21:01:11Z

Is qwen2-vl using the shared m-rope as mentioned in #24165 ? Because it seems to have already built its own m-rope in qwen2-vl.py

vllm/vllm/model_executor/models/qwen2_vl.py

Line 507 in a43a3f1

class Qwen2VisionRotaryEmbedding(nn.Module):

AzizCode92 · 2025-09-03T21:12:41Z

Is qwen2-vl using the shared m-rope as mentioned in #24165 ? Because it seems to have already built its own m-rope in qwen2-vl.py

vllm/vllm/model_executor/models/qwen2_vl.py

Line 507 in a43a3f1

class Qwen2VisionRotaryEmbedding(nn.Module):

Good question, I think in vllm/model_executor/layers/rotary_embedding/mrope.py the _vl_get_input_positions_tensor referes to Qwen2-VL/Qwen2.5-VL.

cc @Isotr0py

DarkLight1337 · 2025-09-04T03:58:25Z

vllm/model_executor/models/interfaces.py

+        MRO of your model class.
+    """
+
+    @classmethod


Can we make this an instance method? Also the model runner should actually make use of this method instead of calling the one in mrope.py if available

Signed-off-by: AzizCode92 <azizbenothman76@gmail.com>

vllm/worker/model_runner.py

Isotr0py · 2025-09-04T07:03:49Z

Good question, I think in vllm/model_executor/layers/rotary_embedding/mrope.py the _vl_get_input_positions_tensor referes to Qwen2-VL/Qwen2.5-VL.

The Qwen2VisionRotaryEmbedding is only used in ViT, which is different from the MRotaryEmbedding used in Qwen2-VL's backbone.

divyanshsinghvi · 2025-09-04T07:51:03Z

Seems a duplicate of #24172 ? @AzizCode92

Signed-off-by: AzizCode92 <azizbenothman76@gmail.com>

wwl2755 · 2025-09-04T22:39:53Z

Good question, I think in vllm/model_executor/layers/rotary_embedding/mrope.py the _vl_get_input_positions_tensor referes to Qwen2-VL/Qwen2.5-VL.

The Qwen2VisionRotaryEmbedding is only used in ViT, which is different from the MRotaryEmbedding used in Qwen2-VL's backbone.

Oh, yeah. I missed that. You're right. Thanks for pointing out.

wwl2755 · 2025-09-04T22:41:32Z

vllm/v1/worker/gpu_model_runner.py

-                    context_len=num_computed_tokens + prompt_part_len,
-                    num_new_tokens=completion_part_len,
-                )
+                if supports_mrope(self.model):


Is this neccesary? The original logic is to call get_next_input_positions_tensor()

Yes. The original logic was using using get_input_positions_tensor from vllm.model_executor.layers.rotary_embedding and now we are switching to the get_mrope_input_positions defined in the new interface. To check if the model supports mrope, we have defined supports_mrope.
See here: https://github.com/AzizCode92/vllm/blob/8df4a1091bb465e4a3058ba9a8a10cb168d27d82/vllm/model_executor/models/interfaces.py#L872-L923

Uh, I mean, here it is using get_next_input_positions_tensor() instead of get_input_positions_tensor(). They are different logic I think.

If you want to refactor this as well, you may need another get_next_mrope_input_positions() in your interface.

Great catch! That was definitely unnecessary to change.

vllm/worker/model_runner.py

Signed-off-by: AzizCode92 <azizbenothman76@gmail.com>

DarkLight1337 · 2025-09-17T13:08:13Z

Sorry for the delay, let's merge this one first, then #24172 can be modified to update the rest of the models.

DarkLight1337 · 2025-09-18T04:10:43Z

PTAL at the test failure in multimodal tests

Signed-off-by: AzizCode92 <azizbenothman76@gmail.com>

AzizCode92 · 2025-09-18T16:57:42Z

@DarkLight1337 multimodal tests are green now.
Let me know your feedback if anything needs to be adjusted. Thanks!

DarkLight1337 · 2025-09-18T17:00:06Z

Merging with main to fix the unrelated CI failures

Signed-off-by: AzizCode92 <azizbenothman76@gmail.com> Signed-off-by: Aziz <azizbenothman76@gmail.com> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Co-authored-by: Cyrus Leung <tlleungac@connect.ust.hk>

Signed-off-by: AzizCode92 <azizbenothman76@gmail.com> Signed-off-by: Aziz <azizbenothman76@gmail.com> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Co-authored-by: Cyrus Leung <tlleungac@connect.ust.hk> Signed-off-by: charlifu <charlifu@amd.com>

Signed-off-by: AzizCode92 <azizbenothman76@gmail.com> Signed-off-by: Aziz <azizbenothman76@gmail.com> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Co-authored-by: Cyrus Leung <tlleungac@connect.ust.hk> Signed-off-by: xuebwang-amd <xuebwang@amd.com>

Signed-off-by: AzizCode92 <azizbenothman76@gmail.com> Signed-off-by: Aziz <azizbenothman76@gmail.com> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Co-authored-by: Cyrus Leung <tlleungac@connect.ust.hk>

Signed-off-by: AzizCode92 <azizbenothman76@gmail.com> Signed-off-by: Aziz <azizbenothman76@gmail.com> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Co-authored-by: Cyrus Leung <tlleungac@connect.ust.hk> Signed-off-by: xuebwang-amd <xuebwang@amd.com>

[feat]: Create interface for model-specific M-RoPE

3dbf104

Signed-off-by: AzizCode92 <azizbenothman76@gmail.com>

AzizCode92 requested a review from sighingnow as a code owner September 3, 2025 19:44

mergify bot added the qwen Related to Qwen models label Sep 3, 2025

gemini-code-assist bot reviewed Sep 3, 2025

View reviewed changes

vllm/model_executor/models/interfaces.py Outdated Show resolved Hide resolved

vllm/model_executor/models/interfaces.py Outdated Show resolved Hide resolved

vllm/model_executor/models/qwen2_vl.py Outdated Show resolved Hide resolved

AzizCode92 closed this Sep 3, 2025

AzizCode92 reopened this Sep 3, 2025

AzizCode92 and others added 3 commits September 3, 2025 22:00

Update vllm/model_executor/models/interfaces.py

b177802

Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Signed-off-by: Aziz <azizbenothman76@gmail.com>

Update vllm/model_executor/models/interfaces.py

55281ce

Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Signed-off-by: Aziz <azizbenothman76@gmail.com>

[fix]: better design implementation for qwen_2_vl

44b784a

Signed-off-by: AzizCode92 <azizbenothman76@gmail.com>

DarkLight1337 reviewed Sep 4, 2025

View reviewed changes

fix: incorporate PR feedback

1fe3d3e

Signed-off-by: AzizCode92 <azizbenothman76@gmail.com>

DarkLight1337 reviewed Sep 4, 2025

View reviewed changes

vllm/worker/model_runner.py Outdated Show resolved Hide resolved

[fix]: refactor usage of mrope in v1 gpu model worker

4825d96

Signed-off-by: AzizCode92 <azizbenothman76@gmail.com>

AzizCode92 requested review from WoosukKwon, alexm-redhat, comaniac, njhill, robertgshaw2-redhat and ywang96 as code owners September 4, 2025 07:59

mergify bot added the v1 label Sep 4, 2025

ywang96 self-assigned this Sep 4, 2025

[fix]: pass proper method name for mrope

8df4a10

Signed-off-by: AzizCode92 <azizbenothman76@gmail.com>

AzizCode92 mentioned this pull request Sep 4, 2025

[Refactor]: Let each modeling file define M-RoPE implementation #24165

Closed

1 task

wwl2755 reviewed Sep 4, 2025

View reviewed changes

AzizCode92 added 3 commits September 5, 2025 23:04

[fix]: revert the usage of get_next_input_positions_tensor

b395878

Signed-off-by: AzizCode92 <azizbenothman76@gmail.com>

[fix]: make get_mrope_input_positions an instance method

24a7546

Signed-off-by: AzizCode92 <azizbenothman76@gmail.com>

[fix]: correct return type of get_mrope_input_positions

6018a58

Signed-off-by: AzizCode92 <azizbenothman76@gmail.com>

DarkLight1337 approved these changes Sep 17, 2025

View reviewed changes

Merge branch 'main' into refactor-mrope

6aa4687

DarkLight1337 enabled auto-merge (squash) September 17, 2025 13:12

github-actions bot added the ready ONLY add when PR is ready to merge/full CI is needed label Sep 17, 2025

fix: add hf_config to get_mrope_input_positions

9460218

Signed-off-by: AzizCode92 <azizbenothman76@gmail.com>

auto-merge was automatically disabled September 18, 2025 07:31
Head branch was pushed to by a user without write access

fix: failing tests

97361a3

Signed-off-by: AzizCode92 <azizbenothman76@gmail.com>

Merge branch 'main' into refactor-mrope

ffc2dc9

DarkLight1337 enabled auto-merge (squash) September 18, 2025 16:59

DarkLight1337 merged commit 38db529 into vllm-project:main Sep 18, 2025
49 checks passed

DarkLight1337 mentioned this pull request Oct 6, 2025

[Refactor]: Use M-RoPE interface directly while defining model class instead of maintaining model specific M-RoPE implementation in mrope.py #24172

Merged

5 tasks

wwl2755 mentioned this pull request Oct 9, 2025

[MM][BugFix] Fix checking condition on SupportsMRoPE #26531

Closed

Uh oh!

[feat]: Create interface for model-specific M-RoPE #24194

[feat]: Create interface for model-specific M-RoPE #24194

Uh oh!

Conversation

AzizCode92 commented Sep 3, 2025 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose

Test Plan

Test Result

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pratapyash commented Sep 3, 2025

Uh oh!

AzizCode92 commented Sep 3, 2025

Uh oh!

wwl2755 commented Sep 3, 2025

Uh oh!

AzizCode92 commented Sep 3, 2025

Uh oh!

DarkLight1337 Sep 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AzizCode92 Sep 4, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Isotr0py commented Sep 4, 2025

Uh oh!

divyanshsinghvi commented Sep 4, 2025

Uh oh!

wwl2755 commented Sep 4, 2025

Uh oh!

wwl2755 Sep 4, 2025

Choose a reason for hiding this comment

Uh oh!

AzizCode92 Sep 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wwl2755 Sep 5, 2025

Choose a reason for hiding this comment

Uh oh!

AzizCode92 Sep 5, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

DarkLight1337 commented Sep 17, 2025

Uh oh!

DarkLight1337 commented Sep 18, 2025

Uh oh!

AzizCode92 commented Sep 18, 2025

Uh oh!

DarkLight1337 commented Sep 18, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

AzizCode92 commented Sep 3, 2025 •

edited by github-actions bot

Loading

DarkLight1337 Sep 4, 2025 •

edited

Loading

AzizCode92 Sep 5, 2025 •

edited

Loading