Add TFConvNextModel
Author: sayakpaul
Pull request: yes

This PR adds the ConvNeXt [1] model in TensorFlow. It was developed by @arig23498, @gante, and myself. 

Fun facts about this PR:

* Probably the first pure conv model in `transformers`.
* Probably the second pure vision model in TensorFlow in `transformers`.

**References**:

[1] A ConvNet for the 2020s: https://arxiv.org/abs/2201.03545.

@gante @LysandreJik @Rocketknight1 

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

TODO for me before merging:
1. add image size to the model config files (so we can use models that expect 384x384 inputs)
2. convert checkpoints from PT to TF and add them to the FB org
3. check if the docs are in a good state

>     2. convert checkpoints from TF to PT and add them to the FB org
            (I guess you mean PT -> TF ?)


> > ```
> > 2. convert checkpoints from TF to PT and add them to the FB org
> > ```
> 
> ```
>         (I guess you mean PT -> TF ?)
> ```

haha yes! (edited)

> I have a few minor comments for clarification, but I don't think any of them are blocking, and the tests look comprehensive.

I think I have addressed them. 

Hi, @sayakpaul 

In a (currently not on master) extended test, I found that the returned `hidden_states` (all layers, not just the last layer one) have different format. 

See
https://github.com/huggingface/transformers/blob/15c916f416dfc5c820528f25c0257544847ceb50/src/transformers/models/convnext/modeling_tf_convnext.py#L494-L496
(where only the first element, i.e. `last_hidden_states` is changed to mathc PT output).

Would you mind to work on this? (I can work on it too if necessary)

cc @gante (I really need to push this test ASAP, but there are still a few pt/tf issues to fix)

> Hi, @sayakpaul
> 
> In a (currently not on master) extended test, I found that the returned `hidden_states` (all layers, not just the last layer one) have different format.
> 
> See
> 
> https://github.com/huggingface/transformers/blob/15c916f416dfc5c820528f25c0257544847ceb50/src/transformers/models/convnext/modeling_tf_convnext.py#L494-L495
> 
> 
> (where only the first element, i.e. `last_hidden_states` is changed to mathc PT output).
> Would you mind to work on this?

This change was from @gante. Let's take their inputs and sure, I can work on it. 

> but there are still a few pt/tf issues to fix

Is this something I can do in this PR? Or is it more on the general side? 

> > Hi, @sayakpaul
> > In a (currently not on master) extended test, I found that the returned `hidden_states` (all layers, not just the last layer one) have different format.
> > See
> > https://github.com/huggingface/transformers/blob/15c916f416dfc5c820528f25c0257544847ceb50/src/transformers/models/convnext/modeling_tf_convnext.py#L494-L495
> > 
> > (where only the first element, i.e. `last_hidden_states` is changed to mathc PT output).
> > Would you mind to work on this?
> 
> This change was from @gante. Let's take their inputs and sure, I can work on it.

OK. Let's see what @gante says.

> 
> > but there are still a few pt/tf issues to fix
> 
> Is this something I can do in this PR? Or is it more on the general side?

No, it's just a comment for @gante only about a general test (not particular for this PR). Sorry for the confusion.




@sayakpaul You can run `make style` to fix the test (check_code_quality)

>In a (currently not on master) extended test, I found that the returned hidden_states (all layers, not just the last layer one) have different format.

@ydshieh @sayakpaul -- Those are great points and deserve to be thoroughly discussed. This is one of the first vision models for the TF side of `transformers`, so the decision we will make here will likely set a precedence for other models. 

The root issue is that PT operates NCHW by default, while TF expects NHWC. In `TFConvNextEmbeddings` we convert from NCHW (the input shape) to NHWC to enable the model's use on CPU, so all dense layers and their outputs will have their dimensions swapped (compared to PT).

We have three main options:
1. Add transpose operations before and after each Conv2D layer, so that the TF model perfectly matches the PT model;
2. [current approach] Ensure that the same input generates the same output, without caring too much about the inner hidden layers;
3. Assume NHWC all the way from the input to the output, as a TF model would.

Keep in mind that options 2. and 3. imply that we will only be able to test TF<>PT equivalence if we transpose layers in the tests, but option 1. adds extra code (and probably a slower model) in addition to a PT-code vibe. Option 3. is more disruptive, as PT and TF would have different interfaces, which seems to be a big no-no.

All in all, option 2. seemed to be the most sensible, so I went with that. I'd like to hear the input from @Rocketknight1 and @LysandreJik, which are more experienced with this sort of trade-offs :)

Too many transpositions will definitely impact the speed of the model. 

> > In a (currently not on master) extended test, I found that the returned hidden_states (all layers, not just the last layer one) have different format.
> 
> @ydshieh @sayakpaul -- Those are great points and deserve to be thoroughly discussed. This is one of the first vision models for the TF side of `transformers`, so the decision we will make here will likely set a precedence for other models.
> 
> The root issue is that PT operates NCHW by default, while TF expects NHWC. In `TFConvNextEmbeddings` we convert from NCHW (the input shape) to NHWC to enable the model's use on CPU, so all dense layers and their outputs will have their dimensions swapped (compared to PT).
> 
> We have three main options:
> 
>     1. Add transpose operations before and after each Conv2D layer, so that the TF model perfectly matches the PT model;
> 
>     2. [current approach] Ensure that the same input generates the same output, without caring too much about the inner hidden layers;
> 
>     3. Assume NHWC all the way from the input to the output, as a TF model would.
> 
> 
> Keep in mind that options 2. and 3. imply that we will only be able to test TF<>PT equivalence if we transpose layers in the tests, but option 1. adds extra code (and probably a slower model) in addition to a PT-code vibe. Option 3. is more disruptive, as PT and TF would have different interfaces, which seems to be a big no-no.
> 
> All in all, option 2. seemed to be the most sensible, so I went with that. I'd like to hear the input from @Rocketknight1 and @LysandreJik, which are more experienced with this sort of trade-offs :)

Very good point @gante! I completely forgot the option 1.

Option 2 seems good, but in text models, there are a few tests where we test some internal modules.

For example, [BartStandaloneDecoderModelTester](https://github.com/ydshieh/transformers/blob/2b5603f6ac58f0cd3b2116c01d6b9f62575248b2/tests/test_modeling_bart.py#L851)

Not sure if such cases will happen for vision models. However, this is rare, and probably can be treated for individual models.


@sayakpaul @ariG23498 -- TF checkpoints uploaded to the hub (e.g. [here](https://huggingface.co/facebook/convnext-tiny-224/tree/main)). The config files were also updated to include `image_size` -- I've confirmed that models for `image_size=384` work as expected and match their PT counterpart.

Feel free to update the tests relying on the `from_pt` flag 👍 

> @sayakpaul @ariG23498 -- TF checkpoints uploaded to the hub (e.g. [here](https://huggingface.co/facebook/convnext-tiny-224/tree/main)). The config files were also updated to include `image_size` -- I've confirmed that models for `image_size=384` work as expected and match their PT counterpart.
> 
> Feel free to update the tests relying on the `from_pt` flag 👍

Done!

It looks like #15730 (merged) is the likely cause of the failing test in `run_tests_tf` here.
(but that PR has green checks, strange)

> It looks like #15730 (merged) is the likely cause of the failing test in `run_tests_tf` here. (but that PR has green checks, strange)

I have run this failing test `tests/test_modeling_tf_gpt2.py::TFGPT2ModelTest::test_gpt2_model_past_large_inputs` locally, and the difference of `output_from_past_slice` and `output_from_no_past_slice` in
```
tf.debugging.assert_near(output_from_past_slice, output_from_no_past_slice, rtol=1e-3)
``` 
are in the range `1e-8 ~ 1e-6` for 1000 runs. It's unclear why this test failed.

However, I think the failing test is irrelevant to this PR.

@sgugger , regarding [the output shapes](https://github.com/huggingface/transformers/pull/15750#issuecomment-1047088954), do you have some opinion?

(it's about the Conv2D - NCHW vs NHWC format in the intermediate values & final outputs)


I'm fine with the proposed option for the shapes. Option 1 is a no-go because of performance, and option 3 would be too disruptive for PT/TF compatibility.

@sgugger I have noted your comments and will address them as soon as I can. I am currently traveling. 

> Keep in mind that options 2. and 3. imply that we will only be able to test TF<>PT equivalence if we transpose layers in the tests, but option 1. adds extra code (and probably a slower model) in addition to a PT-code vibe. Option 3. is more disruptive, as PT and TF would have different interfaces, which seems to be a big no-no.
> 

@gante, I didn't realized that, by option 2., you mean we don't change back to `NCHW` for `hidden_states` etc. in the outputs of, say `TFConvNextMainLayer`.

It would be better to have a uniform format between frameworks, at the levels of modules a user will use the library.
(and it will simplify the testing a lot)

What I think is:
- Ensure that the same input generates the same output
    - without caring too much about the inner hidden layers **during the computation**
    - but change all output values to have the same format as PT (i.e. `NCHW` format) at the `MainLayer` level
      - (so the `BaseModel` and `For...Task` models will also have the same format as their PT equivalences) 

WDYT?




> > Keep in mind that options 2. and 3. imply that we will only be able to test TF<>PT equivalence if we transpose layers in the tests, but option 1. adds extra code (and probably a slower model) in addition to a PT-code vibe. Option 3. is more disruptive, as PT and TF would have different interfaces, which seems to be a big no-no.
> 
> @gante, I didn't realized that, by option 2., you mean we don't change back to `NCHW` for `hidden_states` etc. in the outputs of, say `TFConvNextMainLayer`.
> 
> It would be better to have a uniform format between frameworks, at the levels of modules a user will use the library. (and it will simplify the testing a lot)
> 
> What I think is:
> 
> * Ensure that the same input generates the same output
>   
>   * without caring too much about the inner hidden layers **during the computation**
>   * but change all output values to have the same format as PT (i.e. `NCHW` format) at the `MainLayer` level
>     
>     * (so the `BaseModel` and `For...Task` models will also have the same format as their PT equivalences)
> 
> WDYT?

Yeah, that makes sense @ydshieh :) @sayakpaul can you move the final transpose from `TFConvNextModel` to `TFConvNextMainLayer`?

> > > Keep in mind that options 2. and 3. imply that we will only be able to test TF<>PT equivalence if we transpose layers in the tests, but option 1. adds extra code (and probably a slower model) in addition to a PT-code vibe. Option 3. is more disruptive, as PT and TF would have different interfaces, which seems to be a big no-no.
> > 
> > 
> > @gante, I didn't realized that, by option 2., you mean we don't change back to `NCHW` for `hidden_states` etc. in the outputs of, say `TFConvNextMainLayer`.
> > It would be better to have a uniform format between frameworks, at the levels of modules a user will use the library. (and it will simplify the testing a lot)
> > What I think is:
> > 
> > * Ensure that the same input generates the same output
> >   
> >   * without caring too much about the inner hidden layers **during the computation**
> >   * but change all output values to have the same format as PT (i.e. `NCHW` format) at the `MainLayer` level
> >     
> >     * (so the `BaseModel` and `For...Task` models will also have the same format as their PT equivalences)
> > 
> > WDYT?
> 
> Yeah, that makes sense @ydshieh :) @sayakpaul can you move the final transpose from `TFConvNextModel` to `TFConvNextMainLayer`?

Also make `pooler_output` and `hidden_states` have the same shape as PT's `ConvNextModel`, please. Thanks.
 
A bit subtle here: `hidden_states` not always exists -> requires to check `output_hidden_states`

https://github.com/huggingface/transformers/blob/0f98fb5d55a69729f4efb9315857fdab31ce1034/src/transformers/models/convnext/modeling_tf_convnext.py#L338-L339


Sorry that all the tests are failing. I must have messed something up during rebasing. But the good news is I was able to isolate the equivalence test from the TF ConvNeXt modeling subclass. It is passing locally. Now, I will investigate the rebasing issue and update. 

@ydshieh could you clarify this a bit? 

https://github.com/huggingface/transformers/pull/15750#issuecomment-1048939309

> @ydshieh could you clarify this a bit?
> 
> [#15750 (comment)](https://github.com/huggingface/transformers/pull/15750#issuecomment-1048939309)

Sure. The objective is to make `TFConvNextModel` return the same values as its PyTorch version `ConvNextModel`.
This includes `last_hidden_state`, `pooler_output`, and `hidden_states` (if `output_hidden_states=True`).

The same requirement is also for `TFConvNextForImageClassification`. (In general, the top level models that users will interact).

Since TF (top level) models in `transformers` use `... MainLayer`, it is easier and clean to make sure `TFConvNextMainLayer` and `ConvNextModel` have the same return value format. This means, we transpose `NHWC` format in `TFConvNextMainLayer` back to `NCHW` for the outputs just once, and there is no need to transpose in `TFConvNextModel`/`TFConvNextForImageClassification`.

Hope this is more clear now :-)







    

> > @ydshieh could you clarify this a bit?
> > [#15750 (comment)](https://github.com/huggingface/transformers/pull/15750#issuecomment-1048939309)
> 
> Sure. The objective is to make `TFConvNextModel` return the same values as its PyTorch version `ConvNextModel`. This includes `last_hidden_state`, `pooler_output`, and `hidden_states` (if `output_hidden_states=True`).
> 
> The same requirement is also for `TFConvNextForImageClassification`. (In general, the top level models that users will interact).
> 
> Since TF (top level) models in `transformers` use `... MainLayer`, it is easier and clean to make sure `TFConvNextMainLayer` and `ConvNextModel` have the same return value format. This means, we transpose `NHWC` format in `TFConvNextMainLayer` back to `NCHW` for the outputs just once, and there is no need to transpose in `TFConvNextModel`/`TFConvNextForImageClassification`.
> 
> Hope this is more clear now :-)

Yes, it is! Thank you so much. 

I got confused with:

> A bit subtle here: hidden_states not always exists -> requires to check output_hidden_states

With respect to [`check_code_quality`](https://app.circleci.com/pipelines/github/huggingface/transformers/35050/workflows/bc1915df-9ea9-4876-9b52-3ab7e01095d0/jobs/376812) step being failed: 

@ydshieh I did run `make style` before committing. As per the error trace from CircleCI, I also ran black separately. It returned the file unchanged. I am on Black `21.4b0`. Do you have any suggestions?

> With respect to [`check_code_quality`](https://app.circleci.com/pipelines/github/huggingface/transformers/35050/workflows/bc1915df-9ea9-4876-9b52-3ab7e01095d0/jobs/376812) step being failed:
> 
> @ydshieh I did run `make style` before committing. As per the error trace from CircleCI, I also ran black separately. It returned the file unchanged. I am on Black `21.4b0`. Do you have any suggestions?

Let me check (in a few hours though)

> > With respect to [`check_code_quality`](https://app.circleci.com/pipelines/github/huggingface/transformers/35050/workflows/bc1915df-9ea9-4876-9b52-3ab7e01095d0/jobs/376812) step being failed:
> > @ydshieh I did run `make style` before committing. As per the error trace from CircleCI, I also ran black separately. It returned the file unchanged. I am on Black `21.4b0`. Do you have any suggestions?
> 
> Let me check (in a few hours though)

No rush. I appreciate your help :) 

> With respect to [`check_code_quality`](https://app.circleci.com/pipelines/github/huggingface/transformers/35050/workflows/bc1915df-9ea9-4876-9b52-3ab7e01095d0/jobs/376812) step being failed:
> 
> @ydshieh I did run `make style` before committing. As per the error trace from CircleCI, I also ran black separately. It returned the file unchanged. I am on Black `21.4b0`. Do you have any suggestions?

@sayakpaul , I am on a newer version

```
(py39) λ black --version
black, 22.1.0 (compiled: yes)
```

When I run `make_style` on this PR branch, there is indeed some changes.

Could you follow the instructions [here](https://github.com/huggingface/transformers/issues/15628#issuecomment-1036418742), and see if it helps?

@ydshieh I created a new virtual environment and installed `pip install "[.quality]"`. And then ran `make style`. I only committed the files associated with this PR. The tests that do not suppose to fail are not failing. 

> @ydshieh I created a new virtual environment and installed `pip install "[.quality]"`. And then ran `make style`. I only committed the files associated with this PR. The tests that do not suppose to fail are not failing.

Not yet check the detail. But why your last commit changed `tests/test_modeling_common.py` a lot? Do you know the reason?

https://github.com/huggingface/transformers/pull/15750/commits/229a817ad8d9eb004fa11f548c483ad26a3a4283

> > @ydshieh I created a new virtual environment and installed `pip install "[.quality]"`. And then ran `make style`. I only committed the files associated with this PR. The tests that do not suppose to fail are not failing.
> 
> Not yet check the detail. But why your last commit changed `tests/test_modeling_common.py` a lot? Do you know the reason?
> 
> [229a817](https://github.com/huggingface/transformers/commit/229a817ad8d9eb004fa11f548c483ad26a3a4283)

I am myself not sure. I just reverted to the original file. 

Just two now probably because of the `output_attentions` thing. I will get to that once the test file that I modified today looks good.

> After the issue above gets fixed, the transpose gets moved to TFConvNextMainLayer, and self.output_attentions = None gets removed from the config, I'm happy to approve 💪

@gante we're almost there. I was able to move the transposition to `TFConvNextMainLayer` (not committed yet) but I am still having problems with the transposition of `hidden_states`.

> > After the issue above gets fixed, the transpose gets moved to TFConvNextMainLayer, and self.output_attentions = None gets removed from the config, I'm happy to approve 💪
> 
> @gante we're almost there. I was able to move the transposition to `TFConvNextMainLayer` (not committed yet) but I am still having problems with the transposition of `hidden_states`.

not sure if you know it already or not, but `hidden_states` is a tuple of tensors, not a single tensor. You will need to transpose each element, and pack them again into a tuple. Don't hesitate if you have further question about this part.

> > > After the issue above gets fixed, the transpose gets moved to TFConvNextMainLayer, and self.output_attentions = None gets removed from the config, I'm happy to approve 💪
> > 
> > 
> > @gante we're almost there. I was able to move the transposition to `TFConvNextMainLayer` (not committed yet) but I am still having problems with the transposition of `hidden_states`.
> 
> not sure if you know it already or not, but `hidden_states` is a tuple of tensors, not a single tensor. You will need to transpose each element, and pack them again into a tuple. Don't hesitate if you have further question about this part.

I'm aware :) 

I am through and through till the packing but tuple assignment is the blocker now. 

> > > > After the issue above gets fixed, the transpose gets moved to TFConvNextMainLayer, and self.output_attentions = None gets removed from the config, I'm happy to approve 💪
> > > 
> > > 
> > > @gante we're almost there. I was able to move the transposition to `TFConvNextMainLayer` (not committed yet) but I am still having problems with the transposition of `hidden_states`.
> > 
> > 
> > not sure if you know it already or not, but `hidden_states` is a tuple of tensors, not a single tensor. You will need to transpose each element, and pack them again into a tuple. Don't hesitate if you have further question about this part.
> 
> I'm aware :)
> 
> I am through and through till the packing but tuple assignment is the blocker now.

Does this help:

```
hidden_states = tuple([**transpose_method**(h)  for h in hidden_states])
```
?

Or you are blocked in another place involving tuple?

Thanks, @ydshieh. I should have thought about it a bit more. Looks like all the things suggested [here](https://github.com/huggingface/transformers/pull/15750#pullrequestreview-892824548) have been taken care of. 

Cc: @gante 

> tests/test_modeling_tf_common.py is still with the unwanted style changes. For me, we can merge the PR after we revert it and the tests pass -- approving

@gante should I run `make fixup` and just commit `tests/test_modeling_tf_common.py` after that? 

> > tests/test_modeling_tf_common.py is still with the unwanted style changes. For me, we can merge the PR after we revert it and the tests pass -- approving
> 
> @gante should I run `make fixup` and just commit `tests/test_modeling_tf_common.py` after that?

I'd say `make fixup` should be the last thing it is done before the final commit (assuming it is working well on your env -- if it is not, let us know).

@sgugger this last commit should have all requested changes factored in (thank you @sayakpaul 🤜 🤛 ), let us know if you approve them 

@sayakpaul Are you sure you are applying the suggestions exactly as I left them? If you leave the trailing comma, black will reformat again on several lines. I've checked out your PR locally and applied all my suggestions/changes requested; `make style` doesn't change it again.

I'm happy to push the result directly to your branch if that helps (and you left permissions for the maintainers) or to make a PR against your branch, whichever is easier to have those fixes inside the PR and we can merge. 

Thank you.

> I'm happy to push the result directly to your branch if that helps

This seems like the fastest route and I am okay with it. 

Ah, I actually checked out the branch by its pull request ID instead of your origin/branch name so then git didn't let me push. There is a PR [here](https://github.com/sayakpaul/transformers/pull/3) if you can merge it, we should be fine here. Sorry about that.

Thank you!

Thanks again for all your wok on this model!

Ccing @ariG23498 @gante and @ydshieh. Thank you so much all.