T5Tokenizer adds EOS token if not already added

[sshleifer]

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:

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:

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]

Codecov Report

Merging #5866 into master will decrease coverage by 0.67%.
The diff coverage is 89.47%.

@@            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	Coverage Δ
src/transformers/tokenization_t5.py	95.32% <89.47%> (-1.42%)	⬇️
src/transformers/modeling_tf_openai.py	22.58% <0.00%> (-72.26%)	⬇️
src/transformers/tokenization_roberta.py	87.67% <0.00%> (-10.96%)	⬇️
src/transformers/tokenization_utils_base.py	86.58% <0.00%> (-7.19%)	⬇️
src/transformers/tokenization_transfo_xl.py	38.73% <0.00%> (-3.76%)	⬇️
src/transformers/tokenization_openai.py	82.57% <0.00%> (-1.52%)	⬇️
src/transformers/tokenization_utils_fast.py	92.85% <0.00%> (-1.43%)	⬇️
src/transformers/modeling_openai.py	80.96% <0.00%> (-1.30%)	⬇️
src/transformers/generation_tf_utils.py	86.21% <0.00%> (-0.51%)	⬇️
src/transformers/tokenization_bert.py	91.07% <0.00%> (-0.45%)	⬇️
... and 2 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7e6397a...d977bff. Read the comment docs.

[stas00]

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

[sgugger]

LGTM thanks! Just a few nits in the docs.

[patrickvonplaten]

Great! Very clean implementation. Not sure what to say about the evaluation. I think the functionality should be added anyways and the user can optionally set add_special_tokens=False if he/she wants

[LysandreJik]

Great, nice addition. Maybe not the cleanest to check if there's already an existing token as we don't do that in other implementations, but cool that this brings backwards-compatibility.

I believe we should align this with other tokenizers' prepare inputs methods in a future version, by always adding the EOS token even if there's already one in the input.

[LysandreJik]

Could we raise a warning/info telling the user that this is handled by the method, and that adding it manually + using the function would result in two tokens being added in a future version?

[sshleifer]

sure.

[sshleifer]

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

[LysandreJik]

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?

[ahoho]

Will this behavior cause problems for the unsupervised setting? Per the docs, </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?).

[sshleifer]

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