[generation] consistently add eos tokens
Author: stas00
Pull request: yes

Currently beam search returns inconsistent outputs - if hypos have different lengths we get eos, if they are the same - we don't. I found a sentence where if I change one letter in one of the input words the beam search outputs all suddenly lack eos, which is an inconsistent behavior.

This PR makes the output more consistent. (but not 100%, please see below)

---------


Also why not replace:

```
            if sent_lengths[i] < max_length:
                decoded[i, sent_lengths[i]] = eos_token_id
```
with:
```
            decoded[i, sent_lengths[i]] = eos_token_id
```
Shouldn't eos always be there? If generated data gets truncated, the caller needs to use a larger `max_length`.

Currently, if the hypos lengths are on the cusp of `max_length`, some of them will get eos, whereas others won't, which is again inconsistent.

Please correct me if my logic is flawed.

-----

I also looked at `_generate_no_beam_search` - there eos adding logic uses a somewhat different logic. 

Should the two functions (beam/no_beam) be consistent eos-injection wise?



Comments:
IMO, `eos` should not always be there. The reason is that if the user defines `max_length=30` and the EOS token was not generated by the model, then no EOS token should be added. I think EOS token should only be added if it is produced by the model. *E.g.* a generated sentence like "I will go to the office and" should not have an added EOS token at the end.

> [...] _E.g._ a generated sentence like "I will go to the office and" should not have an added EOS token at the end.

Thank you for explaining that, @patrickvonplaten.

Could you also please review this PR, as it's unrelated to max_length or undefined EOS, that was a related question that I wasn't sure about.


@stas00
Have you run the slow tests that might be effected? (will take 10-30 mins)

```
run_generation_integration_tests () {
	# assumes USE_CUDA is exported, rather than passed
	RUN_SLOW=1 pytest tests/test_modeling_pegasus.py
	RUN_SLOW=1 pytest tests/test_modeling_bart.py
	RUN_SLOW=1 pytest tests/test_modeling_t5.py
	RUN_SLOW=1 pytest tests/test_modeling_marian.py
	RUN_SLOW=1 pytest tests/test_modeling_mbart.py
	RUN_SLOW=1 pytest tests/test_modeling_encoder_decoder.py
	RUN_SLOW=1 pytest tests/test_pipelines.py
	RUN_SLOW=1 pytest tests/test_modeling_gpt2.py
}
```

Good call, @sshleifer! (I edited the last one to `tests/test_modeling_gpt2.py`)

```
RUN_SLOW=1 pytest --disable-warnings tests/test_modeling_pegasus.py tests/test_modeling_bart.py tests/test_modeling_t5.py tests/test_modeling_marian.py tests/test_modeling_mbart.py tests/test_modeling_encoder_decoder.py tests/test_pipelines.py tests/test_modeling_gpt2.py
====================================================================== test session starts =======================================================================
platform linux -- Python 3.7.5, pytest-5.4.1, py-1.8.1, pluggy-0.13.1
rootdir: /mnt/nvme1/code/huggingface/transformers
plugins: hypothesis-5.5.4, timeout-1.4.2, filter-subpackage-0.1.1, arraydiff-0.3, flaky-3.6.1, ipynb-1.1.1.dev0, cov-2.10.0, astropy-header-0.1.2, forked-1.2.0, doctestplus-0.5.0, openfiles-0.4.0, remotedata-0.3.2, xdist-1.32.0, repeat-0.8.0, flakefinder-1.0.0
collected 211 items

tests/test_modeling_pegasus.py ..                                                                                                                          [  0%]
tests/test_modeling_bart.py ..............s....s............................                                                                               [ 23%]
tests/test_modeling_t5.py ......................s..s...........                                                                                            [ 41%]
tests/test_modeling_marian.py ................                                                                                                             [ 48%]
tests/test_modeling_mbart.py s...s.                                                                                                                        [ 51%]
tests/test_modeling_encoder_decoder.py ...............................                                                                                     [ 66%]
tests/test_pipelines.py .......................................                                                                                            [ 84%]
tests/test_modeling_gpt2.py .......................s........                                                                                               [100%]

==================================================== 204 passed, 7 skipped, 45 warnings in 980.34s (0:16:20) =====================================================
```

# [Codecov](https://codecov.io/gh/huggingface/transformers/pull/6982?src=pr&el=h1) Report
> Merging [#6982](https://codecov.io/gh/huggingface/transformers/pull/6982?src=pr&el=desc) into [master](https://codecov.io/gh/huggingface/transformers/commit/ce37be9d94da57897cce9c49b3421e6a8a927d4a?el=desc) will **increase** coverage by `2.39%`.
> The diff coverage is `100.00%`.

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

```diff
@@            Coverage Diff             @@
##           master    #6982      +/-   ##
==========================================
+ Coverage   77.60%   80.00%   +2.39%     
==========================================
  Files         161      161              
  Lines       30120    30119       -1     
==========================================
+ Hits        23374    24096     +722     
+ Misses       6746     6023     -723     
```


| [Impacted Files](https://codecov.io/gh/huggingface/transformers/pull/6982?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [src/transformers/generation\_utils.py](https://codecov.io/gh/huggingface/transformers/pull/6982/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9nZW5lcmF0aW9uX3V0aWxzLnB5) | `97.20% <100.00%> (-0.01%)` | :arrow_down: |
| [src/transformers/modeling\_tf\_electra.py](https://codecov.io/gh/huggingface/transformers/pull/6982/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9tb2RlbGluZ190Zl9lbGVjdHJhLnB5) | `25.13% <0.00%> (-73.83%)` | :arrow_down: |
| [src/transformers/modeling\_marian.py](https://codecov.io/gh/huggingface/transformers/pull/6982/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9tb2RlbGluZ19tYXJpYW4ucHk=) | `60.00% <0.00%> (-30.00%)` | :arrow_down: |
| [src/transformers/activations.py](https://codecov.io/gh/huggingface/transformers/pull/6982/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9hY3RpdmF0aW9ucy5weQ==) | `85.00% <0.00%> (-5.00%)` | :arrow_down: |
| [src/transformers/modeling\_bart.py](https://codecov.io/gh/huggingface/transformers/pull/6982/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9tb2RlbGluZ19iYXJ0LnB5) | `93.90% <0.00%> (-0.51%)` | :arrow_down: |
| [src/transformers/modeling\_tf\_utils.py](https://codecov.io/gh/huggingface/transformers/pull/6982/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9tb2RlbGluZ190Zl91dGlscy5weQ==) | `86.97% <0.00%> (-0.33%)` | :arrow_down: |
| [src/transformers/tokenization\_utils\_base.py](https://codecov.io/gh/huggingface/transformers/pull/6982/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy90b2tlbml6YXRpb25fdXRpbHNfYmFzZS5weQ==) | `93.76% <0.00%> (-0.28%)` | :arrow_down: |
| [src/transformers/generation\_tf\_utils.py](https://codecov.io/gh/huggingface/transformers/pull/6982/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9nZW5lcmF0aW9uX3RmX3V0aWxzLnB5) | `85.21% <0.00%> (+1.00%)` | :arrow_up: |
| [src/transformers/modeling\_lxmert.py](https://codecov.io/gh/huggingface/transformers/pull/6982/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9tb2RlbGluZ19seG1lcnQucHk=) | `90.76% <0.00%> (+20.74%)` | :arrow_up: |
| [src/transformers/modeling\_tf\_flaubert.py](https://codecov.io/gh/huggingface/transformers/pull/6982/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9tb2RlbGluZ190Zl9mbGF1YmVydC5weQ==) | `88.34% <0.00%> (+63.80%)` | :arrow_up: |
| ... and [2 more](https://codecov.io/gh/huggingface/transformers/pull/6982/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/huggingface/transformers/pull/6982?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/6982?src=pr&el=footer). Last update [ce37be9...85dd09d](https://codecov.io/gh/huggingface/transformers/pull/6982?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).