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

[numpy] Update NPY201 to include exception deprecations #12065

Merged
merged 2 commits into from
Jun 27, 2024

Conversation

mtsokol
Copy link
Contributor

@mtsokol mtsokol commented Jun 27, 2024

Hi!

This PR updates NPY201 rule to address #12034 and partially numpy/numpy#26800.

@mtsokol
Copy link
Contributor Author

mtsokol commented Jun 27, 2024

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
Copy link
Member

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

@charliermarsh charliermarsh self-assigned this Jun 27, 2024
@MichaReiser
Copy link
Member

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
Copy link
Member

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

@MichaReiser MichaReiser linked an issue Jun 27, 2024 that may be closed by this pull request
@MichaReiser
Copy link
Member

MichaReiser commented Jun 27, 2024

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
Copy link
Member

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

@MichaReiser
Copy link
Member

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

// We also add a no-op edit to force conflicts with any other fixes that might try to
// remove the import. Consider:
//
// ```python
// import sys
//
// quit()
// ```
//
// Assume you omit this no-op edit. If you run Ruff with `unused-imports` and
// `sys-exit-alias` over this snippet, it will generate two fixes: (1) remove the unused
// `sys` import; and (2) replace `quit()` with `sys.exit()`, under the assumption that `sys`
// is already imported and available.
//
// By adding this no-op edit, we force the `unused-imports` fix to conflict with the
// `sys-exit-alias` fix, and thus will avoid applying both fixes in the same pass.
let import_edit = Edit::range_replacement(
self.locator.slice(imported_name.range()).to_string(),
imported_name.range(),
);

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

@charliermarsh
Copy link
Member

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
Copy link
Member

I'll do it and merge this.

@MichaReiser
Copy link
Member

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

@charliermarsh
Copy link
Member

Too late!

@charliermarsh charliermarsh enabled auto-merge (squash) June 27, 2024 18:54
@MichaReiser
Copy link
Member

Fair enough

@charliermarsh charliermarsh changed the title Update NPY201 rule [numpy] Update NPY201 to include exception deprecations Jun 27, 2024
@charliermarsh charliermarsh merged commit 59ea94c into astral-sh:main Jun 27, 2024
19 checks passed
Copy link

codspeed-hq bot commented Jun 27, 2024

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

⚠️ 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 mtsokol deleted the update-numpy2-rule branch June 28, 2024 07:20
@mtsokol
Copy link
Contributor Author

mtsokol commented Jun 28, 2024

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.

@dhruvmanila dhruvmanila added the rule Implementing or modifying a lint rule label Jul 5, 2024
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 this pull request may close these issues.

Add np.ComplexWarning, etc., to NPY201
4 participants