Bug with Training T5 Tokenizers on New Data 
Author: yangky11
Pull request: no
Labels: name: bug

### System Info

- `transformers` version: 4.22.2
- Platform: Linux-5.10.135-122.509.amzn2.x86_64-x86_64-with-glibc2.17
- Python version: 3.8.13
- Huggingface_hub version: 0.10.0
- PyTorch version (GPU?): 1.12.1 (False)
- Tensorflow version (GPU?): not installed (NA)
- Flax version (CPU?/GPU?/TPU?): not installed (NA)
- Jax version: not installed
- JaxLib version: not installed
- Using GPU in script?: No
- Using distributed or parallel set-up in script?: No

### Who can help?

@patrickvonplaten @SaulLu



### Information

- [ ] The official example scripts
- [X] My own modified scripts

### Tasks

- [ ] An officially supported task in the `examples` folder (such as GLUE/SQuAD, ...)
- [X] My own task or dataset (give details below)

### Reproduction

```python
from transformers import AutoTokenizer

# Train T5's tokenizer on some new data.
training_corpus = ["12rdpo2rkfp", "$##@sdfag", "ja23m d@#"]
tokenizer = AutoTokenizer.from_pretrained("t5-large")
new_tokenizer = tokenizer.train_new_from_iterator(training_corpus, vocab_size=110)

# Print the vocabulary sequentially.
for i in range(110):
    print(new_tokenizer.convert_ids_to_tokens([i])[0])

# You'll see sentinel tokens such as `<extra_id_1>` are NOT at the end of the vocabulary.
```

### Expected behavior

The sentinel tokens in T5 must be at the end of the vocabulary. This constraint is stated in the documentation (e.g., [here](https://huggingface.co/docs/transformers/model_doc/t5#transformers.T5Tokenizer.extra_ids)), and official examples are relying on it. The code below is trying to find sentinel tokens from the back of the vocabulary (`len(self.tokenizer) - sentinel_ids`).
https://github.com/huggingface/transformers/blob/5cd16f01db3b5499d4665e8624801ed30ba87bdd/examples/flax/language-modeling/run_t5_mlm_flax.py#L378

However, when I follow [Hugging Face Course](https://huggingface.co/course/en/chapter6/2?fw=pt) to train T5's tokenizer on new data. The new tokenizer does not conform to this constraint. 

Comments:
Hi @yangky11 ,

Thank you very much for pointing this out! It is indeed a problem!

To fix it, we'll have to look into whether we need to pay special attention to the `train_new_from_iterator` method of the `T5TokenizerFast` tokenizer or (even better) whether we really need the sentinel ids to be at the end.

@SaulLu Thanks for your response! 

I'm not familiar with every detail of T5. But to my knowledge, the only reason we need them to be at the end is that user code sometimes relies on this feature (as in the example I mentioned). It's also possible that you add APIs to `T5TokenizerFast` to allow the user to query the ids of sentinel tokens. Then the user code can handle them on their side.

Also cc @ArthurZucker here FYI

@patrickvonplaten @SaulLu  I can pick this issue. What should be the approach we need to take ? 


Also cc @LysandreJik @sgugger here 

Any luck on this?

@patrickvonplaten  @sgugger  Can we decide what approach to take to fix this ? 

I think choosing how to resolve this issue requires some discussion.

I think we should change the example scripts that look for sentinel tokens based on the fact that they are the last tokens in the vocabulary. Rather, it should be based on the fact that they are the tokens in the `additional_special_tokens` attribute (respectively `additional_special_tokens_ids` for their ids).

However, an important thing to know with this suggestion is that even if for T5 tokenizers there is no reason to have any additional special tokens other than sentinel ids, there is still a risk that a user adds a new additional special token that is not a sentinel token and this would lead to a (silent) error.  

Which leads me to a second opinion, I think this issue shows here that a `breaking change `would be very beneficial to the user experience. Indeed, for T5, sentinel ids have a very important meaning for the model (as much as bos, eos, or sep tokens for bert type models for example) and I think it would be justified to have a dedicated attribute for them (such as `sentinel_tokens`) rather than being in the additional tokens list of `SpecialTokensMixin`. Or, another possibility would be to be able to name the tokens in the list of additional tokens so that we can rely on this naming to retrieve them from the list rather than their position in the list. 

@SaulLu I was also thinking along same lines, as a simpler fix remove the pointers of sentinel tokens as the last tokens and then use an additional attribute to get those token. 

@patrickvonplaten  @sgugger  what do you folks think of this approach ? 

As long as it's done in examples that are focused on T5-only (e.g. not the generic `run_summarization`), no problem with me!

This issue has been automatically marked as stale because it has not had recent activity. If you think this still needs to be addressed please comment on this thread.

Please note that issues that do not follow the [contributing guidelines](https://github.com/huggingface/transformers/blob/main/CONTRIBUTING.md) are likely to be ignored.

Hi,

Has this issue been resolved? I see at least the `run_t5_mlm_flax.py` example is still relying on the old behavior.