Skip to content

Fix Qwen2.5-VL Video Processor#38366

Merged
ArthurZucker merged 8 commits intohuggingface:mainfrom
yeliudev:main
May 27, 2025
Merged

Fix Qwen2.5-VL Video Processor#38366
ArthurZucker merged 8 commits intohuggingface:mainfrom
yeliudev:main

Conversation

@yeliudev
Copy link
Copy Markdown
Contributor

@yeliudev yeliudev commented May 26, 2025

What does this PR do?

This PR fixes the bug in Qwen2_5_VLProcessor, which would consistently logging "Unused or unrecognized kwargs: fps, return_tensors." during training. This issue seems to be introduced in #35206.

Before submitting

  • This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
  • Did you read the contributor guideline,
    Pull Request section?
  • Was this discussed/approved via a Github issue or the forum? Please add a link
    to it if that's the case.
  • Did you make sure to update the documentation with your changes? Here are the
    documentation guidelines, and
    here are tips on formatting docstrings.
  • Did you write any new necessary tests?

Who can review?

@zucchini-nlp

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.

Copy link
Copy Markdown
Member

@zucchini-nlp zucchini-nlp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix! Yes, I've been seeing the issue with return_tensors for a while and had a bigger PR to refactor further.

The problem isn't specific for Qwen models, and needs to be fixed in video_processing_utils.py. Can you try to update the PR to fix all video processors from base class?

@yeliudev
Copy link
Copy Markdown
Contributor Author

Thanks for your comment! I'm afraid that I have no idea how to fix it in video_processing_utils.py as I'm not familiar with this new validate_kwargs feature. I've reverted the fix for return_tensors and am leaving that for fps only.

@zucchini-nlp
Copy link
Copy Markdown
Member

@yeliudev since it is the only common kwarg and is not in VideosKwargs, we can manually pass it to valid_kwargs as follows. LMK if that works and doesn't flood output with warnings

# `return_tensors` is a common kwargs thus not in processing_utils.VideosKwargs
        validate_kwargs(
            captured_kwargs=kwargs.keys(),
            valid_processor_keys=list(self.valid_kwargs.__annotations__.keys()) + ["return_tensors"],
        )

@yeliudev
Copy link
Copy Markdown
Contributor Author

@zucchini-nlp Yes it works. Many thanks!

Copy link
Copy Markdown
Member

@zucchini-nlp zucchini-nlp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect thanks!

@zucchini-nlp zucchini-nlp enabled auto-merge (squash) May 26, 2025 10:24
Copy link
Copy Markdown
Collaborator

@ArthurZucker ArthurZucker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merging as test is unrelated! not super fan of adding the return_tensors like that but can be done afterwards

@ArthurZucker ArthurZucker disabled auto-merge May 27, 2025 11:46
@ArthurZucker ArthurZucker merged commit 10ae443 into huggingface:main May 27, 2025
17 of 20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants