Uniformize LlavaNextVideoProcessor kwargs#35613
Conversation
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
zucchini-nlp
left a comment
zucchini-nlp
left a comment
There was a problem hiding this comment.
Sorry for late review, forgot about this PR.
Cool that we're standardizing video LLMs. Overall LGTM, we just need a few tests with video processors to make sure nothing breaks
There was a problem hiding this comment.
tiny concern about squishing in audio before the videos, but I don't think any user passes videos as positional arg so maybe we are oke
I don't want to add more complexity by trying to validate the order of audio and video now, so let's leave as is and just take this noted
There was a problem hiding this comment.
am I right that we don't need legacy=False anymore?
There was a problem hiding this comment.
I ended up keeping it because I had put v5.0.0 as the deprecation version
There was a problem hiding this comment.
Oops thanks for catching that :)
There was a problem hiding this comment.
afaik ProcessorTesterMixin doesn't test videos_kwargs yet. I think we need to add video tests to make sure that llava-next-video processor works as expected
There was a problem hiding this comment.
Indeed! I added tests for video_kwargs, very similar to those on images_kwargs :)
a75909c to
fd69d5c
Compare
|
Thanks for the feedback @zucchini-nlp ! src/transformers/models/llava_onevision/processing_llava_onevision.py:167: in __call__
one_video = to_numpy_array(video_inputs.get("pixel_values_videos")[0])
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
def to_numpy_array(img) -> np.ndarray:
if not is_valid_image(img):
> raise ValueError(f"Invalid image type: {type(img)}")
E ValueError: Invalid image type: <class 'list'>
src/transformers/image_utils.py:231: ValueErrorThis is the case in main as well so seems unrelated to this PR. |
|
Yep, will be fixed by #35660 |
|
I had to make some modifications to llava_next_video processor tests following this PR #35953 . |
molbap
left a comment
molbap
left a comment
There was a problem hiding this comment.
LGTM as well - for the processing common tests for videos there's a couple models that were recently/will be soon merged, would be cool to check if they work !
| "image_kwargs": {}, | ||
| "videos_kwargs": {}, |
There was a problem hiding this comment.
Not sure you have to specify an empty dictionary here!
5 participants
What does this PR do?
Adds uniformized processors following #31911 for LlavaNextVideoProcessor .
Fixes #35602
Who can review?
Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.