-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
fix: improve support for Python 3.11-dev #3368
Conversation
36a7f67
to
061751d
Compare
https://bugs.python.org/issue44032, specifically python/cpython#26771 is what broke us in #3367. Looks like the replacement is |
e7aee30
to
792bd79
Compare
PyTest doesn't support 3.11 yet (see pypa/build#376), so 3.11 is broken. I've fixed the API issues, though. Feel free to suggest cleanups if there are nicer ways to do things now, I focused on replicating the current design using 3.9+ API. |
For the latest python 3.11, we are broken due to #3374. For 3.11a1, we get a segfault halfway through the tests. :/ I think we should move the 3.11 tests to a job that runs on a schedule or if a label is set, it will likely randomly break on Python updates and that will interfere with normal PRs. |
Do we need to jump in that early? Maybe starting with local tests for the alphas, and bring up the entire CI only for b2 or b3 will cover all practical needs? |
The earlier we start, the smaller the changelog between each failure, and we can find what changed more easily, and it's much easier to fix one issue at a time, instead of being hit with a dozen changes. And if something really breaks us, we can provide timely feedback. Basically the best parts of Google's live-at-HEAD philosophy. ;) But I think we agree here; I don't want to add this to our test matrix yet (ignore what you see in the current state of the PR), I want to make a separate workflow that just checks 3.11 and runs on a schedule or by request, and not on normal PRs. I don't want to do this locally, because I have no interest in manually building 3.11 or running this by hand every so often, that's what CI is for. |
Sounds great. I'm actually curious to see how you're getting Python. Pre-built, or do we have the ability to build from sources in the CI? (Not an actual question, I'll just look what you do.) |
GitHub Actions provides the alphas. And deadsnakes seems to provide a more often updated dev build. |
We still get core dumps on CPytohn 3.11. It's not really necessary to merge, GHA always runs on the merged version. I can redo this to just run the 3.11 on a special job (though maybe not today). |
Also update 3.10 to final, better PyPy usage
5e519e7
to
4c5c4cc
Compare
6feeb22
to
4866433
Compare
c8e7835
to
b04f4cd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Henry, that looks like a lot of work!
I didn't review the ci.yml and requirements.txt changes in detail.
@@ -2335,6 +2335,28 @@ inline function get_type_override(const void *this_ptr, const type_info *this_ty | |||
/* Don't call dispatch code if invoked from overridden function. | |||
Unfortunately this doesn't work on PyPy. */ | |||
#if !defined(PYPY_VERSION) | |||
|
|||
#if PY_VERSION_HEX >= 0x03090000 | |||
PyFrameObject *frame = PyThreadState_GetFrame(PyThreadState_Get()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a really long and complex block of code. It might be nice to move to a separate function somewhere under detail.
This PR is very large already, we could do that separately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, can be done separately later.
tests/CMakeLists.txt
Outdated
set(PYBIND11_EIGEN_VERSION | ||
"929bc0e191d0927b1735b9a1ddc0e8b77e3a25ec" | ||
CACHE STRING "Eigen version to use for tests") | ||
# Pretty print the version (keep in sync) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we move this up and in all caps?
### PYBIND11_EIGEN_VERSION and PYBIND11_EIGEN_VERSION_STRING MUST BE KEPT IN SYNC.
Alternatively, does cmake have something like Python tuples?
set(PYBIND11_EIGEN_VERSION_AND_HASH
("3.4.0", "929bc0e191d0927b1735b9a1ddc0e8b77e3a25ec")
CACHE STRING "Eigen version to use for tests")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Used a list.
202b605
to
7f1fa8c
Compare
Also update 3.10 to final, better PyPy usage
Fixes #3367. However, 3.11 segfaults, so this doesn't work yet, and we shouldn't be running 3.11 in the normal workflow anyway, but in some separate workflow that's not tied to general PRs.
Description
Suggested changelog entry:
Also slims down our PyPy numpy/scipy testing to official wheels only.
Also correctly displays the Eigen version when downloaded automatically.