Handle audio/ video default arguments in processor's apply_chat_template

[eustlb]

What does this PR do?

For multimodal models, it is often required to pass kwargs indicating how the passed inputs have been sampled for the Processor: fps for video inputs, sampling_rate for audio inputs.

Such values can be set as default values for in ModelProcessorKwargs, nevertheless they are not passed to the processor's __call__ method when using apply_chat_template, resulting in silent errors.

For example in Qwen2.5-VL:

from transformers import AutoProcessor

processor = AutoProcessor.from_pretrained("Qwen/Qwen2.5-VL-7B-Instruct")

conversation = [
    {
        "role": "user",
        "content": [
            {"type": "video", "path": "example.mp4"},
            {"type": "text", "text": "What happened in the video?"},
        ],
    }
]

inputs = processor.apply_chat_template(
    conversation,
    video_fps=1,
    add_generation_prompt=True,
    tokenize=True,
    return_dict=True,
    return_tensors="pt"
) 

→ images will be sampled at 1 fps

Yet such value (fps=1) is not passed in kwargs in:

out = self(
    text=prompt,
    images=batch_images if batch_images else None,
    videos=batch_videos if batch_videos else None,
    audio=batch_audios if batch_audios else None,
    **kwargs,
)

making that the default value fps=2 is used in the processor's __call__ method.

This is also the case for audio processors that need to know at which sampling_rate the audio has been sampled.

Fix attempt

I provide an attempt to fix by initialising such values to the specified default value in the ModelProcessorKwargs if provided, and passing this value to the Processors __call__ kwargs.

cc @zucchini-nlp and @molbap since we talked offline about this

[github-actions]

Hi 👋, thank you for opening this pull request! The pull request is converted to draft by default. The CI will be paused while the PR is in draft mode. When it is ready for review, please click the Ready for review button (at the bottom of the PR page). This will assign reviewers and trigger CI.

[HuggingFaceDocBuilderDev]

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.

[pcuenca]

Thanks @zucchini-nlp for the ping, I was starting to test a simpler solution just for Qwen, by forwarding video_fps from the subclass implementation of apply_chat_template. I like that this is more general and works for any video / audio models, very cool!

I think we should add "fps": 2.0 to Qwen2_5OmniProcessorKwargs._defaults.videos_kwargs. This way loading the model will still use the default (2 fps), even if the user does not provide a video_fps kwarg in their call to apply_chat_template. If we don't do this, the processor will still receive 2.0, but self._load_video_for_model won't, and it will read all the individual frames.

[zucchini-nlp]

Thanks for the PR but I am not sure I got it right. IIUC we want to pass the fps/sampling_rate used to sample frames/audio to the processor's call as well, because qwen uses it to further process videos. In that case I don't fully understand why we need to modify kwargs used for in load_video. Might have missed something

Also, I would like us to deprecate on of the kwargs, probably it will the make changes in this PR minimal. WDYT on keeping only fps and deprecating video_fps?

fps": 2.0 to Qwen2_5OmniProcessorKwargs._defaults.videos_kwargs

+1 on this comment

[zucchini-nlp]

Ugh, totally missed this one, better if we align in naming for future models and use either video_fps or fps. Since we already added qwen with fps, imo we can converge under that name and deprecate video_fps in this PR

[zucchini-nlp]

this is getting way too overengineered imo and it's not this PR's fault. Prob it works for a while but a longer term solution I had is to use dataclasses or similar to split out kwargs per type

Couldn't get time to draft it though

[pcuenca]

+1, this is too complicated 😅 I considered an attempt to reuse _merge_kwargs (which hides a lot of similarly complex logic), but it was not obvious at all.

[pcuenca]

A short term option would be to just special-case the missing params (fps and sampling_rate) instead of doing it in a general way.

[zucchini-nlp]

yeah, same, but _merge_kwargs wasn't much generalizable unless rewritten 🥲

[eustlb]

Yep, I wanted to go for a general approach but it's indeed overengineered. Since it is only relevant for fps and sampling_rate, I am gonna special-case if. Thanks both for the feedback!

[zucchini-nlp]

afaik qwen doesn't have a special load_for_model defined, why do we need this?

[zucchini-nlp]

And also, I am planning to move the whole video sampling into video_processors after it is merged. There are some ideas on how it will look like with SmolVLM (the most complicated sampling logic) in this PR. I will make this idea open to discussion after it's unblocked.

Until then we can start by deprecating similar kwargs and passing sampling args further to model as well

[pcuenca]

I don't fully understand why we need to modify kwargs used for in load_video.

I think it's because, if the user provides a value for fps, we want to apply it during video loading so the correct number of frames is retrieved. Otherwise, it would load all frames (current behaviour; undesired, imo), or the new default we set in Qwen2_5OmniProcessorKwargs._defaults.videos_kwargs, which could be different to the value supplied by the user.

[pcuenca]

And also, I am planning to move the whole video sampling into video_processors after it is merged. There are some ideas on how it will look like with SmolVLM (the most complicated sampling logic) in this PR. I will make this idea open to discussion after it's unblocked.

Nice! Feel free to ping when you are ready, happy to test drive.

[zucchini-nlp]

if the user provides a value for fps, we want to apply it during video loading so the correct number of frames is retrieved

Oh right, haven't thought of it. Making fps the only correct arg and deprecating video_fps solves the issue then, I think

[eustlb]

Yep perfect, handling the deprecation of video_fps in this PR!