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

ENH: Add a mypy plugin for inferring platform-specific np.number precisions #17843

Merged
merged 13 commits into from Dec 22, 2020

Conversation

BvB93
Copy link
Member

@BvB93 BvB93 commented Nov 25, 2020

Alternative to #17514.

This PR adds a mypy plugin that automatically assigns a platform-specific precision
to certain np.number subclasses (or aliases thereof) while static type checking.
Most notably this includes np.int_, which is always equivalent to np.int32 on windows
but np.int64 on most (all?) other 64-bit platforms.

While #17514 attempts to accomplish this via a dynamically generated .pyi file,
this PR instead uses a mypy plugin. Note that while usage of the plugin is completelly
optional, without it the precision of the likes of np.int_ will be inferred as Any.

@BvB93
Copy link
Member Author

BvB93 commented Nov 25, 2020

Hm, anyone knows why only PyPy cannot seem to find the new numpy.typing.mypy_plugin module (test)?

EDIT: Found the issue (f21c8f6).

numpy/typing/mypy_plugin.py Outdated Show resolved Hide resolved
@rgommers
Copy link
Member

Interesting, tricky puzzle to get the details of this typing right. I'm not sure if one approach is clearly preferable to the other.

The mypy plugin may possibly be slightly more robust to situations like cross-compiling (e.g. there's an open PR for ARM64 build on Windows, which may get the intp size wrong). However those are pretty niche cases that likely won't matter at all in practice.

On the other hand, the plugin approach will require the user to add it in their own mypy.ini as well as have some runtime overhead (any idea how much?).

@BvB93
Copy link
Member Author

BvB93 commented Nov 30, 2020

On the other hand, the plugin approach will require the user to add it in their own mypy.ini as well as have some runtime overhead (any idea how much?).

The runtime overhead should still be zero, as the new numpy.typing.mypy_plugin module should generally only be imported by mypy itself (so during static type checking). There are a couple of contingencies inside the module to ensure that it can always be imported during runtime, but this is moreso because the numpy tests require it to be so (regardless of what optional dependencies are present or not).

@rgommers
Copy link
Member

The runtime overhead should still be zero, as the new numpy.typing.mypy_plugin module should generally only be imported by mypy itself (so during static type checking).

Yeah I get that - I meant runtime of mypy (which can be slow).

@BvB93
Copy link
Member Author

BvB93 commented Nov 30, 2020

Yeah I get that - I meant runtime of mypy (which can be slow).

Ah right, when running mypy on the script below I'm observing a minor increase in runtime when initially generating the mypy cache (~7.4 vs ~7.8 sec). Once the cache has been generated the difference disappears in its entirety though (~0.4 sec in both cases).

import numpy as np

@BvB93 BvB93 added the 56 - Needs Release Note. Needs an entry in doc/release/upcoming_changes label Dec 9, 2020
@charris
Copy link
Member

charris commented Dec 11, 2020

Needs rebase.

@BvB93
Copy link
Member Author

BvB93 commented Dec 11, 2020

Needs rebase.

Yup, I'm planning on adding the release note + rebase later this evening.

@BvB93 BvB93 removed the 56 - Needs Release Note. Needs an entry in doc/release/upcoming_changes label Dec 11, 2020
@BvB93
Copy link
Member Author

BvB93 commented Dec 11, 2020

Alright, the branch has been rebased and a release note (5c7c22a) has been added.

@rgommers
Copy link
Member

This accumulated another merge conflict unfortunately.

@BvB93
Copy link
Member Author

BvB93 commented Dec 22, 2020

Give me a sec.

@BvB93
Copy link
Member Author

BvB93 commented Dec 22, 2020

And the conflict has been resolved.

Copy link
Member

@rgommers rgommers left a comment

Choose a reason for hiding this comment

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

Thanks @BvB93, in it goes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants