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

ci: start testing on 3.13-dev #4184

Merged
merged 6 commits into from
May 26, 2024
Merged

ci: start testing on 3.13-dev #4184

merged 6 commits into from
May 26, 2024

Conversation

davidhewitt
Copy link
Member

Now that 3.13 beta 1 is out, we should really look at testing 3.13. Let's start by running in CI and see what happens...

Copy link

codspeed-hq bot commented May 14, 2024

CodSpeed Performance Report

Merging #4184 will not alter performance

Comparing davidhewitt:3.13-dev (5209e6f) with main (7790dab)

Summary

✅ 68 untouched benchmarks

Comment on lines 150 to 152
# FIXME this is a temporary hack for testing 3.13 prereleases, probably we should have a flag
# PYO3_ALLOW_3_13_PRERELEASES or similar so that users don't need to run this flag in their CI
UNSAFE_PYO3_SKIP_VERSION_CHECK: "1"
Copy link
Member Author

Choose a reason for hiding this comment

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

For now I'd like to merge this PR with this flag set, but I think for users to do real testing with 3.13 we shouldn't ask them to set this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is the idea that we'd merge this as is, get the tests passing, and then declare support and remove this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it looks like there are some test cases to fix beyond the FFI changes which I tested locally.

I'm not totally sure what "support" looks like. We don't expect ABI changes any more, but it's possible, and unlike programs using the headers which will compile fine if there's a slight adjustment, we'd have an ABI incompatibility.

Maybe that's fine and we just declare 3.13 support anyway as good enough, or maybe we need an opt-in to 3.13 prereleases and save declaring full 3.13 support until after the release (or the RC).

Copy link

@henryiii henryiii May 15, 2024

Choose a reason for hiding this comment

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

Last beta is always the ABI stable point.

Copy link
Member Author

Choose a reason for hiding this comment

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

I had a quick chat with @hugovk today at PyCon. What I took from that conversation is that, as you say, there should be no ABI breaks, and it's desirable for packages to start uploading PyPI so that downstream code can get testing.

So all in all, the feeling was that we should get on with declaring 3.13 support and the CPython release team are ok with the unlikely possibility that there may be changes which cause breakages of early wheels.

I'll take a look at the test break and see if I can get this PR totally green.

@davidhewitt davidhewitt marked this pull request as ready for review May 15, 2024 11:12
@davidhewitt davidhewitt added the CI-skip-changelog Skip checking changelog entry label May 15, 2024
@henryiii
Copy link

FYI, the manylinux images have a free threading build if you need it.

@davidhewitt
Copy link
Member Author

FYI, the manylinux images have a free threading build if you need it.

Thanks, great to know! I will hopefully be looking into that before long...

@davidhewitt davidhewitt removed the CI-skip-changelog Skip checking changelog entry label May 16, 2024
pytests/noxfile.py Outdated Show resolved Hide resolved
@davidhewitt
Copy link
Member Author

🎉 looks like this is ready! I think there's a lot of FFI cleanup yet to be actioned, but that can happen later as a follow-up ticket. Here I just did enough to get CI green.

@davidhewitt
Copy link
Member Author

Anyone want to give this a review before I click to merge? It would be nice to proceed towards unblocking downstream!

@henryiii
Copy link

henryiii commented May 24, 2024

I notice pyo3-ffi/src/ceval.rs has various functions deprecated by https://peps.python.org/pep-0667/. I think only PyEval_GetBuiltins is used elsewhere, though, so I think it's fine for now (3.13 PyEval_GetLocals was leaking in pybind11). Maybe these should be updated at some point?

$ git grep -E 'PyEval_GetLocals|PyEval_GetGlobals|PyEval_GetBuiltins|\.f_locals'
pyo3-ffi/src/ceval.rs:55:    #[cfg_attr(PyPy, link_name = "PyPyEval_GetBuiltins")]
pyo3-ffi/src/ceval.rs:56:    pub fn PyEval_GetBuiltins() -> *mut PyObject;
pyo3-ffi/src/ceval.rs:57:    #[cfg_attr(PyPy, link_name = "PyPyEval_GetGlobals")]
pyo3-ffi/src/ceval.rs:58:    pub fn PyEval_GetGlobals() -> *mut PyObject;
pyo3-ffi/src/ceval.rs:59:    #[cfg_attr(PyPy, link_name = "PyPyEval_GetLocals")]
pyo3-ffi/src/ceval.rs:60:    pub fn PyEval_GetLocals() -> *mut PyObject;
src/marker.rs:682:                let builtins = ffi::PyEval_GetBuiltins();
src/marker.rs:684:                // `PyDict_SetItem` doesn't take ownership of `builtins`, but `PyEval_GetBuiltins`

Edit: Ah, didn't read the above

I think there's a lot of FFI cleanup yet to be actioned, but that can happen later as a follow-up ticket.

Perfect, never mind then. :)

@davidhewitt davidhewitt added this pull request to the merge queue May 25, 2024
Merged via the queue into PyO3:main with commit 388d176 May 26, 2024
80 checks passed
@davidhewitt davidhewitt deleted the 3.13-dev branch May 26, 2024 00:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants