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

Replace deprecated typing.Hashable with collections.abc.Hashable. #1068

Conversation

carlosgmartin
Copy link
Contributor

According to the Python documentation, typing.Hashable is deprecated and should be replaced with collections.abc.Hashable. This PR fixes optax accordingly.

Copy link
Collaborator

@vroulet vroulet left a comment

Choose a reason for hiding this comment

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

Thanks! I didn't know about PEP 585.
In fact all Callable, Mapping, NamedTuple, etc... should also be imported from collections.abc too, if I understand well the docs https://docs.python.org/3/library/stdtypes.html#types-genericalias.
We can start with Hashable here, and you may add an issue for the other types to be treated later. If that's what you prefer, tell me by answering this comment and I'll approve this PR.
If you are up to make a whole pass on these deprecations in this PR, let me know, I'll let you do it and add the approval once the pass is done.
Thank you again @carlosgmartin !

@carlosgmartin
Copy link
Contributor Author

@vroulet I see a deprecation notice for

but not for

collections.abc doesn't seem to have the latter two. Is that right? If so, I can edit the first two.

@vroulet
Copy link
Collaborator

vroulet commented Sep 23, 2024

We keep Union as long as we support python 3.9 (we'll drop Union and Optional if we drop 3.9).
For NamedTuple, you're right. The docs https://docs.python.org/3/library/stdtypes.html#types-genericalias are a bit confusing:

For heterogeneous collections of data where access by name is clearer than access by index, collections.namedtuple() may be a more appropriate choice than a simple tuple object.
But we are using NamedTuple as a class, which is not supported by collections.namedtuple. Thanks for checking this.

@carlosgmartin
Copy link
Contributor Author

@vroulet Updated.

Copy link
Collaborator

@vroulet vroulet left a comment

Choose a reason for hiding this comment

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

Thanks for doing this!

@copybara-service copybara-service bot merged commit 9785171 into google-deepmind:main Sep 24, 2024
8 checks passed
@carlosgmartin carlosgmartin deleted the typing_hashable_deprecated branch September 24, 2024 16:31
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.

2 participants