Add tests for batching support #29297

zucchini-nlp · 2024-02-26T13:23:37Z

What does this PR do?

Fixes #29179 . Currently the tests are passing for all models, and batched and unbatched outputs have nearly same outputs.

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?

@gante

HuggingFaceDocBuilderDev · 2024-02-26T13:43:40Z

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.

gante · 2024-02-26T18:12:00Z

@zucchini-nlp Running your test script on my setup I get

tensor(0., device='cuda:0')
tensor(4.7684e-07, device='cuda:0')

Could it be due to different hardware and/or versions? 🤔 For context, I'm running with CUDA 12.1, torch==2.3.0.dev20240208+cu121, and an nvidia RTX3090

gante

Thank you for working on this important test case 🔥

src/transformers/models/fastspeech2_conformer/modeling_fastspeech2_conformer.py

src/transformers/models/tvp/modeling_tvp.py

tests/models/fastspeech2_conformer/test_modeling_fastspeech2_conformer.py

tests/models/timm_backbone/test_modeling_timm_backbone.py

tests/test_modeling_common.py

…ech2_conformer.py Co-authored-by: Joao Gante <[email protected]>

Co-authored-by: Joao Gante <[email protected]>

zucchini-nlp · 2024-02-27T12:29:05Z

Using cosine distance to check cv/audio models helped to solve the issue. The way we check if the model is conv-based or not might not be the best option, I will see if I can make it better.

Now everything is passing the test, yay!

gante

The end result is very cool! Thank you for iterating on this 🔥

tests/models/vilt/test_modeling_vilt.py

gante · 2024-02-28T12:05:08Z

Next steps:

make fixup :D
LayoutLMv2ModelTest seems to be failing. Perhaps skip it as well?
Update the PR header (we no longer have issues with CV models)
Hit ready to review so it is no longer a draft PR

Then:

Tag both core maintainers (Arthur to review since he's on rotation, Amy to have a look on the CV model skips)

Co-authored-by: Joao Gante <[email protected]>

zucchini-nlp · 2024-02-28T13:49:32Z

@amyeroberts @ArthurZucker ready for review

gante · 2024-02-28T14:55:39Z

@amyeroberts: you're not on rotation, but this PR includes a new test that, to make CI green, @zucchini-nlp had to a) make minor CV model changes; b) add skips on CV models 🤗

Let us know if you find anything wrong! If the skips are not supposed to be skips, then I'd like to ask to keep them as TODOs so as to be tended by someone with CV expertise 🙏

amyeroberts

Thanks for adding this - this is a great addition to making sure our models are well behaved 💪

Just a few small comments

tests/models/timm_backbone/test_modeling_timm_backbone.py

tests/models/vilt/test_modeling_vilt.py

tests/models/mra/test_modeling_mra.py

tests/models/longformer/test_modeling_longformer.py

tests/models/layoutlmv2/test_modeling_layoutlmv2.py

tests/test_modeling_common.py

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

This reverts commit 525f3a0.

amyeroberts

Thanks for adding - great work! 💪

tests/test_modeling_common.py

amyeroberts · 2024-03-05T12:56:33Z

tests/test_modeling_common.py

+                if "head_mask" in key:
+                    single_row_input[key] = value


I don't think we need this condition given the if/elif structure and L776

Done, fixed the cases when batch_size == num_hidden_layers in certain testers, so there's no need for extra check now. Removed this if

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

gante · 2024-03-05T19:25:09Z

@zucchini-nlp the change in batch size has made the whisper tests unhappy :D after that is cleared up, we can merge this PR 🤗

ArthurZucker

Very nice PR! Thanks

FYI @ydshieh 😉

zucchini-nlp · 2024-03-07T14:49:49Z

This PR is now ready to be merged. An unexpected bug was found yesterday, but it's fixed already. I will also merge main to resolve conflicts
@gante

gante · 2024-03-08T14:00:47Z

@zucchini-nlp needs a make fixup and it is ready to be merged

ydshieh · 2024-03-13T14:08:38Z

Hi @zucchini-nlp Thank you for your work. Our CI reports 2 failing tests:

tests/models/mamba/test_modeling_mamba.py::MambaModelTest::test_batching_equivalence
(line 726)  AttributeError: 'MambaCache' object has no attribute 'dim'

and

tests/models/seggpt/test_modeling_seggpt.py::SegGptModelTest::test_batching_equivalence
(line 765)  AssertionError: tensor(False, device='cuda:0') is not true : Batched and Single row outputs are not equal in SegGptModel for key=hidden_states. Difference=0.1510644555091858.

If you can take a look on these, that would be great 🙏 Thank you.

gante · 2024-03-13T14:23:22Z

@ydshieh (and @amyeroberts, since you're working on the test fetcher): in this case, the new failures arise because the branch merged in this PR, which adds a new mixin test, was out of sync with main with respect to new models. Is there any way we can prevent this without too much effort?

I'm not seeing an automated way to do it. The only thing that comes to mind is to ask to rebase before merging this type of PRs :D

ydshieh · 2024-03-14T08:14:29Z

@gante Yeah, I don't see an satisfying way neither.

One possibility is to change circleCI config file to rebase (on the fly) on the latest main during the CI runs.

gante · 2024-03-14T10:04:24Z

One possibility is to change circleCI config file to rebase (on the fly) on the latest main during the CI runs.

@ydshieh That could be... interesting! Try to rebase with main, roll back if a conflict arises, then run tests. If an informative message is added in the report (e.g. "the tests were run after rebasing on main"), we might save ourselves a few situations like this :D

amyeroberts · 2024-03-14T10:10:50Z

One possibility is to change circleCI config file to rebase (on the fly) on the latest main during the CI runs.

I'm not sure this is a great idea, for two reasons:

the code version being used to run tests is not the version on the branch, which can lead to all sorts of confusion
It's pretty common in development to not rebase for a few commits. Even though technically PRs should be opened when the branch is review ready, this isn't what happens. It's going to end up forcing people to constantly rebase and force push, even if there aren't meaningful commits being added. As someone who gets a notification for every commit being pushed, I'd really rather avoid this.

To test this out, I'd rather have an optional feature, similar to the run all tests command we can have in commits e.g. run-with-base, than we can comment on github, or ask users to run asa final step.

gante · 2024-03-14T10:24:17Z

than we can comment on github

That would also be quite cool -- and could save some time from the core maintainers, if the other reviewers diligently test against main on wide-reaching changes before merging 🤗

ydshieh · 2024-03-14T10:40:26Z

Totally agree with @amyeroberts that's why I say I don't see an satisfying way

* add tests for batching support * Update src/transformers/models/fastspeech2_conformer/modeling_fastspeech2_conformer.py Co-authored-by: Joao Gante <[email protected]> * Update src/transformers/models/fastspeech2_conformer/modeling_fastspeech2_conformer.py Co-authored-by: Joao Gante <[email protected]> * Update tests/test_modeling_common.py Co-authored-by: Joao Gante <[email protected]> * Update tests/test_modeling_common.py Co-authored-by: Joao Gante <[email protected]> * Update tests/test_modeling_common.py Co-authored-by: Joao Gante <[email protected]> * fixes and comments * use cosine distance for conv models * skip mra model testing * Update tests/models/vilt/test_modeling_vilt.py Co-authored-by: Joao Gante <[email protected]> * finzalize and make style * check model type by input names * Update tests/models/vilt/test_modeling_vilt.py Co-authored-by: amyeroberts <[email protected]> * fixed batch size for all testers * Revert "fixed batch size for all testers" This reverts commit 525f3a0. * add batch_size for all testers * dict from model output * do not skip layoutlm * bring back some code from git revert * Update tests/test_modeling_common.py Co-authored-by: amyeroberts <[email protected]> * Update tests/test_modeling_common.py Co-authored-by: amyeroberts <[email protected]> * clean-up * where did minus go in tolerance * make whisper happy * deal with consequences of losing minus * deal with consequences of losing minus * maskformer needs its own test for happiness * fix more models * tag flaky CV models from Amy's approval * make codestyle --------- Co-authored-by: Joao Gante <[email protected]> Co-authored-by: amyeroberts <[email protected]>

add tests for batching support

48f9d0a

zucchini-nlp marked this pull request as draft February 26, 2024 13:23

gante reviewed Feb 26, 2024

View reviewed changes

zucchini-nlp and others added 8 commits February 27, 2024 00:25

Update src/transformers/models/fastspeech2_conformer/modeling_fastspe…

d5c8ff8

…ech2_conformer.py Co-authored-by: Joao Gante <[email protected]>

Update src/transformers/models/fastspeech2_conformer/modeling_fastspe…

23e6142

…ech2_conformer.py Co-authored-by: Joao Gante <[email protected]>

Update tests/test_modeling_common.py

8b15059

Co-authored-by: Joao Gante <[email protected]>

Update tests/test_modeling_common.py

77399c5

Co-authored-by: Joao Gante <[email protected]>

Update tests/test_modeling_common.py

cb2466c

Co-authored-by: Joao Gante <[email protected]>

fixes and comments

e30de97

use cosine distance for conv models

0f6662a

skip mra model testing

20b74e4

gante approved these changes Feb 28, 2024

View reviewed changes

tests/models/vilt/test_modeling_vilt.py Outdated Show resolved Hide resolved

zucchini-nlp and others added 3 commits February 28, 2024 17:30

Update tests/models/vilt/test_modeling_vilt.py

f5b9c96

Co-authored-by: Joao Gante <[email protected]>

finzalize and make style

c4cd9d5

check model type by input names

9c9b562

zucchini-nlp marked this pull request as ready for review February 28, 2024 13:49

gante requested a review from ArthurZucker February 28, 2024 14:48

gante requested a review from amyeroberts February 28, 2024 14:56

amyeroberts reviewed Feb 28, 2024

View reviewed changes

zucchini-nlp and others added 2 commits February 29, 2024 14:10

Update tests/models/vilt/test_modeling_vilt.py

580b73e

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

fixed batch size for all testers

525f3a0

zucchini-nlp mentioned this pull request Feb 29, 2024

Mra models have nan in output last_hidden_states #29373

Closed

4 tasks

zucchini-nlp added 2 commits March 4, 2024 09:25

Revert "fixed batch size for all testers"

c7905f3

This reverts commit 525f3a0.

add batch_size for all testers

49a256f

amyeroberts approved these changes Mar 5, 2024

View reviewed changes

zucchini-nlp and others added 3 commits March 5, 2024 22:27

Update tests/test_modeling_common.py

958e8ec

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

Update tests/test_modeling_common.py

0f782bc

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

clean-up

b3f0ea5

ArthurZucker approved these changes Mar 6, 2024

View reviewed changes

zucchini-nlp added 6 commits March 6, 2024 10:14

where did minus go in tolerance

c2dbd3a

make whisper happy

a69901c

deal with consequences of losing minus

c556b3a

deal with consequences of losing minus

1d77be5

maskformer needs its own test for happiness

4696875

fix more models

e6fe22e

zucchini-nlp mentioned this pull request Mar 7, 2024

Track for flaky batching tests #29516

Open

4 tasks

tag flaky CV models from Amy's approval

25de9c9

Merge 'upstream/main' into batching_test

06aead0

make codestyle

b14cfe8

gante merged commit 8e64ba2 into huggingface:main Mar 12, 2024
18 checks passed

ydshieh mentioned this pull request Mar 13, 2024

Fix multi_gpu_data_parallel_forward for MusicgenTest #29632

Merged

zucchini-nlp mentioned this pull request Mar 13, 2024

Fix batching tests for new models (Mamba and SegGPT) #29633

Merged

5 tasks

voidism mentioned this pull request Mar 22, 2024

Generate: Add new decoding strategy "DoLa" in .generate() #29619

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tests for batching support #29297

Add tests for batching support #29297

zucchini-nlp commented Feb 26, 2024 •

edited

Loading

HuggingFaceDocBuilderDev commented Feb 26, 2024

gante commented Feb 26, 2024

gante left a comment

zucchini-nlp commented Feb 27, 2024

gante left a comment

gante commented Feb 28, 2024 •

edited

Loading

zucchini-nlp commented Feb 28, 2024

gante commented Feb 28, 2024

amyeroberts left a comment

amyeroberts left a comment

amyeroberts Mar 5, 2024

zucchini-nlp Mar 5, 2024

gante commented Mar 5, 2024

ArthurZucker left a comment

zucchini-nlp commented Mar 7, 2024

gante commented Mar 8, 2024

ydshieh commented Mar 13, 2024

gante commented Mar 13, 2024 •

edited

Loading

ydshieh commented Mar 14, 2024

gante commented Mar 14, 2024

amyeroberts commented Mar 14, 2024

gante commented Mar 14, 2024

ydshieh commented Mar 14, 2024 •

edited

Loading

Add tests for batching support #29297

Add tests for batching support #29297

Conversation

zucchini-nlp commented Feb 26, 2024 • edited Loading

What does this PR do?

Before submitting

Who can review?

HuggingFaceDocBuilderDev commented Feb 26, 2024

gante commented Feb 26, 2024

gante left a comment

Choose a reason for hiding this comment

zucchini-nlp commented Feb 27, 2024

gante left a comment

Choose a reason for hiding this comment

gante commented Feb 28, 2024 • edited Loading

zucchini-nlp commented Feb 28, 2024

gante commented Feb 28, 2024

amyeroberts left a comment

Choose a reason for hiding this comment

amyeroberts left a comment

Choose a reason for hiding this comment

amyeroberts Mar 5, 2024

Choose a reason for hiding this comment

zucchini-nlp Mar 5, 2024

Choose a reason for hiding this comment

gante commented Mar 5, 2024

ArthurZucker left a comment

Choose a reason for hiding this comment

zucchini-nlp commented Mar 7, 2024

gante commented Mar 8, 2024

ydshieh commented Mar 13, 2024

gante commented Mar 13, 2024 • edited Loading

ydshieh commented Mar 14, 2024

gante commented Mar 14, 2024

amyeroberts commented Mar 14, 2024

gante commented Mar 14, 2024

ydshieh commented Mar 14, 2024 • edited Loading

zucchini-nlp commented Feb 26, 2024 •

edited

Loading

gante commented Feb 28, 2024 •

edited

Loading

gante commented Mar 13, 2024 •

edited

Loading

ydshieh commented Mar 14, 2024 •

edited

Loading