Skip to content

[Bugfix] Make mrope kwargs optional in HunyuanImage3 get_mrope_input_positions#2654

Merged
Gaohan123 merged 3 commits into
vllm-project:mainfrom
ianliuy:fix/hunyuanimage3-mrope-params
Apr 14, 2026
Merged

[Bugfix] Make mrope kwargs optional in HunyuanImage3 get_mrope_input_positions#2654
Gaohan123 merged 3 commits into
vllm-project:mainfrom
ianliuy:fix/hunyuanimage3-mrope-params

Conversation

@ianliuy
Copy link
Copy Markdown
Contributor

@ianliuy ianliuy commented Apr 9, 2026

Purpose

Fix #2642 HunyuanImage3ForConditionalGeneration.get_mrope_input_positions() crashes with TypeError: missing 3 required keyword-only arguments: 'hf_config', 'image_grid_thw', and 'video_grid_thw' when called from the upstream vllm caller.

Root cause

The SupportsMRoPE protocol defines get_mrope_input_positions(self, input_tokens, mm_features) only 2 positional args. But HunyuanImage3's implementation declares hf_config, image_grid_thw, and video_grid_thw as required keyword-only parameters (after *, no defaults). The upstream vllm caller follows the protocol and only passes 2 args crash.

Key finding

These 3 kwargs are never referenced in the ~180-line method body. The method extracts all needed data from self.config and mm_features internally (e.g., vae_token_grid_hw and vit_spatial_shapes from mm_feature.data). They were likely carried over from an older Qwen2-VL style signature.

Fix

Give the 3 unused kwargs None defaults so both callers work:

  • Upstream vllm (passes 2 positional args) now works
  • vllm-omni override (passes all kwargs explicitly) still works
- hf_config: PretrainedConfig,
- image_grid_thw: list[list[int]] | torch.Tensor,
- video_grid_thw: list[list[int]] | torch.Tensor,
+ hf_config: PretrainedConfig | None = None,
+ image_grid_thw: list[list[int]] | torch.Tensor | None = None,
+ video_grid_thw: list[list[int]] | torch.Tensor | None = None,

Test Plan

Signature-only change no new logic paths introduced. The 3 kwargs are confirmed unused in the method body (verified by reading all ~180 lines). Both callers (upstream 2-arg and vllm-omni full-kwargs) continue to work correctly.

Test Result

Before: TypeError: get_mrope_input_positions() missing 3 required keyword-only arguments
After: Method accepts calls with or without these kwargs.


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. Please provide the test scripts & test commands. Please state the reasons if your codes don't require additional test scripts.
  • The test results. Please paste the results comparison before and after, or the e2e results.
  • (Optional) The necessary documentation update, such as updating supported_models.md and examples for a new model.
  • (Optional) Release notes update.

…positions (vllm-project#2642)

HunyuanImage3ForConditionalGeneration.get_mrope_input_positions() declared
hf_config, image_grid_thw, and video_grid_thw as required keyword-only
parameters, but the upstream vllm caller only passes 2 positional args per
the SupportsMRoPE protocol contract, causing a TypeError crash.

These 3 kwargs are never referenced in the ~180-line method body  the
method extracts all needed data from self.config and mm_features internally.
Give them None defaults so both callers (upstream 2-arg and vllm-omni
full-kwargs) work correctly.

Fixes vllm-project#2642

Signed-off-by: Yiyang Liu <yiyangliu@microsoft.com>
@ianliuy ianliuy requested a review from hsliuustc0106 as a code owner April 9, 2026 18:03
@chatgpt-codex-connector
Copy link
Copy Markdown

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

@ianliuy
Copy link
Copy Markdown
Contributor Author

ianliuy commented Apr 12, 2026

@hsliuustc0106 @lishunyang12 The readthedocs check passed on the original commit (65f076f) but failed on the merge commit (eba1ebc) with a build timeout (15m limit). Since this PR only modifies a single Python file and no documentation, the timeout was likely introduced by recent changes in main that were pulled in via "Update branch". Could we re-trigger the readthedocs build, or bypass it for merge?

@ianliuy
Copy link
Copy Markdown
Contributor Author

ianliuy commented Apr 14, 2026

@hsliuustc0106 @lishunyang12 Gentle ping the readthedocs timeout on the merge commit appears to be a repo-wide issue (not specific to this PR). All other checks pass. Could you take a look when convenient?

@Gaohan123 Gaohan123 added the ready label to trigger buildkite CI label Apr 14, 2026
@Gaohan123 Gaohan123 added this to the v0.20.0 milestone Apr 14, 2026
@Gaohan123 Gaohan123 enabled auto-merge (squash) April 14, 2026 14:13
@Gaohan123 Gaohan123 merged commit bc4a659 into vllm-project:main Apr 14, 2026
6 of 8 checks passed
y123456y78 pushed a commit to y123456y78/vllm-omni that referenced this pull request Apr 15, 2026
…positions (vllm-project#2654)

Signed-off-by: Yiyang Liu <yiyangliu@microsoft.com>
Co-authored-by: SYLAR <125541396+lishunyang12@users.noreply.github.com>
lvliang-intel pushed a commit to lvliang-intel/vllm-omni that referenced this pull request Apr 20, 2026
…positions (vllm-project#2654)

Signed-off-by: Yiyang Liu <yiyangliu@microsoft.com>
Co-authored-by: SYLAR <125541396+lishunyang12@users.noreply.github.com>
lengrongfu pushed a commit to lengrongfu/vllm-omni that referenced this pull request May 1, 2026
…positions (vllm-project#2654)

Signed-off-by: Yiyang Liu <yiyangliu@microsoft.com>
Co-authored-by: SYLAR <125541396+lishunyang12@users.noreply.github.com>
clodaghwalsh17 pushed a commit to clodaghwalsh17/nm-vllm-omni-ent that referenced this pull request May 12, 2026
…positions (vllm-project#2654)

Signed-off-by: Yiyang Liu <yiyangliu@microsoft.com>
Co-authored-by: SYLAR <125541396+lishunyang12@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready label to trigger buildkite CI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: In vllm-omni v0.18.0, when I run the HunyuanImage-3.0-Instruct model, the mrope parameter is missing.

3 participants