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.

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

Uniformize model processors #31368

Merged
merged 107 commits into from
Oct 2, 2024
Merged

Conversation

molbap
Copy link
Contributor

@molbap molbap commented Jun 11, 2024

What does this PR do?

Adds uniformized processors following #30511, #31197 in particular, and #31198 .

Adds support for:

  • Blip
  • Blip2
  • Bridgetower
  • Donut
  • Wav2vec2

@molbap
Copy link
Contributor Author

molbap commented Sep 24, 2024

@amyeroberts I finished digging through feature extractors for audio, not trivial, but seems it was missing something testwise. Now all seems to work 🧹 🧹

@molbap
Copy link
Contributor Author

molbap commented Sep 24, 2024

One slow test model fails because of #33678

@amyeroberts
Copy link
Collaborator

@molbap Re this comment - is this PR ready for review now?

@molbap
Copy link
Contributor Author

molbap commented Sep 26, 2024

It should! failing tests are unrelated and should be fixed in #33678

@molbap
Copy link
Contributor Author

molbap commented Sep 27, 2024

I fixed a concat bug I noticed in another PR, #33678 , where I fixed it for instructblip but it remained for blip_2 🫣

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.

Thanks! Just a few small comments

tests/test_processing_common.py Outdated Show resolved Hide resolved
tests/test_processing_common.py Show resolved Hide resolved
src/transformers/models/altclip/processing_altclip.py Outdated Show resolved Hide resolved
src/transformers/models/altclip/processing_altclip.py Outdated Show resolved Hide resolved
src/transformers/models/altclip/processing_altclip.py Outdated Show resolved Hide resolved
src/transformers/models/blip_2/processing_blip_2.py Outdated Show resolved Hide resolved
src/transformers/models/donut/processing_donut.py Outdated Show resolved Hide resolved
@HuggingFaceDocBuilderDev

Hey! 🤗 Thanks for your contribution to the transformers library!

Before merging this pull request, slow tests CI should be triggered. To enable this:

  • Add the run-slow label to the PR
  • When your PR is ready for merge and all reviewers' comments have been addressed, push an empty commit with the command [run-slow] followed by a comma separated list of all the models to be tested, i.e. [run_slow] model_to_test_1, model_to_test_2
    • If the pull request affects a lot of models, put at most 10 models in the commit message
  • A transformers maintainer will then approve the workflow to start the tests

(For maintainers) The documentation for slow tests CI on PRs is here.

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.

Thanks!

@molbap molbap merged commit 50290cf into huggingface:main Oct 2, 2024
36 checks passed
@molbap molbap deleted the uniform_processors_3 branch October 2, 2024 08:41
NielsRogge pushed a commit to NielsRogge/transformers that referenced this pull request Oct 21, 2024
* add initial design for uniform processors + align model

* add uniform processors for altclip + chinese_clip

* add uniform processors for blip + blip2

* fix mutable default 👀

* add configuration test

* handle structured kwargs w defaults + add test

* protect torch-specific test

* fix style

* fix

* rebase

* update processor to generic kwargs + test

* fix style

* add sensible kwargs merge

* update test

* fix assertEqual

* move kwargs merging to processing common

* rework kwargs for type hinting

* just get Unpack from extensions

* run-slow[align]

* handle kwargs passed as nested dict

* add from_pretrained test for nested kwargs handling

* [run-slow]align

* update documentation + imports

* update audio inputs

* protect audio types, silly

* try removing imports

* make things simpler

* simplerer

* move out kwargs test to common mixin

* [run-slow]align

* skip tests for old processors

* [run-slow]align, clip

* !$#@!! protect imports, darn it

* [run-slow]align, clip

* [run-slow]align, clip

* update common processor testing

* add altclip

* add chinese_clip

* add pad_size

* [run-slow]align, clip, chinese_clip, altclip

* remove duplicated tests

* fix

* add blip, blip2, bridgetower

Added tests for bridgetower which override common. Also modified common
tests to force center cropping if existing

* fix

* update doc

* improve documentation for default values

* add model_max_length testing

This parameter depends on tokenizers received.

* Raise if kwargs are specified in two places

* fix

* removed copied from

* match defaults

* force padding

* fix tokenizer test

* clean defaults

* move tests to common

* add missing import

* fix

* adapt bridgetower tests to shortest edge

* uniformize donut processor + tests

* add wav2vec2

* extend common testing to audio processors

* add testing + bert version

* propagate common kwargs to different modalities

* BC order of arguments

* check py version

* revert kwargs merging

* add draft overlap test

* update

* fix blip2 and wav2vec due to updates

* fix copies

* ensure overlapping kwargs do not disappear

* replace .pop by .get to handle duplicated kwargs

* fix copies

* fix missing import

* add clearly wav2vec2_bert to uniformized models

* fix copies

* increase number of features

* fix style

* [run-slow] blip, blip2, bridgetower, donut, wav2vec2, wav2vec2_bert

* [run-slow] blip, blip_2, bridgetower, donut, wav2vec2, wav2vec2_bert

* fix concatenation

* [run-slow] blip, blip_2, bridgetower, donut, wav2vec2, wav2vec2_bert

* Update tests/test_processing_common.py

Co-authored-by: amyeroberts <[email protected]>

* 🧹

* address comments

* clean up + tests

* [run-slow] instructblip, blip, blip_2, bridgetower, donut, wav2vec2, wav2vec2_bert

---------

Co-authored-by: amyeroberts <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants