Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Whisper CI #34617

Merged
merged 3 commits into from
Nov 18, 2024
Merged

Fix Whisper CI #34617

merged 3 commits into from
Nov 18, 2024

Conversation

ydshieh
Copy link
Collaborator

@ydshieh ydshieh commented Nov 5, 2024

Finally CI is green

@HuggingFaceDocBuilderDev

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.

@ydshieh ydshieh changed the title Revert "Revert "Fix Whisper CI"" "Fix Whisper CI"" Nov 5, 2024
Comment on lines 400 to +405
else: # by default let's always generate 10 new tokens
generation_config.max_length = generation_config.max_length + input_ids_seq_length
if generation_config.max_length == GenerationConfig().max_length:
generation_config.max_length = generation_config.max_length + input_ids_seq_length
max_position_embeddings = getattr(self.config, "max_position_embeddings", None)
if max_position_embeddings is not None:
generation_config.max_length = min(generation_config.max_length, max_position_embeddings)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Diverge a bit from torch one, but let's try to match them in a follow-up when we feel it's time to do it.
Here is just to make CI green

@ydshieh ydshieh marked this pull request as ready for review November 5, 2024 15:46
@ydshieh ydshieh changed the title "Fix Whisper CI"" Fix Whisper CI Nov 5, 2024
@ydshieh
Copy link
Collaborator Author

ydshieh commented Nov 5, 2024

tests/models/rt_detr/test_image_processing_rt_detr.py::RtDetrImageProcessingTest::test_fast_is_faster_than_slow

it's flaky

@LysandreJik
Copy link
Member

Should this be merged?

@ydshieh
Copy link
Collaborator Author

ydshieh commented Nov 18, 2024

Should this be merged?

Yes, but I am waiting for an approval. Currently, whisper has ~170 failed tests without this PR.

Copy link
Collaborator

@ArthurZucker ArthurZucker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! 🤗

Comment on lines +1455 to +1459
if generation_config.max_length == GenerationConfig().max_length:
generation_config.max_length = generation_config.max_length + input_ids_length
max_position_embeddings = getattr(self.config, "max_position_embeddings", None)
if max_position_embeddings is not None:
generation_config.max_length = min(generation_config.max_length, max_position_embeddings)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for putting this back!

@ydshieh ydshieh merged commit eed11f3 into main Nov 18, 2024
22 of 25 checks passed
@ydshieh ydshieh deleted the revert-34605-revert-34541-fix_whisper branch November 18, 2024 20:37
@gante
Copy link
Member

gante commented Nov 19, 2024

Hey guys 👋 @ydshieh @ArthurZucker

The generate changes in this PR are incorrect (changing max_length) :) I'm reverting them as part of #34807, but I'd recommend rolling back this PR.

If Whisper's tests are failing, then it is probably something wrong with either whisper or whisper's tests

@ydshieh
Copy link
Collaborator Author

ydshieh commented Nov 19, 2024

Hi @gante This PR is only adding

if generation_config.max_length == GenerationConfig().max_length

to avoid the failure caused in #34377 (which was an attempt fix for #34026)

The generate changes in this PR are incorrect

Is the code changes in the previous PR #34377 (and/or #34026) are already wrong, or it's wrong because of the if condition added in this PR?

@gante
Copy link
Member

gante commented Nov 19, 2024

Yeah I think it's from the older pipeline PR 😭 (generation_config.max_length = generation_config.max_length + input_ids_seq_length shouldn't exist)

@gante
Copy link
Member

gante commented Nov 19, 2024

But the check for max_position_embeddings should exist either -- some models can generate beyond this value (e.g. llama)

@ydshieh
Copy link
Collaborator Author

ydshieh commented Nov 19, 2024

So if we want to rollback, it's more involved (i.e. not revert this PR but some previous changes too).
In any case, ping me before merging a new change on this PR so I can see if the Whisper CI is still OK 🙏 .
(or if you want to check that part on your side 😄 )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants