-
Notifications
You must be signed in to change notification settings - Fork 32k
[Whisper] handle deprecation of forced_decoder_ids
#38232
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
Conversation
|
|
||
| > Wild card | ||
|
|
||
| generation_kwargs: |
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.
note: this flag didn't have any uses in our codebase. I've added it in one of the first commits, but we added support to arbitrary kwargs a few weeks after that (for whisper)
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.
Hmm, I see. Is it still needed in the future or just kept for BC; not 100% familiar yet.
In an ideal world, we would deprecate this sometime in the future imo. It makes it hard to know what is supported and what not.
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.
not used at all in our library!
BC is not broken when this attribute exists in a file: we load arbitrary kwargs at init time :P The difference is that a) it is not documented b) self.generation_kwargs won't exist by default on instances (IMO not worth the extra work of a deprecation cycle for this detail)
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.
Yea, not worth the effort (for now). Good to know tho!
|
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. |
vasqu
left a comment
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.
LGTM! Mostly nits, functionality wise looks good :)
|
|
||
| > Wild card | ||
|
|
||
| generation_kwargs: |
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.
Hmm, I see. Is it still needed in the future or just kept for BC; not 100% familiar yet.
In an ideal world, we would deprecate this sometime in the future imo. It makes it hard to know what is supported and what not.
| self, | ||
| generate_config, | ||
| begin_index: Optional[int] = None, | ||
| generate_config: "GenerationConfig", |
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.
Any reason we just don't typecheck by default? I.e. meaning
#if TYPE_CHECKING:
from ..generation.configuration_utils import GenerationConfigThere 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.
yeah -- it would result in a circular import
(technically there shouldn't be one, so at some point we need to clear things)
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.
Oof, yea that's rough. Can you add a comment on the import? Otherwise, others (me) might forget again :D
| forced_decoder_ids = getattr(generation_config, "forced_decoder_ids", None) | ||
| # fallback: check the model config for forced_decoder_ids | ||
| if forced_decoder_ids is None and getattr(config, "forced_decoder_ids", None) is not None: | ||
| forced_decoder_ids = config.forced_decoder_ids |
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.
Looks doubled, overwriting forced_decoder_ids. I doubt it's intended?
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.
It's intentional: for BC reasons, we fallback generation_config attributes to the ones set in config.
The actual logic generate uses is more complex, this is a simplified version. Note that I didn't write these lines, I simply moved them around :)
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.
Omg, i mistook config with generation_config - now it makes more sense. Yea, no worries - looks good (even if it's ugly through BC)
vasqu
left a comment
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 and good work!
|
|
||
| > Wild card | ||
|
|
||
| generation_kwargs: |
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.
Yea, not worth the effort (for now). Good to know tho!
| self, | ||
| generate_config, | ||
| begin_index: Optional[int] = None, | ||
| generate_config: "GenerationConfig", |
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.
Oof, yea that's rough. Can you add a comment on the import? Otherwise, others (me) might forget again :D
| forced_decoder_ids = getattr(generation_config, "forced_decoder_ids", None) | ||
| # fallback: check the model config for forced_decoder_ids | ||
| if forced_decoder_ids is None and getattr(config, "forced_decoder_ids", None) is not None: | ||
| forced_decoder_ids = config.forced_decoder_ids |
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.
Omg, i mistook config with generation_config - now it makes more sense. Yea, no worries - looks good (even if it's ugly through BC)
What does this PR do?
Fixes #37172
forced_decoder_ids, an oldwhisperflag, was deprecated in favor of the more explicittaskandlanguageflags (#28687). However, we need to handle it for BC: the original checkpoints have them.This PR does a few deprecation-related changes, in an attempt to move us further away from using this flag:
generation_config.forced_decoder_idsas possible, except to handle BC in whisper'sgenerate.task/language) are set, don't throw any warning, i.e. the deprecated flag is ignored if the newer flags are set. Users who loaded base whisper and applied these new flags were seeing warnings, and they shouldn't.generation_config.forced_decoder_ids, and ignore it inGenerationMixin.generate✅ no regressions in SLOW whisper tests