Skip to content

Convnext flax - #21485

Closed
Shubhamai wants to merge 15 commits into
huggingface:mainfrom
Shubhamai:convnext-flax
Closed

Convnext flax#21485
Shubhamai wants to merge 15 commits into
huggingface:mainfrom
Shubhamai:convnext-flax

Conversation

@Shubhamai

@Shubhamai Shubhamai commented Feb 7, 2023

Copy link
Copy Markdown
Contributor

Flax Implementation of facebook/convnext-tiny-224

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?

Flax: @sanchit-gandhi

TODO

Last Updated : 10 Feb, 2023

@HuggingFaceDocBuilderDev

Copy link
Copy Markdown

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint.

@Shubhamai Shubhamai changed the title [WIP] Convnext flax Convnext flax Feb 10, 2023
@Shubhamai
Shubhamai marked this pull request as ready for review February 11, 2023 18:12
@Shubhamai

Copy link
Copy Markdown
Contributor Author

@sanchit-gandhi this PR is also ready for your review in case it was missed. And again, thank you so much for taking the time to review the PR.

@Shubhamai Shubhamai mentioned this pull request Feb 28, 2023
7 tasks
@Shubhamai

Copy link
Copy Markdown
Contributor Author

@sanchit-gandhi Reminder incase my previous message got missed! Also the #21472 ( previous reviews implemented ) and #21867 PR are awaiting review. Thanks!

@huggingface huggingface deleted a comment from github-actions Bot Apr 4, 2023

@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.

Looks great already @Shubhamai! Think we just need to propagate some of the changes from Flax ResNet / RegNet into this PR, then we're good to go 🚀

The initial value for the layer scale.
drop_path_rate (`float`, *optional*, defaults to 0.0):
The drop rate for stochastic depth.
image_size (`int`, *optional*, defaults to 224):

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 we can just remove this from the docstring (see #21472 (comment))

dtype: jnp.dtype = jnp.float32

@nn.compact
def __call__(self, hidden_states: jnp.ndarray, train: bool = False) -> jnp.ndarray:

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.

We can also swap train for deterministic (see #21472 (comment))

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, missed it there, should be fixed in recent commit

shape = (hidden_states.shape[0],) + (1,) * (
hidden_states.ndim - 1
) # work with diff dim tensors, not just 2D ConvNets
random_tensor = keep_prob + random.uniform(random.PRNGKey(0), shape, 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.

We probably need to pass a PRNG key here (rather than always starting from seed 0), see

dropout_rng = None
if not deterministic and self.config.attention_probs_dropout_prob > 0.0:
dropout_rng = self.make_rng("dropout")
and #21023 (review)

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.

Replaced random.PRNGKey(0) with self.make_rng("dropout")


def __call__(self, pixel_values: jnp.ndarray) -> jnp.ndarray:
num_channels = pixel_values.shape[-1]
if num_channels != self.config.num_channels:

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.

Same here regarding the config and num_channels

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.

I assume you mean removing the if statement and num_channels in the config file. If yes, then the changed should be made in the recent commit

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.

Sorry we shouldn't remove num_channels from the config as this will break the PyTorch code!

Comment thread src/transformers/models/convnext/modeling_flax_convnext.py
Comment thread src/transformers/models/convnext/modeling_flax_convnext.py
)
return_dict = return_dict if return_dict is not None else self.config.use_return_dict

if pixel_values is None:

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 won't play very nicely with JIT since we have control flow on our input (see https://jax.readthedocs.io/en/latest/notebooks/Common_Gotchas_in_JAX.html#control-flow)

Think we can safely assume here that pixel_values will not be None (otherwise the next line self.embeddings(pixel_values) will complain anyway that pixel_values is None), and so can remove this check.

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.

Agree, should be removed in the recent commit

>>> image = Image.open(requests.get(url, stream=True).raw)

>>> image_processor = AutoImageProcessor.from_pretrained("facebook/convnext-tiny-224")
>>> model = FlaxConvNextModel.from_pretrained("facebook/convnext-tiny-224")

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.

Cool! Have we uploaded the Flax weights already?

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.

from transformers import AutoFeatureExtractor


class FlaxConvNextModelTester(unittest.TestCase):

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.

Would be awesome to update the tester according to the Flax ResNet model tester :)


@slow
def test_inference_image_classification_head(self):
model = FlaxConvNextForImageClassification.from_pretrained("Shubhamai/convnext-tiny-224")

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.

Feel free to open a PR for the Flax weights on the Facebook org!

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.


def __init__(
self,
num_channels=3,

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.

@Shubhamai Shubhamai May 5, 2023

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.

Hi, I should have mentioned the review changes weren't fully complete, I am aware of the error due to failed tests, I will fix it asap ( probably on this weekend )

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.

Cool! Sounds good! Feel free to ping me when it's ready and I'll get you a final review :)

Comment thread src/transformers/models/convnext/modeling_flax_convnext.py

def __call__(self, pixel_values: jnp.ndarray) -> jnp.ndarray:
num_channels = pixel_values.shape[-1]
if num_channels != self.config.num_channels:

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.

Sorry we shouldn't remove num_channels from the config as this will break the PyTorch code!

layer_norm_eps=1e-12,
layer_scale_init_value=1e-6,
drop_path_rate=0.0,
image_size=224,

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.

In general there shouldn't be a need to change the config file at all for a Flax port

@github-actions

Copy link
Copy Markdown
Contributor

This issue has been automatically marked as stale because it has not had recent activity. If you think this still needs to be addressed please comment on this thread.

Please note that issues that do not follow the contributing guidelines are likely to be ignored.

@github-actions github-actions Bot closed this Jun 7, 2023
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.

3 participants