-
-
Notifications
You must be signed in to change notification settings - Fork 12k
[Bugfix] Fix qwen3 vl dummy data generation with overrides #26193
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Signed-off-by: Roger Wang <[email protected]>
Signed-off-by: Roger Wang <[email protected]>
There was a problem hiding this 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 a regression in dummy data generation for Qwen3-VL models with overrides. The changes correctly apply video overrides for number of frames, width, and height. My review focuses on improving code maintainability by addressing code duplication and an unused parameter. I've suggested refactoring the override logic to be more centralized and adding clarification for an unused parameter to prevent future confusion.
Signed-off-by: Roger Wang <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! nit: How about changing to make them in a consistent format like in
vllm/vllm/model_executor/models/glm4_1v.py
Lines 1143 to 1173 in 0879736
| def _get_dummy_videos( | |
| self, | |
| *, | |
| width: int, | |
| height: int, | |
| num_frames: int, | |
| num_videos: int, | |
| overrides: Optional[VideoDummyOptions] = None, | |
| ) -> list[VideoItem]: | |
| if overrides: | |
| if overrides.num_frames: | |
| if overrides.num_frames > num_frames: | |
| logger.warning( | |
| "video.num_frames override (%d) exceeds model's " | |
| "maximum number of frames (%d), will be ignored", | |
| overrides.num_frames, num_frames) | |
| num_frames = min(num_frames, overrides.num_frames) | |
| if overrides.width: | |
| if overrides.width > width: | |
| logger.warning( | |
| "video.width override (%d) exceeds model's " | |
| "maximum width (%d), will be ignored", overrides.width, | |
| width) | |
| width = min(width, overrides.width) | |
| if overrides.height: | |
| if overrides.height > height: | |
| logger.warning( | |
| "video.height override (%d) exceeds model's " | |
| "maximum height (%d), will be ignored", | |
| overrides.height, height) | |
| height = min(height, override.height) |
This is unfortunately not doable for this model. For other models, typically max frames & height & size are constant, but for Qwen3-VL, max height & width needs to depend on how many frames there are, which is why this particular check needs to happen inside |
Make sense. Thanks for the explaination! |
mgoin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just a typing nit
Signed-off-by: Roger Wang <[email protected]>
Signed-off-by: Roger Wang <[email protected]>
…ect#26193) Signed-off-by: Roger Wang <[email protected]> Signed-off-by: Tomer Asida <[email protected]>
…ect#26193) Signed-off-by: Roger Wang <[email protected]> Signed-off-by: Karan Goel <[email protected]>
…ect#26193) Signed-off-by: Roger Wang <[email protected]>
…ect#26193) Signed-off-by: Roger Wang <[email protected]> Signed-off-by: xuebwang-amd <[email protected]>
…ect#26193) Signed-off-by: Roger Wang <[email protected]>
…ect#26193) Signed-off-by: Roger Wang <[email protected]>
…ect#26193) Signed-off-by: Roger Wang <[email protected]> Signed-off-by: xuebwang-amd <[email protected]>
…ect#26193) Signed-off-by: Roger Wang <[email protected]>
…ect#26193) Signed-off-by: Roger Wang <[email protected]>
Purpose
Fix regression from #25631
FIXES #26190
Test Plan
Test Result
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.