Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Standardize image-text-to-text-models outputs #32471

Closed
wants to merge 3 commits into from
Closed

Standardize image-text-to-text-models outputs #32471

wants to merge 3 commits into from

Conversation

yonigozlan
Copy link
Member

@yonigozlan yonigozlan commented Aug 6, 2024 •
edited
Loading

What does this PR do?

Standardize outputs for existing image-text-to-text models by adding a post_process_image_text_to_text function to their processor.
Blocking PR for image-text-to-text pipeline.

The following models' processors need to be modified:

  • GIT
  • BLIP
  • BLIP-2
  • IDEFICS
  • InstructBLIP
  • LLaVa
  • Fuyu
  • Pix2Struct/DePlot/MatCha
  • UDOP
  • Donut
  • KOSMOS-2
  • Idefics2
  • LLaVA-NeXT
  • PaliGemma
  • VipLlava
  • Chameleon
  • LlaVa OneVision
  • Qwen2-VL
  • Idefics3
  • Pixtral
  • MLlama

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?

@molbap @amyeroberts

@HuggingFaceDocBuilderDev
Copy link

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.

@yonigozlan yonigozlan marked this pull request as ready for review August 6, 2024 15:36
@yonigozlan yonigozlan requested a review from molbap August 6, 2024 15:36
@leloykun
Copy link
Contributor

leloykun commented Aug 6, 2024

@yonigozlan Chameleon can also do image-text-to-text

@yonigozlan
Copy link
Member Author

@yonigozlan Chameleon can also do image-text-to-text

Thanks! Will add it to the list

@yonigozlan yonigozlan mentioned this pull request Aug 7, 2024
Open
40 tasks
molbap
molbap reviewed Aug 7, 2024
View reviewed changes
Copy link
Contributor

@molbap molbap 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 working on this! left a few comments, moving on to 32472 now

src/transformers/models/donut/processing_donut.py Show resolved Hide resolved
src/transformers/models/fuyu/processing_fuyu.py Outdated Show resolved Hide resolved
src/transformers/models/idefics/processing_idefics.py Outdated Show resolved Hide resolved
src/transformers/models/idefics/processing_idefics.py Outdated Show resolved Hide resolved
src/transformers/models/paligemma/processing_paligemma.py Outdated Show resolved Hide resolved
amyeroberts
amyeroberts reviewed Aug 13, 2024
View reviewed changes
Copy link
Collaborator

@amyeroberts amyeroberts left a comment

Choose a reason for hiding this comment

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

Very nice! Looking forward to having all of the processing behaviour more standardized ❤️

Main comment is on the handling of the legacy behaviour

src/transformers/models/paligemma/processing_paligemma.py Outdated Show resolved Hide resolved
src/transformers/models/pix2struct/processing_pix2struct.py Outdated Show resolved Hide resolved
src/transformers/models/donut/processing_donut.py Show resolved Hide resolved
src/transformers/models/donut/processing_donut.py Outdated Show resolved Hide resolved
src/transformers/models/donut/processing_donut.py Outdated Show resolved Hide resolved
src/transformers/models/paligemma/processing_paligemma.py Outdated Show resolved Hide resolved
src/transformers/models/udop/processing_udop.py Outdated Show resolved Hide resolved
src/transformers/models/idefics/processing_idefics.py Outdated Show resolved Hide resolved
@yonigozlan yonigozlan force-pushed the standardize-inputs-outputs branch 2 times, most recently from 4137b24 to 04fb918 Compare October 3, 2024 11:18
@yonigozlan
Standardize image-text-to-text-models-output
639ad09 
add post_process_image_text_to_text to chameleon and cleanup

Fix legacy kwarg behavior and deprecation warning

add post_process_image_text_to_text to qwen2_vl and llava_onevision

Add post_process_image_text_to_text to idefics3, mllama, pixtral processor
@yonigozlan
nit var name post_process_image_text_to_text udop
70926ae
@yonigozlan
nit fix deprecation warnings
bc5cf3c
@yonigozlan yonigozlan mentioned this pull request Oct 15, 2024
Merged
2 tasks
@yonigozlan
Copy link
Member Author

@LysandreJik This should be ready for a final review, and should significantly reduce the loc count and number of files changed for the image-text-to-text pipeline PR :).
One thing to note is the use of a "legacy" kwarg for BC, as some changes are needed in the preprocessing of some models if we want them to work seamlessly with the image-text-to-text pipeline. I'm open to suggestion if there is a better way to handle this

@LysandreJik
Copy link
Member

cc @molbap can you do an initial review please?

@LysandreJik
Copy link
Member

Maybe not initial, but pre-final ? 😁

@yonigozlan
Copy link
Member Author

The changes from this PR were merged in #34170

@yonigozlan yonigozlan closed this Oct 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amyeroberts amyeroberts amyeroberts left review comments

@LysandreJik LysandreJik Awaiting requested review from LysandreJik

@molbap molbap Awaiting requested review from molbap

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@yonigozlan @HuggingFaceDocBuilderDev @leloykun @LysandreJik @amyeroberts @molbap