Skip to content

fix: allow subinterpreters to be manually disabled #5708

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.

Sign up for GitHub

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
May 31, 2025
Merged

fix: allow subinterpreters to be manually disabled #5708

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
81fe3f2
fix: allow subinterp support to be disabled
henryiii May 31, 2025
ba78f4c
ci: drop main tests on main
henryiii May 31, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 7 additions & 12 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,6 @@ on:
- synchronize
- reopened
- ready_for_review
push:
branches:
- master
- stable
- v*

permissions: read-all

Expand Down Expand Up @@ -92,7 +87,7 @@ jobs:
cmake-args: -DCMAKE_CXX_STANDARD=20 -DPYBIND11_DISABLE_HANDLE_TYPE_NAME_DEFAULT_IMPLEMENTATION=ON
- runs-on: ubuntu-latest
python-version: '3.14'
cmake-args: -DCMAKE_CXX_STANDARD=14
cmake-args: -DCMAKE_CXX_STANDARD=14 -DCMAKE_CXX_FLAGS="-DPYBIND11_HAS_SUBINTERPRETER_SUPPORT=0"
- runs-on: ubuntu-latest
python-version: 'pypy-3.10'
cmake-args: -DCMAKE_CXX_STANDARD=14
Expand Down Expand Up @@ -175,22 +170,22 @@ jobs:
matrix:
include:
- runs-on: ubuntu-latest
python: '3.9'
python-version: '3.9'
- runs-on: macos-latest
python: '3.12'
python-version: '3.12'
- runs-on: windows-latest
python: '3.11'
python-version: '3.11'

name: "🐍 ${{ matrix.python }} • ${{ matrix.runs-on }} • x64 inplace C++14"
name: "🐍 ${{ matrix.python-version }} • ${{ matrix.runs-on }} • x64 inplace C++14"
runs-on: ${{ matrix.runs-on }}

steps:
- uses: actions/checkout@v4

- name: Setup Python ${{ matrix.python }}
- name: Setup Python ${{ matrix.python-version }}
uses: actions/setup-python@v5
with:
python-version: ${{ matrix.python }}
python-version: ${{ matrix.python-version }}
allow-prereleases: true

- name: Install uv
Expand Down
10 changes: 7 additions & 3 deletions include/pybind11/detail/common.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,9 +256,13 @@

// Slightly faster code paths are available when PYBIND11_HAS_SUBINTERPRETER_SUPPORT is *not*
// defined, so avoid defining it for implementations that do not support subinterpreters. However,
// defining it unnecessarily is not expected to break anything.
#if PY_VERSION_HEX >= 0x030C0000 && !defined(PYPY_VERSION) && !defined(GRAALVM_PYTHON)
# define PYBIND11_HAS_SUBINTERPRETER_SUPPORT
// defining it unnecessarily is not expected to break anything (other than old iOS targets).
#ifndef PYBIND11_HAS_SUBINTERPRETER_SUPPORT
# if PY_VERSION_HEX >= 0x030C0000 && !defined(PYPY_VERSION) && !defined(GRAALVM_PYTHON)
# define PYBIND11_HAS_SUBINTERPRETER_SUPPORT 1
# else
# define PYBIND11_HAS_SUBINTERPRETER_SUPPORT 0
# endif
Comment on lines +261 to +265
Copy link
Contributor

Choose a reason for hiding this comment

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

Changing defined/undefined to 1/0 is a backward-incompatible change, which breaks preprocessor #ifdef PYBIND11_HAS_SUBINTERPRETER_SUPPORT.

undefined:         false
defined but empty: true
defined as 1:      true
defined as 0:      false

See also:

/* Define to 1 when compiling for experimental free-threaded builds */
#ifdef Py_GIL_DISABLED
/* We undefine if it was set to zero because all later checks are #ifdef.
 * Note that non-Windows builds do not do this, and so every effort should
 * be made to avoid defining the variable at all when not desired. However,
 * sysconfig.get_config_var always returns a 1 or a 0, and so it seems likely
 * that a build backend will define it with the value.
 */
#    if Py_GIL_DISABLED == 0
#        undef Py_GIL_DISABLED
#    endif
#endif

Copy link
Collaborator

Choose a reason for hiding this comment

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

Changing defined/undefined to 1/0 is a backward-incompatible change

