Add sdpa for Vivit #33757

RUFFY-369 · 2024-09-27T10:09:06Z

What does this PR do?

Towards #28005

Adds sdpa for the ViViT model.

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?

@amyeroberts

…ain branch too)

RUFFY-369 · 2024-09-27T10:24:05Z

@amyeroberts All green 🟢 😃

amyeroberts

Thanks for adding!

Two things before merge:

Running the slow tests for the model. Could you push an empty commit with the message [run_slow] vivit?
Adding info and benchmarks to the model's doc page. A good example is here. Benchmarking scripts from @fxmarty for training and inference.

RUFFY-369 · 2024-09-27T13:03:51Z

Hi @amyeroberts , I think in the ViViT model, originally interpolate_pos_encoding method here hasn't been correctly implemented and has just been copied from ViT because I made a commit in this current PR regarding patch_size here as patch_size itself wasn't declared in the method. I gave benefit of doubt to the work done during the port of this model and just fixed it. But when I tested the slow tests with RUN_SLOW, test_inference_interpolate_pos_encoding failed because the implementation hasn't been done properly. So, should I raise an issue separately or fix it here.

One thing can be that apart from the test_inference_interpolate_pos_encoding slow test rest can tested by [run_slow] vivit commit message and I will push the training and inference benchmark too just in some time. And when this PR gets merged, I will open a separate issue for test_inference_interpolate_pos_encoding test fix.

Or the other thing can be that I fix it here in this PR.

Please suggest what works well for you?!

vignesh1507

looks good to me, appreciate the effort @RUFFY-369

RUFFY-369 · 2024-09-28T08:10:58Z

Hi @amyeroberts , I think in the ViViT model, originally interpolate_pos_encoding method here hasn't been correctly implemented and has just been copied from ViT because I made a commit in this current PR regarding patch_size here as patch_size itself wasn't declared in the method. I gave benefit of doubt to the work done during the port of this model and just fixed it. But when I tested the slow tests with RUN_SLOW, test_inference_interpolate_pos_encoding failed because the implementation hasn't been done properly. So, should I raise an issue separately or fix it here.

One thing can be that apart from the test_inference_interpolate_pos_encoding slow test rest can tested by [run_slow] vivit commit message and I will push the training and inference benchmark too just in some time. And when this PR gets merged, I will open a separate issue for test_inference_interpolate_pos_encoding test fix.

Or the other thing can be that I fix it here in this PR.

Please suggest what works well for you?!

Update: This and this commit was enough to fix the interpolate method and the only thing required right now is a nit in test_inference_interpolate_pos_encoding for it to pass, basically a correction in the input being passed. Right now, I'm keeping that nit change in hold because I need a confirmation from your side that to fix test_inference_interpolate_pos_encoding failure in this issue or simply open a separate issue.

And regarding this PR,

Thanks for adding!

Two things before merge:

Running the slow tests for the model. Could you push an empty commit with the message [run_slow] vivit?

Adding info and benchmarks to the model's doc page. A good example is here. Benchmarking scripts from @fxmarty for training and inference.

Both of the things asked has been completed and pushed. So, this PR is ready for merge.

RUFFY-369 · 2024-09-30T09:54:54Z

Hi @amyeroberts , I just created a separate PR where interpolate_pos_encoding for ViViT has been fixed here #33815 , fixing the slow test with it.
Also, your suggestions have already been addressed for this PR so, its ready to be merged.

Please review both of them when you get the time.
Thank you 😄

RUFFY-369 · 2024-09-30T12:31:02Z

@amyeroberts Can you please add run_slow label here as well so that the empty commit can work.
Thanks

amyeroberts · 2024-09-30T12:49:49Z

@RUFFY-369 You should be able to add the label yourself, I think. Could you try? It would be useful to know if external collaborators can or can't add this, as we'd like to make this so that contributors can do as much as possible independently

RUFFY-369 · 2024-09-30T12:54:36Z

@RUFFY-369 You should be able to add the label yourself, I think. Could you try? It would be useful to know if external collaborators can or can't add this, as we'd like to make this so that contributors can do as much as possible independently

@amyeroberts No, I can't. On my side I don't have any gear/settings button near label:

Usually if I can add the label, I would see something like this:

So, yes, I guess external collaborators can't add this

amyeroberts · 2024-09-30T15:57:23Z

@RUFFY-369 OK, thanks for checking! @ydshieh This means the message for the slow model tests bot might need to be changed as contributors can't add the label as instructed

amyeroberts · 2024-09-30T15:58:03Z

@RUFFY-369 I've added the label. Could you push another [run-slow] vivit commit to trigger the tests?

HuggingFaceDocBuilderDev · 2024-09-30T16:00:26Z

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.

RUFFY-369 · 2024-09-30T16:00:50Z

@RUFFY-369 I've added the label. Could you push another [run-slow] vivit commit to trigger the tests?

@amyeroberts Done 👍

HuggingFaceDocBuilderDev · 2024-09-30T16:51:31Z

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.

RUFFY-369 · 2024-10-01T19:38:31Z

Failing tests are unrelated

amyeroberts · 2024-10-01T20:04:42Z

@RUFFY-369 Could you rebase? A fix has been merged in

RUFFY-369 · 2024-10-01T20:28:05Z

@RUFFY-369 Could you rebase? A fix has been merged in

@amyeroberts Done

amyeroberts

Thanks for adding @RUFFY-369!

The only thing missing is a fallback for when output_attentions are passed e.g. like here

cc @qubvel

RUFFY-369 · 2024-10-02T17:12:33Z

@amyeroberts Fallback has been added for output_attentions

Cheers!

RUFFY-369 · 2024-10-02T17:32:40Z

All green, slow tests are left to trigger
If a maintainer can trigger them before merge
cc @amyeroberts

RUFFY-369 · 2024-10-08T07:33:20Z

soft ping @amyeroberts
Thank you

qubvel

Thanks, looks good to me! cc @ArthurZucker for final review

RUFFY-369 · 2024-10-14T15:53:41Z

soft ping @ArthurZucker
Thank you 😄

ArthurZucker

LGTM thanks for the details documentation update!

ArthurZucker · 2024-10-15T09:28:02Z

🤗

* chore:add sdpa to vivit * fix:failing slow test_inference_interpolate_pos_encoding(failing on main branch too) * chore:fix nits * ci:fix repo consistency failure * chore:add info and benchmark to model doc * [run_slow] vivit * chore:revert interpolation test fix for new issue * [run_slow] vivit * [run_slow] vivit * [run_slow] vivit * chore:add fallback for output_attentions being True * [run_slow] vivit * style:make fixup * [run_slow] vivit

RUFFY-369 added 4 commits September 27, 2024 05:10

chore:add sdpa to vivit

c591bc4

fix:failing slow test_inference_interpolate_pos_encoding(failing on m…

9c46bf2

…ain branch too)

chore:fix nits

50b9f1a

ci:fix repo consistency failure

8b613e2

amyeroberts reviewed Sep 27, 2024

View reviewed changes

vignesh1507 approved these changes Sep 27, 2024

View reviewed changes

RUFFY-369 added 3 commits September 28, 2024 02:38

chore:add info and benchmark to model doc

fefd6ab

Merge remote-tracking branch 'upstream/main' into vivit_sdpa

0f5b326

[run_slow] vivit

ce07cf4

chore:revert interpolation test fix for new issue

b05f42d

amyeroberts added the run-slow label Sep 30, 2024

[run_slow] vivit

0de7170

RUFFY-369 added 3 commits October 2, 2024 00:50

[run_slow] vivit

0890226

Merge remote-tracking branch 'upstream/main' into vivit_sdpa

dfd2fba

[run_slow] vivit

eae30b1

Merge remote-tracking branch 'upstream/main' into vivit_sdpa

694060b

amyeroberts reviewed Oct 2, 2024

View reviewed changes

RUFFY-369 added 2 commits October 2, 2024 22:41

chore:add fallback for output_attentions being True

c4a0256

Merge remote-tracking branch 'upstream/main' into vivit_sdpa

1a8abb8

RUFFY-369 added 3 commits October 2, 2024 22:42

