-
Notifications
You must be signed in to change notification settings - Fork 34.1k
feat: Whisper prompting #22496
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
feat: Whisper prompting #22496
Changes from 32 commits
e4e1315
a8a28ec
dcaad98
c5d3ab5
052d60d
ce324b0
3097061
5a122ed
8c638b7
1ce5a1e
4a45a86
2b12d21
f5f2ab6
45992aa
5dcba16
f57577b
1f5e596
2ce4035
17d1046
bb4d2f5
44a1d08
9ab0c6c
3962906
af5d8e8
0a92f36
ba4c652
cae449e
7184f5f
26cb7e7
7c2a1d2
f0df1f1
0add3c7
f0a0364
1af5e8f
caff8be
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -34,7 +34,12 @@ | |
| SequenceClassifierOutput, | ||
| ) | ||
| from ...modeling_utils import PreTrainedModel | ||
| from ...utils import add_start_docstrings, add_start_docstrings_to_model_forward, logging, replace_return_docstrings | ||
| 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 | ||
|
|
||
|
|
@@ -1464,6 +1469,7 @@ def generate( | |
| task=None, | ||
| language=None, | ||
| is_multilingual=None, | ||
| prompt_ids: Optional[Union[torch.Tensor, np.ndarray]] = None, | ||
| **kwargs, | ||
| ): | ||
| """ | ||
|
|
@@ -1521,6 +1527,11 @@ def generate( | |
| find all the possible language tokens in the `model.generation_config.lang_to_id` dictionary. | ||
| is_multilingual (`bool`, *optional*): | ||
| Whether or not the model is multilingual. | ||
| prompt_ids (`Optional[Union[torch.Tensor, np.ndarray]]`, *optional*): | ||
| Rank-1 tensor of token IDs created by passing text to [`~WhisperProcessor.get_prompt_ids`] that is | ||
|
sanchit-gandhi marked this conversation as resolved.
Outdated
|
||
| provided as a prompt to each chunk. This can be used to provide or "prompt-engineer" a context for | ||
| transcription, e.g. custom vocabularies or proper nouns to make it more likely to predict those words | ||
| correctly. It cannot be used in conjunction with `decoder_start_token_id` as it overwrites this value. | ||
| kwargs: | ||
| Ad hoc parametrization of `generate_config` and/or additional model-specific kwargs that will be | ||
| forwarded to the `forward` function of the model. If the model is an encoder-decoder model, encoder | ||
|
|
@@ -1567,8 +1578,21 @@ def generate( | |
| if task is not None: | ||
| generation_config.task = task | ||
|
|
||
| forced_decoder_ids = [] | ||
| if task is not None or language is not None: | ||
| forced_decoder_ids = None | ||
|
|
||
| # Legacy code for backward compatibility | ||
| if hasattr(self.config, "forced_decoder_ids") and self.config.forced_decoder_ids is not None: | ||
| forced_decoder_ids = self.config.forced_decoder_ids | ||
| elif ( | ||
| hasattr(self.generation_config, "forced_decoder_ids") | ||
| and self.generation_config.forced_decoder_ids is not None | ||
| ): | ||
| forced_decoder_ids = self.generation_config.forced_decoder_ids | ||
| else: | ||
| forced_decoder_ids = kwargs.get("forced_decoder_ids", None) | ||
|
|
||
| if task is not None or language is not None or (forced_decoder_ids is None and prompt_ids is not None): | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is done solely for handling the case where |
||
| forced_decoder_ids = [] | ||
| if hasattr(generation_config, "language"): | ||
| if generation_config.language in generation_config.lang_to_id.keys(): | ||
| language_token = generation_config.language | ||
|
|
@@ -1599,21 +1623,42 @@ def generate( | |
| idx = forced_decoder_ids[-1][0] + 1 if forced_decoder_ids else 1 | ||
| forced_decoder_ids.append((idx, generation_config.no_timestamps_token_id)) | ||
|
|
||
| # Legacy code for backward compatibility | ||
| elif hasattr(self.config, "forced_decoder_ids") and self.config.forced_decoder_ids is not None: | ||
| forced_decoder_ids = self.config.forced_decoder_ids | ||
| elif ( | ||
| hasattr(self.generation_config, "forced_decoder_ids") | ||
| and self.generation_config.forced_decoder_ids is not None | ||
| ): | ||
| forced_decoder_ids = self.generation_config.forced_decoder_ids | ||
| if forced_decoder_ids is not None: | ||
| generation_config.forced_decoder_ids = forced_decoder_ids | ||
|
|
||
| if prompt_ids is not None: | ||
| if kwargs.get("decoder_start_token_id") is not None: | ||
| raise ValueError( | ||
| "When specifying `prompt_ids`, you cannot also specify `decoder_start_token_id` as it gets overwritten." | ||
| ) | ||
| prompt_ids = prompt_ids.tolist() | ||
| decoder_start_token_id, *text_prompt_ids = prompt_ids | ||
| # Set the decoder_start_token_id to <|startofprev|> | ||
| kwargs.update({"decoder_start_token_id": decoder_start_token_id}) | ||
|
|
||
| # Update the max generation length to include the prompt | ||
|
sanchit-gandhi marked this conversation as resolved.
Outdated
|
||
| specified_max_length = kwargs.pop("max_new_tokens", None) or kwargs.pop("max_length", None) | ||
| default_max_length = generation_config.max_new_tokens or generation_config.max_length | ||
| non_prompt_max_length = specified_max_length or default_max_length | ||
| generation_config.max_new_tokens = non_prompt_max_length + len(text_prompt_ids) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this is safe. If I do the following: The config is modified in place such that
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 to Amy's comment
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah yes thanks for catching, changed to update the kwargs with |
||
|
|
||
| # Reformat the forced_decoder_ids to incorporate the prompt | ||
| non_prompt_forced_decoder_ids = ( | ||
| kwargs.pop("forced_decoder_ids", None) or generation_config.forced_decoder_ids | ||
| ) | ||
|
Comment on lines
1646
to
1648
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice! I think this now supports the different ways that
It would be good if there are unit tests for these different methods.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't believe
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It does allow that (and I think it might even be the preferred method now) but for some reason the language needs to be the token, such as
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. cc @gante
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @connor-henderson update on the language code: we now support passing the language token, the language code, or the language name. See this (very recent) PR :) (not sure if this info has gotten to you, many conversations in parallel in this PR)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note that the language code change was @connor-henderson's most recent PR! This
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. haha derp, I didn't look at the author 🙈 my bad! |
||
| forced_decoder_ids = [ | ||
| # Slicing the text prompt ids in a manner consistent with the OpenAI implementation | ||
| # to accomodate context space for the prefix (see https://github.com/openai/whisper/blob/c09a7ae299c4c34c5839a76380ae407e7d785914/whisper/decoding.py#L599) | ||
| *text_prompt_ids[-self.config.max_length // 2 - 1 :], | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a reason behind this slicing? Intuitively it makes sense to me, but I'm curious to know if there is a reference behind this choice :)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure I'll leave a comment in the code too, this is done to match Whisper's implementation. I believe the reason they do the -1 is to make room for the first token to generate, and the reason they do // 2 is to halve it to share context space with a prefix if one is provided (which also gets halved). I don't believe there's prefix support yet in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hello @connor-henderson, as I am using the prompting feature I noticed a bug for long prompts. It might be caused by the slicing, where it should be
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hey @Helene-Maxcici, feel free to open a new issue to track this bug, tagging myself (and optionally @connor-henderson). In particular, it would be super helpful to have a reproducible code snippet to emulate the behaviour locally. See the following page for details: https://github.com/huggingface/transformers/blob/main/CONTRIBUTING.md#submitting-a-bug-related-issue-or-feature-request |
||
| generation_config.decoder_start_token_id, | ||
| *[token for _rank, token in non_prompt_forced_decoder_ids], | ||
|
sanchit-gandhi marked this conversation as resolved.
Outdated
|
||
| ] | ||
| forced_decoder_ids = [(rank + 1, token) for rank, token in enumerate(forced_decoder_ids)] | ||
| generation_config.forced_decoder_ids = forced_decoder_ids | ||
|
|
||
| if generation_config.return_timestamps: | ||
| logits_processor = [WhisperTimeStampLogitsProcessor(generation_config)] | ||
|
|
||
| if len(forced_decoder_ids) > 0: | ||
| generation_config.forced_decoder_ids = forced_decoder_ids | ||
|
|
||
| return super().generate( | ||
| inputs, | ||
| generation_config, | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -606,6 +606,11 @@ def _decode( | |||||||||||||||||||||||||||||||||||||
| ) -> str: | ||||||||||||||||||||||||||||||||||||||
| self._decode_use_source_tokenizer = kwargs.pop("use_source_tokenizer", False) | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| if skip_special_tokens: | ||||||||||||||||||||||||||||||||||||||
| prompt_token_id = self.convert_tokens_to_ids("<|startofprev|>") | ||||||||||||||||||||||||||||||||||||||
| decoder_start_token_id = self.convert_tokens_to_ids("<|startoftranscript|>") | ||||||||||||||||||||||||||||||||||||||
| token_ids = self._strip_prompt(token_ids, prompt_token_id, decoder_start_token_id) | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| filtered_tokens = self.convert_ids_to_tokens(token_ids, skip_special_tokens=skip_special_tokens) | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| # To avoid mixing byte-level and unicode for byte-level BPT | ||||||||||||||||||||||||||||||||||||||
|
|
@@ -714,6 +719,31 @@ def _decode_asr(self, model_outputs, *, return_timestamps, return_language, time | |||||||||||||||||||||||||||||||||||||
| time_precision=time_precision, | ||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| def get_prompt_ids(self, text: str, return_tensors="np"): | ||||||||||||||||||||||||||||||||||||||
| """Converts prompt text to IDs that can be passed to [`~WhisperForConditionalGeneration.generate`].""" | ||||||||||||||||||||||||||||||||||||||
| batch_encoding = self("<|startofprev|>", text.strip(), add_prefix_space=True, add_special_tokens=False) | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| # Check for special tokens | ||||||||||||||||||||||||||||||||||||||
| prompt_text_ids = batch_encoding["input_ids"][1:] | ||||||||||||||||||||||||||||||||||||||
| special_token_id = next((x for x in prompt_text_ids if x >= self.all_special_ids[0]), None) | ||||||||||||||||||||||||||||||||||||||
| if special_token_id is not None: | ||||||||||||||||||||||||||||||||||||||
| token = self.convert_ids_to_tokens(special_token_id) | ||||||||||||||||||||||||||||||||||||||
| raise ValueError(f"Encountered text in the prompt corresponding to disallowed special token: {token}.") | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| batch_encoding.convert_to_tensors(tensor_type=return_tensors) | ||||||||||||||||||||||||||||||||||||||
| return batch_encoding["input_ids"] | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| @staticmethod | ||||||||||||||||||||||||||||||||||||||
| def _strip_prompt(token_ids: List[int], prompt_token_id: int, decoder_start_token_id: int): | ||||||||||||||||||||||||||||||||||||||
| has_prompt = isinstance(token_ids, list) and token_ids and token_ids[0] == prompt_token_id | ||||||||||||||||||||||||||||||||||||||
| if has_prompt: | ||||||||||||||||||||||||||||||||||||||
| if decoder_start_token_id in token_ids: | ||||||||||||||||||||||||||||||||||||||
| return token_ids[token_ids.index(decoder_start_token_id) :] | ||||||||||||||||||||||||||||||||||||||
| else: | ||||||||||||||||||||||||||||||||||||||
| return [] | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| return token_ids | ||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+737
to
+745
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: suggestion, feel free to ignore I would write for early returns to make the logic a bit clearer here
Suggested change
|
||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| def _decode_asr(tokenizer, model_outputs, *, return_timestamps, return_language, time_precision): | ||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -312,6 +312,11 @@ def decode( | |
| return text | ||
|
|
||
| def _decode(self, *args, normalize: bool = False, **kwargs) -> str: | ||
| if kwargs["skip_special_tokens"]: | ||
| prompt_token_id = self.convert_tokens_to_ids("<|startofprev|>") | ||
| decoder_start_token_id = self.convert_tokens_to_ids("<|startoftranscript|>") | ||
| kwargs["token_ids"] = self._strip_prompt(kwargs["token_ids"], prompt_token_id, decoder_start_token_id) | ||
|
|
||
| text = super()._decode(*args, **kwargs) | ||
|
|
||
| if normalize: | ||
|
|
@@ -485,3 +490,30 @@ def _decode_asr(self, model_outputs, *, return_timestamps, return_language, time | |
| return_language=return_language, | ||
| time_precision=time_precision, | ||
| ) | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are we missing the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah yes, thanks |
||
| # Copied from transformers.models.whisper.tokenization_whisper.WhisperTokenizer.get_prompt_ids | ||
| def get_prompt_ids(self, text: str, return_tensors="np"): | ||
| """Converts prompt text to IDs that can be passed to [`~WhisperForConditionalGeneration.generate`].""" | ||
| batch_encoding = self("<|startofprev|>", text.strip(), add_prefix_space=True, add_special_tokens=False) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Quick comment here. By default, from transformers import WhisperTokenizerFast
tokenizer = WhisperTokenizerFast.from_pretrained("openai/whisper-tiny")
tokenizer("<|startofprev|>", "test", add_special_tokens=False).input_ids
> [50361, 31636]
tokenizer("<|startofprev|>", "test", add_special_tokens=False, add_prefix_space=True).input_ids
> TypeError: PreTrainedTokenizerFast._batch_encode_plus() got an unexpected keyword argument 'add_prefix_space'Is it necessary to have
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since Whisper always infers an extra space after the I'm not sure there's a clean way we can do this directly? Probably easiest is to instantiate the tokenizer with from transformers import WhisperTokenizerFast
tokenizer = WhisperTokenizerFast.from_pretrained("openai/whisper-tiny", add_prefix_space=True)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For training with initial prompts, right now we are using two tokenizers, which is not ideal. |
||
|
|
||
| # Check for special tokens | ||
| prompt_text_ids = batch_encoding["input_ids"][1:] | ||
| special_token_id = next((x for x in prompt_text_ids if x >= self.all_special_ids[0]), None) | ||
| if special_token_id is not None: | ||
| token = self.convert_ids_to_tokens(special_token_id) | ||
| raise ValueError(f"Encountered text in the prompt corresponding to disallowed special token: {token}.") | ||
|
|
||
| batch_encoding.convert_to_tensors(tensor_type=return_tensors) | ||
| return batch_encoding["input_ids"] | ||
|
|
||
| @staticmethod | ||
| # Copied from transformers.models.whisper.tokenization_whisper.WhisperTokenizer._strip_prompt | ||
| def _strip_prompt(token_ids: List[int], prompt_token_id: int, decoder_start_token_id: int): | ||
| has_prompt = isinstance(token_ids, list) and token_ids and token_ids[0] == prompt_token_id | ||
| if has_prompt: | ||
| if decoder_start_token_id in token_ids: | ||
| return token_ids[token_ids.index(decoder_start_token_id) :] | ||
| else: | ||
| return [] | ||
|
|
||
| return token_ids | ||
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -1013,6 +1013,46 @@ def test_mask_time_prob(self): | |||||||
| encoder_last_hidden_state = model(**input_dict).encoder_last_hidden_state | ||||||||
| self.assertTrue(encoder_last_hidden_state.shape, (13, 30, 16)) | ||||||||
|
|
||||||||
| def test_generate_with_prompt_ids_and_task_and_language(self): | ||||||||
| config, input_dict = self.model_tester.prepare_config_and_inputs_for_common() | ||||||||
| model = WhisperForConditionalGeneration(config).eval().to(torch_device) | ||||||||
| input_features = input_dict["input_features"] | ||||||||
| prompt_ids = np.asarray(range(5)) | ||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||
| language = "<|de|>" | ||||||||
| task = "translate" | ||||||||
| lang_id = 6 | ||||||||
| task_id = 7 | ||||||||
| model.generation_config.__setattr__("lang_to_id", {language: lang_id}) | ||||||||
| model.generation_config.__setattr__("task_to_id", {task: task_id}) | ||||||||
|
|
||||||||
| output = model.generate(input_features, max_new_tokens=5, task=task, language=language, prompt_ids=prompt_ids) | ||||||||
|
|
||||||||
| expected_output_start = [ | ||||||||
| *prompt_ids.tolist(), | ||||||||
| model.generation_config.decoder_start_token_id, | ||||||||
| lang_id, | ||||||||
| task_id, | ||||||||
| ] | ||||||||
| self.assertTrue(all(row[: len(expected_output_start)] == expected_output_start for row in output.tolist())) | ||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: up to you, but I would iterate over this with a
Suggested change
|
||||||||
|
|
||||||||
| def test_generate_with_prompt_ids_and_forced_decoder_ids(self): | ||||||||
| config, input_dict = self.model_tester.prepare_config_and_inputs_for_common() | ||||||||
| model = WhisperForConditionalGeneration(config).eval().to(torch_device) | ||||||||
| input_features = input_dict["input_features"] | ||||||||
| prompt_ids = np.asarray(range(5)) | ||||||||
| forced_decoder_ids = [(1, 6), (2, 7), (3, 8)] | ||||||||
|
|
||||||||
| output = model.generate( | ||||||||
| input_features, max_new_tokens=5, forced_decoder_ids=forced_decoder_ids, prompt_ids=prompt_ids | ||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we allow passing |
||||||||
| ) | ||||||||
|
|
||||||||
| expected_output_start = [ | ||||||||
| *prompt_ids.tolist(), | ||||||||
| model.generation_config.decoder_start_token_id, | ||||||||
| *[token for _rank, token in forced_decoder_ids], | ||||||||
| ] | ||||||||
| self.assertTrue(all(row[: len(expected_output_start)] == expected_output_start for row in output.tolist())) | ||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here re using |
||||||||
|
|
||||||||
|
|
||||||||
| @require_torch | ||||||||
| @require_torchaudio | ||||||||
|
|
@@ -1429,6 +1469,57 @@ def test_tiny_specaugment_librispeech(self): | |||||||
| # fmt: on | ||||||||
| self.assertTrue(torch.allclose(logits[0][0, 0, :30].cpu(), EXPECTED_LOGITS, atol=1e-4)) | ||||||||
|
|
||||||||
| @slow | ||||||||
| def test_generate_with_prompt_ids(self): | ||||||||
| processor = WhisperProcessor.from_pretrained("openai/whisper-tiny") | ||||||||
| model = WhisperForConditionalGeneration.from_pretrained("openai/whisper-tiny") | ||||||||
| model.to(torch_device) | ||||||||
| input_speech = self._load_datasamples(4)[-1:] | ||||||||
| input_features = processor(input_speech, return_tensors="pt").input_features | ||||||||
|
|
||||||||
| output_without_prompt = model.generate(input_features) | ||||||||
| prompt_ids = processor.get_prompt_ids("Leighton") | ||||||||
| output_with_prompt = model.generate(input_features, prompt_ids=prompt_ids) | ||||||||
|
|
||||||||
| expected_without_prompt = "<|startoftranscript|><|en|><|transcribe|><|notimestamps|> He has grave doubts whether Sir Frederick Layton's work is really Greek after all and can discover in it but little of Rocky Ithaca.<|endoftext|>" | ||||||||
| expected_with_prompt = "<|startofprev|> Leighton<|startoftranscript|><|en|><|transcribe|><|notimestamps|> He has grave doubts whether Sir Frederick Leighton's work is really Greek after all and can discover in it but little of Rocky Ithaca.<|endoftext|>" | ||||||||
| self.assertEqual(processor.decode(output_without_prompt[0]), expected_without_prompt) | ||||||||
| self.assertEqual(processor.decode(output_with_prompt[0]), expected_with_prompt) | ||||||||
|
|
||||||||
| @slow | ||||||||
| def test_generate_with_prompt_ids_and_forced_decoder_ids(self): | ||||||||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I feel like this is most readable / simplest as one test with comments clarifying the cases, lmk if you want them split into separate unit tests
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree it's very readable but there's a potential issue: the Maybe it's also a good idea to test what happens after you do the following, just to make sure the code can handle both of these things being None: model.config.forced_decoder_ids = None
model.generation_config.forced_decoder_ids = None
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added a case for the above which involved a change in
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, I wasn't able to fully understand the last comment - for testing the case when: is this tested?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, moving parts, we had a test explicitly for this when there we 5 test cases. Then we trimmed them, per this #22496 (comment) I changed the tl;dr it was tested, then wasn't, now is again |
||||||||
| processor = WhisperProcessor.from_pretrained("openai/whisper-tiny") | ||||||||
| model = WhisperForConditionalGeneration.from_pretrained("openai/whisper-tiny") | ||||||||
| model.to(torch_device) | ||||||||
| input_speech = self._load_datasamples(1) | ||||||||
| input_features = processor(input_speech, return_tensors="pt").input_features | ||||||||
| task = "translate" | ||||||||
| language = "de" | ||||||||
| expected_tokens = [f"<|{task}|>", f"<|{language}|>"] | ||||||||
| prompt = "test prompt" | ||||||||
| prompt_ids = processor.get_prompt_ids(prompt) | ||||||||
|
|
||||||||
| output = model.generate(input_features, task=task, language=language, prompt_ids=prompt_ids) | ||||||||
| text = processor.decode(output[0]) | ||||||||
|
|
||||||||
| self.assertTrue(prompt in text) | ||||||||
| self.assertTrue(all([token in text for token in expected_tokens])) | ||||||||
|
|
||||||||
| @slow | ||||||||
| def test_generate_with_prompt_ids_and_no_non_prompt_forced_decoder_ids(self): | ||||||||
| processor = WhisperProcessor.from_pretrained("openai/whisper-tiny.en") | ||||||||
| model = WhisperForConditionalGeneration.from_pretrained("openai/whisper-tiny.en") | ||||||||
| model.to(torch_device) | ||||||||
| input_speech = self._load_datasamples(1) | ||||||||
| input_features = processor(input_speech, return_tensors="pt").input_features | ||||||||
| prompt = "test prompt" | ||||||||
| prompt_ids = processor.get_prompt_ids(prompt) | ||||||||
|
|
||||||||
| output = model.generate(input_features, prompt_ids=prompt_ids, return_timestamps=True) | ||||||||
| text = processor.decode(output[0]) | ||||||||
|
|
||||||||
| self.assertTrue(prompt in text) | ||||||||
|
|
||||||||
|
|
||||||||
| def prepare_whisper_encoder_inputs_dict(config, input_features, head_mask=None): | ||||||||
| if head_mask is None: | ||||||||
|
|
||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,6 +16,8 @@ | |
| import tempfile | ||
| import unittest | ||
|
|
||
| import pytest | ||
|
|
||
| from transformers import WhisperTokenizer, is_speech_available | ||
| from transformers.testing_utils import require_sentencepiece, require_torch, require_torchaudio | ||
|
|
||
|
|
@@ -146,3 +148,32 @@ def test_get_decoder_prompt_ids(self): | |
|
|
||
| expected_ids = [TRANSCRIBE, NOTIMESTAMPS] | ||
| self.assertListEqual([ids[-1] for ids in forced_decoder_ids], expected_ids) | ||
|
|
||
| def test_get_prompt_ids(self): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you also add some tests for edge cases? For example:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The second will definitely confuse the model and decoding if they were passed to the current
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't really know what would be the best approach here, was just trying to think of things that might go wrong. ;-) Perhaps raising an error on unexpected input is the best choice, but only if it doesn't add a lot of complexity to the code.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like they have their tiktoken package handle it and it raises an error if any special token is included, so will look to do the same |
||
| processor = WhisperProcessor(tokenizer=self.get_tokenizer(), feature_extractor=self.get_feature_extractor()) | ||
| prompt_ids = processor.get_prompt_ids("Mr. Quilter") | ||
| decoded_prompt = processor.tokenizer.decode(prompt_ids) | ||
|
|
||
| self.assertListEqual(prompt_ids.tolist(), [50360, 1770, 13, 2264, 346, 353]) | ||
| self.assertEqual(decoded_prompt, "<|startofprev|> Mr. Quilter") | ||
|
|
||
| def test_empty_get_prompt_ids(self): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice :) |
||
| processor = WhisperProcessor(tokenizer=self.get_tokenizer(), feature_extractor=self.get_feature_extractor()) | ||
| prompt_ids = processor.get_prompt_ids("") | ||
| decoded_prompt = processor.tokenizer.decode(prompt_ids) | ||
|
|
||
| self.assertListEqual(prompt_ids.tolist(), [50360, 220]) | ||
| self.assertEqual(decoded_prompt, "<|startofprev|> ") | ||
|
|
||
| def test_get_prompt_ids_with_special_tokens(self): | ||
|
sanchit-gandhi marked this conversation as resolved.
Outdated
|
||
| processor = WhisperProcessor(tokenizer=self.get_tokenizer(), feature_extractor=self.get_feature_extractor()) | ||
|
|
||
| def _test_prompt_error_raised_helper(prompt, special_token): | ||
| with pytest.raises(ValueError) as excinfo: | ||
| processor.get_prompt_ids(prompt) | ||
| expected = f"Encountered text in the prompt corresponding to disallowed special token: {special_token}." | ||
| self.assertEqual(expected, str(excinfo.value)) | ||
|
|
||
| _test_prompt_error_raised_helper("<|startofprev|> test", "<|startofprev|>") | ||
| _test_prompt_error_raised_helper("test <|notimestamps|>", "<|notimestamps|>") | ||
| _test_prompt_error_raised_helper("test <|zh|> test <|transcribe|>", "<|zh|>") | ||
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.
Sorry to be a stickler about this one, but why was
np.ndarrayadded back in here?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.
No worries, I thought it was implied by @sanchit-gandhi 's ask to have the default return_tensors of get_prompt_ids be 'np'. Should I take it out?
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.
I'll let Sanchit have the final say on this then. 😉
Uh oh!
There was an error while loading. Please reload this page.
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.
This was for compatibility with TF/Flax models (as well as PyTorch ones) - in the PyTorch modelling code we can safely assume that we're working in PyTorch and that the prompt ids will be torch tensors. In the TF/Flax we expect tf/np arrays respectively.
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.
Got it thanks, put the type back to being just pytorch