Introduce get_python_state_dict() for Python 3.12 compatibility.

[rwgk]

Description

Based on PR #4293 but without ABI break.

Following a discussion under wjakob/nanobind#96: to ensure compatibility with future Python versions, pybind11 needs to stop stashing its internals data in the global builtins dictionary.

To maximize backward compatibility, this PR preserves the status quo for PYBIND11_INTERNALS_VERSION 4 and Python versions <= 3.11. There is no benefit to changing anything for existing Python versions as long as PYBIND11_INTERNALS_VERSION 4 is used, except maybe for a very minor reduction in code complexity (a simpler preprocessor #if condition in get_python_state_dict()). That seems like an extremely small price to pay for maintaining backward compatibility.

Compatible with smart_holder (PR #4571) and pywrapcc (PR google/pybind11clif#30007).

Tested with Python 3.12 via pywrapcc.

Suggested changelog entry:

Compatibility with Python 3.12 (alpha). Note that the minimum pybind11 ABI version for Python 3.12 is version 5. (The default ABI version for Python versions up to and including 3.11 is still version 4.)

[Skylion007]

Hmm, thoughts on this @henryiii?

[Skylion007]

Hmm, I wonder if we can do some handling of if this is called when the python interpreter is closed to improve/fix #4505

[rwgk]

I think (but could be wrong) that

• finalize_interpreter()-related problems (like Fix #4459 (cpp atexit callbacks) without segfault (#4500) #4505 and related PRs)
• and the builtins vs state_dict problem solved in this PR

are different & independent for the most part.

(It bugs me a lot that we don't have a solid tear-down solution in finalize_interpreter(), but unfortunately that's not something I can devote my time to anytime soon.)

[Skylion007]

Can we enable the CI for Python 3.12 yet, btw?

[rwgk]

Can we enable the CI for Python 3.12 yet, btw?

Yes, although not from sources. Each time there is a new alpha (or later beta) release we have to wait for a few days until actions/setup-python@v4 pulls it in and we can test with it.

Currently I'm working with https://github.com/google/pywrapcc/blob/92741e75f85829d13ed18fbfa0fab491eeaa2896/.github/workflows/python312dev.yml

@henryiii didn't support that I wanted a separate yml for each pre-release Python version, and that I keep track of intermediate states in comments. I find it too difficult (and not very obvious) to repurpose upstream.yml, and believe that trying to keep something so fluid by nature pure is only slowing down progress, therefore I transferred the 3.12 testing to pywrapcc. If someone wants to jump in to maintain 3.12 testing here differently that would be great.

[henryiii]

Why not just add it to our regular testing with a pinned version? We'd have to remember to regularly update it, but it would be stable.

The only thing I disagreed with was renaming the workflow every year. There will always be a dev version so we can have a dev workflow.

Currently we add a tag "dev" to a PR to enable the dev workflow.

[rwgk]

Thanks @Skylion007!
I'll hold off merging until @henryiii has made the 2.10.4 release. (Not sure if merging now will make it more difficult to make the release.)

[rwgk]

Argh... I typed in the previous comment in the wrong window.

[EthanSteinberg]

Thanks for fixing this. It looks good, with one suggestion that you can make the ifdefs easier by bumping the abi version when 3.12 is detected

[EthanSteinberg]

I think it might be simpler if you just had a define that you are always using internals version 5 if the python version is 3.12

[rwgk]

I added this commit: a version bump for Python 3.12+ (I think of it as a free lunch version bump):

 #ifndef PYBIND11_INTERNALS_VERSION
-#    define PYBIND11_INTERNALS_VERSION 4
+#    if PY_VERSION_HEX >= 0x030C0000
+// Version bump for Python 3.12+, before first 3.12 beta release.
+#        define PYBIND11_INTERNALS_VERSION 5
+#    else
+#        define PYBIND11_INTERNALS_VERSION 4
+#    endif
 #endif

Then I looked long and hard at the condition here but eventually decided to leave it at a no-op shuffle, solely to increase readability slightly (see e8b34b4).

it might be simpler

Did you have in mind removing the PY_VERSION_HEX < 0x030C0000 sub-condition here?

What worries me: if someone defines PYBIND11_INTERNALS_VERSION 4 externally, we'd select the wrong code here if the sub-condition is missing. We don't know yet what the resulting errors might be, if any, for future Python releases.

I guess we could do something like this to guard the PyEval_GetBuiltins() path:

#if PY_VERSION_HEX >= 0x030C0000
#error Python 3.12+ requires PYBIND11_INTERNALS_VERSION >= 5
#endif

But that's strictly speaking an arbitrary forcing function. Maybe someone will have a good reason to experiment with PYBIND11_INTERNALS_VERSION 4 even for 3.12+? E.g. when debugging some issue with the TLS-related changes connected to version 5. This is totally made up of course, but I'm generally trying to stay clear of non-essential forcing functions (anecdotes of getting burned omitted).

Could you please re-approve if you think the latest version here is fine, or let me know if I misunderstood what you had in mind?

BTW: I re-tested with Python 3.12.0a6 under google/pybind11clif#30019 (https://github.com/google/pywrapcc/actions/runs/4526056859/jobs/7970965421?pr=30019).

[EthanSteinberg]

But that's strictly speaking an arbitrary forcing function.

Yeah, it's an "arbitrary forcing function", but it reduces the matrix of Python version / pybind11 version abi combinations you have to support. I don't understand why you would want version abi version 4 and python 3.12 together, but if you do, feel free to disregard this suggestion because I was assuming that combination was sorta worthless (since you can't get abi version 4 and python 3.11 code to work together anyways).

Your code in mind is exactly what I had in mind, but here are a couple of suggestions.

#if PY_VERSION_HEX < 0x03080000                                                                   \
    || (PYBIND11_INTERNALS_VERSION <= 4 && PY_VERSION_HEX < 0x030C0000) || defined(PYPY_VERSION)
    state_dict = reinterpret_borrow<object>(PyEval_GetBuiltins());
#else

Why isn't this just

#if PYBIND11_INTERNALS_VERSION <= 4
    state_dict = reinterpret_borrow<object>(PyEval_GetBuiltins());
#else

I guess we could do something like this to guard the PyEval_GetBuiltins() path:

I would add this to the very top where versions are defined

#ifndef PYBIND11_INTERNALS_VERSION
#    if PY_VERSION_HEX >= 0x030C0000
// Version bump for Python 3.12+, before first 3.12 beta release.
#        define PYBIND11_INTERNALS_VERSION 5
#    else
#        define PYBIND11_INTERNALS_VERSION 4
#    endif
#endif

// Require at least version 5 ABI for Python 3.12+
static_assert(!(PY_VERSION_HEX == 0x030C0000) || (PYBIND11_INTERNALS_VERSION >= 5);

[rwgk]

Why isn't this just

#if PYBIND11_INTERNALS_VERSION <= 4

We need to keep the < Python 3.8 condition because _PyInterpreterState_Get() only appeared with Python 3.8 (I looked in the 3.6 and 3.7 sources yesterday).

I'm trying the simpler condition right now. I'm not sure what we need to do for PyPy.

I'm on board enforcing ABI version 5 for 3.12+ (see latest code here), but just to make sure I'm not missing something:

(since you can't get abi version 4 and python 3.11 code to work together anyways)

Did you mean to write 3.12 here? — We are testing with 3.11, e.g.:

============================= test session starts ==============================
platform linux -- Python 3.11.2, pytest-7.0.0, pluggy-1.0.0
C++ Info: 9.4.0 C++11 __pybind11_internals_v4_gcc_libstdcpp_cxxabi1013__ PYBIND11_SIMPLE_GIL_MANAGEMENT=True

Currently ABI 4 & 3.12 still work (from pywrapcc GHA):

============================= test session starts ==============================
platform linux -- Python 3.12.0a6, pytest-7.0.0, pluggy-1.0.0
C++ Info: 11.3.0 C++11 __pybind11_internals_v4_gcc_libstdcpp_cxxabi1016_sh_def__ PYBIND11_SIMPLE_GIL_MANAGEMENT=False

[EthanSteinberg]

(since you can't get abi version 4 and python 3.11 code to work together anyways)

Oops, typo. Yep, Meant that you can't get abi version 4 Python 3.12 code and abi version 4 Python 3.11 code to work together

I'm not sure what we need to do for PyPy.

PyPy should be following the CPython API, so we should be able to use identical code for both it and CPython. Unless you know otherwise of course.

We need to keep the < Python 3.8 condition because _PyInterpreterState_Get() only appeared with Python 3.8 (I looked in the 3.6 and 3.7 sources yesterday).

I would add a wrapper for _PyInterpreterState_Get to pybind11/detail/common.h with #ifdefs to switch back and forth between the _ and regular version depending on the Python version.

[rwgk]

PyPy should be following the CPython API, so we should be able to use identical code for both it and CPython.

Unfortunately not. PyPy with 3.8 and 3.9 still don't work (our GHA). I just looked in the latest PyPy sources: PyInterpreterState_Get still doesn't appear there. I tried to use PY_VERSION_HEX but apparently that's not meaningful with PyPy. It's unclear to me at the moment if/how we can do better than just testing for PYPY_VERSION. Leaving that for later. While looking around I saw that there are 7.3.10 and 7.3.11 releases which we don't have in our GHA. Something to do separately maybe. (I misunderstood: we're actually testing with 7.3.11: pypy-3.8 & pypy-3.9 in our GHA.)

I would add a wrapper for _PyInterpreterState_Get to pybind11/detail/common.h with #ifdefs to switch back and forth between the _ and regular version depending on the Python version.

This get_python_state_dict() is essentially that already. I don't feel factoring out the branch for >= 3.9 will be helpful, especially given the PyPy complication.

[EthanSteinberg]

Arg, another unfortunate PyPy incompatibility. I guess this is the best we can do.

[EthanSteinberg]

@rwgk Created a PyPy issue for this: https://foss.heptapod.net/pypy/pypy/-/issues/3909

[rwgk]

I retested with Python 3.12a6 under google/pybind11clif#30019: all good there, too.

Thanks @lalaland and @Skylion007 for the reviews!