Skip to content

Conversation

@DarkLight1337
Copy link
Member

@DarkLight1337 DarkLight1337 commented Nov 10, 2025

Purpose

From my understanding, these arguments are only passed in V0, as the slicing is now handled by model runner inside V1, so we can remove this from modeling code.

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.

@DarkLight1337 DarkLight1337 added the ready ONLY add when PR is ready to merge/full CI is needed label Nov 10, 2025
@DarkLight1337 DarkLight1337 force-pushed the mrope-remove-context-len branch from cb698b7 to cf9ba38 Compare November 10, 2025 13:53
@mergify mergify bot added the qwen Related to Qwen models label Nov 10, 2025
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 the unused context_len and seq_len arguments from get_mrope_input_positions across multiple model files, which is a good cleanup as part of the V0 deprecation. The changes are mostly consistent and correct.

However, I've found two critical issues that need to be addressed:

  1. The SupportsMRoPE protocol in vllm/model_executor/models/interfaces.py still defines context_len and seq_len in its get_mrope_input_positions signature. This protocol should be updated to match the new function signatures in the implementing classes to avoid signature mismatches.
  2. In vllm/model_executor/models/qwen3_omni_moe_thinker.py, only the function signature was updated, but the usage of context_len and seq_len within the function body appears to have been missed. This will likely cause a NameError at runtime.

Please address these issues to ensure the correctness and consistency of the codebase.

Signed-off-by: DarkLight1337 <[email protected]>
@DarkLight1337
Copy link
Member Author

/gemini review

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 aims to remove the unused context_len and seq_len arguments from get_mrope_input_positions functions across multiple models, which is a good cleanup. The changes are mostly correct and consistent. However, I've found a critical issue in qwen3_omni_moe_thinker.py where removing seq_len from the function signature without updating its usage in the function body will lead to a runtime error. Please address this to ensure the code remains functional.

@Isotr0py Isotr0py merged commit d0e186c into vllm-project:main Nov 10, 2025
53 checks passed
@DarkLight1337 DarkLight1337 deleted the mrope-remove-context-len branch November 10, 2025 16:36
xuebwang-amd pushed a commit to xuebwang-amd/vllm that referenced this pull request Nov 13, 2025
devpatelio pushed a commit to SumanthRH/vllm that referenced this pull request Nov 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

qwen Related to Qwen models ready ONLY add when PR is ready to merge/full CI is needed

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants