noamaanMulla-03/transformers-issues huggingface dataset:
Add TFVisionEncoderDecoderModel
Author: ydshieh
Pull request: yes

# What does this PR do?

To make Vision-Encoder-Text-Decoder family complete by adding `TFVisionEncoderDecoderModel`.

To complete this PR, it requires to wait #13778 being merged to master (then rebase)
(And if we want to include a real integration test using the recent image-captioning ViT + GPT2 model, need to wait #14038 too)


Comments:
@sgugger, thank you for your review, I have addressed them.

I am impressed by your ability to spot the `fits in one line` places.

I feel that sometimes `make style` can reformat in this case, but doesn't work well in a few places, as happened in this PR.

I dive a bit deeper, and found:

this one won't be reformatted by `make style`
```
    def prepare_inputs_for_generation(
        self,
        decoder_input_ids,
        past,
        use_cache=None,
        **kwargs,
    ):
```

but the following one will work well.

```
    def prepare_inputs_for_generation(
        self,
        decoder_input_ids,
        past,
        use_cache=None,
        **kwargs
    ):
```

The difference is the ending comma after the last argument. Is this a bug? I can open an issue if so.
Maybe I should just remove all the commas after the last arguments in my PR.

> This is a great addition @ydshieh! Thanks a lot for the contribution.
> 
> From my side it would be great if we could:
> 
>     * remove all pytorch specific changes that are not needed to get the TF version working (I'll tackle this in a future PR :-) )
> 
>     * add one slow test that ensures that the model works correctly
> 
> 
> Thanks a bunch!

Hi @patrickvonplaten 

- OK for removing the changes on PT code.
- For the test, I think I forgot to do the same as I have done for PT/Flax

    https://github.com/huggingface/transformers/blob/8395f14de6068012787d83989c3627c3df6a252b/tests/test_modeling_vision_encoder_decoder.py#L666

    https://github.com/huggingface/transformers/blob/8395f14de6068012787d83989c3627c3df6a252b/tests/test_modeling_flax_vision_encoder_decoder.py#L467

    I will add it to TF test :)

I removed the changes on PT code. I added 

```
class TFViT2GPT2ModelIntegrationTest(unittest.TestCase):
    @slow
    def test_inference_coco_en(self):
```
here
https://github.com/huggingface/transformers/blob/a179c4a5087e48127914e26e9895a455b42bac0d/tests/test_modeling_tf_vision_encoder_decoder.py#L775

Hi, @patrickvonplaten @Rocketknight1 @NielsRogge 

I removed the changes on PT code. I also added 

```
class TFViT2GPT2ModelIntegrationTest(unittest.TestCase):
    @slow
    def test_inference_coco_en(self):
```
here
https://github.com/huggingface/transformers/blob/b3255863540fac38d49e5067c14fef7ae66dc31f/tests/test_modeling_tf_vision_encoder_decoder.py#L769

This PR is ready for review when you have the time :-)

> * auto.rst no?

Yes. I added the (empty) file to commit by mistake during git rebase/merge.

@NielsRogge @Rocketknight1 - could you guys take a look here as well?

Applied @sgugger  review suggestions. Failed tests are unrelated.

Thanks again for all your work on this!
Add from_encoder_decoder_pretrained to some dummy obj
Author: ydshieh
Pull request: yes

# What does this PR do?

Fix 

https://discuss.huggingface.co/t/flaxvisionencoderdecodermodel-decoder-start-token-id/13635

## Who can review?

@NielsRogge 

Comments:
`check_dummies.py` need to be adjusted to make it work

https://github.com/huggingface/transformers/blob/9a94bb8e218033cffa1ef380010b528410ba3ca7/utils/check_dummies.py#L36-L52

Info: should not edit these dummy files manually

```
# This file is autogenerated by the command `make fix-copies`, do not edit.
```
[Fix doc example] missing import
Author: ydshieh
Pull request: yes

# What does this PR do?

Add `from transformers import EncoderDecoderModel, TFEncoderDecoderModel` in a doc example.

Comments:
Thank you for your PR! The docs for this PR live [here](https://moon-ci-docs.huggingface.co/docs/transformers/pr_15240). All of your documentation changes will be reflected on that endpoint.

Great job merging this PR! the documentation will now be removed from the staging environment.
Remove "inputs" in tf common test script (no longer required)
Author: ydshieh
Pull request: yes

# What does this PR do?

The blocks appeared in `test_modeling_tf_common.py`

```
            # need to rename encoder-decoder "inputs" for PyTorch
            if "inputs" in pt_inputs_dict and self.is_encoder_decoder:
                pt_inputs_dict["input_ids"] = pt_inputs_dict.pop("inputs")
```
was introduced in #3547.

The PR #8602 changed `inputs` back to `input_ids` (particularly in `TFT5` model). The above block is longer necessary.

This PR removes it to avoid confusion (in particular, not to be confused with the new introduced `inputs` in PyTorch `generate`, which is expected to be applied to TF's `generate` too in the near future).

Comments:
_The documentation is not available anymore as the PR was closed or merged._

> Thank you @ydshieh !
> 
> To double-check, before merging -- can you confirm that this particular test is successful for a few models? (e.g. `RUN_PT_TF_CROSS_TESTS=1 py.test tests/test_modeling_tf_bert.py)`

Hi, @gante I run it with BERT, BART and GPT2 --> all pass.

(I believe that these tests are already run by CI, see https://app.circleci.com/pipelines/github/huggingface/transformers/33021/workflows/63e4488b-7d35-46ee-9209-5e036adfe6ba/jobs/347995/parallel-runs/0/steps/0-112)
Fix the inconsistency of loss calculation between PT/TF XLNetLMHeadModel
Author: ydshieh
Pull request: yes

# What does this PR do?

The loss calculation in `XLNetLMHeadModel` doesn't cut the logits/labels (unlike other models say `BertLMHeadModel`).
However, `TFXLNetLMHeadModel` works as other TF Causal LM models, and cut the logits.
This causes the loss difference higher than `4e-2` sometimes.

I believe `XLNet` works somehow differently from the usual causal LM models, and the provided labels and computed logits shouldn't be cut, as  `XLNetLMHeadModel` does.

This PR fixes this inconsistency.

Comments:
_The documentation is not available anymore as the PR was closed or merged._

Could @Rocketknight1 or @gante give this a quick look?

This looks good! Should it be folded into PR #15256 though, or do you think it's better separate?

> This looks good! Should it be folded into PR #15256 though, or do you think it's better separate?

Hi, @Rocketknight1 

 #15256  we still cut the logits to compute the loss, but we will return the complete logits
But for this PR, we want to **use the complete logits to compute** the loss (and then return the complete one).
(that's my understanding)

They are different, and would be better to be separate PRs. There will be a conflict once one version is merged, but I can take care of resolving it.



Nice finding @ydshieh!

I agree with @ydshieh, these are two separate issues -- the other one is general to causal models, this one pertains to the TF/PT mismatch in XLNet. 

> 🚀 (please wait for @Rocketknight1's approval before merging)

Sure, I can't merge anyway 😅

Sorry for the delay! I looked at this earlier in the week and thought it looked good, and didn't realize you were waiting on me now. I'm happy to merge at this point - @ydshieh is there anything else you want to add, or should I go ahead?

> Sorry for the delay! I looked at this earlier in the week and thought it looked good, and didn't realize you were waiting on me now. I'm happy to merge at this point - @ydshieh is there anything else you want to add, or should I go ahead?

You can go ahead :-) Thank you!

Merging then 👍 
Thank you for the PR, @ydshieh!
[Do not merge] Chase more pt tf inconsistency
Author: ydshieh
Pull request: yes

# What does this PR do?



Comments:
The docs for this PR live [here](https://moon-ci-docs.huggingface.co/docs/transformers/pr_15321). All of your documentation changes will be reflected on that endpoint.

In terms of running time

- Circle CI
  - current : [56.11s](https://circleci.com/api/v1.1/project/github/huggingface/transformers/384755/output/112/0?file=true&allocation-id=62238c1d4f148c1110a1efa8-0-build%2F1610CB69)
  - this PR : [61.21s](https://circleci.com/api/v1.1/project/github/huggingface/transformers/384731/output/112/0?file=true&allocation-id=622385347ccc3f3894ec7697-0-build%2F351FF724)
Fix doc examples: name '...' is not defined
Author: ydshieh
Pull request: yes

# What does this PR do?

For some doc examples in PT model files, we get exception: `NameError: name '...' is not defined`

This PR fixes this issue.

In `modeling_visual_bert.py`, there are some places using an assumed-existing function `get_visual_embeddings`.

```
# Assumption: `get_visual_embeddings(image)` gets the visual embeddings of the image in the batch.
visual_embeds = get_visual_embeddings(image).unsqueeze(0)
```
These places won't pass the doc example test - so need to a way to not run against them.
(Or an example implementation for this function should be provided)

## Who can review?

@sgugger 

Comments:
> Thanks for fixing. For the one in visual BERT, it shouldn't be run by doctest, so the `>>>` should be removed.

I removed `>>>` and `...` as suggested.

Thnaks!
Fix doc examples: unexpected keyword argument
Author: ydshieh
Pull request: yes

# What does this PR do?

Fix `unexpected keyword argument` in doc examples (in PT model files)


## Who can review?

@sgugger 

Comments:
Not sure why `ProphetNetTokenizer` returns `token_type_ids` by default while `ProphetNetEncoder` can't accept it.

```
            >>> tokenizer = ProphetNetTokenizer.from_pretrained('microsoft/prophetnet-large-uncased')
            >>> model = ProphetNetEncoder.from_pretrained('patrickvonplaten/prophetnet-large-uncased-standalone')
            >>> inputs = tokenizer("Hello, my dog is cute", return_tensors="pt")
            >>> del inputs["token_type_ids"]  # `ProphetNetEncoder` doesn't use `token_type_ids`
```

That should be fixed in the tokenizer, not the doc example.

> That should be fixed in the tokenizer, not the doc example.

OK, I will revert it.

> > That should be fixed in the tokenizer, not the doc example.
> 
> OK, I will revert it.

@sgugger 
May I change it to 

```
inputs = tokenizer("Hello, my dog is cute", return_tensors="pt", return_token_type_ids=False)
```

for now in this PR, and open an issue for `ProphetNetTokenizer`.

I'm afraid we will forget to re-change the documentation after the fix is done, so it should just stay as is until the tokenizer is fixed.

Thanks a lot @ydshieh! 

@sgugger - feel free to merge if it's good to go from your side.
Fix wrong checkpoint paths in doc examples
Author: ydshieh
Pull request: yes

# What does this PR do?

In some (PT) model files, there are doc examples with the wrong checkpoint paths. For example, 

```
model = BigBirdPegasusForConditionalGeneration.from_pretrained('bigbird-pegasus-large-arxiv')
```
which should be `'google/bigbird-pegasus-large-arxiv'`

This PR fixes this issue.


There is
```
MegatronBertLMHeadModel.from_pretrained('nvidia/megatron-bert-cased-345m', is_decoder=True)
```
but there is no model file at all when I searched `megatron-bert`. I am not sure what to do in this case.

## Who can review?

@sgugger @LysandreJik 

Comments:
> Thanks for fixing those. For Megatron BERT, a manual download is required, see the [doc page](https://huggingface.co/docs/transformers/model_doc/megatron_bert) for more information.

OK, so I should update the doc example by using a manual download, I guess.

No need for that (as it requires some bash commands), we just shouldn't test this one.
Fix doc examples: 'CausalLMOutput...' object has no attribute 'last_hidden_state'
Author: ydshieh
Pull request: yes

# What does this PR do?

In some doc examples, the line

```
last_hidden_states = outputs.last_hidden_state
```

will fail when the return value if of type `CausalLMOutput...` (e.g. `CausalLMOutputWithCrossAttentions`).

This PR fixes these lines by using

```
logits = outputs.logits
```

## Who can review

@patrickvonplaten 

Comments:
Awesome!
Use cross_attention_hidden_size in Encoder-Decoder models
Author: ydshieh
Pull request: yes

# What does this PR do?

- Add a projection layer (`enc_to_dec_proj`) between encoder and decoder models in composite models, incorporating the attribute `cross_attention_hidden_size`.

    - add some `pt/tf equivalence` and `pt/flax equivalence` tests in tf/flax composite model test scripts.
    - also make some logging and ValueError messages consistent across composite model scripts.

Comments:
I ran slow tests for all the encoder-decoder models test scripts, and it is fine. (e.g. `RUN_SLOW=1 python -m pytest ...`)

BTW, is there an easy way to run all cross tests in a test script, i.e. disabling `@is_pt_tf_cross_test` or `@is_pt_flax_cross_test`?

Hey @ydshieh, 

We need to slightly update this PR for the speech encoder decoder classes sadly so that the newly introduced variable `config.output_hidden_size` as shown here: https://github.com/huggingface/transformers/blob/cea17acd8cc5190684da944924116fe10742ad81/src/transformers/models/speech_encoder_decoder/modeling_speech_encoder_decoder.py#L227 is compatible with it.

The other files can stay the same :-)

> Hey @ydshieh,
> 
> We need to slightly update this PR for the speech encoder decoder classes sadly so that the newly introduced variable `config.output_hidden_size` as shown here:
> 
> https://github.com/huggingface/transformers/blob/cea17acd8cc5190684da944924116fe10742ad81/src/transformers/models/speech_encoder_decoder/modeling_speech_encoder_decoder.py#L227
> is compatible with it.
> 
> The other files can stay the same :-)

No problem, @patrickvonplaten. But I have a slight doubt at this line:

```
self.enc_to_dec_proj = nn.Linear(self.encoder.config.hidden_size, self.decoder.config.hidden_size)
```
Should it be
```
self.enc_to_dec_proj = nn.Linear(self.encoder_output_dim, self.decoder.config.hidden_size)
```
if `config.output_hidden_size` is introduced in the config and used here? I didn't go through the speech model, but it looks more natural to do so.


I made the necessary updates where `config.output_hidden_size` is involved.
I didn't change the line
```
self.enc_to_dec_proj = nn.Linear(self.encoder.config.hidden_size, self.decoder.config.hidden_size)
```
despite a slight doubt.

(Fixed)

The failed TF/Torch test is due to #14016 being merged to master (and I rebased this PR on master), which is expected. I will take care of this issue.
Fix weight loading issue
Author: ydshieh
Pull request: yes

# What does this PR do?

Fix #14002 + add 2 more tests

(I uploaded the converted TF checkpoint to [ydshieh](https://huggingface.co/ydshieh/bert2bert-cnn_dailymail-fp16)). Might be better for @patrickvonplaten to upload to [patrickvonplaten](https://huggingface.co/patrickvonplaten/bert2bert-cnn_dailymail-fp16/blob/main/README.md)

## Who can review?

@Rocketknight1 @patrickvonplaten @LysandreJik 

Comments:
The issue in #14002 comes from the fact, when using `from_pt=True`, the block shown at the end doesn't use `load_weight_prefix`.

However, it is required to extend the variable scope for the 2 components (encoder & decoder) of a TF composite models - to make the subsequent `save_pretrained -> from_pretrained` work after a creation from `from_encoder_decoder_pretrained`.

I feel it would be better to modify `load_pytorch_weights_in_tf2_model` to address this situation, but I tried to avoid modify this Hugging Face's TF core method.

https://github.com/huggingface/transformers/blob/d5b82bb70c2e8c4b184a6f2a7d1c91d7fd156956/src/transformers/modeling_tf_utils.py#L1445-L1449


Looks great to me!

Hi, I just realized that `TFEncoderDecoderModel` is released with `v4.12.0` without this fix being merged.
Fix flax summarization example: save checkpoint after each epoch and push checkpoint to the hub
Author: ydshieh
Pull request: yes

# What does this PR do?

Current `run_summarization_flax.py` saves checkpoint & pushs to the hub UNDER the block `if training_args.do_predict:`

https://github.com/huggingface/transformers/blob/7051b892671982717388dd6dfca368a90da60252/examples/flax/summarization/run_summarization_flax.py#L811

This block is also outside the epoch looping block, and the saving/pushing won't happen after each epoch as expected.

This PR moves saving/pushing block to the right place.

## Who can review?

@patil-suraj  @patrickvonplaten 

Comments:
Fix non-negligible difference between GPT2 and TFGP2
Author: ydshieh
Pull request: yes

# What does this PR do?

Fix #13666 

Comments:
The following test should have caught that error: `test_pt_tf_model_equivalence` in `modeling_tf_common.py` - would you like to check why it was passing even though it shouldn't have been?

> 
> 
> The following test should have caught that error: `test_pt_tf_model_equivalence` in `modeling_tf_common.py` - would you like to check why it was passing even though it shouldn't have been?

Sure. Will do it tomorrow :)

Thank you @ydshieh, this is very helpful.

Hi, @LysandreJik ,

In `test_modeling_tf_gpt2.py` , `TFGPT2ModelTester` uses a config with `"gelu"` activation.

https://github.com/huggingface/transformers/blob/8e908c8c74f556a82534f4cf1e7a1b4f7b55d24c/tests/test_modeling_tf_gpt2.py#L56

In `test_pt_tf_model_equivalence.py`, both tf & pytorch models are created using this same config, and therefore both use `"gelu"`.

Since the issue in `TFGPT2Model` was that it used `self.act = get_tf_activation("gelu")`, the test would pass.

The issue would be observed when the config uses an activation functions other than `"gelu"`, which is the case when I used the pretrained GPT2 (`"gelu_new"` is used).

https://huggingface.co/gpt2/blob/main/config.json

So `test_pt_tf_model_equivalence` is fine in my opinion, but it might be good to add other tests to prevent this situation (using pretrained models' configs, but reduce the hidden size & no. of layers).
(well, this doesn't 100% guarantee, if an attribute in a pretrained config happens to be the same as a hard coded attribute in a model).

@ydshieh, thank you for your detailed analysis, this is perfect! I understand the limitation of the test and this edge case.

Merging this PR!
Change a parameter name in FlaxBartForConditionalGeneration.decode()
Author: ydshieh
Pull request: yes

# What does this PR do?

In short: Change a parameter name in `FlaxBartForConditionalGeneration.decode()`: `deterministic` -> `train`.

In the current version of `FlaxBartForConditionalGeneration.decode()` method, it takes an argument `deterministic`, while 
`FlaxBartPreTrainedModel.decode()`, `FlaxT5PreTrainedModel.decode()`, `FlaxT5ForConditionalGeneration.decode()`, and similar places in `FlaxGPT2`, they all use `train` as the argument.

It seems to me that there is a (implicit?) convention that, in Flax models, we use `deterministic` parameter for `nn.Module` and parameter `train` for models inheriting from `FlaxPreTrainedModel`.

This PR fix this small inconsistency in `FlaxBartForConditionalGeneration.decode()`. I hope this PR makes sense, despite the change is really small.

## Before submitting

- [ x] Did you read the [contributor guideline](https://github.com/huggingface/transformers/blob/master/CONTRIBUTING.md#start-contributing-pull-requests)

## Who can review?

@patrickvonplaten 
@patil-suraj 




Comments:
I made the same change to flax marian and mbart as suggested.

Thanks a lot for fixing this!

Merging, the failing test is un-related.
Fix flax gpt2 hidden states
Author: ydshieh
Pull request: yes

# What does this PR do?

Fixes #13102 

## 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] Was this discussed/approved via a Github issue or the [forum](https://discuss.huggingface.co/)? 
    [ inconsistency of the last element in hidden_states between PyTorch/Flax GPT2(Neo) #13102 ](https://github.com/huggingface/transformers/issues/13102#issuecomment-897687182)

## Who can review?

@patrickvonplaten @patil-suraj 

Comments:
Hi, @patil-suraj , thank you for the suggestions. There is however some issues regarding

```
        if output_hidden_states:
            all_hidden_states = outputs[1] + (hidden_states,)
            outputs = (hidden_states, all_hidden_states) + outputs[2:]
        else:
            outputs = (hidden_states,) + outputs[1:]
```
because this will change `outputs` to a `tuple` even if it is previously a `FlaxBaseModelOutput`, and this causes problem to the end (i.e. if `return_dict=True`)

```
        return FlaxBaseModelOutput(
            last_hidden_state=hidden_states,
            hidden_states=outputs.all_hidden_states,
            attentions=outputs.attentions,
        )
```

Do you have a good solution to address this while keep your suggestions?

Ahh, yeah, you're right!

I wanted to avoid multiple if/else conds, but seems we will need to add one either way.

I could see two options:

- we have already stored `all_hidden_states`, we could store the `all_attentions` using 

```python3
all_attentions = outputs[-1] if output_attentions else None
```

and then use that in the output class

- another option is, `FlaxGPT2BlockCollection` is only used internally, so we could also just always return `outputs` (including `None` values) as a `tuple`. So in the `FlaxGPT2Module`, we could do 

```python3
if output_hidden_states:
	all_hidden_states = outputs[1] + (hidden_states,)
	outputs = (hidden_states, all_hidden_states) + outputs[2:]
else:
	outputs = (hidden_states,) + outputs[1:]

if not return_dict:
    return tuple(v for v in outputs if v is not None)

return FlaxBaseModelOutput(
    last_hidden_state=hidden_states,
    hidden_states=all_hidden_states,
    attentions=outputs[-1],
)
```

@patil-suraj  I went for option 2, with a slightly change `hidden_states=all_hidden_states,` -> `hidden_states=outputs[1],` (all_hidden_states not always define).
Add vision_encoder_decoder to models/__init__.py
Author: ydshieh
Pull request: yes

# What does this PR do?

The recently added `vision_encoder_decoder` wasn't added to `models/__init__.py`. This had some consequence, like `utils.check_repo.get_model_modules()` missing `vision_encoder_decoder`, and therefore models like `VisionEncoderDecoderModel` (and the upcoming Flax/TF versions) would pass `check_all_models_are_tested` even without providing a test file.

This (one-line) PR fixes this issue.

## Who can review?

@sgugger @NielsRogge 

Comments:
Yes. It's easy. I can fix it in the same PR? It's ok?

Yes we can't merge it otherwise ;-)

Done.

P.S.: I don't know the exact reason for which we put encoder-decoder model family in the list `_ignore_modules`, but I saw`modeling_encoder_decoder` was already there since the 1st version `6ba540b7` of `check_repo.py`.

https://github.com/huggingface/transformers/blob/6ba540b7475f095d591b4766cac897007b1d5db0/utils/check_repo.py#L108-L112

The error reported is
```
test_modeling_vision_encoder_decoder.py should define `all_model_classes` to apply common tests to the models it tests. If this intentional, add the test filename to `TEST_FILES_WITH_NO_COMMON_TESTS` in the file `utils/check_repo.py`.
```
I think `TEST_FILES_WITH_NO_COMMON_TESTS` is the place to address (?), but maybe I don't see the full picture here.

No that's the right way.