Skip to content

Conversation

zucchini-nlp
Copy link
Contributor

@zucchini-nlp zucchini-nlp commented Oct 2, 2025

Fixes the recently skipped test by creating image_grid_thw as batched fields. Otherwise they get an extra dimension and fail when preparing mrope positions in vLLM's model runners

Also, adds/rewrites some comments in processor code to keep them up to date. I tried to follow the base class apply() method, but transformers unfortunately cannot split the logic for placeholder and the rest into two. It will force us to call transformer utilities twice which is not very fast. So I am keeping it as is and simply adding comments

cc @hmellor as we have been talking about it internally

Signed-off-by: raushan <[email protected]>
@mergify mergify bot added the multi-modality Related to multi-modality (#4194) label Oct 2, 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 fixes an mrope issue in the Transformers backend by correctly configuring image_grid_thw and video_grid_thw as batched fields. It also includes several refactorings and cleanups, such as using a public API for setting attention implementation and removing unused code. The changes are generally good, but I've identified a critical issue where unguarded dictionary access could lead to a KeyError when processing text-only inputs in a multimodal model. I've provided a suggestion to make the code more robust.

Signed-off-by: raushan <[email protected]>
Copy link

mergify bot commented Oct 3, 2025

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @zucchini-nlp.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@mergify mergify bot added the needs-rebase label Oct 3, 2025
zucchini-nlp and others added 2 commits October 3, 2025 11:09
@mergify mergify bot removed the needs-rebase label Oct 3, 2025
hmellor
hmellor previously requested changes Oct 3, 2025
@hmellor hmellor dismissed their stale review October 3, 2025 14:17

issue has been fixed

Copy link

mergify bot commented Oct 6, 2025

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @zucchini-nlp.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@mergify mergify bot added the needs-rebase label Oct 6, 2025
Copy link
Member

@hmellor hmellor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Let's see what CI thinks

@mergify mergify bot removed the needs-rebase label Oct 6, 2025
@hmellor hmellor enabled auto-merge (squash) October 6, 2025 09:54
@github-actions github-actions bot added the ready ONLY add when PR is ready to merge/full CI is needed label Oct 6, 2025
@hmellor hmellor merged commit ab5e7d9 into vllm-project:main Oct 6, 2025
55 checks passed
karan pushed a commit to karan/vllm that referenced this pull request Oct 6, 2025
Signed-off-by: raushan <[email protected]>
Signed-off-by: Harry Mellor <[email protected]>
Co-authored-by: Harry Mellor <[email protected]>
Signed-off-by: Karan Goel <[email protected]>
@hmellor hmellor moved this to Done in Transformers backend Oct 7, 2025
southfreebird pushed a commit to southfreebird/vllm that referenced this pull request Oct 7, 2025
Signed-off-by: raushan <[email protected]>
Signed-off-by: Harry Mellor <[email protected]>
Co-authored-by: Harry Mellor <[email protected]>
xuebwang-amd pushed a commit to xuebwang-amd/vllm that referenced this pull request Oct 10, 2025
Signed-off-by: raushan <[email protected]>
Signed-off-by: Harry Mellor <[email protected]>
Co-authored-by: Harry Mellor <[email protected]>
Signed-off-by: xuebwang-amd <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

multi-modality Related to multi-modality (#4194) 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