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

CI,TYP: Bump mypy to 1.4.1 #23764

Merged
merged 7 commits into from
Aug 25, 2023
Merged

CI,TYP: Bump mypy to 1.4.1 #23764

merged 7 commits into from
Aug 25, 2023

Conversation

BvB93
Copy link
Member

@BvB93 BvB93 commented May 15, 2023

This PR bumps the mypy version used throughout the test suite from 0.981 to 1.3.0 (it's been a while...).

In addition to mypy now having implemented support for typing.Concatenate there seems been some changes in how it deals with the joining of super- and sub-classes. The end result as that, e.g. the likes of np.float64 + np.float32 were previously interpretted as producing np.floating[_64Bit] while not it results in a np.floating[_64Bit | _32Bit] union.

While non-ideal, this upstream change is admitetly a minor inconvenience in the grand scheme of things, mostly because the typing of dtype bitsizes in numpy is severely lacking due to the sheer number of challenges associated with it.

Lastly, while not implemented in this PR there's also typing.Self support ever since mypy 1.0.0. This type is essentially a bit of syntactic sugar to make typing involving self and cls a bit easier, removing their need for hyper-specialized typevars. Something for a future PR perhaps.

@BvB93 BvB93 added this to the 1.25.0 release milestone May 15, 2023
@BvB93
Copy link
Member Author

BvB93 commented May 15, 2023

>               pytest.fail(f"Unexpected mypy standard error\n\n{stderr}")
E               Failed: Unexpected mypy standard error
E               
E               mypy: "../../venv-for-wheel/lib/python3.9/site-packages/mypy_extensions.py" shadows library module "mypy_extensions"
E               note: A user-defined top-level module with name "mypy_extensions" is not supported

Some more debugging for tomorrow. The error messages differ a bit from platform to platform, though this seems to suggest that a mypy from outside venv-for-wheel/lib/python3.9/... is clashing with a mypy_extensions installation inside the venv environment. Thus far I've been unable to reproduce the issue locally though (be it either in mac or windows) with a simple python .\runtests.py -t numpy/typing --mode=full.

Any thoughts?

@seberg
Copy link
Member

seberg commented May 17, 2023

A quick google search finds this: https://stackoverflow.com/questions/64806361/installing-mypy-leads-to-shadows-library-module-error
EDIT: Also this doc update to mypy python/mypy@4825e34

The travis-CI run sets the MYPYPATH to the python path, which is strongly discouraged (and I guess mypy broke it entirely?). Sounds like fixing MYPYPATH to just include numpy should do the trick?

@seberg
Copy link
Member

seberg commented May 17, 2023

Hmmm, fun... there is another issue now about C and D drive in some runs, and the error didn't actual go away in others? So probably that was at least not the core of the issue, or any chance of a caching problem?

@BvB93
Copy link
Member Author

BvB93 commented May 17, 2023

It did solve the Travis failure though; and the fact that all other runs that had previously failed do so again do make me suspect that there's more going on here than a simple caching problem.

@seberg
Copy link
Member

seberg commented May 17, 2023

Hmmm, some of the failures seem to be about not installing typing_extensions? It still shows up in test_requirements.txt and the cirrus setup?

@BvB93
Copy link
Member Author

BvB93 commented May 17, 2023

Hm, not sure about typing_extensions lacking. Pretty much all the errors mention either a C/D path mismatch or, more directly, that either the mypy_extensions or typing_extension in the testing venv is shadowing wherever the package was installed originally.

@BvB93
Copy link
Member Author

BvB93 commented May 17, 2023

Think I may have found something here; seems the problem might be that it is directly run against something in site-packages: python/mypy#11477. It's annoying that this has now become an issue all of a sudden, though I wonder if we could run mypy directly against the numpy source? The .pyi files aren't affected by the build stage after all.

@charris charris added the 09 - Backport-Candidate PRs tagged should be backported label May 22, 2023
@charris charris removed this from the 1.25.0 release milestone May 22, 2023
@BvB93 BvB93 marked this pull request as draft June 6, 2023 10:48
@BvB93
Copy link
Member Author

BvB93 commented Jun 6, 2023

Marking as draft for now until I have time to take a closer look.

@rgommers
Copy link
Member

rgommers commented Aug 7, 2023

It's annoying that this has now become an issue all of a sudden, though I wonder if we could run mypy directly against the numpy source? The .pyi files aren't affected by the build stage after all.

It should sorta work I think; the exceptions like generated .py/.pyi files are corner cases where there may be issues. The tests under numpy/typing/ import numpy though, so they'll work with editable installs and spin test, but not with a regular installed package.

That Mypy issue is a bit sad of course; they can/should just fix the thing where typing_extensions being installed breaks things for them.

@rgommers
Copy link
Member

I tried to add a Mypy run with Python 3.12.0rc1 (as part of gh-24291), but it chokes on:

 ERROR numpy/typing/tests/test_typing.py - DeprecationWarning: ast.Bytes is deprecated and will be removed in Python 3.14; use ast.Constant instead

I suspect that this Mypy upgrade will fix that, so I haven't bothered to look at it in more detail.

@BvB93 BvB93 changed the title CI,TYP: Bump mypy to 1.3.0 CI,TYP: Bump mypy to 1.4.1 Aug 23, 2023
@BvB93 BvB93 marked this pull request as ready for review August 23, 2023 18:35
@BvB93
Copy link
Member Author

BvB93 commented Aug 23, 2023

Right, have yet to get mypy 1.5.1 to cooperate, but 1.4.1 now works making this PR good to go.

Mypy 1.5.1 seems to truncate its type evaluation before all fail-related tests are completed, which causes issue. I suspect that this is related to the new "Consistently Avoid Type Checking Unreachable Code" improvements (1.5.1 changelog); hopefully this is something that can be fixed by implementing #21505 via useage of typing.assert_type and .assert_never.

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.

This looks great, thanks @BvB93. After going through the changes, the one thing that stood out to me was the "Union of scalars" thing which looks like a regression. But then I saw that you already explained it:

In addition to mypy now having implemented support for typing.Concatenate there seems been some changes in how it deals with the joining of super- and sub-classes. The end result as that, e.g. the likes of np.float64 + np.float32 were previously interpreted as producing np.floating[_64Bit] while not it results in a np.floating[_64Bit | _32Bit] union.

While non-ideal, this upstream change is admitetly a minor inconvenience in the grand scheme of things, mostly because the typing of dtype bitsizes in numpy is severely lacking due to the sheer number of challenges associated with it.

Fair enough, I agree it's a minor thing and we can simply ignore it for now.

I'll put back the typing_extensions requirement in environment.yml and then this should be good to go.

…onment.yml

It's still needed, so keep it in the environment. The added import
guards here match the ones already present in two other `.pyi` files.

[skip cirrus] [skip circle] [skip azp] [skip travis]
@rgommers rgommers added this to the 2.0.0 release milestone Aug 24, 2023
@rgommers
Copy link
Member

I found another small issue with the use of typing_extensions - pushed a fix.

@BvB93
Copy link
Member Author

BvB93 commented Aug 24, 2023

I found another small issue with the use of typing_extensions - pushed a fix.

Thanks Ralf. Considering typing_extensions isn't used during runtime anymore (only while static type checking) I'm not 100% sure whether it's worthwhile to even keep it around, as we're sort-of-but-not-fully managing a mypy dependency (the two are very tightly intertwined as far as lower version bounds are concerned).

In any case, keeping it for now can't hurt either way.

@rgommers
Copy link
Member

We're almost at the end of the support window for Python 3.9; once we drop it, the last usages of typing_extensions will be automatically cleaned up. So let's just leave it alone until then?

@rgommers rgommers merged commit 4b849d4 into numpy:main Aug 25, 2023
41 checks passed
@rgommers
Copy link
Member

Okay, in it goes. Thanks Bas!

@charris charris removed the 09 - Backport-Candidate PRs tagged should be backported label Aug 25, 2023
@charris charris removed this from the 2.0.0 release milestone Aug 25, 2023
@BvB93 BvB93 deleted the mypy branch September 6, 2023 09:35
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.

4 participants