Skip to content

Conversation

@zucchini-nlp
Copy link
Member

What does this PR do?

Whisper code was changed recently to use prepare_generation_config from the GenerationMixin here, which started causing some bugs. See failing test in this PR.

Before, Whisper generation config did not do any generation_config.update and manually updated kwargs along the way. After the above change, kwargs cannot be used anymore because it contains unused kwargs from generation config.

This PR replaces everywhere we had kwargs.pop() with generaion_config. All tests are passing, except for some slow tests. But the slow tests are not passing in main also, so should be unrelated.

@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.

Copy link
Contributor

@gante gante left a comment

Choose a reason for hiding this comment

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

LGTM, thank you for fixing 💛

(I am surprised that no failures were raised in the previous PR's CI 👀 )

@gante gante requested a review from ArthurZucker April 3, 2024 14:19
@ydshieh
Copy link
Collaborator

ydshieh commented Apr 3, 2024

likely test fetcher didn't find it ...

@ydshieh
Copy link
Collaborator

ydshieh commented Apr 3, 2024

well, i see , whisper is not in IMPORTANT_MODELS

IMPORTANT_MODELS = [
"auto",
# Most downloaded models
"bert",
"clip",
"t5",
"xlm-roberta",
"gpt2",
"bart",
"mpnet",
"gpt-j",
"wav2vec2",
"deberta-v2",
"layoutlm",
"llama",
"opt",
"longformer",
"vit",
# Pipeline-specific model (to be sure each pipeline has one model in this list)
"tapas",
"vilt",
"clap",
"detr",
"owlvit",
"dpt",
"videomae",
]

should we put it ... 😄 @sanchit-gandhi @ArthurZucker ?

Copy link
Contributor

@sanchit-gandhi sanchit-gandhi left a comment

Choose a reason for hiding this comment

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

Awesome - thanks for the fix @zucchini-nlp! Requesting final review from @ArthurZucker

@sanchit-gandhi
Copy link
Contributor

I would be in favour of adding Whisper and possible Wav2Vec2 as well to this list as the two most used audio models @ydshieh

@ydshieh
Copy link
Collaborator

ydshieh commented Apr 3, 2024

@sanchit-gandhi wav2vec2 is already in that list.

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.

Could you resolve merge conflicts since #29225 was merged!

@zucchini-nlp
Copy link
Member Author

Done!

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

@ydshieh
Copy link
Collaborator

ydshieh commented Apr 5, 2024

@zucchini-nlp Do you have the merge permission (i.e. click the squash and merge button above)?

@zucchini-nlp zucchini-nlp merged commit 76fa17c into huggingface:main Apr 5, 2024
@zucchini-nlp
Copy link
Member Author

yep, merged!

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.

6 participants