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

Fix annotations #18395

Merged
merged 1 commit into from
Mar 13, 2024
Merged

Conversation

NeilGirdhar
Copy link
Contributor

@NeilGirdhar NeilGirdhar commented Nov 5, 2023

This pull request probably needs some reviewer guidance. It fixes all (but one) of the MyPy errors after the jit-annotation PR. I will comment on various changes in line.

The final type error should be fixed by this PR.

jax/_src/core.py Outdated Show resolved Hide resolved
jax/_src/numpy/lax_numpy.py Outdated Show resolved Hide resolved
jax/_src/numpy/lax_numpy.py Outdated Show resolved Hide resolved
jax/_src/numpy/lax_numpy.py Outdated Show resolved Hide resolved
jax/_src/numpy/lax_numpy.py Outdated Show resolved Hide resolved
jax/_src/numpy/polynomial.py Show resolved Hide resolved
jax/_src/numpy/polynomial.py Outdated Show resolved Hide resolved
jax/_src/state/types.py Show resolved Hide resolved
jax/_src/state/utils.py Outdated Show resolved Hide resolved
jax/_src/util.py Show resolved Hide resolved
jax/_src/numpy/lax_numpy.py Outdated Show resolved Hide resolved
@jakevdp
Copy link
Collaborator

jakevdp commented Nov 6, 2023

Overall comment here: the approach in this PR is one I've tried in other contexts and often ended up having to roll back: namely, you're trying to fix type check errors by changing the runtime behavior of the function. It's very likely that such changes will have unexpected effects in unanticipated corners of downstream JAX use, and so this kind of PR will likely lead to a rollback.

Instead, we should fix type checking errors by fixing type annotations, without changing the runtime logic. That's a much safer change in general.

@NeilGirdhar
Copy link
Contributor Author

NeilGirdhar commented Nov 6, 2023

Instead, we should fix type checking errors by fixing type annotations, without changing the runtime logic. That's a much safer change in general.

Okay, I know you're right. Thanks for being patient with this PR. I'll scan over this change and try to eliminate behavior changes.

@NeilGirdhar NeilGirdhar force-pushed the general_annotations branch 5 times, most recently from 9054002 to 5746e59 Compare November 6, 2023 18:43
@jakevdp
Copy link
Collaborator

jakevdp commented Nov 6, 2023

Also, I think some of this PR does not make sense separate from the jit annotation change, while some of it does make sense to be separate. A lot of the little fiddly changes should probably be landed in concert with the JIT change. It's not worth doing those separately now, because the jit change will probably evolve before we land it, so a different set of fiddly changes will eventually be needed.

Some changes here make sense as stand-alone though: for example adding real type annotations to properties, or adding overloads for atleast_1d, etc. Each of those should be done as a separate PR, mainly because the more changes you lump into a single PR, the higher probability you have of having to roll back the whole thing because one part broke something downstream.

@NeilGirdhar
Copy link
Contributor Author

Makes perfect sense.

Just to let you know that I have tested this PR by rebasing it onto the jit annotation change, making sure it still has no type errors, and then rebasing back onto main.

Do you want to let me know how you'd like this PR split?

@jakevdp jakevdp self-assigned this Nov 8, 2023
@NeilGirdhar NeilGirdhar force-pushed the general_annotations branch 3 times, most recently from 696b2fb to 68ed53d Compare November 10, 2023 08:10
a.py Outdated Show resolved Hide resolved
@NeilGirdhar
Copy link
Contributor Author

@jakevdp FYI this change is on top of #18465, so I made some of your changes there and rebased over it.

@jakevdp
Copy link
Collaborator

jakevdp commented Nov 10, 2023

FYI this change is on top of #18465, so I made some of your changes there and rebased over it.

Sorry, I should have realized that.

@NeilGirdhar NeilGirdhar force-pushed the general_annotations branch 2 times, most recently from 0ea740a to 07119f1 Compare November 16, 2023 20:48
@NeilGirdhar
Copy link
Contributor Author

NeilGirdhar commented Nov 16, 2023

@jakevdp Just checking that we're on the same page. These remaining changes are just what you described as the "fiddly" changes for the annotate Jit PR that you said would eventually change when we go to do that. Sorry for the rebase, I just wanted to make sure that it was clean with Ruff.

Let me know if there's anything you want me to split off into another PR though, and I'm happy to do that.

@jakevdp
Copy link
Collaborator

jakevdp commented Nov 16, 2023

Thanks! I think all this looks pretty reasonable now. We'll need to do from __future__ import annotations in a few places to fix the Python 3.9 errors, and there are a few existing comments to address, but other than that I'm happy to merge this. Thanks so much for working on this!

@NeilGirdhar
Copy link
Contributor Author

@jakevdp Is this okay to merge?

@jakevdp
Copy link
Collaborator

jakevdp commented Mar 13, 2024

Let's see what the tests say 😀

@google-ml-butler google-ml-butler bot added kokoro:force-run pull ready Ready for copybara import and testing labels Mar 13, 2024
@jakevdp
Copy link
Collaborator

jakevdp commented Mar 13, 2024

Test failures are real, it looks like the out_indices change is modifying the logic.

@copybara-service copybara-service bot merged commit 626c7ab into google:main Mar 13, 2024
13 checks passed
@NeilGirdhar NeilGirdhar deleted the general_annotations branch March 13, 2024 23:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pull ready Ready for copybara import and testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants