astral-sh / ruff

An extremely fast Python linter and code formatter, written in Rust.
https://docs.astral.sh/ruff
MIT License
28.59k stars 926 forks source link

[`numpy`] Update `NPY201` to include exception deprecations #12065

Closed mtsokol closed 3 days ago

mtsokol commented 4 days ago

Hi!

This PR updates NPY201 rule to address https://github.com/astral-sh/ruff/issues/12034 and partially https://github.com/numpy/numpy/issues/26800.

mtsokol commented 4 days ago

I'm having trouble generating new snapshots. The current NPY201.py.snap snapshot works with NPY201.py file up to the line 125. With two last lines removed in NPY201.py the command cargo insta test -p ruff_linter passes.

But when I add another function that is covered by the rule (as right now in the PR):


    np.compare_chararrays

The above test command fails and doesn't generate new snap update file. How can I solve it?

I did the existing changes to the NumPy snapshot file by running the above test command and cargo insta review which worked for the first functions that I added to the .py file. Is there a snapshot file line count limit or some other constraint?

charliermarsh commented 4 days ago

I'll take a look. We might be hitting the limit we set on number of iterations during testing (this is just a safeguard).

MichaReiser commented 3 days ago

It seems that the fixes never converge

Failed to converge after 10 iterations. This likely indicates a bug in the implementation of the fix. Last diagnostics:
NPY201.py:15:5: NPY201 `np.add_newdoc_ufunc` will be removed in NumPy 2.0. `add_newdoc_ufunc` is an internal function.
   |
13 |     add_newdoc
14 | 
15 |     np.add_newdoc_ufunc
   |     ^^^^^^^^^^^^^^^^^^^ NPY201
16 | 
17 |     np.asfarray([1,2,3])
charliermarsh commented 3 days ago

I think more likely is that it needs more than 10 iterations due to conflicts.

MichaReiser commented 3 days ago

It seems we're only replacing one ExprName at a time...

Probably because they all overlap because each of them tries to add the numpy import. Although it's unclear why we still try to add the numpy import when it already exists.

charliermarsh commented 3 days ago

They probably all touch the numpy import to ensure that it doesn't get removed by another rule.

MichaReiser commented 3 days ago

Yeah, the cullprint is this. Which works as intended for removals but also means that it only applies one fix at the time.

https://github.com/astral-sh/ruff/blob/bf5b62edaccdc007dfcf559ab9e870be66c19877/crates/ruff_linter/src/importer/mod.rs#L304-L324

An easy workaround could be to split the test into two files

charliermarsh commented 3 days ago

Yeah, or bump the limit a little bit. Either is ok with me. (We probably want a smarter strategy for this in the future.)

charliermarsh commented 3 days ago

I'll do it and merge this.

MichaReiser commented 3 days ago

I'm leaning toward splitting the tests. Many iterations can make it more difficult to debug real issues.

charliermarsh commented 3 days ago

Too late!

MichaReiser commented 3 days ago

Fair enough

codspeed-hq[bot] commented 3 days ago

CodSpeed Performance Report

Merging #12065 will degrade performances by 5.15%

Comparing mtsokol:update-numpy2-rule (8726e6e) with mtsokol:update-numpy2-rule (38ca9b8)

Summary

❌ 1 regressions ✅ 29 untouched benchmarks

:warning: Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark mtsokol:update-numpy2-rule mtsokol:update-numpy2-rule Change
linter/all-rules[unicode/pypinyin.py] 2.1 ms 2.3 ms -5.15%
mtsokol commented 3 days ago

Thank you for solving it! One comment - the python file test is still missing these new entries added to the rule:

np.DTypePromotionError
np.ModuleDeprecationWarning
np.RankWarning
np.TooHardError
np.VisibleDeprecationWarning
np.chararray
np.format_parser

I didn't add them yesterday to reproduce the issue in a minimal configuration. We can still add them or leave the test as it's right now - both options are fine with me.