Add TFEncoderDecoderModel + Add cross-attention to some TF models
Author: ydshieh
Pull request: yes

# What does this PR do?

- Add TFEncoderDecoderModel + Add cross-attention to some TF models
- Add cross attention & cache mechanism (`use_cache` & `past_key_values`) to some TF models
- Add `test_modeling_tf_encoder_decoder.py`

## Before submitting
- [x] Did you read the [contributor guideline](https://github.com/huggingface/transformers/blob/master/CONTRIBUTING.md#start-contributing-pull-requests), Pull Request section?
- [x] Did you make sure to update the documentation with your changes?
- [x] Did you write any new necessary tests?

## Who can review?

@patrickvonplaten @LysandreJik 

Closes https://github.com/huggingface/transformers/issues/9863

Comments:
Hey @ydshieh,

It's awesome that you already give `TFEncoderDecoder` a stab! Note that they were a lot of difficulties when adding TFRag with saving/loading and parameter scopes - see: https://github.com/huggingface/transformers/pull/9002 so it's maybe a good idea to not include to many models in the first PR and try to keep it as simple as possible :-) 

The most important part here is to make sure that saving & loading works correctly depending on how the TFEncoderDecoder was constructed. *E.g.* we should have all those tests we have for TFRag also for TFEncoderDecoder: https://github.com/huggingface/transformers/blob/cf5744764821c3254773a62e4cc160dd6f09df8e/tests/test_modeling_tf_rag.py#L945 . It's very much not easy to make sure saving and loading works correctly for all models in TF so it would be important to focus on that part first I think before adding cross-attention to many other models :-) 

Happy to help you here whenever you're stuck, but we should be careful to keep it simple in the beginning :-)

@patrickvonplaten Yes, I did have some troubles with saving/loading and parameter scopes. That took me quite some time, but currently I am able to solve the issues I had, but I will check the PR you mentioned, and will also try to add the equivalent tests contained in TFRag.

Hi @patrickvonplaten , a silly question, but it would be great if you can explain to me what `Model templates runner / run_tests_templates (pull_request)` does, and why it failed here (if possible). I am out of idea about the reason

> Hi @patrickvonplaten , a silly question, but it would be great if you can explain to me what `Model templates runner / run_tests_templates (pull_request)` does, and why it failed here (if possible). I am out of idea about the reason

It's a test that makes sure that the cookie cutter keeps working correctly: https://github.com/huggingface/transformers/tree/master/templates/adding_a_new_model

It your case it fails because you've adapted some fundamental TF models which are used in the cookie cutter as a template. In order to make the test pass you should adapt the cookie cutter template analogous so that the changes done to TFBart are also added here: https://github.com/huggingface/transformers/blob/master/templates/adding_a_new_model/cookiecutter-template-%7B%7Bcookiecutter.modelname%7D%7D/modeling_tf_%7B%7Bcookiecutter.lowercase_modelname%7D%7D.py and here: https://github.com/huggingface/transformers/blob/master/templates/adding_a_new_model/cookiecutter-template-%7B%7Bcookiecutter.modelname%7D%7D/test_modeling_tf_%7B%7Bcookiecutter.lowercase_modelname%7D%7D.py

But for the beginning I wouldn't pay too much attention to this test (it's not super important). Once your PR is ready, it's a good idea to fix the cookiecutter test in a final commit. If it doesn't work, I can help you with it :-)

@patrickvonplaten I have added 

https://github.com/huggingface/transformers/blob/f73cab3be8f0dc2cd816ce8f5c9a50e113f8eacb/tests/test_modeling_tf_encoder_decoder.py#L742

similiar to `TFRagModelSaveLoadTests` for `TFRag`.
(There is no pretrained TFEncoderDecoder model on model hub yet, so I made some adjustment for the test)

The PR is ready for review :-)


@patrickvonplaten , I am trying to add `TFBartForCausalLM` similar to `BartForCausalLM`. Howerver, there is one last issue:

In TF / PyTroch CausalLM models, there are shift inside their call method, like:
In TensorFlow
```
        if inputs["labels"] is not None:
            # shift labels to the left and cut last logit token
            logits = logits[:, :-1]
            labels = inputs["labels"][:, 1:]
            loss = self.compute_loss(labels=labels, logits=logits)
```
or in PyTorch
```
        if labels is not None:
            # we are doing next-token prediction; shift prediction scores and input ids by one
            shifted_prediction_scores = prediction_scores[:, :-1, :].contiguous()
            labels = labels[:, 1:].contiguous()
            loss_fct = CrossEntropyLoss()
            lm_loss = loss_fct(shifted_prediction_scores.view(-1, self.config.vocab_size), labels.view(-1))
```
You can find some of them in

https://github.com/huggingface/transformers/blob/0ebda5382b6456cba2d92a3670383f9adf61533a/src/transformers/models/gpt2/modeling_tf_gpt2.py#L745
https://github.com/huggingface/transformers/blob/0ebda5382b6456cba2d92a3670383f9adf61533a/src/transformers/models/gpt2/modeling_gpt2.py#L973
https://github.com/huggingface/transformers/blob/0ebda5382b6456cba2d92a3670383f9adf61533a/src/transformers/models/bert/modeling_bert.py#L1233
https://github.com/huggingface/transformers/blob/0ebda5382b6456cba2d92a3670383f9adf61533a/src/transformers/models/bert/modeling_tf_bert.py#L1244

Howerver, for `BartForCausalLM` (and the new added TF version), this shift is not done inside the call

https://github.com/huggingface/transformers/blob/0ebda5382b6456cba2d92a3670383f9adf61533a/src/transformers/models/bart/modeling_bart.py#L1780

I think for Bart, it expected the `(decoder's) input_ids` and `labels` being preprocessed outside the `call`.

However, this difference will cause a problem in TF test, because for TF causal LM models (Bert/GPT2/...), it returns the truncated `logits`
https://github.com/huggingface/transformers/blob/0ebda5382b6456cba2d92a3670383f9adf61533a/src/transformers/models/bert/modeling_tf_bert.py#L1246

BTW, In PyTorch causal LM models, they return the complete logits
https://github.com/huggingface/transformers/blob/662b143b71eb5ef775e27a8f79798bb28b3283bd/src/transformers/models/bert/modeling_bert.py#L1235

The test for `TFEncoderDecoderModel.check_encoder_decoder_model_labels` therefore expects the logits has `seq_len - 1`.
https://github.com/huggingface/transformers/blob/f73cab3be8f0dc2cd816ce8f5c9a50e113f8eacb/tests/test_modeling_tf_encoder_decoder.py#L279

This works all fine until I introduce `TFBartForCausalLM`, as currently it will retrun logits of `seq_len`.

Do you have some opinions on how should I deal with this situation?

Awesome work so far @ydshieh! Mostly left nits, but the following things should be checked before merging:

1. - `EncoderDecoderModel` and `TFEncoderDecoder` model should be exactly the same. We should write a test for this similar to https://github.com/huggingface/transformers/blob/ba1b3db70907b975b5ca52b9957c5ed7a186a0fa/tests/test_modeling_tf_common.py#L431 . In this test IMO we can use two small BERT models. We also should have added a test for Flax, but I've forgotten to mention it. For Flax we can do this in another PR, for TF we should do it in this PR ideally :-) 

2. - `TFEncoderDecoder` has to load and save weights correctly in multiple scenarios. Essentially we need all those tests that we have for TFRag passing for TFEncoderDecoder as well: https://github.com/huggingface/transformers/blob/ba1b3db70907b975b5ca52b9957c5ed7a186a0fa/tests/test_modeling_tf_rag.py#L945 . Here I can help you, so maybe you can add some tests and let them fail for the moment and then I can go in and fix them :-)

3. - We also have to adapt the TF templates here: https://github.com/huggingface/transformers/blob/master/templates/adding_a_new_model/cookiecutter-template-%7B%7Bcookiecutter.modelname%7D%7D/modeling_tf_%7B%7Bcookiecutter.lowercase_modelname%7D%7D.py since we are doing core changes to TFBert. Feel free to give it a try - otherwise I'm happy to take over this part as well if it becomes time-consuming #13288 

4. - Finally we need to run all BERT & RoBERTa slow tests to make sure nothing is broken. I can do this before merging

=> If ok maybe you can look at the above suggestions and write some tests for 1), 2) and then I can help you make the tests for 2 pass? :-) 

Really great work so far - this is one of the most complex architectures in the repo!

@patrickvonplaten Thanks for the feedbacks. I will make the changes.  I will try to write `test_pt_tf_model_equivalence(self): `. About `class TFRagModelSaveLoadTests(unittest.TestCase): `, the last time I checked, it always passed, but I will verify again (since I reverted some changes done in the core tf weights loading/saving). (All the slow tests have passed when I run them locally, but again, I will verify)

Also @ydshieh rebasing onto master would likely help resolve some of the currently failing tests, they don't seem related to this PR at all.

> 
> 
> Also @ydshieh rebasing onto master would likely help resolve some of the currently failing tests, they don't seem related to this PR at all.

Yes. There is `run_tests_tf` failed which is related to this PR. Once this is resolved, and having a note on the big hack for PT <-> TF, I think this PR will be ready :) - Let's see what Patrick say.

I agree with both of you! Once we fix the https://app.circleci.com/pipelines/github/huggingface/transformers/28285/workflows/d3d182a4-44b3-4bc8-b61e-dafcb341c2eb/jobs/278321?invite=true#step-108-4434 test (which is caused by this PR), we should add a note to `TFEncoderDecoder.from_pretrained(...)` and then we can merge this PR :tada: - very good work @ydshieh :-)

> 
> 
> I agree with both of you! Once we fix the https://app.circleci.com/pipelines/github/huggingface/transformers/28285/workflows/d3d182a4-44b3-4bc8-b61e-dafcb341c2eb/jobs/278321?invite=true#step-108-4434 test (which is caused by this PR), we should add a note to `TFEncoderDecoder.from_pretrained(...)` and then we can merge this PR 🎉 - very good work @ydshieh :-)

I already know a way to fix the issue, but want to know which way might be better in your opinion. (The question I posted on Slack). Let me know what you think once you check it :) - In short, the question is about if setting `GPT2Config.is_decoder=True` makes sense.

@ydshieh - the PR looks to be in a very good state to me! In a final step, could you maybe adapt the test:
`tests/test_modeling_tf_encoder_decoder.py::TFEncoderDecoderModelSaveLoadTests::test_encoder_decoder_save_load_from_encoder_decoder_from_pt` to showcase how the load a checkpoint from pytorch using the encoder and decoder seperately? 

After that I think we are good to merge :-)

> 
> 
> @ydshieh - the PR looks to be in a very good state to me! In a final step, could you maybe adapt the test: `tests/test_modeling_tf_encoder_decoder.py::TFEncoderDecoderModelSaveLoadTests::test_encoder_decoder_save_load_from_encoder_decoder_from_pt` to showcase how the load a checkpoint from pytorch using the encoder and decoder seperately?
> 
> After that I think we are good to merge :-)

Hey, @patrickvonplaten Sure. Let me make sure: you are saying to change the hack in `test_encoder_decoder_save_load_from_encoder_decoder_from_pt` to use encoder and decoder separately (and load their pytorch weights ), right? 

I made the change to 

```
test_encoder_decoder_save_load_from_encoder_decoder_from_pt
```

Here is the change I made

https://github.com/huggingface/transformers/blob/0cd88b8538b8a27b4f3df2e8974a41d7e027dd70/tests/test_modeling_tf_encoder_decoder.py#L684
```
        # PyTorch => TensorFlow
        with tempfile.TemporaryDirectory() as tmp_dirname_1, tempfile.TemporaryDirectory() as tmp_dirname_2:
            encoder_decoder_pt.encoder.save_pretrained(tmp_dirname_1)
            encoder_decoder_pt.decoder.save_pretrained(tmp_dirname_2)
            encoder_decoder_tf = TFEncoderDecoderModel.from_encoder_decoder_pretrained(
                tmp_dirname_1, tmp_dirname_2, encoder_from_pt=True, decoder_from_pt=True
            )
```

We also have a note in the doc of `TFEncoderDecoderModel.from_pretrained` (which also explains how to deal with a pytorch checkpoint)

https://github.com/huggingface/transformers/blob/0cd88b8538b8a27b4f3df2e8974a41d7e027dd70/src/transformers/models/encoder_decoder/modeling_tf_encoder_decoder.py#L243


@patrickvonplaten , @Rocketknight1  Thank you for your review! I am glad we have a TensorFlow Encoder Decoder now :)

Hi @patrickvonplaten & @Rocketknight1, I made the change to 

