Skip to content

Comments

⚠️⚠️[T5Tokenize] Fix T5 family tokenizers⚠️⚠️#24565

Merged
ArthurZucker merged 12 commits intohuggingface:mainfrom
ArthurZucker:fix-t5-tokenizer
Jun 30, 2023
Merged

⚠️⚠️[T5Tokenize] Fix T5 family tokenizers⚠️⚠️#24565
ArthurZucker merged 12 commits intohuggingface:mainfrom
ArthurZucker:fix-t5-tokenizer

Conversation

@ArthurZucker
Copy link
Collaborator

@ArthurZucker ArthurZucker commented Jun 29, 2023 •
edited
Loading

What does this PR do?

Fixes the T5Tokenizer (not the fast one yet). (at the same time adresses part of #11531)
When converting UMT5 I created a reproduction snippet for any t5x model form the original repo. I realized that a very very small variation in the input completely changes the output for non-finetuned models. The issue lies with the way we process <extra_id_xx>.

Example:

# t5-base tokenizer
>>> tokenizer.encode("<extra_id_0>. Hello", add_special_tokens = False)
[32099, 3, 5, 8774] # ['<extra_id_0>', ' ▁', '.', '▁Hello']
# seqio.SentencePieceVocabulary(vocab_path, extra_ids = 300)
>>> processor.encode("<extra_id_0>. Hello")
[32099, 5, 8774] # ['<extra_id_0>', '.', '▁Hello']

#after fix: 
>>> tokenizer.encode("<extra_id_0>. Hello", add_special_tokens = False)
[32099, 5, 8774] # ['<extra_id_0>', '.', '▁Hello']

The reason is that t5x wrapps arround sentencepiece, and adds the extra id to the vocab, but they are not saved that way.
We don't add them to the vocab, so when we tokenize, we split on special tokens, thus the sentencepiece model only sees:

>>> tokenizer.sp_model.encode(". Hello")
[273, 274, 9]

While the original model never sees a . (or a lot of other characters) alone, and thus we add an extra space...

This is a bug fix with regards to training, it is breaking in the sense that is should remove the space.

TODO:

  • Extra checks should be added to make sure this does not add anything else (like stripping a . This for example would break: tokenizer.encode(". Hello") as it remove the prefix space that is normally added.

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Jun 29, 2023 •
edited
Loading

The documentation is not available anymore as the PR was closed or merged.

@ArthurZucker ArthurZucker mentioned this pull request Jun 29, 2023
Closed
4 tasks
@ArthurZucker ArthurZucker marked this pull request as ready for review June 29, 2023 03:28
@ArthurZucker
Copy link
Collaborator Author

ArthurZucker commented Jun 29, 2023 •
edited
Loading

Actually switch t5 tests have to be updated!
This means I have to check if the models were trained with this extra token (if they used HF tokenizer) or not.

  • tests.models.instructblip.test_modeling_instructblip.InstructBlipModelIntegrationTest testMethod=test_inference_flant5_xl failing on main too so not related.....
image

  • tests.models.mt5.test_modeling_flax_mt5.MT5IntegrationTest also fails on main...

  • tests/models/t5/test_tokenization_t5.py the issue comes from the convert_slow modification. Need to investigate
    - [ ] tests/models/t5/test_tokenization_t5.py:399 T5TokenizationTest.test_get_sentinel_token_ids_for_fasttokenizer
    - [ ] tests/test_tokenization_common.py:3425 T5TokenizationTest.test_save_pretrained
    - [ ] tests/models/t5/test_tokenization_t5.py:271 T5TokenizationTest.test_special_tokens_initialization

@ArthurZucker ArthurZucker requested review from Narsil and sgugger June 29, 2023 06:09
@ArthurZucker
Copy link
Collaborator Author

ArthurZucker commented Jun 29, 2023

This can also be made non "breakable" with a flag. Up to debate since it is a bug fix.

sgugger
sgugger approved these changes Jun 29, 2023
View reviewed changes
Copy link
Collaborator

@sgugger sgugger left a comment

Choose a reason for hiding this comment

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

Thanks for the fix! Let's roll with it since it's a bug fix and if people complain about the breaking change we will see if we add a flag to enable the buggy behavior.

@ArthurZucker ArthurZucker merged commit b52a03c into huggingface:main Jun 30, 2023
@dtiarks dtiarks mentioned this pull request Jun 30, 2023
Merged
4 tasks
@ArthurZucker
Copy link
Collaborator Author

ArthurZucker commented Jul 2, 2023 •
edited
Loading

Edit: just to make sure, I did more testing and unfortunately , there is one bug:

>>>tokenizer.tokenize("Hello <extra_id_0>")
['_', '_Hello', '<extra_id_0>']

instead of

>>>tokenizer.tokenize("Hello <extra_id_0>")
['_Hello', '<extra_id_0>']

This is because we have to prepend a _ instead of a space. (text = SPIECE_UNDERLINE + text. Not a single test caught this when runing pytest tests -k t5 which is interesting.
Fixing asap and adding tests. This is becoming very complex 😓

This was referenced Jul 2, 2023
Merged
Closed
@hy928302776 hy928302776 mentioned this pull request Jul 13, 2023
Open
@ashercn97 ashercn97 mentioned this pull request Jul 18, 2023
Closed
@atillabasaran atillabasaran mentioned this pull request Jul 18, 2023
Open
@pointonjoel
Copy link

pointonjoel commented Jul 20, 2023

I'm getting this legacy behaviour warning come up when simply loading a T5 tokenizer - it appears even before using the tokenizer. Is there an updated way to load the tokenizer? The warning appears when running the following lines of code:

from transformers import AutoTokenizer
tokeniser = AutoTokenizer.from_pretrained("google/mt5-small")

The error is:
You are using the legacy behaviour of the <class 'transformers.models.t5.tokenization_t5.T5Tokenizer'>. This means that tokens that come after special tokens will not be properly handled. We recommend you to read the related pull request available at #24565
/usr/local/lib/python3.10/dist-packages/transformers/convert_slow_tokenizer.py:470: UserWarning: The sentencepiece tokenizer that you are converting to a fast tokenizer uses the byte fallback option which is not implemented in the fast tokenizers. In practice this means that the fast version of the tokenizer can produce unknown tokens whereas the sentencepiece version would have converted these unknown tokens into a sequence of byte tokens matching the original piece of text.
warnings.warn(

@ArthurZucker
Copy link
Collaborator Author

ArthurZucker commented Jul 20, 2023

Yep, just set legacy=False. The goal of the warning is for you to decide wether or not you thing the legacy behaviour is alright with you or not.

This was referenced Jul 14, 2025
Open
Open
@rhystz rhystz mentioned this pull request Jul 15, 2025
Open
This was referenced Jul 17, 2025
Open
Open
@HarmlessDiva HarmlessDiva mentioned this pull request Jul 22, 2025
Open
@socket-security socket-security bot mentioned this pull request Aug 1, 2025
Merged
@socket-security socket-security bot mentioned this pull request Aug 12, 2025
Open
@TaffyCarl TaffyCarl mentioned this pull request Aug 20, 2025
Closed
1 task
@vladlen32230 vladlen32230 mentioned this pull request Aug 21, 2025
Closed
1 task
@som-subh som-subh mentioned this pull request Aug 23, 2025
Closed
@socket-security socket-security bot mentioned this pull request Sep 1, 2025
Closed
@moveforever moveforever mentioned this pull request Sep 5, 2025
Closed
This was referenced Sep 15, 2025
Open
Open
Open
Open
Open
This was referenced Sep 25, 2025
Open
Open
Open
@PheonixAi420 PheonixAi420 mentioned this pull request Oct 7, 2025
Closed
@pkbullock pkbullock mentioned this pull request Oct 10, 2025
Open
@socket-security socket-security bot mentioned this pull request Nov 1, 2025
Closed
@Vbird-light Vbird-light mentioned this pull request Nov 7, 2025
Open
This was referenced Nov 21, 2025
Merged
Merged
This was referenced Dec 16, 2025
Closed
Open
@PaleKemono PaleKemono mentioned this pull request Dec 22, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Narsil Narsil Awaiting requested review from Narsil

2 more reviewers

@Ethan-yt Ethan-yt Ethan-yt left review comments

@sgugger sgugger sgugger approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.