Skip to content

Mllama fast image processor - #37539

Open
rootonchair wants to merge 5 commits into
huggingface:mainfrom
rootonchair:mllama_fast_image_processor
Open

Mllama fast image processor#37539
rootonchair wants to merge 5 commits into
huggingface:mainfrom
rootonchair:mllama_fast_image_processor

Conversation

@rootonchair

Copy link
Copy Markdown
Contributor

What does this PR do?

Related #36978

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?

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.

@Rocketknight1

Copy link
Copy Markdown
Member

cc @yonigozlan

@rootonchair
rootonchair marked this pull request as ready for review April 23, 2025 16:20
@github-actions
github-actions Bot requested review from ydshieh and yonigozlan April 23, 2025 16:21
Comment on lines +372 to +382
expected_empty_tiles = [
# sample 1 with 1 image 2x2 grid
[
[False, False, False, False],
[True, True, True, True], # padding
],
# sample 2
[
[False, False, False, True], # 3x1
[False, True, True, True], # 1x1
],

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

There is a mismatch in shape between fast and slow for this section, where slow append another image as padding for sample 1 but fast return the same number of samples (3) because the images have been flatten to a list beforehand

@yonigozlan yonigozlan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hi @rootonchair ! Sorry for the delay. Indeed for backward compatibility and to work seamlessly with the MLlama model, this processor needs to process nested images. I just opened a PR that improves support for image processor handling nested images, you can have a look here. Once this PR is merged, you can do something similar for MLlama than it's done for Idefics2/3/SmolVlm in the PR.

@rootonchair

Copy link
Copy Markdown
Contributor Author

Hi @rootonchair ! Sorry for the delay. Indeed for backward compatibility and to work seamlessly with the MLlama model, this processor needs to process nested images. I just opened a PR that improves support for image processor handling nested images, you can have a look here. Once this PR is merged, you can do something similar for MLlama than it's done for Idefics2/3/SmolVlm in the PR.

Sure @yonigozlan, I will proceed when your PR is merged

@yonigozlan

Copy link
Copy Markdown
Contributor

Hello @rootonchair ! The PR I mentioned was merged btw, if you want this PR another go!

@rootonchair

Copy link
Copy Markdown
Contributor Author

@yonigozlan that's great. I will get back to this PR

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