[run_slow] vivit

9a4bf14

style:make fixup

4d64c3e

[run_slow] vivit

121c44c

qubvel self-requested a review October 8, 2024 07:38

qubvel added Vision SDPA labels Oct 8, 2024

qubvel approved these changes Oct 8, 2024

View reviewed changes

qubvel requested a review from ArthurZucker October 8, 2024 13:55

ArthurZucker approved these changes Oct 15, 2024

View reviewed changes

ArthurZucker merged commit 293e627 into huggingface:main Oct 15, 2024
15 of 17 checks passed

RUFFY-369 deleted the vivit_sdpa branch October 15, 2024 09:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add sdpa for Vivit #33757

Add sdpa for Vivit #33757

RUFFY-369 commented Sep 27, 2024 •

edited

Loading

RUFFY-369 commented Sep 27, 2024

amyeroberts left a comment

RUFFY-369 commented Sep 27, 2024 •

edited

Loading

vignesh1507 left a comment

RUFFY-369 commented Sep 28, 2024 •

edited

Loading

RUFFY-369 commented Sep 30, 2024 •

edited

Loading

RUFFY-369 commented Sep 30, 2024

amyeroberts commented Sep 30, 2024

RUFFY-369 commented Sep 30, 2024 •

edited

Loading

amyeroberts commented Sep 30, 2024

amyeroberts commented Sep 30, 2024

HuggingFaceDocBuilderDev commented Sep 30, 2024

RUFFY-369 commented Sep 30, 2024

HuggingFaceDocBuilderDev commented Sep 30, 2024

RUFFY-369 commented Oct 1, 2024

amyeroberts commented Oct 1, 2024

RUFFY-369 commented Oct 1, 2024

amyeroberts left a comment

RUFFY-369 commented Oct 2, 2024

RUFFY-369 commented Oct 2, 2024 •

edited

Loading

RUFFY-369 commented Oct 8, 2024

qubvel left a comment

RUFFY-369 commented Oct 14, 2024

ArthurZucker left a comment

ArthurZucker commented Oct 15, 2024

Add sdpa for Vivit #33757

Add sdpa for Vivit #33757

Conversation

RUFFY-369 commented Sep 27, 2024 • edited Loading

What does this PR do?

Before submitting

Who can review?

RUFFY-369 commented Sep 27, 2024

amyeroberts left a comment

Choose a reason for hiding this comment

RUFFY-369 commented Sep 27, 2024 • edited Loading

vignesh1507 left a comment

Choose a reason for hiding this comment

RUFFY-369 commented Sep 28, 2024 • edited Loading

RUFFY-369 commented Sep 30, 2024 • edited Loading

RUFFY-369 commented Sep 30, 2024

amyeroberts commented Sep 30, 2024

RUFFY-369 commented Sep 30, 2024 • edited Loading

amyeroberts commented Sep 30, 2024

amyeroberts commented Sep 30, 2024

HuggingFaceDocBuilderDev commented Sep 30, 2024

RUFFY-369 commented Sep 30, 2024

HuggingFaceDocBuilderDev commented Sep 30, 2024

RUFFY-369 commented Oct 1, 2024

amyeroberts commented Oct 1, 2024

RUFFY-369 commented Oct 1, 2024

amyeroberts left a comment

Choose a reason for hiding this comment

RUFFY-369 commented Oct 2, 2024

RUFFY-369 commented Oct 2, 2024 • edited Loading

RUFFY-369 commented Oct 8, 2024

qubvel left a comment

Choose a reason for hiding this comment

RUFFY-369 commented Oct 14, 2024

ArthurZucker left a comment

Choose a reason for hiding this comment

ArthurZucker commented Oct 15, 2024

RUFFY-369 commented Sep 27, 2024 •

edited

Loading

RUFFY-369 commented Sep 27, 2024 •

edited

Loading

RUFFY-369 commented Sep 28, 2024 •

edited

Loading

RUFFY-369 commented Sep 30, 2024 •

edited

Loading

RUFFY-369 commented Sep 30, 2024 •

edited

Loading

RUFFY-369 commented Oct 2, 2024 •

edited

Loading