[Bugfix] Fix Nemotron VL image processing - #22739
Conversation
|
👋 Hi! Thank you for contributing to the vLLM project. 💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in #pr-reviews, coordinate on features in #feat- channels, or join special interest groups in #sig- channels. Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run Once the PR is approved and ready to go, your PR reviewer(s) can run CI to test the changes comprehensively before merging. To run CI, PR reviewers can either: Add 🚀 |
There was a problem hiding this comment.
Code Review
This pull request correctly implements the specific image processing pipeline for Nemotron VL models, addressing a bug in the previous implementation. The changes, which include removing image normalization and adopting a model-specific method for finding the closest aspect ratio, align well with the reference implementation. My review identifies a potential ZeroDivisionError if an image with zero width or height is processed. I've suggested adding input validation to prevent this potential crash, which would improve the robustness of the new logic.
There was a problem hiding this comment.
The calculation of aspect_ratio can lead to a ZeroDivisionError if orig_height is 0. Similarly, the find_closest_aspect_ratio function called later can also face a division by zero if width * height (area) is zero. It's safer to validate that both orig_width and orig_height are positive at the beginning of this function to prevent potential crashes from invalid image inputs.
| aspect_ratio = orig_width / orig_height | |
| if orig_width <= 0 or orig_height <= 0: | |
| raise ValueError("Image dimensions must be positive.") | |
| aspect_ratio = orig_width / orig_height |
|
Hi @simon-mo, could you please re-invite me to the vLLM Slack? I missed the invitation email, and it’s now expired |
0884ee0 to
633548f
Compare
DarkLight1337
left a comment
There was a problem hiding this comment.
Thanks, can you add a correctness test under tests/models/multimodal/generation/test_common.py to avoid future regressions?
|
@DarkLight1337 unfortunately, the config can’t be auto-loaded — it requires patching vllm/tests/models/multimodal/generation/test_common.py Lines 439 to 440 in c583038 Minimal reproduce script: from transformers import AutoConfig
model_path = "nvidia/Llama-3.1-Nemotron-Nano-VL-8B-V1"
# Fails without attn_implementation
config = AutoConfig.from_pretrained(model_path, trust_remote_code=True)
## TypeError: argument of type 'NoneType' is not iterable
# Works with attn_implementation explicitly set
config = AutoConfig.from_pretrained(
model_path, trust_remote_code=True, attn_implementation="flash_attention_2"
)
print("WORK!!")Environment: I plan to add Nemotron to the Hugging Face Transformers repo soon, so no worries about that. |
DarkLight1337
left a comment
There was a problem hiding this comment.
Alright let's just merge this first then, thanks
Co-authored-by: ducviet00-h2 <viet.d.hoang@h2corporation.jp> Signed-off-by: Diego-Castan <diego.castan@ibm.com>
Co-authored-by: ducviet00-h2 <viet.d.hoang@h2corporation.jp>
Co-authored-by: ducviet00-h2 <viet.d.hoang@h2corporation.jp>
Co-authored-by: ducviet00-h2 <viet.d.hoang@h2corporation.jp>
Co-authored-by: ducviet00-h2 <viet.d.hoang@h2corporation.jp> Signed-off-by: Xiao Yu <xiao.yu@amd.com>
Co-authored-by: ducviet00-h2 <viet.d.hoang@h2corporation.jp>
Co-authored-by: ducviet00-h2 <viet.d.hoang@h2corporation.jp>
Co-authored-by: ducviet00-h2 <viet.d.hoang@h2corporation.jp>
Co-authored-by: ducviet00-h2 <viet.d.hoang@h2corporation.jp>
Co-authored-by: ducviet00-h2 <viet.d.hoang@h2corporation.jp>
Purpose
Correct image processing for Nemotron VL:
find_closest_aspect_ratioTest Plan
Using this script to test vLLM
Nemotron VL test script
Test Result
On
mainbranch: repeated nonsense:This branch correctly extracted table from the image and matches HF outputs