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

TYP: fix missing sys import in numeric.pyi #26788

Merged
merged 3 commits into from
Jun 26, 2024

Conversation

Andrej730
Copy link
Contributor

@Andrej730 Andrej730 commented Jun 23, 2024

Fixed missing sys import that's needed after 8de2ed5 for these lines to work:

if sys.version_info >= (3, 10):
from typing import TypeGuard
else:
from typing_extensions import TypeGuard

Perhaps, ping @rgommers

@mattip
Copy link
Member

mattip commented Jun 23, 2024

It would be nice to have this tested somehow so there is a test that fails without the fix, and passes afterwards.

@Andrej730
Copy link
Contributor Author

It would be nice to have this tested somehow so there is a test that fails without the fix, and passes afterwards.

Without this fix stub file had an error "sys" is not defined Pylance reportUndefinedVariable and now it's resolved, shouldn't this cover the need for the test?

Regarding TypeGuard imports - TypeGuard is imported and is working either way even if if sys.version_info >= (3, 10): is broken, so it's just about missing sys error.

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 @Andrej730.

Re testing: I don't think we can do anything about that. We test with Mypy (3 platforms, 3 different Python versions) , so this code is exercised. If different type checkers disagree, there's not much we can do. In this case, PyLance's complaint clearly seems correct; not sure why Mypy is happy with the missing import.

In this case, the problematic if-else can be removed completely though, because 3.10 is the lowest Python version we support now. @Andrej730 could you please do that instead?

@rgommers rgommers changed the title fix missing sys import in numeric.pyi TYP: fix missing sys import in numeric.pyi Jun 23, 2024
@rgommers
Copy link
Member

Or one better, revert all of commit 8de2ed5.

As 3.10 now is the minimum required version for numpy
@Andrej730
Copy link
Contributor Author

Or one better, revert all of commit 8de2ed5.

Removed the sys.version_info >= (3, 10) checks and kept typing_extensions as it still used for from typing_extensions import assert_type in tests and could be useful to resolve #26783.

Re testing: I don't think we can do anything about that. We test with Mypy (3 platforms, 3 different Python versions) , so this code is exercised. If different type checkers disagree, there's not much we can do. In this case, PyLance's complaint clearly seems correct; not sure why Mypy is happy with the missing import.

It seems something is missing - maybe this file is skipped somehow during tests or mypy configured to skip missing symbols?
Just noticed that the other line is also failing (after c8e2343) - "ComplexWarning" is unknown import symbol Pylance reportAttributeAccessIssue. Though this line is just safe to remove as ComplexWarning is not used but maybe there is also some other issue ongoing.

ComplexWarning as ComplexWarning,

@rgommers
Copy link
Member

I have no time to dig into the mystery of why CI doesn't complain right now unfortunately, but I'd be happy to merge this.

Though this line is just safe to remove as ComplexWarning is not used but maybe there is also some other issue ongoing.

Good catch. Would you mind fixing that up as well?

@charris charris added the 09 - Backport-Candidate PRs tagged should be backported label Jun 25, 2024
It's okay to just remove it as it wasn't really used in the code either way.
@Andrej730
Copy link
Contributor Author

Good catch. Would you mind fixing that up as well?

Sure, done.

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.

LGTM, in it goes. Thanks @Andrej730!

@rgommers rgommers merged commit 63dc4c9 into numpy:main Jun 26, 2024
68 checks passed
@rgommers rgommers added this to the 2.1.0 release milestone Jun 26, 2024
charris added a commit to charris/numpy that referenced this pull request Jul 1, 2024
Motivated by numpy#26788, but we cannot employ that fix directly as
NumPy 2.0.x still support Python 3.9.
charris added a commit to charris/numpy that referenced this pull request Jul 1, 2024
Motivated by numpy#26788, but we cannot employ that fix directly as
NumPy 2.0.x still support Python 3.9.
@charris charris removed the 09 - Backport-Candidate PRs tagged should be backported label Jul 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants