[Whisper Tokenizer] Make more user-friendly#19921
Conversation
|
The documentation is not available anymore as the PR was closed or merged. |
| def test_tokenizer_special(self): | ||
| multilingual_tokenizer = WhisperTokenizer.from_pretrained("openai/whisper-tiny.en") | ||
| text = "<|startoftranscript|>Hey! How are you feeling? J'ai l'impression que 郷さん est prêt<|endoftext|>" | ||
| multilingual_tokenizer = WhisperTokenizer.from_pretrained( |
There was a problem hiding this comment.
Refactored to use a multilingual tokenizer and changed the expected ID's accordingly.
| def test_batch_encoding(self): | ||
| multilingual_tokenizer = WhisperTokenizer.from_pretrained("openai/whisper-tiny.en") | ||
| batch = ["<|en|><|notimestamps|>", "<|en|><|notimestamps|>I am sure that"] | ||
| multilingual_tokenizer = WhisperTokenizer.from_pretrained( |
There was a problem hiding this comment.
Refactored to use a multilingual tokeniser and changed the expected ID's accordingly.
| return pairs | ||
|
|
||
|
|
||
| LANGUAGES = { |
There was a problem hiding this comment.
Currently using language (e.g. "spanish") instead of lang_id (e.g. "es") -> this is how the original Whisper model does it. If there's a preference for lang_id I'm happy to switch!
ArthurZucker
left a comment
There was a problem hiding this comment.
Awesome work here! A few nits here an there but thanks a lot
Do you think we can also update the doc or write somewhere on the model card that in order to train the model, just pass the language and run it.
| all_special_ids = self.all_special_ids | ||
| bos_token_id = all_special_ids[-106] | ||
| translate_token_id = all_special_ids[-6] | ||
| transcribe_token_id = all_special_ids[-5] | ||
| notimestamps_token_id = all_special_ids[-1] |
There was a problem hiding this comment.
nit : Not really a fan of chard coded indexes. Maybe using the "all_special_tokens" makes it a bit more readable.
There was a problem hiding this comment.
I see your point! The only downside of using all_special_tokens is that we'll have to do an extra tokenisation step of converting from tokens -> id's in this method:
bos_token = all_special_tokens[-106]
translate_token = all_special_tokens[-6]
...
# get prefix tokens (bos_token, lang_token, task_token, notimestamps_token)
...
prefix_ids = self.encode(prefix_tokens) # <- extra step to convert from tokens to idsCo-authored-by: ArthurZucker <arthur@huggingface.co>
Co-authored-by: ArthurZucker <arthur@huggingface.co>
|
I'm not sure if Patrick currently has the bandwidth to review this, @sgugger would you be able to take a look if you've got a spare few minutes? Thanks! 🙏 |
sgugger
left a comment
There was a problem hiding this comment.
Just have some nits on the doc!
| output = bos_token_ids + token_ids_0 | ||
| ```python | ||
| >>> tokenizer = WhisperTokenizer.from_pretrained("openai/whisper-tiny", language="spanish") | ||
| >>> tokenizer.set_prefix_tokens(language="french") # update the language prefix token |
There was a problem hiding this comment.
Are we switching from Spanish to Franch here? Would be useful if the comment was clearer on that.
Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com>
Co-authored-by: sgugger <sylvain@huggingface.co>
patrickvonplaten
left a comment
There was a problem hiding this comment.
Looks good to me, but let's please add one test for set_prefix_tokens e.g. changing the language of the tokenizer on the fly
|
Test for |
|
Cool good to merge for me |
|
|
||
| self.assertListEqual(batch_output, EXPECTED_MULTI) | ||
|
|
||
| def test_set_prefix_tokens(self): |
* [Whisper Tokenizer] Make more user-friendly * use property * make indexing rigorous * small clean-up * tests * skip seq2seq tests * remove multilingual arg * reorder args * collapse to one function Co-authored-by: ArthurZucker <arthur@huggingface.co> * option to override attributes Co-authored-by: ArthurZucker <arthur@huggingface.co> * add to docs * Apply suggestions from code review Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com> * make comment more clear Co-authored-by: sgugger <sylvain@huggingface.co> * don't add special tokens in get_decoder_prompt_ids * add test for set_prefix_tokens Co-authored-by: ArthurZucker <arthur@huggingface.co> Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com> Co-authored-by: sgugger <sylvain@huggingface.co>
What does this PR do?
Fixes #19864.
In summary, the Whisper tokenizer is modified to prepend several tokens to the start-of-sequence:
<|startoftranscript|>) -> consistent with other sequence-to-sequence models such as BART.<|es|>for Spanish) -> set only when the tokenizer is instantiated with argumentlanguage=X. Otherwise omitted.<|translate|>for speech translation) -> set only when the tokenizer is instantiated with argumenttask=Y. Otherwise omitted.<|notimestamps|>) ->set only when the tokenizer is instantiated with argumentpredict_timestamps=False. Forpredict_timestamps=True, it is omitted.In addition, it is modified to always append the end-of-sequence token to the end of the label sequence (
<|endoftext|>).The updated tokenizer behaves as follows:
Print Output:
The attention mask functionality of the Whisper tokenizer is retained (c.f. #19864 (comment)).
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.