Skip to content

chore: get PyPy 3.7 wheels using NumPy 1.20 #2837

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

Merged
merged 2 commits into from
Jan 31, 2021
Merged

Conversation

henryiii
Copy link
Collaborator

Description

PyPy 3.7 wheels are included with NumPy 1.20. But not 3.6 since they've dropped 3.6 support.

Suggested changelog entry:

Just tests.

@henryiii
Copy link
Collaborator Author

If we have expected warnings, they need to be wrapped in a pytest warns block.

home/runner/work/pybind11/pybind11/tests/test_builtin_casters.py:302: DeprecationWarning: an integer is required (got type Int).  Implicit conversion to integers using __int__ is deprecated, and may be removed in a future version of Python.
    assert convert(Int()) == 42

test_builtin_casters.py::test_numpy_int_convert
  /home/runner/work/pybind11/pybind11/tests/test_builtin_casters.py:332: DeprecationWarning: an integer is required (got type numpy.float32).  Implicit conversion to integers using __int__ is deprecated, and may be removed in a future version of Python.
    assert convert(np.float32(3.14159)) == 3

anyway, the Intel failure is just an infrastructure failure. The vagrind one seems to be a multi threading failure?

@YannickJadoul
Copy link
Collaborator

If we have expected warnings, they need to be wrapped in a pytest warns block.

The warnings are a reminder that we still need to further clean up int casters. I had them ignored, but basically, they are a consequence of calling PyLong_AsLong (or similar) on a non-PyLong object; Python's warning that that's deprecated. To be fixed in #2802, but that was too big of a change to manage to get into #2801 and v2.6.2. (I had first ignored the errors in #2801, until I understood that they are correctly warned for.)

@henryiii
Copy link
Collaborator Author

If they are in a warns block, I can grep for warns blocks. Maybe add a TODO or FIXME comment. I don't think we should have unhandled warnings in our tests. I originally thought it was an issue with the changes in NumPy 1.20.

@henryiii
Copy link
Collaborator Author

Any idea why (just) the valgrind job is failing with NumPy 1.20? All Pythons 3.7-3.9 are getting 1.20 now.

@bstaletic
Copy link
Collaborator

Any idea why (just) the valgrind job is failing with NumPy 1.20? All Pythons 3.7-3.9 are getting 1.20 now.

Because the suppressions in valgrind-numpy.supp don't match correctly now with the new numpy version. Perhaps we should use looser suppression rules.

@YannickJadoul
Copy link
Collaborator

If they are in a warns block, I can grep for warns blocks. Maybe add a TODO or FIXME comment. I don't think we should have unhandled warnings in our tests. I originally thought it was an issue with the changes in NumPy 1.20.

Yep, but there's no "expected warning" thing, like with expected errors? Also, those warnings have been there for a long time, so I don'tt feel like just shutting them up was the right thing to do.

Though maybe we should pick up the habit of turning warnings into errors, then. I don't know how long these warnings have been there (probably since we started supporting 3.8, because they're from Python >=3.8), but no one notices when builds pass.

@YannickJadoul
Copy link
Collaborator

This ought to fix Valgrind finding leaks in the new version of NumPy. At least locally, it works.

@henryiii
Copy link
Collaborator Author

Copy link
Collaborator Author

@henryiii henryiii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (can't add the green check mark in the app?)

@YannickJadoul
Copy link
Collaborator

https://docs.pytest.org/en/stable/warnings.html#warns ?

That's also an option, ofc. That's not filtering warnings, but asserting for them.

It does say that:

DeprecationWarning and PendingDeprecationWarning are treated differently; see Ensuring code triggers a deprecation warning.

If you want, I can still add that? But again, we've had these warnings for ages, I believe. (All old logs have expired, though; just checked.)

@YannickJadoul
Copy link
Collaborator

If you want, I can still add that? But again, we've had these warnings for ages, I believe. (All old logs have expired, though; just checked.)

Done in #2838. Let's not care about that for this PR?

Copy link
Collaborator

@YannickJadoul YannickJadoul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merge if you want, @henryiii.

@henryiii henryiii merged commit 721834b into master Jan 31, 2021
@henryiii henryiii deleted the henryiii-patch-1 branch January 31, 2021 22:29
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Jan 31, 2021
@henryiii henryiii removed the needs changelog Possibly needs a changelog entry label Jul 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants