Skip to content

Resnet flax - #21472

Merged
amyeroberts merged 21 commits into
huggingface:mainfrom
Shubhamai:resnet-flax
Mar 24, 2023
Merged

Resnet flax#21472
amyeroberts merged 21 commits into
huggingface:mainfrom
Shubhamai:resnet-flax

Conversation

@Shubhamai

@Shubhamai Shubhamai commented Feb 6, 2023

Copy link
Copy Markdown
Contributor

Flax Implementation of microsoft/resnet-50

Before submitting

  • This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
  • Did you read the contributor guideline,
    Pull Request section?
  • Was this discussed/approved via a Github issue or the forum? Please add a link
    to it if that's the case.
  • Did you make sure to update the documentation with your changes? Here are the
    documentation guidelines, and
    here are tips on formatting docstrings.
  • Did you write any new necessary tests?

Who can review?

@sanchit-gandhi I guess 😅

Status

Last Updated - Sunday, 12 February 2023

TODO

@HuggingFaceDocBuilderDev

HuggingFaceDocBuilderDev commented Feb 6, 2023

Copy link
Copy Markdown

The documentation is not available anymore as the PR was closed or merged.

@Shubhamai Shubhamai changed the title Resnet flax [WIP] Resnet flax Feb 6, 2023
@sanchit-gandhi

sanchit-gandhi commented Feb 22, 2023

Copy link
Copy Markdown
Contributor

Very cool! Sorry to only reply here now - looks like you've made a really solid start to this PR! Let's get the Batch Norm PR merged ASAP and then go full send on Flax ResNet! 🚀

Feel free to ping me with any questions / queries! More than happy to help with the integration!

@Shubhamai Shubhamai changed the title [WIP] Resnet flax Resnet flax Feb 24, 2023
@Shubhamai
Shubhamai marked this pull request as ready for review February 24, 2023 09:23
@Shubhamai

Copy link
Copy Markdown
Contributor Author

@sanchit-gandhi the PR is also now ready for your review, thanks a lot for your time.

@sanchit-gandhi sanchit-gandhi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @shubhamagarwal92! Super nice PR! Think you've handled all the conversions of nn.Sequential -> LayerCollection really well!

Left a few comments regarding i) Flax train/deterministic naming conventions, ii) concerns about control flow and XLA compatibility, and iii) one or two unused args that we can remove.

These are very specific because I think the PR is otherwise in very good shape! Feel free to ask any questions / queries if you have them, more than happy to help with the final bit of integration here!

Comment thread docs/source/pt/index.mdx Outdated
Comment thread src/transformers/models/resnet/configuration_resnet.py Outdated
Comment thread src/transformers/models/resnet/modeling_flax_resnet.py Outdated
Comment thread src/transformers/models/resnet/modeling_flax_resnet.py Outdated
padding=self.kernel_size // 2,
dtype=self.dtype,
use_bias=False,
kernel_init=nn.initializers.variance_scaling(2.0, mode="fan_out", distribution="normal", dtype=self.dtype),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the correct kernel_init for ResNet? It might very well be! I'm not a computer vision expert so am not familiar what the standard convolutional inits are here, but what I do know is that for Flax VIT we use a slightly different configuration:

kernel_init=jax.nn.initializers.variance_scaling(

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After changing normal to truncated_normal (in the upcoming commit), I think it's good to go.

In pt implementation of resnet, we used nn.init.kaiming_normal_(module.weight, mode="fan_out", nonlinearity="relu") to initialize Conv2D. Jax does provide partial implementation of kaiming_normal_ as he_normal, partial because it currently does not provide the nonlinearity parameter.

he_normal as mentioned in docs, is simply a specialization of jax.nn.initializers.variance_scaling() . Since pt implementation uses fan_out mode. I had to use variance_scaling initializer for more "parameter control".

normal is changed to truncated_normal because, the same is done in the he_normal implementation of Jax.

I don't remember the last time I used initializers (esp. since normalization and skip connections), so I am also probably not the right person for this, but from the current knowledge I encompass, it looks good to go.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome - thanks for the explanation! Sounds good to me!


for stage_module in self.stages:
if output_hidden_states:
hidden_states = hidden_states + (hidden_state.transpose(0, 3, 1, 2),)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think it's ok that we transpose here since we don't do any ops on the hidden state, just return it as part of the dict (is not going to harm XLA)

Comment thread src/transformers/models/resnet/modeling_flax_resnet.py Outdated
Comment thread src/transformers/models/resnet/modeling_flax_resnet.py Outdated
Comment thread src/transformers/models/resnet/modeling_flax_resnet.py
Comment thread src/transformers/models/resnet/modeling_flax_resnet.py Outdated
@Shubhamai

Copy link
Copy Markdown
Contributor Author

Thanks for the review, I will make the changes soon. Many of the reviews here also apply to the Flax Convnext PR so will make corresponding changes there too.

@Shubhamai Shubhamai mentioned this pull request Mar 9, 2023
8 tasks

@sanchit-gandhi sanchit-gandhi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks for iterating @Shubhamai! Really nice work on the clean-up! Almost ready for a third and final review 🤗 Then we can get it merged!

Comment thread docs/source/pt/index.mdx Outdated
Comment thread src/transformers/models/resnet/modeling_flax_resnet.py Outdated
in_channels: int
out_channels: int
stride: int = 1
activation: Optional[str] = "relu"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still unused!

padding=self.kernel_size // 2,
dtype=self.dtype,
use_bias=False,
kernel_init=nn.initializers.variance_scaling(2.0, mode="fan_out", distribution="normal", dtype=self.dtype),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome - thanks for the explanation! Sounds good to me!

Comment thread src/transformers/models/resnet/modeling_flax_resnet.py Outdated
Comment thread src/transformers/models/resnet/modeling_flax_resnet.py
Comment thread tests/models/resnet/test_modeling_flax_resnet.py Outdated
Comment thread tests/models/resnet/test_modeling_flax_resnet.py Outdated
Comment thread tests/models/resnet/test_modeling_flax_resnet.py Outdated
Comment thread tests/models/resnet/test_modeling_flax_resnet.py
@sanchit-gandhi

Copy link
Copy Markdown
Contributor

Hey @Shubhamai! Really nice work on this PR - just a few small changes to go now! Feel free to ping me once you're happy with the last bits and I'll get you a final review. Taking a look at Flax RegNet in the meantime!

@Shubhamai

Copy link
Copy Markdown
Contributor Author

Made the request changes and apologies for the late reply, had a busy schedule lately.

@sanchit-gandhi sanchit-gandhi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super! Thanks for iterating @Shubhamai! Requesting final review from @amyeroberts (maybe also @sgugger since it's the end of Amy's watch!)

@amyeroberts
amyeroberts self-requested a review March 24, 2023 16:07

@amyeroberts amyeroberts left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution and adding this model!

Left some tiny nits, but overall very nice and clean PR ❤️

RESNET_START_DOCSTRING = r"""

This model inherits from [`FlaxPreTrainedModel`]. Check the superclass documentation for the generic methods the
library implements for all its model (such as downloading, saving and converting weights from PyTorch models)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
library implements for all its model (such as downloading, saving and converting weights from PyTorch models)
library implements for all its model (such as downloading, saving and converting weights from Flax models)

@sanchit-gandhi sanchit-gandhi Mar 24, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be PyTorch! We're converting from PyTorch models to Flax models - it's the same docstring we use in all Flax modelling files, e.g. for Flax BERT:

library implements for all its model (such as downloading, saving and converting weights from PyTorch models)

cc @Shubhamai

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, that makes sense, my bad 😅

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No - my bad - sorry for the incorrect suggestion!

Comment thread tests/models/resnet/test_modeling_flax_resnet.py Outdated
Comment thread src/transformers/models/resnet/modeling_flax_resnet.py Outdated
Comment thread src/transformers/models/resnet/modeling_flax_resnet.py Outdated
Comment thread tests/models/resnet/test_modeling_flax_resnet.py Outdated
Shubhamai and others added 2 commits March 24, 2023 22:35
Co-authored-by: amyeroberts <22614925+amyeroberts@users.noreply.github.com>
@sanchit-gandhi

Copy link
Copy Markdown
Contributor

Thanks for the review @amyeroberts 🙌 Feel free to propagate the changes forward to Flax RegNet @Shubhamai and we can get a final review there!

@sanchit-gandhi

Copy link
Copy Markdown
Contributor

We can merge when the CI is green! 🟢

@amyeroberts
amyeroberts merged commit a0cbbba into huggingface:main Mar 24, 2023
@Shubhamai

Copy link
Copy Markdown
Contributor Author

Thank you so much for the review & merge @sanchit-gandhi @amyeroberts , really appreciate your work & time on this ❤️

raghavanone pushed a commit to raghavanone/transformers that referenced this pull request Apr 5, 2023
* [WIP] flax resnet

* added pretrained flax models, results reproducible

* Added pretrained flax models, results reproducible

* working on tests

* no real code change, just some comments

* [flax] adding support for batch norm layers

* fixing bugs related to pt+flax integration

* removing loss from modeling flax output class

* fixing classifier tests

* fixing comments, model output

* cleaning comments

* review changes

* review changes

* Apply suggestions from code review

Co-authored-by: amyeroberts <22614925+amyeroberts@users.noreply.github.com>

* renaming Flax to PyTorch

---------

Co-authored-by: amyeroberts <22614925+amyeroberts@users.noreply.github.com>
novice03 pushed a commit to novice03/transformers that referenced this pull request Jun 23, 2023
* [WIP] flax resnet

* added pretrained flax models, results reproducible

* Added pretrained flax models, results reproducible

* working on tests

* no real code change, just some comments

* [flax] adding support for batch norm layers

* fixing bugs related to pt+flax integration

* removing loss from modeling flax output class

* fixing classifier tests

* fixing comments, model output

* cleaning comments

* review changes

* review changes

* Apply suggestions from code review

Co-authored-by: amyeroberts <22614925+amyeroberts@users.noreply.github.com>

* renaming Flax to PyTorch

---------

Co-authored-by: amyeroberts <22614925+amyeroberts@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants