-
Notifications
You must be signed in to change notification settings - Fork 28.2k
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] don't overwrite GenerationConfig's return_timestamps
when return_timestamps
is not passed to generate
function
#31296
Conversation
Possibly cc @gante as it relates to generation |
cc @kamilakesbi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @hrl,
Thanks for working on this! it LGTM :)
@amyeroberts we should be able to merge
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for handling this!
Could you add a test?
I'd just like to confirm this behaviour is consistent with the rest of the generation API cc @gante
@hrl did you have time to iterate on this ? thank you! |
@kamilakesbi conflicts resolved.
@amyeroberts I'm not familiar with the rest of the generation API. Should I write a test for this? The behavior will be different only when |
Oh, no worries, this was a question for @gante, our generate king and who knows all the ins-and-outs best :) |
Gentle ping @gante |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies for the delayed review, and thank you for fixing 🙏
(@amyeroberts @kamilakesbi the root cause for the issue behind this PR is that Whisper's |
… when `return_timestamps` is not passed to `generate` function (huggingface#31296) [whisper] don't overwrite return_timestamps when not passed to generate
What does this PR do?
When
WhisperGenerationMixin.generate
called withoutreturn_timestamps
, GenerationConfig'sreturn_timestamps
will be overwritten toNone
.It's okay if we can control
generate
's arguments. But in some particular case, for example, inTrainer.evaluation_loop
, then inSeq2SeqTrainer.prediction_step
, there is no way to passgen_kwargs
togenerate
function. (#19777)Which makes it impossible to generate with timestamps to calculate generative metrics when training.
This PR fixes this case by initializing
return_timestamps
to GenerationConfig's value.Before submitting
Pull Request section?
to it if that's the case.
documentation guidelines, and
here are tips on formatting docstrings.
Who can review?
Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.
@patrickvonplaten @sanchit-gandhi