Shorten the conversation tests for speed + fixing position overflows by Rocketknight1 · Pull Request #26960 · huggingface/transformers

Rocketknight1 · 2023-10-20T12:43:15Z

Some of the conversation tests were still overflowing the maximum position embeddings for very short models like BlenderBotSmall. I reduced the limits a lot, which also speeds up those tests! cc @ydshieh

HuggingFaceDocBuilderDev · 2023-10-20T13:00:07Z

The documentation is not available anymore as the PR was closed or merged.

ydshieh · 2023-10-20T13:44:37Z

Hi @Rocketknight1 . Thanks a lot. I think it's probably better to simply skip this test for blenderbot small.

Could you remind me why the test was never failing before?

Rocketknight1 · 2023-10-20T16:16:15Z

@ydshieh the test never failed before because the ConversationalPipeline class wasn't working very well, and didn't actually generate any text in these tests!

ydshieh · 2023-10-20T17:06:14Z

Thanks. Then let's just skip it for this model. You can search def is_pipeline_test_to_skip to see how to skip pipeline tests.

Rocketknight1 · 2023-10-20T17:14:29Z

@ydshieh are you sure? I think the test makes more sense with very short outputs anyway - we don't need 20 tokens to confirm the pipeline works!

ydshieh · 2023-10-20T17:28:55Z

That's true, but making it this way also hide the fact of some models fail this test. Having a skip make it explicit. We can definitely use 2 instead of 20 if we do have a gain of running time (not that usually the model used for pipeline testing is very small, and not really obvious to say we will have a gain).

Rocketknight1 · 2023-10-20T19:33:39Z

BlenderBotSmall can pass the test! The problem only arises because the test BlenderBotSmallConfig has max_position_embeddings=20. Maybe I keep the test at a higher number, but increase the BlenderBotSmallConfig in the test to have more position embeddings, so it passes more easily?

ArthurZucker · 2023-10-23T07:03:29Z

Hey! Let's just skip it for now it's blocking a few prs!

ArthurZucker · 2023-10-23T10:33:49Z

I'll merge #27013 so you have time to work on the fix

Rocketknight1 · 2023-10-23T16:04:56Z

cc @ydshieh @ArthurZucker this should be ready to go now! I've skipped the test for BlenderBotSmall as @ydshieh suggested because the problem is in the hf-internal-testing checkpoint, and I don't think the model has much usage anymore. The model actually works fine - the test checkpoint just has a very low value for max_position_embeddings.

I also increased max_new_tokens back to 5 in the general test and removed the workarounds that @ArthurZucker added. I don't think it needs to go higher - it just makes the test slower, and doesn't tell us anything new!

Rocketknight1 · 2023-10-27T15:18:10Z

Quick ping again @ydshieh @ArthurZucker! This PR should be ready to go

ydshieh · 2023-10-24T07:20:11Z

tests/models/blenderbot/test_modeling_blenderbot.py

        hidden_dropout_prob=0.1,
        attention_probs_dropout_prob=0.1,
-        max_position_embeddings=20,
+        max_position_embeddings=50,


This change won't be reflect on the tiny models uploaded to Hub (hf-internal-testing). So the pipeline testing will still use the previous value. We still skip the test, so it is fine, but the change here has no effect.

(the above is just a comment, not a request to change)

ydshieh · 2023-10-24T07:25:07Z

tests/models/blenderbot_small/test_modeling_blenderbot_small.py

+    @unittest.skip("Tiny random model has too few position embeddings for this.")
+    def test_pipeline_conversational(self):
+        pass


Let's put this back to is_pipeline_test_to_skip (above) and just remove # TODO @Rocketnight1 to fix with the comment Tiny random model has too few position embeddings for this.

ydshieh · 2023-10-24T07:25:25Z

tests/models/blenderbot_small/test_modeling_flax_blenderbot_small.py

+    @unittest.skip("Tiny random model has too few position embeddings for this.")
+    def test_pipeline_conversational(self):
+        pass


ydshieh · 2023-10-24T07:25:32Z

tests/models/blenderbot_small/test_modeling_tf_blenderbot_small.py

+    @unittest.skip("Tiny random model has too few position embeddings for this.")
+    def test_pipeline_conversational(self):
+        pass


ydshieh · 2023-10-24T07:25:45Z

tests/pipelines/test_pipelines_conversational.py

    def run_pipeline_test(self, conversation_agent, _):
        # Simple
-        outputs = conversation_agent(Conversation("Hi there!"), max_new_tokens=20)
+        outputs = conversation_agent(Conversation("Hi there!"), max_new_tokens=5)


ydshieh · 2023-10-27T15:42:01Z

Damm, I made review but forgot to commit the comments. Sorry

Rocketknight1 · 2023-10-27T16:56:45Z

@ydshieh Changes made as you suggested - I used is_pipeline_test_to_skip. I left the updated sequence lengths alone because even though they don't fix our tiny-random models, they do fix some other tests, especially for BlenderBot.

amyeroberts

Thanks for fixing!

tests/models/blenderbot_small/test_modeling_blenderbot_small.py

tests/models/blenderbot_small/test_modeling_flax_blenderbot_small.py

tests/models/blenderbot_small/test_modeling_tf_blenderbot_small.py

Co-authored-by: amyeroberts <22614925+amyeroberts@users.noreply.github.com>

…all.py Co-authored-by: amyeroberts <22614925+amyeroberts@users.noreply.github.com>

…l.py Co-authored-by: amyeroberts <22614925+amyeroberts@users.noreply.github.com>

…uggingface#26960) * Shorten the conversation tests for speed + fixing position overflows * Put max_new_tokens back to 5 * Remove test skips * Increase max_position_embeddings in blenderbot tests * Add skips for blenderbot_small * Correct TF test skip * make fixup * Reformat skips to use is_pipeline_test_to_skip * Update tests/models/blenderbot_small/test_modeling_blenderbot_small.py Co-authored-by: amyeroberts <22614925+amyeroberts@users.noreply.github.com> * Update tests/models/blenderbot_small/test_modeling_flax_blenderbot_small.py Co-authored-by: amyeroberts <22614925+amyeroberts@users.noreply.github.com> * Update tests/models/blenderbot_small/test_modeling_tf_blenderbot_small.py Co-authored-by: amyeroberts <22614925+amyeroberts@users.noreply.github.com> --------- Co-authored-by: amyeroberts <22614925+amyeroberts@users.noreply.github.com>

Rocketknight1 requested a review from ydshieh October 20, 2023 12:43

Rocketknight1 added 2 commits October 23, 2023 16:26

Shorten the conversation tests for speed + fixing position overflows

42c4d25

Put max_new_tokens back to 5

f44f2af

Rocketknight1 force-pushed the reduce_conversation_test_length branch from e26da79 to f44f2af Compare October 23, 2023 15:26

Rocketknight1 added 5 commits October 23, 2023 16:28

Remove test skips

131754f

Increase max_position_embeddings in blenderbot tests

6922575

Add skips for blenderbot_small

23cddb1

Correct TF test skip

1d6f7b6

make fixup

d4eaeea

Rocketknight1 requested a review from ArthurZucker October 27, 2023 15:18

ydshieh reviewed Oct 27, 2023

View reviewed changes

ydshieh approved these changes Oct 27, 2023

View reviewed changes

Reformat skips to use is_pipeline_test_to_skip

bfd2813

amyeroberts approved these changes Oct 30, 2023

View reviewed changes

Rocketknight1 and others added 3 commits October 31, 2023 14:00

Update tests/models/blenderbot_small/test_modeling_blenderbot_small.py

0421bb9

Co-authored-by: amyeroberts <22614925+amyeroberts@users.noreply.github.com>

Update tests/models/blenderbot_small/test_modeling_flax_blenderbot_sm…

5b19e9c

…all.py Co-authored-by: amyeroberts <22614925+amyeroberts@users.noreply.github.com>

Update tests/models/blenderbot_small/test_modeling_tf_blenderbot_smal…

6d5a2b4

…l.py Co-authored-by: amyeroberts <22614925+amyeroberts@users.noreply.github.com>

Rocketknight1 merged commit 08fadc8 into main Oct 31, 2023

Rocketknight1 deleted the reduce_conversation_test_length branch October 31, 2023 14:20

Conversation

Rocketknight1 commented Oct 20, 2023

Uh oh!

HuggingFaceDocBuilderDev commented Oct 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ydshieh commented Oct 20, 2023

Uh oh!

Rocketknight1 commented Oct 20, 2023

Uh oh!

ydshieh commented Oct 20, 2023

Uh oh!

Rocketknight1 commented Oct 20, 2023

Uh oh!

ydshieh commented Oct 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Rocketknight1 commented Oct 20, 2023

Uh oh!

ArthurZucker commented Oct 23, 2023

Uh oh!

ArthurZucker commented Oct 23, 2023

Uh oh!

Rocketknight1 commented Oct 23, 2023

Uh oh!

Rocketknight1 commented Oct 27, 2023

Uh oh!

ydshieh Oct 24, 2023

Choose a reason for hiding this comment

Uh oh!

ydshieh Oct 27, 2023

Choose a reason for hiding this comment

Uh oh!

ydshieh Oct 24, 2023

Choose a reason for hiding this comment

Uh oh!

Rocketknight1 Oct 27, 2023

Choose a reason for hiding this comment

Uh oh!

ydshieh Oct 24, 2023

Choose a reason for hiding this comment

Uh oh!

ydshieh Oct 24, 2023

Choose a reason for hiding this comment

Uh oh!

ydshieh Oct 24, 2023

Choose a reason for hiding this comment

Uh oh!

ydshieh commented Oct 27, 2023

Uh oh!

Rocketknight1 commented Oct 27, 2023

Uh oh!

amyeroberts left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

HuggingFaceDocBuilderDev commented Oct 20, 2023 •

edited

Loading

ydshieh commented Oct 20, 2023 •

edited

Loading