Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Flax] Bump to v0.4.1 #17966

Merged
merged 1 commit into from
Jul 5, 2022
Merged

Conversation

sanchit-gandhi
Copy link
Contributor

@sanchit-gandhi sanchit-gandhi commented Jun 30, 2022

What does this PR do?

The flatten_dict operator with the kwarg argument sep was added to modeling_flax_utils in #17760:

weights = flatten_dict(params, sep="/")

This kwarg was only added to Flax in v0.4.1: https://github.com/google/flax/releases/tag/v0.4.1

This PR bumps the required Flax version in Transformers from v0.3.5 to v0.4.1.

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?

Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.

cc @ArthurZucker

@ArthurZucker
Copy link
Collaborator

Nice finding! I am not competent regarding which versions we want to always support, but LGTM

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Jun 30, 2022

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

Copy link
Contributor

@patil-suraj patil-suraj left a comment

Choose a reason for hiding this comment

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

Good catch! I'm fine with always using the latest flax version

@sanchit-gandhi
Copy link
Contributor Author

Thanks @patil-suraj! The latest version would be v0.5.2 (see https://github.com/google/flax/releases). Is the best way to test this by setting flax>=0.5.2 and observe if the tests past?

@patrickvonplaten
Copy link
Contributor

IMO we should also try to stay compatible with older Flax versions, so let's not go as high as we can but only as high as we have to! PR looks good to me - but let's try to not always use the most recent Flax features in order to stay compatible with older versions as well

Copy link
Contributor

@patrickvonplaten patrickvonplaten left a comment

Choose a reason for hiding this comment

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

Ok to bump out of necessity (nice find @sanchit-gandhi) - not because it's best to force the user to always use the newest version. E.g. one might very well have an old research project that only works with Flax 0.3.5, but would like to use the newest transformers. Now, the person could not combine the two -> let's try to stay compatible with older versions in the future :-)

@sanchit-gandhi
Copy link
Contributor Author

Thanks for the details @patrickvonplaten. I'm in agreement that we should avoid potentially breaking use cases purely for the sake of being on the latest version, but try and integrate the latest features where applicable.

Interestingly, I actually went away and had a play with three different versions of Flax on my personal research project https://github.com/sanchit-gandhi/seq2seq-speech:

  1. v0.3.5: issues regarding the sep arg of flatten_dict in modeling_flax_utils (described above)
  2. v0.4.2: no apparent issues
  3. v0.5.2: (latest) had issues with Flax's scan not working depending on JAX version

Seems like v0.4.2 sits in the sweet spot for newer Flax version whilst providing backwards compatibility!

@patrickvonplaten
Copy link
Contributor

Yes, but it's often worth to also not directly implement all the new features of Flax since:

  • a) they might not work very well because they are new
  • b) it breaks backwards comp

@sanchit-gandhi sanchit-gandhi merged commit ec07ecc into huggingface:main Jul 5, 2022
@sanchit-gandhi sanchit-gandhi deleted the flax-version branch July 5, 2022 14:17
viclzhu pushed a commit to viclzhu/transformers that referenced this pull request Jul 18, 2022
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.

5 participants