[Design proposal] Fix EncoderDecoderModel classes to be more like BART and T5
Author: NielsRogge
Pull request: yes

# What does this PR do?

TLDR: recently, we've added some new classes called `SpeechEncoderDecoderModel` (#13186) and `VisionEncoderDecoderModel` (#13874). These are very similar to the existing `EncoderDecoderModel` class, allowing to combine any speech/vision encoder with any text decoder. However, there's currently an issue in the sense that we have 2 flavors of decoder models in the library, namely:

* "shifting afterwards": those that shift the logits inside them, when calculating the loss. Examples are `BertLMHeadModel`, `BertGeneration`, `BigbirdForCausalLM`, `GPT2LMHeadModel`, `GPTNeoForCausalLM`, `RobertaForCausalLM`. For these models, the `input_ids` you provide should be equal to the `labels`, and the shifting occurs as follows:
```
loss = None
if labels is not None:
   # Shift so that tokens < n predict n
   shift_logits = lm_logits[..., :-1, :].contiguous()
   shift_labels = labels[..., 1:].contiguous()
   # Flatten the tokens
   loss_fct = CrossEntropyLoss()
   loss = loss_fct(shift_logits.view(-1, shift_logits.size(-1)), shift_labels.view(-1))
```
* "shifting before": those that don't shift the logits, but expect this shifting to already have happened before providing the `input_ids` to the decoder. Examples are `BartForCausalLM`, `PegasusForCausalLM`, and the recently added `Speech2Text2ForCausalLM` and `TrOCRForCausalLM`. Here, the loss calculation is simply:
```
loss = None
if labels is not None:
   loss_fct = CrossEntropyLoss()
   loss = loss_fct(logits.view(-1, self.config.vocab_size), labels.view(-1))
```

## Proposal
In any case, we would like the `EncoderDecoderModel` classes to automatically create the `decoder_input_ids` based on the `labels` provided by the user, similar to how this is done for "vanilla" seq2seq models as BART and T5. However, in order to support both flavors of decoders in the EncoderDecoderModel classes, there are 2 options, either:

* option 1: work with an if-else statement, that creates the `decoder_input_ids` based on the "type" of decoder (type 1 would require to simply set `decoder_input_ids = labels`, whereas type 2 would require to shift the `decoder_input_ids` to be a shifted version of the `labels`). This way, the loss can be computed inside each decoder individually. The disadvantage is that one would need to keep some sort of an exhaustive list of classes that enumerates the model classes of the 2 flavors of decoders.
* option 2: don't compute the loss inside the decoders themselves, but rather in the `EncoderDecoderModel` classes itself. In that way, one can implement the `EncoderDecoderModel` classes similar to the "vanilla" models T5 and BART, which means: (1) first create the `decoder_input_ids` based on the labels provided by the user (i.e. shifting), (2) next forward these through the decoder, and then (3) use the `logits` of the decoder to compute the loss. 

Both would allow to use the `DataCollatorForSeq2Seq`, and in turn the `Seq2SeqTrainer` for the EncoderDecoderModel classes. This enables us to make demo notebooks to fine-tune these model classes using the `Seq2SeqTrainer`.

This draft PR implements option 2. 

Comments:
As discussed offline with @NielsRogge, I'm also in favor of option 2) here because:
- It allows for a nicer, unified API (we can wrap away the complexity of `"shifting before"` and `"shifting afterwards"`) for the user
- Also it allows the loss computation for `Bert2Bert` to be exactly the same as for `BartForConditionalGeneration` . Before this PR it was not exactly the same since Bert2Bert "cut" the first input_ids and the last input_ids respetively for `decoder_input_ids` and `labels`. 
- It also arguably more readable as the user doesn't have to look into `modeling_bert.py` of `modeling_bart.py` to see how the loss is computed exactly

The cons of this approach are:
- It's slighly backwards breaking in a sense that with the new design the labels, input_ids are not cut anymore but shifted (think that's fine though since it's training that is changed here and not inference). Also, it's a good opportunity to update this blog post: https://huggingface.co/blog/warm-starting-encoder-decoder#warm-starting-the-encoder-decoder-model a bit to show the new feature
- We assume here that all `EncoderDecoderModel` combinations always use the "cross entropy / next token language model loss". So if a new model is published that uses a new special loss we probably would have to add some if else statements to the `EncoderDecoderModel`. In a sense we do lose flexibility since we can't define individual loss computations in the specific models anymore...However IMO a unified framework is more important here and so far all models that we know do use the "cross entropy / next token language model loss"

=> So overall I'm in favor of option 2) here.

Once we have merged this PR, @NielsRogge and I could work on making some nice notebooks and/or blog posts on how to fine-tune:
- `EncoderDecoderModel`
- `VisionEncoderDecoderModel`
- `SpeechEncoderDecoderModel` 

and give an overview of all the possible tasks that can be covered with it (Image Captioning, Speech Translation, ...).

Also cc @ydshieh - If you're interested we could work on a nice blog post (to be added here: https://huggingface.co/blog) on how to leverage `VisionEncoderDecoder` for image captioning if you're interested :-)


> Also cc @ydshieh - If you're interested we could work on a nice blog post (to be added here: https://huggingface.co/blog) on how to leverage `VisionEncoderDecoder` for image captioning if you're interested :-)

Yes, I am interested :) Thank you for the opportunity! 

A minor question, do we still need to pass `labels` to the decoder here (`labels=labels, `)?

https://github.com/huggingface/transformers/blob/6880957810143e08c2e773639cc5a74841033115/src/transformers/models/encoder_decoder/modeling_encoder_decoder.py#L461-L475

(I also like option 2, but would like to check if it works fine for TF models too :). I didn't check the recent change @Rocketknight1 made about loss computation for TF models.)

Great point, there's no need to pass `labels` to the decoder anymore if we go for option 2, as the loss is computed outside the decoder.

And yeah we need to make sure this also works for the other frameworks. Will ask @Rocketknight1 for his opinion.

@ydshieh The change to TF loss computation is mostly cosmetic/UX. It just changes `compile()` so that if the user doesn't specify a loss, we use a `dummy_loss` loss on the model's `loss` output, and the change in `train_step` just patches things so the loss values in the progress bar look okay. So hopefully that shouldn't cause any problems for any of this!