T5Tokenizer adds EOS token if not already added
Author: sshleifer
Pull request: yes

T5 Tokenizer should add `
` to the end of sequences. Since some users are doing this on their own, this PR only adds `
` if it has already been added.



On my machine, this makes zero shot validation BLEU go from 27.87 -> 27.65. Since this change is needed for finetuning, and the empirical difference is small and doesn't happen on Stas' machine, I would recommend merging this.

If others want to test, the command takes about 3 mins to run on brutasse.

### Zero Shot BLEU Scores
For english -> romanian
I grabbed the WMT english-romanian dataset:
```bash
wget https://s3.amazonaws.com/datasets.huggingface.co/translation/wmt_en_ro.tar.gz
tar -xzvf wmt_en_ro.tar.gz
```
Then ran evaluation (without finetuning) on the validation split:

```bash
export DATA_DIR=wmt_en_ro
python run_eval.py t5-base \
    $DATA_DIR/val.source t5_val_generations.txt \
    --reference_path $DATA_DIR/val.target \
    --score_path t5_enro_bleu_eos.json \
    --task translation_en_to_ro \
    --device cuda \
    --fp16 \
    --bs 32
```
(this branch) (with EOS):27.65
master (no EOS):  27.87
```
sacrebleu==1.4.3
torch==1.5.1
```
Will merge and fix tests if others have positive results.

Comments:
# [Codecov](https://codecov.io/gh/huggingface/transformers/pull/5866?src=pr&el=h1) Report
> Merging [#5866](https://codecov.io/gh/huggingface/transformers/pull/5866?src=pr&el=desc) into [master](https://codecov.io/gh/huggingface/transformers/commit/7e6397a7d8e7433aa4c4cafba98e08e5c73f087c?el=desc) will **decrease** coverage by `0.67%`.
> The diff coverage is `89.47%`.

[![Impacted file tree graph](https://codecov.io/gh/huggingface/transformers/pull/5866/graphs/tree.svg?width=650&height=150&src=pr&token=9qOlN6Hb1c)](https://codecov.io/gh/huggingface/transformers/pull/5866?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master    #5866      +/-   ##
==========================================
- Coverage   80.10%   79.42%   -0.68%     
==========================================
  Files         156      156              
  Lines       28411    28426      +15     
==========================================
- Hits        22758    22578     -180     
- Misses       5653     5848     +195     
```


| [Impacted Files](https://codecov.io/gh/huggingface/transformers/pull/5866?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [src/transformers/tokenization\_t5.py](https://codecov.io/gh/huggingface/transformers/pull/5866/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy90b2tlbml6YXRpb25fdDUucHk=) | `95.32% <89.47%> (-1.42%)` | :arrow_down: |
| [src/transformers/modeling\_tf\_openai.py](https://codecov.io/gh/huggingface/transformers/pull/5866/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9tb2RlbGluZ190Zl9vcGVuYWkucHk=) | `22.58% <0.00%> (-72.26%)` | :arrow_down: |
| [src/transformers/tokenization\_roberta.py](https://codecov.io/gh/huggingface/transformers/pull/5866/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy90b2tlbml6YXRpb25fcm9iZXJ0YS5weQ==) | `87.67% <0.00%> (-10.96%)` | :arrow_down: |
| [src/transformers/tokenization\_utils\_base.py](https://codecov.io/gh/huggingface/transformers/pull/5866/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy90b2tlbml6YXRpb25fdXRpbHNfYmFzZS5weQ==) | `86.58% <0.00%> (-7.19%)` | :arrow_down: |
| [src/transformers/tokenization\_transfo\_xl.py](https://codecov.io/gh/huggingface/transformers/pull/5866/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy90b2tlbml6YXRpb25fdHJhbnNmb194bC5weQ==) | `38.73% <0.00%> (-3.76%)` | :arrow_down: |
| [src/transformers/tokenization\_openai.py](https://codecov.io/gh/huggingface/transformers/pull/5866/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy90b2tlbml6YXRpb25fb3BlbmFpLnB5) | `82.57% <0.00%> (-1.52%)` | :arrow_down: |
| [src/transformers/tokenization\_utils\_fast.py](https://codecov.io/gh/huggingface/transformers/pull/5866/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy90b2tlbml6YXRpb25fdXRpbHNfZmFzdC5weQ==) | `92.85% <0.00%> (-1.43%)` | :arrow_down: |
| [src/transformers/modeling\_openai.py](https://codecov.io/gh/huggingface/transformers/pull/5866/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9tb2RlbGluZ19vcGVuYWkucHk=) | `80.96% <0.00%> (-1.30%)` | :arrow_down: |
| [src/transformers/generation\_tf\_utils.py](https://codecov.io/gh/huggingface/transformers/pull/5866/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9nZW5lcmF0aW9uX3RmX3V0aWxzLnB5) | `86.21% <0.00%> (-0.51%)` | :arrow_down: |
| [src/transformers/tokenization\_bert.py](https://codecov.io/gh/huggingface/transformers/pull/5866/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy90b2tlbml6YXRpb25fYmVydC5weQ==) | `91.07% <0.00%> (-0.45%)` | :arrow_down: |
| ... and [2 more](https://codecov.io/gh/huggingface/transformers/pull/5866/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/huggingface/transformers/pull/5866?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/huggingface/transformers/pull/5866?src=pr&el=footer). Last update [7e6397a...d977bff](https://codecov.io/gh/huggingface/transformers/pull/5866?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


FWIW, I get identical results of `27.84` with this branch and the master.

Happy to eventually remove the check to see if it's already there.

I think we can keep it like this right now with a warning for future versions. It would create a breaking change to users, and I feel it would be especially hard to debug an unknown drop in performance due to an additional token being added, right?

Will this behavior cause problems for the unsupervised setting? Per the [docs](https://huggingface.co/transformers/model_doc/t5.html#training), `
` is not added during denoising training:

```
input_ids = tokenizer.encode('The <extra_id_0> walks in <extra_id_1> park', return_tensors='pt')
labels = tokenizer.encode('<extra_id_0> cute dog <extra_id_1> the <extra_id_2> 
', return_tensors='pt')
model(input_ids=input_ids, labels=labels)
```

Not sure if this will cause problems. (Also, Aa a somewhat related question, should the sentinel tokens in the `labels` be excluded from the loss in this setting, as I believe is the case with `[MASK]` in BERT?).

I'm not sure about either question:
made an issue verifying the docs: https://github.com/huggingface/transformers/issues/7904
Feel free to make an issue about the sentinel tokens question. I'd tag thomwolf/patrickvonplaten.