[generation] decoder priority for choosing decoder_start_token_id value
Author: stas00
Pull request: yes

`config.decoder` needs to be checked first before model's `config` to set `decoder_start_token_id`. 

This is needed for https://github.com/huggingface/transformers/pull/6940 where I think for the first time there is an actual `config.decoder`


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

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

```diff
@@            Coverage Diff             @@
##           master    #6996      +/-   ##
==========================================
- Coverage   80.58%   80.03%   -0.56%     
==========================================
  Files         161      161              
  Lines       30123    30123              
==========================================
- Hits        24276    24109     -167     
- Misses       5847     6014     +167     
```


| [Impacted Files](https://codecov.io/gh/huggingface/transformers/pull/6996?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [src/transformers/generation\_utils.py](https://codecov.io/gh/huggingface/transformers/pull/6996/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9nZW5lcmF0aW9uX3V0aWxzLnB5) | `97.21% <100.00%> (ø)` | |
| [src/transformers/modeling\_tf\_electra.py](https://codecov.io/gh/huggingface/transformers/pull/6996/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9tb2RlbGluZ190Zl9lbGVjdHJhLnB5) | `25.13% <0.00%> (-73.83%)` | :arrow_down: |
| [src/transformers/modeling\_tf\_utils.py](https://codecov.io/gh/huggingface/transformers/pull/6996/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/6996/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy90b2tlbml6YXRpb25fdXRpbHNfYmFzZS5weQ==) | `93.63% <0.00%> (-0.14%)` | :arrow_down: |
| [src/transformers/data/data\_collator.py](https://codecov.io/gh/huggingface/transformers/pull/6996/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9kYXRhL2RhdGFfY29sbGF0b3IucHk=) | `91.90% <0.00%> (+5.26%)` | :arrow_up: |
| [src/transformers/modeling\_tf\_flaubert.py](https://codecov.io/gh/huggingface/transformers/pull/6996/diff?src=pr&el=tree#diff-c3JjL3RyYW5zZm9ybWVycy9tb2RlbGluZ190Zl9mbGF1YmVydC5weQ==) | `88.34% <0.00%> (+63.80%)` | :arrow_up: |

------

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


Shouldn't this be part of #6996? Or would this PR be needed either way?

> Shouldn't this be part of #6996? Or would this PR be needed either way?

Did you mean "part of " https://github.com/huggingface/transformers/pull/6940?

It's there already, and yes it's required to work. I just thought that it's the best to change core functions in separate PRs and not as a part of a much larger PR. Please advise if this is not the right approach.

In this particular case `config.decoder` is more specific than `config` and therefore should be checked first. I don't think currently there is any more that actively uses `config.decoder` (grep didn't find any), therefore it must have been untested since it was added in first place perhaps?



Don't really agree with this PR - I think `bos_token_id` should have higher priority than `model.config.decoder.bos_token_id`. The `model.config.decoder.bos_token_id` was mainly added because of the `EncoderDecoderModel` framework

Yes I meant "part of " #6940? 
If two PRs do not exist/make sense without each other I think they should be together. 
Otherwise we can merge one without the other and have either broken or dead code

@stas00 why can't you use `decoder_start_token_id` for FSMT?

> why can't you use `decoder_start_token_id` for FSMT?

That works - thank you for the suggestion, @sshleifer 


> Don't really agree with this PR - I think `bos_token_id` should have higher priority than `model.config.decoder.bos_token_id`. The `model.config.decoder.bos_token_id` was mainly added because of the `EncoderDecoderModel` framework

The way I read the intention is that if it goes:
```
        if self.config.is_encoder_decoder:
```
we are in encoder-decoder zone and as such `.decoder` should take priority

I guess Bart and friends are semi-encoder-decoder as far as the framework goes.

Thank you for the feedback, @patrickvonplaten 

I suppose in the ideal world we should have tests that validate such scenarios.