Skip to content

[🛠️] Fix-whisper-breaking-changes #21965

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.

Sign up for GitHub

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

Merged
ArthurZucker merged 8 commits into from
Mar 14, 2023
Merged

[🛠️] Fix-whisper-breaking-changes #21965

Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
389f35b
temp fix
ArthurZucker Mar 3, 2023
10a2175
temporary fix
ArthurZucker Mar 6, 2023
25c0711
update
ArthurZucker Mar 6, 2023
2262ca6
fix tests
ArthurZucker Mar 6, 2023
ea0eca4
fixup
ArthurZucker Mar 6, 2023
5d18c67
update based on reveiew
ArthurZucker Mar 8, 2023
a7d706f
update to fix tests
ArthurZucker Mar 8, 2023
f3c4556
update docstring
ArthurZucker Mar 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 35 additions & 10 deletions src/transformers/models/whisper/modeling_whisper.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,9 @@
# limitations under the License.
""" PyTorch Whisper model."""


import math
import random
import warnings
from typing import Optional, Tuple, Union

import numpy as np
Expand All @@ -36,6 +36,7 @@
from ...modeling_utils import PreTrainedModel
from ...utils import add_start_docstrings, add_start_docstrings_to_model_forward, logging, replace_return_docstrings
from .configuration_whisper import WhisperConfig
from .tokenization_whisper import TASK_IDS, TO_LANGUAGE_CODE


logger = logging.get_logger(__name__)
Expand Down Expand Up @@ -1504,11 +1505,16 @@ def generate(
generation_config = self.generation_config

if return_timestamps is not None:
generation_config.return_timestamps = return_timestamps

if task is not None:
generation_config.task = task
if not hasattr(generation_config, "no_timestamps_token_id"):
raise ValueError(
"You are trying to return timestamps, but the generation config is not properly set."
"Make sure to initialize the generation config with the correct `no_timestamps_token_id`."
"For more details on how to generate the approtiate config, refer to [#21878](https://github.com/huggingface/transformers/issues/21878)"

@sanchit-gandhi sanchit-gandhi Mar 7, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Don't think markdown formatting applies to python logger / print statements no? Running the following in a Python shell:

raise ValueError("Hey this is a test [#21878](https://github.com/huggingface/transformers/issues/21878)")

Print Output:

ValueError: Hey this is a test [#21878](https://github.com/huggingface/transformers/issues/21878)

Think we can also point to the specific fix? Up to you!

Suggested change
"For more details on how to generate the approtiate config, refer to [#21878](https://github.com/huggingface/transformers/issues/21878)"
"For more details on how to generate the approtiate config, refer to https://github.com/huggingface/transformers/issues/21878#issuecomment-1451902363"

)

generation_config.return_timestamps = return_timestamps
else:
generation_config.return_timestamps = False
if is_multilingual is not None:
generation_config.is_multilingual = is_multilingual

Expand All @@ -1519,18 +1525,37 @@ def generate(

if hasattr(generation_config, "is_multilingual") and generation_config.is_multilingual:
if hasattr(generation_config, "language"):
forced_decoder_ids.append((1, generation_config.lang_to_id[generation_config.language]))
if generation_config.language in generation_config.lang_to_id.keys():
language_token = generation_config.language
elif generation_config.language in TO_LANGUAGE_CODE.keys():
language_token = f"<|{TO_LANGUAGE_CODE[generation_config.language]}|>"
else:
raise ValueError(f"The `{generation_config.language}` language token is not supported.")
Comment thread
ArthurZucker marked this conversation as resolved.
Outdated
forced_decoder_ids.append((1, generation_config.lang_to_id[language_token]))
else:
forced_decoder_ids.append((1, None))

if hasattr(generation_config, "task"):
forced_decoder_ids.append((2, generation_config.task_to_id[generation_config.task]))
if generation_config.task in TASK_IDS:
forced_decoder_ids.append((2, generation_config.task_to_id[generation_config.task]))
else:
raise ValueError(
f"The `{generation_config.task}`task is not supported. The task should be one of `{TASK_IDS}`"

@sanchit-gandhi sanchit-gandhi Mar 10, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice

)
else:
forced_decoder_ids.append((2, generation_config.task_to_id["transcribe"]))

if (
hasattr(generation_config, "return_timestamps") and generation_config.return_timestamps
) or return_timestamps:
# Legacy code for backward compatibility
if hasattr(self.config, "forced_decoder_ids") and forced_decoder_ids != self.generation.forced_decoder_ids:

@sanchit-gandhi sanchit-gandhi Mar 7, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should this be generation_config.forced_decoder_ids? We don't have an attribute self.generation AFAIK

Suggested change
if hasattr(self.config, "forced_decoder_ids") and forced_decoder_ids != self.generation.forced_decoder_ids:
if hasattr(self.config, "forced_decoder_ids") and forced_decoder_ids != generation_config.forced_decoder_ids:

I think we should favour forced_decoder_ids created using the generation_config over the config. Currently, as things stand, if a user passes language=..., task=..., these arguments will be ignored, and the forced decoder ids will always be set based on config.forced_decoder_ids.

What I think we should do is only set forced_decoder_ids = config.forced_decoder_ids if neither language or task are passed.

This way, a user can still control the forced decoder ids using the generate args even if they've set config.forced_decoder_ids. This means older Whisper models will still be compatible with the new generate method.

WDYT?

@ArthurZucker ArthurZucker Mar 7, 2023

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Agreed! Thanks for the suggestion

warnings.warn(
"You have modified the pretrained model configuration to control generation. This is a"
" deprecated strategy to control generation and will be removed soon, in a future version."
" Please use a generation configuration file (see"
" https://huggingface.co/docs/transformers/main_classes/text_generation)"
)
forced_decoder_ids = self.config.forced_decoder_ids

@patrickvonplaten patrickvonplaten Jan 29, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this line is hit when neither language nor task is specified (which is the default case)


if generation_config.return_timestamps:
logits_processor = [WhisperTimeStampLogitsProcessor(generation_config)]
else:
if forced_decoder_ids and forced_decoder_ids[-1][0] != generation_config.no_timestamps_token_id:
Expand Down