```
test_encoder_decoder_save_load_from_encoder_decoder_from_pt
```

Here is the change I made

https://github.com/huggingface/transformers/blob/0cd88b8538b8a27b4f3df2e8974a41d7e027dd70/tests/test_modeling_tf_encoder_decoder.py#L684
```
        # PyTorch => TensorFlow
        with tempfile.TemporaryDirectory() as tmp_dirname_1, tempfile.TemporaryDirectory() as tmp_dirname_2:
            encoder_decoder_pt.encoder.save_pretrained(tmp_dirname_1)
            encoder_decoder_pt.decoder.save_pretrained(tmp_dirname_2)
            encoder_decoder_tf = TFEncoderDecoderModel.from_encoder_decoder_pretrained(
                tmp_dirname_1, tmp_dirname_2, encoder_from_pt=True, decoder_from_pt=True
            )
```

We also have a note in the doc of `TFEncoderDecoderModel.from_pretrained` (which also explains how to deal with a pytorch checkpoint)

https://github.com/huggingface/transformers/blob/0cd88b8538b8a27b4f3df2e8974a41d7e027dd70/src/transformers/models/encoder_decoder/modeling_tf_encoder_decoder.py#L243

Do you have any further comments?

Thank you for your review! Looking forward for the merge and having a TensorFlow Encoder Decoder in HF :)

@ydshieh At this point I'm pretty happy with it! @patrickvonplaten do you have any objections, or should we merge?

@sgugger, 

About `assert self.is_decoder, f"{self} should be used as a decoder model if cross attention is added"`, I copied it from Pytorch models. Do you think it is a good idea for me to change all of them (PT/TF files) to `if not self.is_decoder: raise ValueError(xxx)` in this PR, or just the TF files involved currently (and a new PR for all other occurrences)?

> About assert self.is_decoder, f"{self} should be used as a decoder model if cross attention is added", I copied it from Pytorch models. 

Yes we have some old ones in the codebase, we are just not accepting new ones, so please adapt your PR. We can adapt the PyTorch files in a separate PR.

@patrickvonplaten , I tried to run slow tests for the changed models, and found some issues. (Previously, I only run the tests for `TFEncoderDecoderModel`). I will let you know when I finish fixing them :)

> Are there any caveats to be known for this implementation vs the PyTorch implementation which should be put in the docs, or should they behave identically?
> 

There is also one thing I pointed much earlier: For a given `TFEncoderDecoderModel`, if we do

```
model.encoder.save_pretrained(encoder_path)
model.decoder.save_pretrained(decoder_path)
```

Then

```
new_model = TFEncoderDecoderModel.from_encoder_decoder_pretrained(
    encoder_path, decoder_path
)
```
won't load the TF checkpoint weights correctly.

This is somehow strange (logically), but the chance of doing so is very low -> If we already have a `TFEncoderDecoderModel`, it's more likely `save_pretrained` will be used rather than saving the 2 components separately.

I can add this to the doc if necessary. (I will verify again to make sure)

@LysandreJik I added the PT->TF information to `encoderdecoder.rst`, along with the model contributors (I hope this is fine).

https://github.com/huggingface/transformers/blob/f021eec0c7e97334a6c2fc3b9b1a1b43ec06fce3/docs/source/model_doc/encoderdecoder.rst#L30

All the suggestions have been addressed.

@sgugger , I left the following unchanged in this PR (we can clean things up in another PR)

```
# T5 has a mask that can compare sequence ids,
```

@patrickvonplaten I ran the slow tests locally with all the models changed in this PR, except `TFRemBert` (my poor laptop just can't ran it). It's ready for you to do a final verification, thank you! (The `get_tf_activation("gelu")` issue is fixed )

This looks good to me, thank you @ydshieh!

Awesome - looked through the PR again and it looks good to me! Thanks a lot for all your amazing work on this :-)

@patrickvonplaten , Thank you!

Do you want to upload a converted TF checkpoint to `"patrickvonplaten/bert2bert-cnn_dailymail-fp16"` (so we can change the examples, and adding 1 or 2 more tests).

Otherwise, would it be a good idea for me to upload to `"ydshieh/bert2bert-cnn_dailymail-fp16"`? I assume that the checkpoints used officially for the tests/examples should be under the name of Hugging Face or its staffs.

Kindly tag @LysandreJik for this.