⚠️⚠️[`T5Tokenize`] Fix T5 family tokenizers⚠️⚠️
Author: ArthurZucker
Pull request: yes

# What does this PR do?
Fixes the `T5Tokenizer` (not the fast one yet). (at the same time adresses part of https://github.com/huggingface/transformers/issues/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:
```python
# 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](https://github.com/google/seqio/blob/4d3097973e9e24ec2963319ec3c5ff518811060f/seqio/vocabularies.py#L362), 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: 
```python
>>> 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:
- [x] 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.


Comments:
_The documentation is not available anymore as the PR was closed or merged._

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.
- [x] `tests.models.instructblip.test_modeling_instructblip.InstructBlipModelIntegrationTest testMethod=test_inference_flant5_xl` failing on `main` too so not related.....
<img width="1560" alt="image" src="https://github.com/huggingface/transformers/assets/48595927/529a1cdf-6907-42c3-9c48-1d2a6177c8e6">

- [x] `tests.models.mt5.test_modeling_flax_mt5.MT5IntegrationTest` also fails on main...


- [x] `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

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

Edit: just to make sure, I did more testing and unfortunately , there is one bug: 
```python 
>>>tokenizer.tokenize("Hello <extra_id_0>")
['_', '_Hello', '<extra_id_0>']
``` 
instead of 
```python 
>>>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 😓 

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 https://github.com/huggingface/transformers/pull/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(

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. 

so tokenizers just have to be loaded with `legacy=False` forever now? Seems like an odd choice.

Since this is a breaking change, the next version of transformers will probably have `legacy=False` by default, and remove the warning. We don't really have much choice in order to preserve backward compatibility 😅 

+1 for this proposal, since my users are asking why this error shows up for them and if the incorrect behavior is the default that is not desirable for anyone just wanting to use a model. People needing the legacy behavior can then opt-in. Hardcoding legacy=False in my own code feels wrong to me.

I am also a little confused when I use LLaMA tokenizer. Is it wrong for LLaMA?

For LlaMa, it is a bit special. The original tokenizer does not have the eos and bos as part of the special tokens This means that they are parsed (split) by the model, while this does not happen with transformers.
I am testing to make sure whether this should apply for llama based on llama2. Whatever the case, if you don't use legacy, and a special token is in the middle of a sequence:
- `Hey how are you?
 I'm fine and you`'s encoding will be wrong for two reasons. 1, we split special tokens. 2, an extra space will be added between `
 I`. Using `legacy=False` the extra space is not added.
- `
Hey how are you?` will have a different encoding because llama would tokenize it as [`<`, `s`, `>`, `Hey`]  while we would tokenizer it as [`
`, `_Hey`] note the extra `_`. With `legacy=False` we will have [`
`, `Hey`] which is already better. I am implementing now the possibility to split special tokens (meaning ignore that they are special) which will bridge the final gap to allow us to have  [`<`, `s`, `>`, `Hey`].

An issue can come up if:
- You manualy add the eos at the beginning using ` tokenizer.encode("
Hey")` the `tokenizer.sp_model` only sees `Hey` which get's encoded as [`_Hey`], but the `_` is removed by `legacy=False`. This is not what you want in this specific case because the special token is placed at the start. With #25081, the special tokens should be split by default for llama, which will make things right! 

So if text does not explicitly contain special tokens, the tokenizer can work well?

and so how does one continue using the legacy mode, but remove the warning?

Surely if the model designer wants the legacy mode they should be able to select that w/o having the user puzzle over the warning.

Ok will make it default to None, that way you can be warned only if you just have not set the argument! Thanks for the feedback 

that's a great idea, Arthur - thank you!

See #25131 😉 

- I just tested the transformers=4.31.0,  however, the bug is still there, shown as following:

```
>>> btokenizer = transformers.AutoTokenizer.from_pretrained(model_path,legacy=False) 
>>> btokenizer.decode(btokenizer("Hey how are you?
 I'm fine and you")['input_ids'])

 Hey how are you?
  I'm fine and you
```

Note that there are two spaces between` 
 and I`.

-  Another issue that may require more clarification that should a token at the start of a string, with the current tokenizer, be added with a prefix space, no matter with or without a special token in front of it? 

- In transformers of LLama , it says 

> The LLaMA tokenizer is a BPE model based on [sentencepiece](https://github.com/google/sentencepiece). One quirk of sentencepiece is that when decoding a sequence, if the first token is the start of the word (e.g. “Banana”), the tokenizer does not prepend the prefix space to the string.

It is confusing how can the first token is not a start of a word?





Hey! A few answers:
- you are using the fast version, which was never mentioned to be fixed yet.
- you are comparing the decoded outputs, which were not touched either. 
- I don't understand your question. My recommendation is to wait a little bit until we fix the default stripping mecanism with #23909, then will adresse the final issues with Llama. Decoding issue was mentioned here #25073. The first token is always a start of a word, but when you are the first token after a special token, you are just a token, not the first one 😉 

@ArthurZucker You can fix whatever possible now, but many people have finetuned v2 models without using `legacy=False`, so that's not really fair to put on the user. There should have been a strict error to prevent this, not a warning as most people ignore, and while I appreciate what the HF team does, this was handled very poorly. 

I hope the above can be resolved in a patch release, I had expected that earlier. Waiting until 4.32 while the default is causing issues in real world usage seems to long, if we have 4.31.1 with legacy=false by default it solves the issues.

The released llama2 models on the hub were all using `legacy = False`, so if they were not using the original tokenizer, not entirely sure what we could have done better. A lot of people trained `llama1` with `legacy=False` and use the `LlamaTokenizer` for other places, we cannot have a breaking change raising an error this way. 
More over, a lot of people use the `fast` version of the tokenizer, which was not changed either. 

Our goal is not to default to `legacy=False` but leave users choose the best solution for them, thus a warning. We are now defaulting to `True` if the `legacy` parameter was not set, with a warning. If you want to suggest maybe improvements for the warning I am all ears! We can maybe make it more visible? 


Note that the decoding is the same regardless of `legacy`, and `legacy` fixes only affects slow tokenizers.

```python 
>>> from transformers import AutoTokenizers
>>> btokenizer = AutoTokenizer.from_pretrained("meta-llama/Llama-2-7b-hf",legacy=True)
>>> btokenizer.decode(btokenizer("Hey how are you?
 I'm fine and you")['input_ids'])
"
 Hey how are you?
  I'm fine and you"
```

@official-elinas if they finetuned the model, then they can keep using `legacy=False`, since the model learned that there is an extra space after the special tokens. There are a lot of applications, but if you added a new token like `<bot>`, then anyway the model learns a new concepts, will learn at the same time that there is an extra space afterwards. 

Anyway I'm also really sorry there was a hidden bug, and the first Llama release was also very messy, I should have checked more than twice! Thanks for your feedbacks 🤗 


Just to give two cents here. I run my tests with both fast and slow tokenizers and I found that using legacy=True leads to more inconsistent behavior between the two. In the examples below the slow tokenizer yields `unk` when the fast one does not. Interestingly, I would expect the legacy one to be broken, _not_ the non-legacy one. I do not quite understand why the legacy one seems to work fine but the new one does not, so I will stick with the legacy behavior.

```
from transformers import AutoTokenizer

# LEGACY
########
s = "
 quiet"  # 
 is a special token
tokenizer_slow = AutoTokenizer.from_pretrained("google/mt5-base", use_fast=False, legacy=True)
tokenizer_fast = AutoTokenizer.from_pretrained("google/mt5-base", use_fast=True, legacy=True)

print(tokenizer_slow.decode(tokenizer_slow(s).input_ids))
# 
 quiet

print(tokenizer_fast.decode(tokenizer_fast(s).input_ids))
# 
 quiet


# Without space
s = "
quiet"  # 
 is a special token
tokenizer_slow = AutoTokenizer.from_pretrained("google/mt5-base", use_fast=False, legacy=True)
tokenizer_fast = AutoTokenizer.from_pretrained("google/mt5-base", use_fast=True, legacy=True)

print(tokenizer_slow.decode(tokenizer_slow(s).input_ids))
# 
 quiet

print(tokenizer_fast.decode(tokenizer_fast(s).input_ids))
# 
 quiet


# NOT LEGACY
############
s = "
 quiet"  # 
 is a special token
tokenizer_slow = AutoTokenizer.from_pretrained("google/mt5-base", use_fast=False, legacy=False)
tokenizer_fast = AutoTokenizer.from_pretrained("google/mt5-base", use_fast=True, legacy=False)

print(tokenizer_slow.decode(tokenizer_slow(s).input_ids))
# 
<unk>

print(tokenizer_fast.decode(tokenizer_fast(s).input_ids))
# 
 quiet


# Without space
s = "
quiet"  # 
 is a special token
tokenizer_slow = AutoTokenizer.from_pretrained("google/mt5-base", use_fast=False, legacy=False)
tokenizer_fast = AutoTokenizer.from_pretrained("google/mt5-base", use_fast=True, legacy=False)

print(tokenizer_slow.decode(tokenizer_slow(s).input_ids))
# 
<unk>

print(tokenizer_fast.decode(tokenizer_fast(s).input_ids))
# 
 quiet


```

Yes yes, if you look at #25224 will fix the unknown. Reported in #25176. It's a bit of a mess, but should all be fixed now! Pretty deep bug indeed

Hey @ArthurZucker -- so if I want to add a new token to the LLamaTokenizer like 'your_token', should I add the token to the sentencepiece model [#25224](https://github.com/huggingface/transformers/pull/25224) and then load the tokenizer using huggingface?  Do I have to change the tokenizer.json/special_tokens_map.json/tokenizer_config.json of the LLamaTokenizer as well and how should I change them?  

Currently using huggingface version 4.31.0.

It depends on the expected behaviour. ( simply put, if you want an extra space or not. But should not make a difference overall in results)
If you train the model, it won't make a difference (intuition mostly but you are either teaching him that `<new_token>` always has a space after it or not).


After cloning the repository for the ProtT5 model (Embedding/PyTorch/Advanced/ProtT5-XL-UniRef50.ipynb) and running the code with the exact same set of inputs as seen in the original, we are getting a different output. The numbers are varying from what would be expected given that we didn't change anything. Does anybody know why this might be happening?

We are also receiving an error that mentions the Legacy behavior, and are not sure of the significance.

The warning is just here to tell you to chose whether you want the legacy behaviour or not. Then you save the model and can use it without the warning. If the model was trained with the previous legacy behaviour, you should probably use it too.

@ArthurZucker  I already set legacy = True load llama2-7b-chat ,  but not lucky ,  also get this warning 


transformers==4.31.0
my code:
tokenizer = AutoTokenizer.from_pretrained(model_name,legcy=False,use_fast=False)


warning:

You are using the legacy behaviour of the <class 'transformers.models.llama.tokenization_llama.LlamaTokenizer'>. 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 https://github.com/huggingface/transformers/pull/24565

The code you copy pasted has a type:
`tokenizer = AutoTokenizer.from_pretrained(model_name,legcy=False,use_fast=False)` shoud be 
`tokenizer = AutoTokenizer.from_pretrained(model_name,legacy=False,use_fast=False)`

when i use api there is some warning:   
You are using the legacy behaviour of the <class 'transformers.models.llama.tokenization_llama.LlamaTokenizer'>. This means that tokens that come after special tokens will not be properly handled. 
and 
 requests.exceptions.ConnectionError: HTTPSConnectionPool(host='cdn-lfs.huggingface.co', port=443): Read timed out.
please tell me what should i do,thank you

> 

i also want know if use lora to fine-tuning llama2-7b, how many resoures do i need?

@fangyifei222, for these kind of question, please refer to [the forum](https://discuss.huggingface.co/) were community contributors share their experiences, and is the correct place to ask about resources. 
🤗 also provides the following:
- https://huggingface.co/blog/llama2#fine-tuning-with-peft 
- https://lnkd.in/eceMyUBW 

Meta AI provides the following ressources: https://github.com/facebookresearch/llama-recipes 

Above "the next version of transformers will probably have legacy=False by default, and remove the warning" means that right now, you get a warning no matter whether you set legacy or not, and no matter how you set it?

I am using https://huggingface.co/daryl149/llama-2-7b-chat-hf, and get the following warning: "You are using the legacy behaviour of the <class 'transformers.models.llama.tokenization_llama.LlamaTokenizer'>. 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 https://github.com/huggingface/transformers/pull/24565". So this means "legacy = True" and not ALL released llama2 models on the hub were using legacy = False. Right?

With "The released llama2 models on the hub were all using legacy = False" you mean meta's Llama2 models, e.g. https://huggingface.co/meta-llama/Llama-2-7b? 


Yes, meta's llama models all use `legacy=False`. The next transformers version will not set it to `False` by default. 
We set it to `None`, which will default to `True` with a warning. This is meant to force users to chose one, as it is important! 

> 

Arthur, I think your definition of users differs from my definition of users so I am going to need a hand here.
When you say users you seem to mean developers creating their own python scripts who know about these choices and can adapt it to their current use.
My definition of users is people who have very little experience with AI using our UI to just use a model.

I will need to set a default that makes sense and works on all models, or somehow automate this choice. So what do I do here? Should I hardcode either and then potentially break models? Because if I hardcode it model authors can't specify it in their config json anymore and now every transformers model now and in the future has either option.

Or is it best for me to leave it as is and put up with the warning to all my users?

Okay, you should open an issue on the hub to the corresponding repo if you don't know which one should be used. Otherwise if you just want to remove the warning and your code was working as expected, just force it to `False`. I don't expect a lot of people to use `legacy = False`, as it might be breaking the expected (but wrong) behaviour that was already there. **Don't panic** it's a minor discrepancy for anyone it seems at is was almost never noticed/reported! 

**TLDR! You can always hardcode `legacy = True` and your code will just be the same as before, which will give what you have been using for the past X years** 

The warning only appears once as we used `logger.warning_once`. If it helps we can improve the warning to whatever helps the understanding! Feel free to suggest anything

The issue is that I don't know which model my users will use, its a single unified loader for every model on the hub that uses AutoModelForCausalLM as the basis. So any possible model they use of any possible model format where this variable is reused would then be overridden in the future. So if I default to legacy = True this will begin to apply to every model and I might have worse behavior of models in the future.

We do have tokenizer workarounds in our code to handle an issue with it swallowing the space at the beginning of a generation (So that a generation ending in "My favorite sport is foot" would generate "My favorite sport is football." and not "My favorite sport is foot ball", this was a transformers setting on our request that unfortunately got removed prior to the 4.28 release so we ended up implementing it on our side). 

So I am fine with using legacy = False as a forced default for every model, but does this merely change behavior during inference or does it actually break tuned models? And in case of the latter, could it break models in both directions if I pick either one?

If you have a custom wrapper, you can probably check if the argument exists in the `tokenizer_config.json` (because fast does not have this attribute) and manually set it to `True` if it is not present no? 

This can affect inference if the model was trained with `legacy = True` and you change it to `False` and vis versa. If you train a model again, it will probably just adapt to the setting. 

> Just to give two cents here. I run my tests with both fast and slow tokenizers and I found that using legacy=True leads to more inconsistent behavior between the two. In the examples below the slow tokenizer yields `unk` when the fast one does not. Interestingly, I would expect the legacy one to be broken, _not_ the non-legacy one. I do not quite understand why the legacy one seems to work fine but the new one does not, so I will stick with the legacy behavior.
> 
> ```
> from transformers import AutoTokenizer
> 
> # LEGACY
> ########
> s = "
 quiet"  # 
 is a special token
> tokenizer_slow = AutoTokenizer.from_pretrained("google/mt5-base", use_fast=False, legacy=True)
> tokenizer_fast = AutoTokenizer.from_pretrained("google/mt5-base", use_fast=True, legacy=True)
> 
> print(tokenizer_slow.decode(tokenizer_slow(s).input_ids))
> # 
 quiet

> print(tokenizer_fast.decode(tokenizer_fast(s).input_ids))
> # 
 quiet

> 
> # Without space
> s = "
quiet"  # 
 is a special token
> tokenizer_slow = AutoTokenizer.from_pretrained("google/mt5-base", use_fast=False, legacy=True)
> tokenizer_fast = AutoTokenizer.from_pretrained("google/mt5-base", use_fast=True, legacy=True)
> 
> print(tokenizer_slow.decode(tokenizer_slow(s).input_ids))
> # 
 quiet

> print(tokenizer_fast.decode(tokenizer_fast(s).input_ids))
> # 
 quiet

> 
> # NOT LEGACY
> ############
> s = "
 quiet"  # 
 is a special token
> tokenizer_slow = AutoTokenizer.from_pretrained("google/mt5-base", use_fast=False, legacy=False)
> tokenizer_fast = AutoTokenizer.from_pretrained("google/mt5-base", use_fast=True, legacy=False)
> 
> print(tokenizer_slow.decode(tokenizer_slow(s).input_ids))
> # 
<unk>

> print(tokenizer_fast.decode(tokenizer_fast(s).input_ids))
> # 
 quiet

> 
> # Without space
> s = "
quiet"  # 
 is a special token
> tokenizer_slow = AutoTokenizer.from_pretrained("google/mt5-base", use_fast=False, legacy=False)
> tokenizer_fast = AutoTokenizer.from_pretrained("google/mt5-base", use_fast=True, legacy=False)
> 
> print(tokenizer_slow.decode(tokenizer_slow(s).input_ids))
> # 
<unk>

> print(tokenizer_fast.decode(tokenizer_fast(s).input_ids))
> # 
 quiet

> ```

Hi, I'm trying to perform inference using exactly the same formatting (prompt template and tokenization) as in training of the models in order to get the best possible performances. As of today, following the tests performed above by @BramVanroy, it seems that the only coherent behaviour between encoding/decoding is STILL using `use_fast=False` and `legacy=True`. 

I'm confused as if this is also the behaviour that is the same (or the closest) to how the models were trained? Indeed, using `use_fast=False` and `legacy=False` STILL wrongly adds `<unk>` tokens instead of correctly encoding the input, so surely this cannot be the correct behaviour.

Also, does the behaviour to use actually depends on the model? I'm especially interested in the Vicuna v1.3 family (e.g. lmsys/vicuna-13b-v1.3) and llama2 (e.g. meta-llama/Llama-2-7b-hf), but maybe the tokenizers for each model should be loaded with different behaviours as vicuna v1.3 is "old" compared to llama2.

I'm sorry if this was already addressed but I feel like the warning and this discussion are very unclear.

Yes, this has already been mention, if you read the thread you'll see that a PR is ont it's way. No The `legacy=True` is not always the most coherent. 
Please refrain from adding new comments if you have not read the full thread. It is important to make sure we don't duplicate answers. 
I'll add some more detailed documentation on the `legacy` vs non `legacy` behaviour. Understand that this is a very easy to to chose behaviour. Whatever works for you, `legacy` means `old` behaviour. If the `old` behaviour works better for now then why not use it? 

#25224 was just merged

Setting `legacy=False` has very unexpected consequences and generates terribly wrong tokenizations when using chatml dialogue formatting, (see TGI https://github.com/huggingface/text-generation-inference/issues/938 )

For example the input `"
assistant\n"` in `legacy=False` mode is tokenized as:

```json
 {"id":32005,"text":"
"},
 {"id":0,"text":"<unk>"},
 {"id":13,"text":"<0x0A>"}
 ```

Apparently it is now fixed in `transformers 4.32.1`.

Yes. An issue persists, which is the extra spaces added when decoding, and the new lines that are stripped around the added tokens. This cannot be mitigated  in slow for now (was not possible before either).
Remove the additional space with:
```python 
>>> t1.decode(t1.encode(prompt), spaces_between_special_tokens = False)
'user\nHello assistant!

assistant\n'
```

Sorry for the inconveniance.

How to avoid/suppress this warning in high level pipeline?

I am using this code, and am uncertain how to suppress this warning:

```python
# Use a pipeline as a high-level helper
from transformers import pipeline

pipe = pipeline("text2text-generation", model="Voicelab/vlt5-base-keywords")
pipe('Hello world')
```

Output:
```
You are using the default legacy behaviour of the <class 'transformers.models.t5.tokenization_t5.T5Tokenizer'>. If you see this, DO NOT PANIC! This is expected, and simply means that the `legacy` (previous) behavior will be used so nothing changes for you. If you want to use the new behaviour, set `legacy=False`. This should only be set if you understand what it means, and thouroughly read the reason why this was added as explained in https://github.com/huggingface/transformers/pull/24565
```

You should set `legacy=False/True` on the model on the hub/ init the tokenizer outside the pipeline
`pipe = pipeline("text2text-generation", model="Voicelab/vlt5-base-keywords", tokenizer = tokenizer)`


@ArthurZucker Thanks. Appreciate the help!

I am trying this but it fails.

```
ilab model train --legacy=False  
Error: Option '--legacy' does not take a value.
```

you might want to open an issue on `ilab`

Thank you for your trust, but should we really understand what to use by reading this thread? May be a page describing the issue would be better.