Merge bart generate into default generate
Author: patrickvonplaten
Pull request: yes

This PR is a first version of how the bart generation() code can be merged into the "default" generation function. I think it's actually much better feasible than we originally thought. 
Please not that this change also includes all the changes from PR #3135 , so the code changes will be much less cluttered after #3135 merged. 

This version was passes the general random language generation tests in found in `test_modeling_common.py` and the easy integration test with the original fairseq model (renamed to `test_cnn_summarization_same_as_fairseq_easy` in `test_modeling_bart`)

There are a couple of things we should discuss:

1. Both Bart generate() and default generate(), encoder-decoder models **must** have a BOS token and an EOS token.

2. Two new parameters were added: `min_length` and `no_repeat_ngram_size` . I think these parameters should be added generally as it is done now.

3. There was one hack which initializes the `decoder_input_ids` to the EOS token and then forces the model to generate the BOS token afterwards (see comment in code line). I changed it to simply start with the BOS token (which makes more sense) and it also passed the "easy integration tests". This hack might be needed to pass the hard integration test though.

4. Fairseq forces the last token of all beam hypotheses to be the EOS token (see comment in line). This is probably necessary to pass the integration tests. It's up for debate whether this the correct way. I would prefer not to do it this way because it will then be impossible to generate unfinished sentences (sentence that end because they hit `max_length`). If one really wants all beam hypotheses to be finished, one could set the `max_length` higher than usual and set the parameter:
`self.early_stopping` in the Beam Hypotheses class to `True`. Up for debate how to handle this. 

5. In order to also pass the hard integration tests (which has a padded batch as an input), we will have to add `attention_masks` to the `generate`() function. Here I see three possibilities: 
         a) add the `attention_mask` as a parameter to the generation() function.
         b) automatically calculate the `attention_mask` from the `input_ids` **if** the model has a `pad_token_id` **and** there is a `pad_token_id` in the input_ids.
         c) Not allow padded batches for the moment. 
I would prefer option b) because some models do have a set `pad_token_id` (such as Bart) so we should be able to allow padded generation. 







Comments:
clarification:

`bart.generate` doesn't add EOS if `max_length` is hit, or require EOS to pass integration tests. It just "finalizes" a hypothesis when the model predicts EOS for a beam.

Example:

```python
ARTICLE_TO_SUMMARIZE = "code doesnt generate EOS if max_length is hit"
inputs = tokenizer.batch_encode_plus([ARTICLE_TO_SUMMARIZE], return_tensors='pt')

generated_ids = model.generate(inputs['input_ids'], attention_mask=inputs['attention_mask'], num_beams=4, max_length=5)
summary_text = tokenizer.decode(generated_ids[0])
print(generated_ids[0], summary_text)
# (tensor([   0, 2387,  964,   32, 3035]), '
My friends are cool')
```


> I would thus like to propose the following workflow for the forward pass of all models:
> [...]
> What do you think especially @LysandreJik and @julien-c

Sounds good to me

By the way, my workflow proposition actually implied that we should use the same workflow and inputs for the `generate()` method as well (I could have been more explicit)

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

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

```diff
@@            Coverage Diff             @@
##           master    #3140      +/-   ##
==========================================
- Coverage   78.14%   77.99%   -0.16%     
==========================================
  Files          98       98              
  Lines       16668    16665       -3     
==========================================
- Hits        13026    12998      -28     
- Misses       3642     3667      +25
```