I was aware of that when I reviewed this PR. My judgement: That's OK/best in this case, because that define was added only very recently, and was included only in release candidates.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Release candidates exist exactly so that we can make last minute backward in compatible changes to new features. :)

Copy link
Collaborator Author

@henryiii henryiii Jun 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Though, honestly, I hadn't thought of that way of doing it, and if you think it's more consistent, I wouldn't be opposed to it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense to undefine the macro if it equals 0?

Like CPython does for no-gil on Windows:

/* Define to 1 when compiling for experimental free-threaded builds */
#ifdef Py_GIL_DISABLED
/* We undefine if it was set to zero because all later checks are #ifdef.
 * Note that non-Windows builds do not do this, and so every effort should
 * be made to avoid defining the variable at all when not desired. However,
 * sysconfig.get_config_var always returns a 1 or a 0, and so it seems likely
 * that a build backend will define it with the value.
 */
#    if Py_GIL_DISABLED == 0
#        undef Py_GIL_DISABLED
#    endif
#endif

Copy link
Collaborator

Choose a reason for hiding this comment

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

Every other define in pybind11 does work via ifdef, though...

Ah ... I remembered that wrong and didn't double-check.

PYBIND11_HAS_STD_LAUNDER is a special beast, I wouldn't use that as a role model.

My vote:

  • Remove #else #define PYBIND11_HAS_SUBINTERPRETER_SUPPORT 0.
  • Let's say sorry for the back and forth between release candidates.
  • In the (much) long(er) run people will be thankful.

Copy link
Contributor

@XuehaiPan XuehaiPan Jun 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Remove #else #define PYBIND11_HAS_SUBINTERPRETER_SUPPORT 0.

We may need to undefine the macro if it is defined as 0 before the first usage (see #5708 (comment)). The macro might be defined via the C flags, like what this PR did:

-DPYBIND11_HAS_SUBINTERPRETER_SUPPORT=0

Since the PYBIND11_HAS_SUBINTERPRETER_SUPPORT macro is a new one and has not been officially released yet, I'm fine with whether it is controlled by presence or non-zeroness.

I workaround this with a macro (https://github.com/metaopt/optree/pull/227/files#diff-b552e0451de79baea976f15c4ee5458fa7b6faa75bebb8b3780a90ec3d40a427R56):

// NOLINTNEXTLINE[bugprone-macro-parentheses]
#define NONZERO_OR_EMPTY(MACRO) ((MACRO + 0 != 0) || (0 - MACRO - 1 >= 0))

#if defined(HAS_XXX) && NONZERO_OR_EMPTY(HAS_XXX)
    // do something
#else
    // do something else
#endif

#undef NONZERO_OR_EMPTY

Copy link
Collaborator

Choose a reason for hiding this comment

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

Applying the "keep it simple" and "be consistent" principles here:

I think this is all we need:

#ifndef PYBIND11_HAS_SUBINTERPRETER_SUPPORT                                     
#    if PY_VERSION_HEX >= 0x030C0000 && !defined(PYPY_VERSION) && !defined(GRAALVM_PYTHON)
#        define PYBIND11_HAS_SUBINTERPRETER_SUPPORT 1                           
#    endif                                                                      
#endif
  • There is nothing inherently wrong with what we have.
  • The only reason we want to make a change is to be consistent with the existing macros.
  • Doing something special (#undef) would be inconsistent.
  • Adding the #undef provision for all macros would add noise.
  • This really is a very minor issue, I wouldn't want to give it any more attention than we have already, as long as we come out with a consistent solution.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, I know what's needed, just teaching in an hour, so haven't had time to apply it. It's something like this:

#ifndef PYBIND11_HAS_SUBINTERPRETER_SUPPORT                                     
#    if PY_VERSION_HEX >= 0x030C0000 && !defined(PYPY_VERSION) && !defined(GRAALVM_PYTHON)
#        define PYBIND11_HAS_SUBINTERPRETER_SUPPORT 1                           
#    endif
#else
#    if PYBIND11_HAS_SUBINTERPRETER_SUPPORT == 0
#        undefine PYBIND11_HAS_SUBINTERPRETER_SUPPORT
#    endif
#endif

Then after that, simple ifdefs can be used throughout the code instead of checking this one and only this one for 1/0.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds good!

Looking around some more after seeing @henryiii's suggestion:

I didn't realize before that PYBIND11_HAS_SUBINTERPRETER_SUPPORT is the only PYBIND11_HAS_ macro in pybind11/detail/common.h for which we have the #ifndef precondition.

Therefore: It's not inconsistent to add the #undef.

(There is at least one other non-PYBIND11_HAS_ macro (PYBIND11_EXPORT_EXCEPTION) that doesn't have such an #undef, but oh well.)

#endif

// 3.12 Compatibility
Expand Down
2 changes: 1 addition & 1 deletion include/pybind11/detail/internals.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,7 +324,7 @@ inline std::atomic<int> &get_num_interpreters_seen() {

template <typename InternalsType>
inline std::unique_ptr<InternalsType> *&get_internals_pp() {
#ifdef PYBIND11_HAS_SUBINTERPRETER_SUPPORT
#if PYBIND11_HAS_SUBINTERPRETER_SUPPORT
if (get_num_interpreters_seen() > 1) {
// Internals is one per interpreter. When multiple interpreters are alive in different
// threads we have to allow them to have different internals, so we need a thread_local.
Expand Down
2 changes: 1 addition & 1 deletion include/pybind11/subinterpreter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@

#include <stdexcept>

#if !defined(PYBIND11_HAS_SUBINTERPRETER_SUPPORT)
#if !PYBIND11_HAS_SUBINTERPRETER_SUPPORT
# error "This platform does not support subinterpreters, do not include this file."
#endif

Expand Down
1 change: 1 addition & 0 deletions tests/mod_per_interpreter_gil.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,4 +12,5 @@ PYBIND11_MODULE(mod_per_interpreter_gil,
py::multiple_interpreters::per_interpreter_gil()) {
m.def("internals_at",
[]() { return reinterpret_cast<uintptr_t>(&py::detail::get_internals()); });
m.attr("PYBIND11_HAS_SUBINTERPRETER_SUPPORT") = PYBIND11_HAS_SUBINTERPRETER_SUPPORT;
}
1 change: 1 addition & 0 deletions tests/mod_shared_interpreter_gil.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,4 +9,5 @@ namespace py = pybind11;
PYBIND11_MODULE(mod_shared_interpreter_gil, m, py::multiple_interpreters::shared_gil()) {
m.def("internals_at",
[]() { return reinterpret_cast<uintptr_t>(&py::detail::get_internals()); });
m.attr("PYBIND11_HAS_SUBINTERPRETER_SUPPORT") = PYBIND11_HAS_SUBINTERPRETER_SUPPORT;
}
2 changes: 1 addition & 1 deletion tests/test_embed/test_subinterpreter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
#include <pybind11/embed.h>
#ifdef PYBIND11_HAS_SUBINTERPRETER_SUPPORT
#if PYBIND11_HAS_SUBINTERPRETER_SUPPORT
# include <pybind11/subinterpreter.h>

// Silence MSVC C++17 deprecation warning from Catch regarding std::uncaught_exceptions (up to
Expand Down
14 changes: 10 additions & 4 deletions tests/test_multiple_interpreters.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,12 @@ def test_independent_subinterpreters():
elif sys.version_info >= (3, 12):
import _xxsubinterpreters as interpreters
else:
pytest.skip("Test requires a the interpreters stdlib module")
pytest.skip("Test requires the interpreters stdlib module")

import mod_per_interpreter_gil as m
m = pytest.importorskip("mod_per_interpreter_gil")

if not m.PYBIND11_HAS_SUBINTERPRETER_SUPPORT:
pytest.skip("Does not have subinterpreter support compiled in")

code = """
import mod_per_interpreter_gil as m
Expand Down Expand Up @@ -94,9 +97,12 @@ def test_dependent_subinterpreters():
elif sys.version_info >= (3, 12):
import _xxsubinterpreters as interpreters
else:
pytest.skip("Test requires a the interpreters stdlib module")
pytest.skip("Test requires the interpreters stdlib module")

m = pytest.importorskip("mod_shared_interpreter_gil")

import mod_shared_interpreter_gil as m
if not m.PYBIND11_HAS_SUBINTERPRETER_SUPPORT:
pytest.skip("Does not have subinterpreter support compiled in")

code = """
import mod_shared_interpreter_gil as m
Expand Down
Loading