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

NPY201 triggers on code written to support both numpy 2 and legacy numpy #12476

Closed
jenshnielsen opened this issue Jul 23, 2024 · 6 comments · Fixed by #12490
Closed

NPY201 triggers on code written to support both numpy 2 and legacy numpy #12476

jenshnielsen opened this issue Jul 23, 2024 · 6 comments · Fixed by #12490
Assignees
Labels
rule Implementing or modifying a lint rule

Comments

@jenshnielsen
Copy link

jenshnielsen commented Jul 23, 2024

Considder an example like this which is explicitly written to support both numpy 2 and legacy numpy

import warnings

try:
    from numpy.exceptions import VisibleDeprecationWarning
except ImportError:
    # numpy < 1.25.0
    from numpy import VisibleDeprecationWarning


with warnings.catch_warnings():
    warnings.filterwarnings(
        "ignore",
        category=VisibleDeprecationWarning,
        message="Creating an ndarray from ragged nested sequences",
    )

Running ruff check file.py with NPY201 enabled triggers

error: Failed to create fix for Numpy2Deprecation: Unable to insert `VisibleDeprecationWarning` into scope due to name conflict
src\qcodes\utils\numpy_utils.py:13:18: NPY201 `np.VisibleDeprecationWarning` will be removed in NumPy 2.0. Use `numpy.exceptions.VisibleDeprecationWarning` instead.
   |
11 |     warnings.filterwarnings(
12 |         "ignore",
13 |         category=VisibleDeprecationWarning,
   |                  ^^^^^^^^^^^^^^^^^^^^^^^^^ NPY201
14 |         message="Creating an ndarray from ragged nested sequences",
15 |     )
   |
   = help: Replace with `numpy.exceptions.VisibleDeprecationWarning`

This is a false positive since the deprecated location is only used in legacy numpy versions that do not have a exceptions submodule.

This started happening in 0.5.1 probably as a result of #12065

Keyworkds Numpy, NPY201

Ruff 0.5.1 to 0.5.4

Config:

[tool.ruff.lint]
select = ["NPY201"]
@MichaReiser
Copy link
Member

I can see how the violation is undesired in this case, and we could probably specialize the rule to detect this very specific pattern. But I think it's also valid to use a suppression comment in this case to explicitly state that the use of the deprecated API is fine.

@mtsokol what do you think?

@MichaReiser MichaReiser added the rule Implementing or modifying a lint rule label Jul 23, 2024
@AlexWaygood
Copy link
Member

I think this pattern is likely to be quite common for projects that want to write code which is compatible with both versions of numpy. Maintaining compatibility with both versions is likely to be something that many projects are going to want to do for a while, as well.

I'd be okay with not flagging deprecated numpy imports iff:

  • The deprecated import is imported in an except ImportError block
  • The try block of that except block imports the undeprecated alternative

We should make sure all NPY rules are consistent about this sort of thing, whatever we do, though.

@charliermarsh
Copy link
Member

That makes sense to me. We should have code to detect the current set of handled exceptions.

@AlexWaygood AlexWaygood self-assigned this Jul 23, 2024
@mtsokol
Copy link
Contributor

mtsokol commented Jul 23, 2024

I can see how the violation is undesired in this case, and we could probably specialize the rule to detect this very specific pattern. But I think it's also valid to use a suppression comment in this case to explicitly state that the use of the deprecated API is fine.

@mtsokol what do you think?

@MichaReiser I agree that this try catch pattern could be recognized by the rule, or users should add a suppression comment.

One note: Apart from try except pattern there's also an if else used downstream, like here in SciPy:
https://github.com/scipy/scipy/blob/c354a0482e08ef68bcdd2163f9b394f2c8e9a0a5/scipy/_lib/_util.py#L25

But generally try except should be preferred.

@jenshnielsen
Copy link
Author

Thanks everybody for the input.

@MichaReiser I agree that this try catch pattern could be recognized by the rule, or users should add a suppression comment.

I personally would be fine with just adding a suppression comment as long as this limitation is documented for the rule.

@AlexWaygood
Copy link
Member

I'm working on a fix as per my suggestion in #12476 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rule Implementing or modifying a lint rule
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants