Update transformers tests generation util v4.45.2 #1441

malkomes · 2024-10-18T14:40:48Z

What does this PR do?

We sync the tests test_utils.py with transformers v4.45.2.

To facilitate review, please check the PRs merged to this branch first:

The list of modifications are:

Update all the tests to transformers v4.45.2.
Mark tests that are not ready with failure.

malkomes · 2024-10-23T16:10:20Z

Running

optimum-habana/tests/transformers/tests/generation# python -m pytest test_utils.py

on main give us:

23 failed, 16 passed, 9 skipped, 14 warnings

this branch increases the number of tests and mark Xfail the tests that need to be updated.

32 passed, 26 skipped, 21 xfailed, 16 warnings

malkomes · 2024-10-23T16:12:28Z

Hello @yafshar, here's the PR to update the test_utils synced with main.

yafshar · 2024-10-23T22:49:42Z

@malkomes thanks for the update. I will start checking the code

conftest.py

add license Co-authored-by: Yaser Afshar <[email protected]>

yafshar · 2024-10-25T20:52:12Z

optimum/habana/transformers/generation/utils.py

+                torch.ones((batch_size, 1), dtype=torch.long, device=device) * decoder_start_token_id
+            )
+
+        if token_idx is not None:


@malkomes this is a bit confusing! Are you repeating the same ops again here? Why did you remove the if condition in the first place if you want to repeat the same operation again

decoder_start_token_id = ( torch.ones((batch_size, 1), dtype=torch.long, device=device) * decoder_start_token_id )

It should only have the max_length and padding when token_idx is not None

Well, here we are adding a logic to handle the case where we have a batch of tokens on the decoder_start_token_id. And, indeed. there different ways to implement this. In fact, in the other PR I did something a little bit "more cute" where I handle the decoder_start_token_id batch case and the static shape with more code changes using different variables names and etc.

This time I realize that it's much easier to maintain the code if we keep the code similar to the transformers code. It so happens that the max_length padding works for batch and non-batch if we do it afterwards.

I was hoping that my comments here would explain it: malkomes#2 let me know if that helps or not. And maybe we should add notes on the code since it is a little bit confusing.

thanks for the explanation, I agree for readability and maintainability, your separation makes sense!

optimum/habana/transformers/generation/utils.py

optimum/habana/transformers/models/bart/modeling_bart.py

tests/transformers/tests/generation/test_framework_agnostic.py

yafshar · 2024-10-25T21:51:53Z

tests/transformers/tests/generation/test_utils.py

-        input_ids = inputs_dict[self.input_name]
+        # TODO: @raushan or @gante, use `model.main_input_name` as the main input instead of relyinn on `input_ids`
+        input_ids = inputs_dict.pop(self.input_name)[:batch_size, :]
+        inputs_dict.pop("attention_mask", None)


Why are you removing attention_mask for these tests?

Is there a reason we don’t use filtered_inputs_dict similar to how it’s done in Transformers? Keeping the code closer to the upstream implementation would make it easier to maintain.

I see, you are looking at the most recent transformer version here

I was updating it based on 4.45.2

I think for this function there's no harm on using the latest version. Let me try.

There are several modifications from 4.45.2 to 4.46. We can work on that after this PR. What do you think?

@malkomes Thank you for checking this. Let’s stick with version 4.45.2 for now. We can plan to update to 4.46 later. The next version has some other changes

sounds good! Yeah, the 4.46 does a refactor on the tests, so it's better to work on a new PR for 4.46, and I'm happy to do it once we finish this one. (:

tests/transformers/tests/generation/test_utils.py

Co-authored-by: Yaser Afshar <[email protected]>

yafshar · 2024-10-25T22:11:43Z

tests/transformers/tests/generation/test_utils.py

+
+        # It is important set set the eos_token_id to None to ensure that no sequences
+        # shorter than `max_length` can be generated
+        config.eos_token_id = None


@malkomes this is confusing again!

Above we are setting config.eos_token_id = [config.eos_token_id] and here you are un-setting it config.eos_token_id = None?
If it is needed to be set to None, we should remove the upper code and just set config.pad_token_id there

great catch! that's something that comes from the upstream version that I was referring. I'll fix it.

yafshar · 2024-10-28T19:57:35Z

tests/transformers/tests/generation/test_utils.py

-                    eos_token_id=bart_model.config.eos_token_id,
-                    **model_kwargs,
-                )
+        max_new_tokens = 20


Suggested change

max_new_tokens = 20

# Controlling max_length via the configuration is deprecated in favor of max_new_tokens

max_new_tokens = 20

https://github.com/malkomes/optimum-habana/blob/658decbc459e2493b1d8779fdf8d79b73eb2c2d6/tests/transformers/tests/generation/test_utils.py#L2479

yafshar · 2024-10-28T19:58:28Z

tests/transformers/tests/generation/test_utils.py

-            )
+        min_length = 10
+        input_len = input_ids.shape[-1]
+        out_gen = model.generate(input_ids=input_ids, min_length=min_length, max_new_tokens=20)


the same as above, please add a comment for the change

https://github.com/malkomes/optimum-habana/blob/658decbc459e2493b1d8779fdf8d79b73eb2c2d6/tests/transformers/tests/generation/test_utils.py#L2494

tests/transformers/tests/generation/test_utils.py

yafshar · 2024-10-28T20:03:10Z

@malkomes thanks a lot. I finished my first check. Would you please address the comments to wrap up and finish this PR.

yafshar · 2024-10-28T21:40:23Z

@malkomes I just checked one of the failure test_cfg_mixin I think we need to enable it for it optimum-habana and update the test. In this one, the gpt2 model does not have a pad token and will create st completely wrong

generated_text=['The dragon flew over Paris, landing on the outskirts of the city centre. He landed safely on the outskirts of Paris, landing on the outskirts of Paris, landing on the outskirts of Paris mosqu']

If I add below to the test

        if tokenizer.pad_token is None:
            tokenizer.pad_token = tokenizer.eos_token
            model.generation_config.pad_token_id = model.generation_config.eos_token_id

It will create

generated_text=['The dragon flew over Paris, landing on the roof of the hotel where he was staying with his wife and children. He then proceeded to climb onto the roof of the hotel where he met his']

different from the original test, but working on optimum-habana, probably should be marked as x

testing Co-authored-by: Yaser Afshar <[email protected]>

Co-authored-by: Yaser Afshar <[email protected]>

malkomes · 2024-11-04T23:13:20Z

@yafshar I think I got all the changes. Let me know if I've missed something. I was mostly comparing this test_utils.py file with the transformers one tagged with 4.45.2. Thanks again for your careful and great review

yafshar · 2024-11-04T23:43:43Z

@malkomes thanks for the updates.

with this PR

>>> RUN_SLOW=true GAUDI2_CI=1 python -m pytest test_utils.py
test_utils.py .xssxx..xx........x.....sx.x...xxx.......xsxsx...x.xx.xxx..xxxxxs..sx.s.sssssss                                      [100%]
38 passed, 15 skipped, 26 xfailed, 19 warnings in 301.97s (0:05:01)

yafshar

LGTM!

@regisss @ssarkar2 this PR is ready. Please check!

malkomes · 2024-11-25T16:36:20Z

@regisss @ssarkar2 can you take a look at this proposal to sync the tests? thanks

emascarenhas

Could you run through slow tests with latest 1.19 docker on 1.19 host?

yafshar · 2024-11-27T12:44:56Z

I ran the test on 1.19

>>> RUN_SLOW=true GAUDI2_CI=1 python -m pytest test_utils.py
test_utils.py .xssxx..xx........x.....sx.x...xxx.......xsxsx...x.xx.xxx..xxxxxs..sx.s.sssssss 
38 passed, 15 skipped, 26 xfailed, 19 warnings in 295.73s (0:04:55)

regisss

LGTM! I just left a couple of questions to understand better the changes to conftest.py and pyproject.toml 🙂

pyproject.toml

conftest.py

malkomes · 2024-12-11T15:39:52Z

@regisss sorry for taking long to reply. The last commit should have fix it. Let me know if anything else is missing ;-)

github-actions · 2024-12-11T16:31:06Z

The code quality check failed, please run make style.

…date_transformers

HuggingFaceDocBuilderDev · 2024-12-11T16:34:21Z

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.

regisss

LGTM! No worries @malkomes

Co-authored-by: Gustavo <gustavo.malkomes> Co-authored-by: Yaser Afshar <[email protected]> Co-authored-by: regisss <[email protected]>

malkomes requested a review from regisss as a code owner October 18, 2024 14:40

malkomes marked this pull request as draft October 18, 2024 14:41

Gustavo and others added 6 commits October 23, 2024 15:40

initial pass

81d2737

refactor tests and add xfails

2c73aed

add one more xfail

da75d36

enable tests and fix modeling_bart issue with cache

476d3b5

fix bach decoder start token id

1551ef7

couple of test updates (#3)

46b89e3

malkomes force-pushed the test_utils_update_transformers branch from ff28ba3 to 46b89e3 Compare October 23, 2024 15:40

malkomes marked this pull request as ready for review October 23, 2024 16:00

malkomes requested review from ssarkar2, bhargaveede and vivekgoe as code owners October 23, 2024 16:00

yafshar reviewed Oct 23, 2024

View reviewed changes

conftest.py Show resolved Hide resolved

yafshar reviewed Oct 24, 2024

View reviewed changes

conftest.py Show resolved Hide resolved

malkomes and others added 4 commits October 24, 2024 09:28

Update conftest.py

3a59f88

add license Co-authored-by: Yaser Afshar <[email protected]>

add pytest_sessionfinish and other lines from transformers pytest

04e4190

conftest more lines

4187282

make style

eaf3d3a

yafshar reviewed Oct 25, 2024

View reviewed changes

optimum/habana/transformers/generation/utils.py Outdated Show resolved Hide resolved

yafshar reviewed Oct 25, 2024

View reviewed changes

optimum/habana/transformers/models/bart/modeling_bart.py Outdated Show resolved Hide resolved

yafshar reviewed Oct 25, 2024

View reviewed changes

tests/transformers/tests/generation/test_framework_agnostic.py Outdated Show resolved Hide resolved

yafshar reviewed Oct 25, 2024

View reviewed changes

tests/transformers/tests/generation/test_utils.py Outdated Show resolved Hide resolved

add is not none

9b53a6f

Co-authored-by: Yaser Afshar <[email protected]>

yafshar reviewed Oct 25, 2024

View reviewed changes

yafshar reviewed Oct 28, 2024

View reviewed changes

tests/transformers/tests/generation/test_utils.py Outdated Show resolved Hide resolved

yafshar reviewed Oct 28, 2024

View reviewed changes

tests/transformers/tests/generation/test_utils.py Show resolved Hide resolved

malkomes and others added 7 commits November 4, 2024 10:31

removing extra space

6a827b1

testing Co-authored-by: Yaser Afshar <[email protected]>

add space

f342a64

Co-authored-by: Yaser Afshar <[email protected]>

adding review comments

8623c35

minor changes

0c3d6b1

mark slow tests

7b4760f

fixing test_beam_sample_generate

658decb

update comment

d631a78

yafshar approved these changes Nov 4, 2024

View reviewed changes

emascarenhas reviewed Nov 26, 2024

View reviewed changes

regisss reviewed Nov 29, 2024

View reviewed changes

pyproject.toml Show resolved Hide resolved

conftest.py Show resolved Hide resolved

regisss reviewed Dec 8, 2024

View reviewed changes

conftest.py Show resolved Hide resolved

adding back

f23ef72

regisss added 2 commits December 11, 2024 16:33

Merge remote-tracking branch 'optimum-habana/main' into test_utils_up…

4375c57

…date_transformers

Make style

a44fc10

regisss approved these changes Dec 11, 2024

View reviewed changes

regisss merged commit 2ba520a into huggingface:main Dec 11, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update transformers tests generation util v4.45.2 #1441

Update transformers tests generation util v4.45.2 #1441

malkomes commented Oct 18, 2024 •

edited

Loading

malkomes commented Oct 23, 2024

malkomes commented Oct 23, 2024

yafshar commented Oct 23, 2024

yafshar Oct 25, 2024

yafshar Oct 25, 2024 •

edited

Loading

malkomes Oct 25, 2024

yafshar Oct 28, 2024

yafshar Oct 25, 2024

yafshar Oct 25, 2024 •

edited

Loading

malkomes Nov 4, 2024

malkomes Nov 4, 2024

yafshar Nov 4, 2024

malkomes Nov 5, 2024

yafshar Oct 25, 2024

malkomes Oct 25, 2024

yafshar Oct 28, 2024

malkomes Nov 4, 2024

yafshar Oct 28, 2024

malkomes Nov 4, 2024

yafshar commented Oct 28, 2024

yafshar commented Oct 28, 2024

malkomes commented Nov 4, 2024

yafshar commented Nov 4, 2024

yafshar left a comment

malkomes commented Nov 25, 2024

emascarenhas left a comment •

edited

Loading

yafshar commented Nov 27, 2024

regisss left a comment

malkomes commented Dec 11, 2024

github-actions bot commented Dec 11, 2024

HuggingFaceDocBuilderDev commented Dec 11, 2024

regisss left a comment

	max_new_tokens = 20
	# Controlling max_length via the configuration is deprecated in favor of max_new_tokens
	max_new_tokens = 20

Update transformers tests generation util v4.45.2 #1441

Update transformers tests generation util v4.45.2 #1441

Conversation

malkomes commented Oct 18, 2024 • edited Loading

What does this PR do?

malkomes commented Oct 23, 2024

malkomes commented Oct 23, 2024

yafshar commented Oct 23, 2024

Choose a reason for hiding this comment

yafshar Oct 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yafshar Oct 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yafshar commented Oct 28, 2024

yafshar commented Oct 28, 2024

malkomes commented Nov 4, 2024

yafshar commented Nov 4, 2024

yafshar left a comment

Choose a reason for hiding this comment

malkomes commented Nov 25, 2024

emascarenhas left a comment • edited Loading

Choose a reason for hiding this comment

yafshar commented Nov 27, 2024

regisss left a comment

Choose a reason for hiding this comment

malkomes commented Dec 11, 2024

github-actions bot commented Dec 11, 2024

HuggingFaceDocBuilderDev commented Dec 11, 2024

regisss left a comment

Choose a reason for hiding this comment

malkomes commented Oct 18, 2024 •

edited

Loading

yafshar Oct 25, 2024 •

edited

Loading

yafshar Oct 25, 2024 •

edited

Loading

emascarenhas left a comment •

edited

Loading