Make ViTPooler configurable #36517

sebbaur · 2025-03-03T15:56:06Z

What does this PR do?

Make parameters of ViTPooler (activation function, output size) configurable, so that I can open-source https://arxiv.org/abs/2403.02522 more easily (the encoder slightly differs from the current implementation).

Fixes # (issue)
#36513

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?

Note that the code didn't have any test to the best of my knowledge, so I have not added any. Besides, the change is trivial so it may not be needed.

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.

…vation function and the number of channels in the output

… string)

github-actions · 2025-03-03T15:56:17Z

Hi 👋, thank you for opening this pull request! The pull request is converted to draft by default. When it is ready for review, please click the Ready for review button (at the bottom of the PR page).

qubvel · 2025-03-03T16:08:12Z

Hey @sebbaur, feel free to ping me when it's ready for review! Btw, we have activation functions defined in ACT2FN mapping, see llama for example.

sebbaur · 2025-03-03T16:15:32Z

Thanks for the prompt reply and the pointers! I hadn't noticed ACT2FN -- I have made the change

Once the CI is happy, I will ping you

src/transformers/models/vit/modeling_flax_vit.py

sebbaur · 2025-03-03T17:33:35Z

I had to modify files in dpt and deit too to make CI happy

sebbaur · 2025-03-03T20:47:32Z

tests are not done but I suppose they will pass

sebbaur · 2025-03-03T20:48:38Z

@qubvel I think it is ready -- it took more work than I initially expected because I had to propagate changes to other parts of the codebase which use the same pooler. This felt a bit out of scope, but I suppose this is WAI?

qubvel

Thanks, looks good to me, just a question re docs:

utils/check_docstrings.py

…sebbaur/transformers into feature/vit_pooler_configurable

sebbaur · 2025-03-04T13:55:01Z

Somebody suggested here #36513 that this change was not appropriate

I am a bit confused why -- it is a no-op and just makes some hardcoded hyperparameters configurable

Rocketknight1 · 2025-03-06T09:44:45Z

Hey! That was me but on reflection I think it's okay - we do have a general rule in our philosophy against adding additional features to existing pretrained model architectures like this, but when it's compact and backward compatibility is guaranteed I think it's probably okay, especially if the PR is already complete.

qubvel

@sebbaur, thanks for making CI happy, just the last nit comment

src/transformers/models/vit/modeling_flax_vit.py

sebbaur · 2025-03-07T16:47:41Z

@qubvel it says 2 workflows awaiting approval -- do you know who I should reach out to to review this?

sebbaur · 2025-03-10T14:06:41Z

@qubvel @Rocketknight1 friendly ping :) -- do you know who should review the workflows? 2 workflows awaiting approval

Rocketknight1 · 2025-03-10T15:55:59Z

@sebbaur the additional workflows are the documentation builder - it's usually not essential to run those. I enabled them anyway, but at this point we just need core maintainer approval and we're good to merge cc @ArthurZucker @Cyrilvallez

HuggingFaceDocBuilderDev · 2025-03-10T16:22:09Z

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.

sebbaur · 2025-03-12T13:16:03Z

hello @ArthurZucker and @Cyrilvallez, would you be able to take a look?

sebbaur · 2025-03-13T16:04:54Z

friendly ping :)

sebbaur · 2025-03-17T13:56:34Z

Happy Monday @ArthurZucker and @Cyrilvallez! Would you please be able to take a quick look? That would unlock a few things on my side

Thanks!

sebbaur · 2025-03-19T15:44:02Z

Hello @Cyrilvallez ! Could you please take a look? Even if this cannot be merged, it would be very helpful for me to know this sooner than later, as I am planning to open-source a Pytorch implementation of https://huggingface.co/google/hear, which this PR allows easily. I need time to find an alternative if this is not ok

Cyrilvallez

Hey! Sorry for the delay! @Rocketknight1 is right that we usually don't like to change modeling code so much, but let's make an exception here!
However, let's still minimize impact and removed unecesary changes, as from my understanding you only need vit:

Ijepa should never have been impacted
let's juste remove "Copied from" for deit and dpt instead of modifying them, and revert the changes to modeling and configs
flax change of vit is wrong -> let's make sure it works by adding tanh in the mapping in modeling_flax_utils

Cyrilvallez · 2025-03-19T16:12:52Z

src/transformers/models/vit/modeling_flax_vit.py

        )
+        self.activation = ACT2FN[self.config.pooler_act]


"tanh" is a KeyError here, it does not exist in the mapping

src/transformers/models/ijepa/configuration_ijepa.py

src/transformers/models/dpt/modeling_dpt.py

src/transformers/models/deit/modeling_deit.py

src/transformers/models/deit/modeling_tf_deit.py

Cyrilvallez · 2025-03-19T16:30:18Z

For the "Copied from", you can replace them with something such as "Originally copied from", it will stop our linter from forcing the changes, and at the same time we still keep track of the fact it was previously similar, in case we need it later

Cyrilvallez · 2025-03-20T11:15:06Z

Alright, after discussions with @qubvel, let's keep the changes that were propagated to the other models, it's fine! You just need to fix the issue on flax vit then 😉 Sorry for the roller coaster! 🙃

…sebbaur/transformers into feature/vit_pooler_configurable

sebbaur · 2025-03-20T20:24:06Z

Thanks for your review!

I have made the change to the ACT2FN dict -- good catch!

Next time I will know that the models' code shouldn't be updated... Sorry about this

Cyrilvallez · 2025-03-21T10:01:03Z

Merging! Thanks!!

