Encoder-decoder model is not working correctly for the latest versions
Author: miguelwon
Pull request: no
Labels: name: WIP

name: bug

### System Info

transformers==4.2.1

### Who can help?

@patrickvonplaten

### Information

- [ ] The official example scripts
- [X] My own modified scripts

### Tasks

- [] An officially supported task in the `examples` folder (such as GLUE/SQuAD, ...)
- [X ] My own task or dataset (give details below)

### Reproduction

Hi,

I'm working with a seq2seq problem,  in particular with `EncoderDecoderModel` model. The problem is that I can't have good results with the latest version (**4.21.3**). I also tried with **4.18.0** because of [this](https://github.com/huggingface/blog/issues/292#issuecomment-1122666099) but didn't work either.  It is however working when using version **4.2.1**

I have made an [public notebook](https://colab.research.google.com/drive/1obQcmRdX89eWJfk_qUMcxa4pBSPIc53X?usp=sharing) you can run to see the issue. Is an example to train a model to generate the written digits, given a number. 




### Expected behavior

You works nicely with version **4.2.1**, but very bad with the most recent versions. 


Comments:
Thank you for reporting, @miguelwon . I will take a look.

This might also be interesting for @ArthurZucker if you're very busy at the moment @ydshieh 

I am having a look RN, will tell you when I know more 👍🏻 

Hey @miguelwon it seems that you are right about the training not converging at all using current version. 
However, since loading a trained model in the new versions does not give bad results, I suspect that the issue comes from either the computation of the loss, or the trainer. 

I will have a look in more details as I believe this is a pretty important bug 😄 
 

Hi @ArthurZucker, just to know if there any news about this issue. Thanks!

Hey! Sorry not yet, it's pretty tricky, but I hope I'll resolve it soon! 🤗 

Hello @miguelwon I discussed with @ArthurZucker internally and decided to take a look on this issue.

In the notebook you provided, inside the function `process_data_to_model_inputs`, you prepared:
  - decoder_input_ids
  - decoder_attention_mask
  - labels

and you left a remark
> because BERT automatically shifts the labels, the labels correspond exactly to `decoder_input_ids`.

In fact, for the encoder-decoder architecture, the loss computation is done in `EncoderDecoderModel.forward` rather than in `decoder.forward` (in your case, the decoder is `BertLMHeadModel`), see [here](https://github.com/huggingface/transformers/blob/ecd7de3dff7ea5713004b2f05e3869c24b8eb6e2/src/transformers/models/encoder_decoder/modeling_encoder_decoder.py#L631). Also, see [this warning message](https://github.com/huggingface/transformers/blob/ecd7de3dff7ea5713004b2f05e3869c24b8eb6e2/src/transformers/models/encoder_decoder/modeling_encoder_decoder.py#L41).

Combine [the way `decoder_input_ids` is prepared here](https://github.com/huggingface/transformers/blob/ecd7de3dff7ea5713004b2f05e3869c24b8eb6e2/src/transformers/models/encoder_decoder/modeling_encoder_decoder.py#L611), we don't need to prepare `decoder_input_ids` and `decoder_attention_mask` in your method `process_data_to_model_inputs`.

However, you can still provide them, but in this case, the `decoder_input_ids` should be a shift of `labels`, instead of being the same value (which is the case in your notebook).

For old versions like `4.2.1`, it was using the decoder's code of loss computation, so the notebook works with it. But since v4.12, this is not the recommended way to run encoder-decoder models.

I have updated your notebook (in a copy), you can check [here](https://colab.research.google.com/drive/1WbPtf7OKar7DbTJ-76n67UrLkUnRc_If?usp=sharing), which shows it doesn't generate non-sense results anymore with the above suggested changes.

I hope this answers your question :-)



Yes it does! :) 
Thanks a lot for you clarification and the updated notebook! 



Thank you @ydshieh .