Complete merge Seq-2-Seq generation into default generation
Author: patrickvonplaten
Pull request: yes

This is a follow-up PR to finalize #3140 .

There was still no conclusion on how to handle the fairseq tricks for generation. To summarize: 

I think we have three options:

1. Remove all faiseq tricks. Here the ROUGE score is: **20.285**
2. Implement the fairseq tricks EXCEPT leaving the starting decoding_inputs_tokens to be the BOS token instead of EOS. Here the ROUGE score is: **19.369**
3. Add all fairseq tricks and maybe add a new argument to `generate()` which is called `decoder_start_token_id=bos_token_id` , but can be overriden to be the `eos_token_id` in the case of Bart. Here the ROUGE score is: **21.072**

ROUGE scores from @sshleifer

For comparison: 

![76256460-5b675780-6226-11ea-8516-28d0427251ca](https://user-images.githubusercontent.com/23423619/76422619-cdb27600-63a5-11ea-82c7-f36addb7fefb.png)

UPDATE:

Given the above scores, option 1. was chosen for the moment to have the same scores as fairseq. 
This means that we have to start the decoder ids with a EOS token (which might be weird and fairseq specific). Therefore, a new argument `decoder_start_token_id` was added to the generate function that defaults to the `bos_token_id`. When using Bart generate this argument should be set to the `eos_token_id` to have good results. To see how `Bart.generate()` should be used take a look at: 
https://github.com/huggingface/transformers/blob/2e81b9d8d76a4d41a13f74eb5e0f4a65d8143cab/tests/test_modeling_bart.py#L470

At the moment option 2. is implemented which seems to give the worst results and is also not the cleanest option. 
This PR implements option 3. 

For me either option 1. or option 3. is fine. Up to discuss @thomwolf , @julien-c , @LysandreJik @sshleifer 


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

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

```diff
@@            Coverage Diff            @@
##           master    #3225     +/-   ##
=========================================
+ Coverage   77.93%   78.03%   +0.1%     
=========================================
  Files          98       98             
  Lines       16666    16668      +2     
=========================================
+ Hits        12988    13007     +19     
+ Misses       3678     3661     -17
```


| [Impacted Files](https://codecov.io/gh/huggingface/transformers/pull/3225?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [src/transformers/modeling\_utils.py](https://codecov.io/gh/huggingface/transformers/pull/3225/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9tb2RlbGluZ191dGlscy5weQ==) | `93.86% <100%> (+0.15%)` | :arrow_up: |
| [src/transformers/modeling\_tf\_utils.py](https://codecov.io/gh/huggingface/transformers/pull/3225/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9tb2RlbGluZ190Zl91dGlscy5weQ==) | `88.55% <0%> (+2.86%)` | :arrow_up: |

------

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


Another issue that came up today from @yjernite, Bart does not support `do_sample=True` before 3140 this was clear and now it is :(

`Bart.generate()` with `do_sample=True` does not throw any errors. If it should not be done this way then we can just write in the docs that `do_sample` should be set to `False` and show an example. I don't see a problem here. We could think about a `Bart.summarize()` that calls `Bart.generate()` with the correct parameters if it's just for the prettier API. 

I prefer option #3, it looks like very little code and the best metrics.
Could you also fix the kwarg in `examples/summarization/bart/evaluate_cnn.py`?

> I prefer option #3, it looks like very little code and the best metrics.
> Could you also fix the kwarg in `examples/summarization/bart/evaluate_cnn.py`?

done :-) 

Linked to PR: https://github.com/huggingface/transformers/pull/3264 
Might need to fix eos_token_id conflicts when rebasing/merging.

This one is ok for me.
Is it currently option 3 which is implemented?

Also, I guess we could default to `do_sample==False` in `generate()`. Seems like the default expectation from the user is simple greedy decoding to me.

Yeah currently option 3 is implemented. 
`do_sample` used to default to `False`, but was changed to `True` by @LysandreJik  (can't find the issue/PR anymore :-/) Does not matter too much for me what is chosen, just would need to update some tests in `modeling_utils.py`

Yeah I'd love to merge this! Having trouble connecting to brutasse to run rouge, but afaict it will be the same as pre 3140 :) 

Good to merge for me! 
changing `do_sample=False` can be done in another PR, I think. 

IMPORNTANT: 
the `config.json` files of:

- `bart-large-cnn`
- `bart-large-mnli`
- `bart-large `

have to be updated on AWS to pass all slow tests. All `special_tokens_id` parameters should be deleted there. For the moment, we  will go with the solution: 
#3264 .

Ok merging. I let you continue in other PRs and ping me.

> Good to merge for me!
> changing `do_sample=False` can be done in another PR, I think.
> 
> IMPORNTANT:
> the `config.json` files of:
> 
> * `bart-large-cnn`
> * `bart-large-mnli`
> * `bart-large `
> 
> have to be updated on AWS to pass all slow tests. All `special_tokens_id` parameters should be deleted there. For the moment, we will go with the solution:
> #3264 .

Changed the configs on AWS. All slow tests pass now.