* Make ViT Pooler configurable, so that it is possible to pick the activation function and the number of channels in the output * Add documentation and allow functions as activations (instead of just string) * formatting change * Use ACT2FN * Formatting change * Formatting changes * force pooler_act to be string * force pooler_act to be string * Add configs to OBJECTS_TO_IGNORE to make check_docstrings happy * Making the same change in ijepa to make check_modular_conversion happy * Add IJepaConfig to make CI happy * rename pooler_size to pooler_output_size as defined in the config * typo * revert change to ignore variable * Ran utils/check_docstrings.py --fix_and_overwrite * revert unrelated change * remove redundant defaults * rename self.act -> self.activation * tanh activation function in mapping

sebbaur added 2 commits March 3, 2025 15:22

Make ViT Pooler configurable, so that it is possible to pick the acti…

97199e3

…vation function and the number of channels in the output

Add documentation and allow functions as activations (instead of just…

43639fa

… string)

github-actions bot marked this pull request as draft March 3, 2025 15:56

formatting change

80e132a

qubvel added the Vision label Mar 3, 2025

Use ACT2FN

9fe0691

sebbaur added 2 commits March 3, 2025 16:17

Formatting change

bd2a001

Formatting changes

18edafb

qubvel reviewed Mar 3, 2025

View reviewed changes

src/transformers/models/vit/modeling_flax_vit.py Outdated Show resolved Hide resolved

force pooler_act to be string

5060f56

sebbaur and others added 7 commits March 3, 2025 12:34

force pooler_act to be string

0a2895f

Add configs to OBJECTS_TO_IGNORE to make check_docstrings happy

9e9dfd1

Making the same change in ijepa to make check_modular_conversion happy

0d2b485

Add IJepaConfig to make CI happy

e0d6928

rename pooler_size to pooler_output_size as defined in the config

0881e5f

typo

e2670cc

Merge branch 'main' into feature/vit_pooler_configurable

0382960

sebbaur marked this pull request as ready for review March 3, 2025 20:47

qubvel reviewed Mar 4, 2025

View reviewed changes

utils/check_docstrings.py Outdated Show resolved Hide resolved

sebbaur and others added 3 commits March 4, 2025 08:47

revert change to ignore variable

c89f69b

Merge branch 'feature/vit_pooler_configurable' of https://github.com/…

6309d96

…sebbaur/transformers into feature/vit_pooler_configurable

Merge branch 'main' into feature/vit_pooler_configurable

6c6a1ac

Merge branch 'main' into feature/vit_pooler_configurable

5ffac41

sebbaur added 2 commits March 4, 2025 18:55

revert unrelated change

bc5ef1b

remove redundant defaults

2d0071f

qubvel approved these changes Mar 6, 2025

View reviewed changes

src/transformers/models/vit/modeling_flax_vit.py Outdated Show resolved Hide resolved

src/transformers/models/vit/modeling_flax_vit.py Outdated Show resolved Hide resolved

sebbaur and others added 3 commits March 6, 2025 09:10

rename self.act -> self.activation

3ca67c1

Merge branch 'main' into feature/vit_pooler_configurable

05499eb

Merge branch 'main' into feature/vit_pooler_configurable

ab56b63

Merge branch 'main' into feature/vit_pooler_configurable

308b5b3

sebbaur added 2 commits March 10, 2025 16:27

Merge branch 'main' into feature/vit_pooler_configurable

7e9bc63

Merge branch 'main' into feature/vit_pooler_configurable

eff0bd1

Merge branch 'main' into feature/vit_pooler_configurable

8028f62

qubvel requested a review from Cyrilvallez March 17, 2025 16:50

Cyrilvallez reviewed Mar 19, 2025

View reviewed changes

sebbaur and others added 3 commits March 20, 2025 16:21

tanh activation function in mapping

87b9162

Merge branch 'feature/vit_pooler_configurable' of https://github.com/…

16dfb96

…sebbaur/transformers into feature/vit_pooler_configurable

Merge branch 'main' into feature/vit_pooler_configurable

366edfe

Cyrilvallez merged commit 62116c9 into huggingface:main Mar 21, 2025
21 checks passed

Make ViTPooler configurable #36517

Make ViTPooler configurable #36517

Uh oh!

Conversation

sebbaur commented Mar 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Before submitting

Who can review?

Uh oh!

github-actions bot commented Mar 3, 2025

Uh oh!

qubvel commented Mar 3, 2025

Uh oh!

sebbaur commented Mar 3, 2025

Uh oh!

Uh oh!

sebbaur commented Mar 3, 2025

Uh oh!

sebbaur commented Mar 3, 2025

Uh oh!

sebbaur commented Mar 3, 2025

Uh oh!

qubvel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sebbaur commented Mar 4, 2025

Uh oh!

Rocketknight1 commented Mar 6, 2025

Uh oh!

qubvel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sebbaur commented Mar 7, 2025

Uh oh!

sebbaur commented Mar 10, 2025

Uh oh!

Rocketknight1 commented Mar 10, 2025

Uh oh!

HuggingFaceDocBuilderDev commented Mar 10, 2025

Uh oh!

sebbaur commented Mar 12, 2025

Uh oh!

sebbaur commented Mar 13, 2025

Uh oh!

sebbaur commented Mar 17, 2025

Uh oh!

sebbaur commented Mar 19, 2025

Uh oh!

Cyrilvallez left a comment

Choose a reason for hiding this comment

Uh oh!

Cyrilvallez Mar 19, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Cyrilvallez commented Mar 19, 2025

Uh oh!

Cyrilvallez commented Mar 20, 2025

Uh oh!

sebbaur commented Mar 20, 2025

Uh oh!

Cyrilvallez commented Mar 21, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sebbaur commented Mar 3, 2025 •

edited

Loading