-
Notifications
You must be signed in to change notification settings - Fork 7k
[bugfix][data][llm] Fix vLLMEngineStage field name inconsistency for images #57980
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
[bugfix][data][llm] Fix vLLMEngineStage field name inconsistency for images #57980
Conversation
Fixes ray-project#57978 - Fixed get_optional_input_keys() to declare 'image' (singular) instead of 'images' - Added clear error message and fix when users provide 'images' field in the custom path (`has_image = False`) Signed-off-by: Nikhil Ghosh <nikhil@anyscale.com>
| # Extract image data from preprocessing output | ||
| if "image" in row: | ||
| image = row.pop("image") | ||
| elif "images" in row: |
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.
this is weird, I am inclined to not throw any errors and dump it on the user to do it right.
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.
I agree - calling it deprecated is also not quite right. Can just keep behavior as it was before, and include just the doc fix for clarification (so at least internally, we are consistent)?
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.
yeah let's keep the old one.
| return { | ||
| "tokenized_prompt": "The tokenized prompt. If provided, the prompt will not be tokenized by the vLLM engine.", | ||
| "images": "The images to generate text from. If provided, the prompt will be a multimodal prompt.", | ||
| "image": "The image(s) for multimodal input. Accepts a single image or list of images.", |
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.
ok.
Signed-off-by: Nikhil Ghosh <nikhil@anyscale.com>
|
Thank you @nrghosh for helping to integrate |
…ay-project#57980) Signed-off-by: Aydin Abiar <aydin@anyscale.com>
Fixes #57978
has_image = False)