T5Tokenizer adds EOS token if not already added
T5 Tokenizer should add `</s>` to the end of sequences. Since some users are doing this on their own, this PR only adds `</s>` 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.
# [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), `</s>` 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> </s>', 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.