| [Impacted Files](https://codecov.io/gh/huggingface/transformers/pull/3140?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [src/transformers/configuration\_bart.py](https://codecov.io/gh/huggingface/transformers/pull/3140/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9jb25maWd1cmF0aW9uX2JhcnQucHk=) | `100% <ø> (ø)` | :arrow_up: |
| [src/transformers/configuration\_utils.py](https://codecov.io/gh/huggingface/transformers/pull/3140/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9jb25maWd1cmF0aW9uX3V0aWxzLnB5) | `96.82% <100%> (+0.07%)` | :arrow_up: |
| [src/transformers/modeling\_bart.py](https://codecov.io/gh/huggingface/transformers/pull/3140/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9tb2RlbGluZ19iYXJ0LnB5) | `96.27% <100%> (+2.69%)` | :arrow_up: |
| [src/transformers/configuration\_t5.py](https://codecov.io/gh/huggingface/transformers/pull/3140/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9jb25maWd1cmF0aW9uX3Q1LnB5) | `96.55% <100%> (ø)` | :arrow_up: |
| [src/transformers/modeling\_tf\_utils.py](https://codecov.io/gh/huggingface/transformers/pull/3140/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9tb2RlbGluZ190Zl91dGlscy5weQ==) | `87.47% <46.96%> (-6.27%)` | :arrow_down: |
| [src/transformers/modeling\_utils.py](https://codecov.io/gh/huggingface/transformers/pull/3140/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9tb2RlbGluZ191dGlscy5weQ==) | `93.84% <90.32%> (-0.57%)` | :arrow_down: |

------

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


UPDATE: This version passes all integration tests now. There are two things which I quite hacky we probably should implement in a cleaner way:

- the function `prepare_scores_for_generation` is a hacky function to make Bart pass the integration tests. See in code-line comment
- initializing all encoder-decoder models with the EOS token is probably not the right thing to do. See in code-line comment.

@sshleifer @thomwolf @LysandreJik 

@thomwolf, @sshleifer Thanks for your reviews guys. I think we are on the same page except for two things:

1. Force tokens to be generated. The BOS token is generated at `step=0` and the EOS token is generated at `step=max_length`. This is necessary to reproduce the original fairseq results. Should we keep that as a model specific "prepare" function?
2. Use the EOS token for the Bart models. This is also necessary to reproduce the original fairseq results. Should we keep that?

My opinion is: 

1. I would remove both statements that force a certain token to be predicted. Even without doing it, Bart produces (in my opinion) good summarization results. 
2. Same goes for this point. Would always use BOS token as the starting `decoder_input_ids` for encoder-decoder models and force encoder-decoder models to have a BOS token to be able to do language generation. 

Doing this would means, we would have to change the integration tests and we won't produce 1:1 the same results as fairseq anymore.


@sshleifer you can probably estimate the consequences for the Bart summarization quality much better than me! From the examples in the `test_modeling_bart.py`, I ran the summarization and the output looked good to me, but didn't measure ROUGE scores or anything...

What do you guys think? 

I ran eval on the full CNN test set with these simplifications, and Rouge decreases from .21072 to .20285.


For context, here are the published Rouge-2 scores of a bunch of different models: 

![image](https://user-images.githubusercontent.com/6045025/76256460-5b675780-6226-11ea-8516-28d0427251ca.png)

Note: the published bart score is a bit higher (.2128) because there are even [more tricks](https://github.com/pytorch/fairseq/issues/1765#issuecomment-593720522) I didn't implement.


> core is a bit higher (.2128) because the

Awesome! Thanks a lot. I'm not super familiar with Rouge - is that drop in performance very significant? @sshleifer @thomwolf 

> I ran eval on the full CNN test set with these simplifications, and Rouge decreases from .21072 to .20285.
> 
> For context, here are the published Rouge-2 scores of a bunch of different models:
> 
> ![image](https://user-images.githubusercontent.com/6045025/76256460-5b675780-6226-11ea-8516-28d0427251ca.png)
> 
> Note: the published bart score is a bit higher (.2128) because there are even [more tricks](https://github.com/pytorch/fairseq/issues/1765#issuecomment-593720522) I didn't implement.

@sshleifer what is exactly the trick you mentioned you didn't implemented? to "force the second token to not be bos"?

Overall I think I'm fine with have a clean distinction between post filtering method that will be optionally called for a model and will store all these post filtering tricks in specific models and a generic `generate()` that (for now) will be cleaner. The weirdest trick to me is to initialize with the EOS token (can we maybe use two times the BOS token here for instance?), the other tricks are less shocking.

Ok let's merge this to be able to move forward with T5 cc @craffel
@julien-c @LysandreJik we think the self-hosted failing tests are not related to this.
Can you have a look later maybe?

@patrickvonplaten You mentioned a way to perform batch inference with `GPT2LMHeadModel` using an attention mask here: https://github.com/huggingface/transformers/issues/3021#issuecomment-591418233.

Does this PR make this possible by calling `model.generate(input_ids=..., attention_mask=...)`?

> @patrickvonplaten You mentioned a way to perform batch inference with `GPT2LMHeadModel` using an attention mask here: [#3021 (comment)](https://github.com/huggingface/transformers/issues/3021#issuecomment-591418233).
> 
> Does this PR make this possible by calling `model.generate(input_ids=..., attention_mask=...)`?

Hi @thesamuel, 

Not yet completely. It's one step to make the generation possible as shown in #3021, but there are still two things that are not considered yet in the generate fn:
1. The position embeddings have to be updated - which generate() does not do yet
2. And this is the hard one: If a padded batch is given as an input, it should not be sampled from the last token, but from the last non-padded token and this can be quite hacky. 

We are currently thinking about how to implement this!

@patrickvonplaten Got it, thanks!

(Investigating) This PR may introduce a BOS bug that reduces rouge to 15.068 from 21.28

Simple bug caused by `do_sample` (which for some reason defaults to True). Anyway, I'm rerunning rouge but it will likely be at a reasonable level.

Possible follow-up states are explained in PR: https://github.com/huggingface/transformers/pull/3225