Strip down gil.h to the simple version only.

[rwgk]

Description

Prompted by discussion under PR #4216. Please see there for background.

This PR removes ~170 lines of code, and two internals variables (guarded by internals version).

This passes Google-global testing (internal testing ID OCL:479233883:BASE:479302840:1665064423602:50af055b).

Suggested changelog entry:

An outdated version of `gil_scoped_acquire` was removed from pybind11/gil.h, resolving #1276 and pytorch/pytorch#83101.

[rwgk]

All CI failures appear to be of the kind below.

Speculation: The failing test relies on gil_scoped_acquire to call detail::get_internals() as a side effect.

2022-10-06T14:35:24.7574365Z ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2022-10-06T14:35:24.7574761Z test_embed is a Catch v2.13.9 host application.
2022-10-06T14:35:24.7575357Z Run with -? for options
2022-10-06T14:35:24.7575524Z
2022-10-06T14:35:24.7575908Z -------------------------------------------------------------------------------
2022-10-06T14:35:24.7576241Z Threads
2022-10-06T14:35:24.7576689Z -------------------------------------------------------------------------------
2022-10-06T14:35:24.7577136Z /__w/pybind11/pybind11/tests/test_embed/test_interpreter.cpp:285
2022-10-06T14:35:24.7577818Z ...............................................................................
2022-10-06T14:35:24.7577989Z
2022-10-06T14:35:24.7578196Z /__w/pybind11/pybind11/tests/test_embed/test_interpreter.cpp:296: FAILED:
2022-10-06T14:35:24.7578582Z   REQUIRE( has_pybind11_internals_static() )
2022-10-06T14:35:24.7578878Z with expansion:
2022-10-06T14:35:24.7579097Z   false
2022-10-06T14:35:24.7579523Z
2022-10-06T14:35:24.8253489Z ===============================================================================
2022-10-06T14:35:24.8254019Z test cases:   12 |   11 passed | 1 failed
2022-10-06T14:35:24.8254379Z assertions: 1553 | 1552 passed | 1 failed

[rwgk]

@jbms Question regarding e70b38b:

+       // This used to be triggered by py::gil_scoped_release().
+       // TODO: Explain why/how this is relevant to this unit test.
+       py::detail::get_internals();
        REQUIRE(has_pybind11_internals_static());

Locally I tried to remove those 4 lines and the test passes, but does that make sense?

I also tried to move the REQUIRE(has_pybind11_internals_static());

1. After the loop creating the threads,
2. To the very end of that unit test,

without adding the explicit get_internals() call, but then the test fails in both cases. I.e. get_internals() is apparently never called as a side-effect of the rest of the code.

[jbms]

It looks like that test isn't testing anything that uses pybind11 internals anymore, so it seems like we could just remove REQUIRE(has_pybind11_internals_static());. Maybe that was just testing a side effect of the old gil_scoped_release implementation.

[rwgk]

It looks like that test isn't testing anything that uses pybind11 internals anymore, so it seems like we could just remove REQUIRE(has_pybind11_internals_static());. Maybe that was just testing a side effect of the old gil_scoped_release implementation.

Cool, thanks! Done: ad126f5

While I was at it, I also took care of this one: be83638

But I see the CI is unhappy, need to look & fix ...

[jbms]

Given that the default internals version is still 4, I think the tstate change can also be gated by PYBIND11_INTERNALS_VERSION > 4.

[rwgk]

Given that the default internals version is still 4, I think the tstate change can also be gated by PYBIND11_INTERNALS_VERSION > 4.

Just to mention: I made a silly mistake in be83638, it should have been < 6 (not > 5).

I tried this (locally only):

#if defined(WITH_THREAD)
#    if PYBIND11_INTERNALS_VERSION <= 4
    PYBIND11_TLS_KEY_INIT(tstate)
#    endif // PYBIND11_INTERNALS_VERSION <= 4 // Intentionally avoiding #else (unrelated features).
#    if PYBIND11_INTERNALS_VERSION > 4
    PYBIND11_TLS_KEY_INIT(loader_life_support_tls_key)
#    endif // PYBIND11_INTERNALS_VERSION > 4

But even with the "unrelated features" comment in that place, the code could easily be misunderstood by someone new glancing through.

We don't have to skimp with the internals numbers, I think making it more obvious that there were two features is more important.

[rwgk]

@wjakob Could you please let us know if we still need the code introduced with 39e97e6#diff-4249f25293fdbc32ba7cb0a011c27ff420af64a7f95931553f953dc41f38915e?

[rwgk]

I looked in nanobind: https://github.com/wjakob/nanobind/blob/526e2c798547185e37f66f77e3876311157eca95/include/nanobind/nb_misc.h

It is almost exactly the simple version here (although it is missing #4183). That strongly suggests @wjakob has decided that the complicated gil_scoped_acquire version in pybind11 is no longer useful. I believe we need to follow that lead and purge the code here as well, to stop sending pybind11 users into situations like #1276 and pytorch/pytorch#83101.

[rwgk]

@henryiii @Skylion007 could you please comment on this PR?

My preference is to have this as a bug fix in 2.10.1.

Note that this was globally tested, which has been highly conclusive in the past. I'm not aware of any arguments for keeping the code that is known to cause issues in practice, and was found to have problems by inspection (see #4216).

[henryiii]

Have you checked performance? My first thought when seeing large amounts of code with a very simple fallback is that it was done that way for performance¹. Some codes might acquire and release the gil quite a bit, and they'll care a lot about performance. I'm also a little worried about dropping in a big change right before releasing a patch release, I'd rather have it sit in master a bit. That why I suggested making it opt-in for the patch release.

Hmm, according to the comments, it's because the simple code doesn't work in some cases. What about the three points listed in the code that you've deleted? Specifically, have you compiled NanoGUI with this change? It is supposed to demonstrate the need for the more complex custom code. Maybe our improvements over the years have made this unnecessary, but I'd like a little bit of confirmation.

Footnotes

1. The deleted code mentions three points, performance is not one of them, so probably never mind. ↩

[henryiii]

If you can prove https://github.com/mitsuba-renderer/nanogui works with this change, I'm in.

[henryiii]

I tested this, and nanogui can't even compile with this change.

[jbms]

Nanogui could copy the old code to its own repo if it wants to use it, or build using an older version of pybind11. But since the old code is buggy and doesn't appear to be fixable without adding new apis to cpython, it would probably be better for nanogui to use a different approach entirely.

I don't think we should keep buggy behavior by default just to support a single user.

[henryiii]

That's fine, but we don't knowingly break users in patch releases. This code has been linked to from the pybind11 source for years, so I'd be quite surprised if this was truly the only user. This can't be stripped out till 2.11. I'm okay to have the new behavior opt-in in 2.10.1, or not, but the default can't change till 2.11. If there are several more users, then I'd say 3.0 is needed for breaking change, I'm only okay with it in a minor release because I'm still assuming the number of users is small.

I'll probably at least make a PR to nanogui to keep from breaking them in 2.11, either by limiting pybind11 or by coping the removed code there.

[henryiii]

Actually, I don't think NanoGUI can copy it, because they can't add the required internals pointers. This will kill NanoGUI eventually if they can't adapt to the new code, either by limiting pybind11 version to something that will eventually no longer work, or by requiring an old internals version that we eventually kill off (which I'd like to do, setting the internals version is ugly, it's better if it's linked to pybind11 version). NanoGUI is just the obvious user, there could easily be more (and it has a combined 5k stars and ~800 forks). (It's also a very nice looking project, there aren't many cross platform OSS graphic libraries that can build from scratch. Though it's also lacking working examples and tests...)

[rwgk]

Thanks @henryiii for trying out the nanogui build. I left a message there:

mitsuba-renderer/nanogui#133

[henryiii]

You should also open an issue in Kitware/SMTK, which would also be broken by this change.

https://github.com/Kitware/SMTK/blob/ac60e3e83857b450d61fb20d52e64d4f8fbc057b/smtk/operation/pybind11/PyOperation.h#L127

https://github.com/Kitware/SMTK/blob/ac60e3e83857b450d61fb20d52e64d4f8fbc057b/smtk/operation/pybind11/PyOperation.h#L140

[rwgk]

You should also open an issue in Kitware/SMTK, which would also be broken by this change.

https://github.com/Kitware/SMTK/blob/ac60e3e83857b450d61fb20d52e64d4f8fbc057b/smtk/operation/pybind11/PyOperation.h#L127

https://github.com/Kitware/SMTK/blob/ac60e3e83857b450d61fb20d52e64d4f8fbc057b/smtk/operation/pybind11/PyOperation.h#L140

Alright, two is a crowd. Thanks for digging this up, too.

The first thing we need is a test in pybind11, if we keep that functionality.

[henryiii]

I expect it's really hard to to test threaded code like this, which is why it's missing tests. Also, I'm okay to have the simple version opt-in, then switch to opt-out eventually (Using the "advanced" (buggy) version breaks pypy compatibility anyway, so making the complex version require setup seems quite reasonable even if we can't remove it). Maybe the projects can work out how to avoid this, and then we can drop it. I agree that 95% of users likely will like the simpler version better.

[rwgk]

Kitware/SMTK#354

[rwgk]

Let's first hear from them, what they actually need. Maybe we can change-and-reduce the non-simple version to something that works for 100% of pybind11 users?

I expect it's really hard to to test threaded code like this,

I agree, but the lack of discipline has a cost, too. My gut estimate: the spread-out cost for people dealing with the buggy behavior, in accumulation, exceeds the cost for getting this tested and right. The problem of course is to collectively get organized enough to fix that.

[rwgk]

Pragmatically, for 2.10.1, my current thinking:

• Keep the non-simple version but make the simple version the default.
• All the presumably few affected projects need to do is pass an extra define to keep working. We could add a comment near our define, asking them to link their PRs here. That will give us a nice overview what we're up against.
• While the vast majority of pybind11 users is already protected from repeats of the reported bugs.

[henryiii]

Even if the two users we've identified and all the others we haven't found yet are able to quickly adapt, I'm not comfortable shipping a known breaking change for an old feature in a patch release. So we either don't put it in 2.10.1 at all, or we have an option that allows the simpler version to be activated. Since PyTorch would like the simpler version, I'm inclined for the latter.

---

Let's make the simple version optional and non-default in 2.10.1, and default in 2.11.0 but with an opt-out. We can put a warning in the changelog / upgrade guide. We can make PRs fixing those two projects to be ready for 2.11. I think 2.11's not terribly far way. Maybe we can remove this in 2.12/3.0 if the transition is smooth in 2.11.

I don't think "repeats of the reported bugs" is that big of an issue - many pybind11 users have happily been using pybind11, and there are lots of other bugs in issues that we can work on if we really want to work out all possible bugs. Let's not cause regressions and grief for ourselves and users, and take this slow.

[rwgk]

I don't know, I've never knowingly released something untested & broken like this, I'm very uncomfortable having my name associated with such a trap. But let's see if we can get more data, maybe it's not as severe as I think?

• Add option to use the PyGILState API in gil_scoped_acquire (#1276) #1286 — @KyleFromKitware
• Importing torchvision.models.detection.faster_rcnn when on CPU results in GIL deadlock when torch tensor from numpy.array is deallocated pytorch/pytorch#83101 — @Chekov2k

How much time did you and your teams spend dealing with issues related to the non-simple gil_scoped_acquire version? What's your opinion for resolving the conflict of interests (trap for many, needed for some)?

[henryiii]

I've never knowingly released something untested & broken

Pybind11 has over 400 issues open. Our test coverage is rather poor, too. We have lots of edge cases and ways to do broken things. Our release and maintenance are on a "best effort" basis. In this case, we do support dissociation of the gil (and have for many years) and we don't support reentry (except on PyPy, apparently). We can change what we support in a minor or major release, but not a patch release. We can add an opt-in flag in a patch release safely.

Patch releases are releases that try to only provide fixes. If we make it an opt-in fix, it's still a fix. If we make it opt-out, it's a breaking change. We follow Python's deprecation period for breaking changes - we will release at least one or two releases with warnings before we make a breaking change.

I'd like to get a patch release out before Monday. That doesn't provide much time. We have more time to decide for 2.11 if we want to change it from opt-in to opt-out, and potentially set a target version for removing the old form entirely.

[jbms]

Actually, I don't think NanoGUI can copy it, because they can't add the required internals pointers. This will kill NanoGUI eventually if they can't adapt to the new code, either by limiting pybind11 version to something that will eventually no longer work, or by requiring an old internals version that we eventually kill off (which I'd like to do, setting the internals version is ugly, it's better if it's linked to pybind11 version). NanoGUI is just the obvious user, there could easily be more (and it has a combined 5k stars and ~800 forks). (It's also a very nice looking project, there aren't many cross platform OSS graphic libraries that can build from scratch. Though it's also lacking working examples and tests...)

I suppose nanogui can't literally copy the code, but there is no reason it needs to use pybind11's internals to hold the thread local storage identifier --- it can create it its own thread local storage identifier that it stores wherever it wishes.

[jbms]

Even if the two users we've identified and all the others we haven't found yet are able to quickly adapt, I'm not comfortable shipping a known breaking change for an old feature in a patch release. So we either don't put it in 2.10.1 at all, or we have an option that allows the simpler version to be activated. Since PyTorch would like the simpler version, I'm inclined for the latter.

Is there reason that the next release needs to be called 2.10.1? It could be called 2.11 instead.

[henryiii]

Because we need to make a release before Python 3.11 release on Monday, and we aren't ready to ship breaking changes. We are not making a breaking change then releasing a couple of days later. Master has already moved on past the 2.10 branch, so we can make it opt-out immediately and then make it opt-in in the backport, I don't care. But "fixing" a perceived bug in pybind11 that's existed for multiple years and forcing your fix on everyone else does not need an emergency minor release tomorrow.

If it makes everyone feel better, I can just not backport #4216 at all and you can pretend "2.10.0" and "2.10.1" are the same thing.

[henryiii]

To be clear, the pybind11 gil handling is not untested & broken. It is tested, and works for the 100K+ pybind11 users. It simply does not support nesting, and this change would allow it to support nesting, which at least a few users (one identified) want, at the expense of being able to dissociate the handling from the current thread, which at least a couple of (identified) users need.

[rwgk]

Pybind11 has over 400 issues open.

Most of them are stale and none rises to the same level of concern, except the severe issues (#1138, #1333) that are fixed in the smart_holder branch.

Our test coverage is rather poor, too.

That is not true. "Not complete" is fair to say, but generally we have >90% coverage.

Is there reason that the next release needs to be called 2.10.1? It could be called 2.11 instead.

Yeah, I already suggested abandoning 2.10 for another reason, suspected unintentional ABI break: #4125 (comment)

Because we need to make a release before Python 3.11 release on Monday,

What is the minimum we'd need to do for 3.11? — I don't know off the top of my head and need to go look through the diffs.

[rwgk]

untested

It is obviously untested in pybind11 (which is what I meant), otherwise the CI for this PR wouldn't pass.

[henryiii]

If we take this more slowly, we can copy the code needed for nanogui, provide that as an example in the upgrade guide for 3.11, and gently move to the new version in a few weeks - 2.11 is not years away or anything terrible. The release in the next couple of days could include this as an opt-in preview, which PyTorch (which I think already worked around this anyway?) could then opt-in to, along with anyone else that encounters this issue. There is no need to rush 2.11 just because we need to ship a few fixes for 2.10 before Python 3.11.

[henryiii]

It is obviously untested in pybind11 (which is what I meant), otherwise the CI for this PR wouldn't pass.

I meant the gil handling is tested. The only thing that's not tested is the dissociation feature (which is hard to test in a test running a python process), or nesting (which is unsupported before this). But the gil handling code is very much tested, and used in many projects.

[rwgk]

A bit random, but maybe useful for later, this PR does NOT fix the TSAN error I've seen ever since I first ran the pybind11 tests with the clang sanitizers (~2 years ago). See also PR #2754. (Note that the assertion failures also appear regularly in flakes, (almost?) always Windows.)

============================= test session starts ==============================
platform linux -- Python 3.9.15, pytest-6.2.5, py-1.10.0, pluggy-0.9.0
cachedir: /build/work/.../google3/tmp
C++ Info: Clang google3-trunk (11897708c0229c92802e747564e7c34b722f045f) C++17 __pybind11_internals_v4_clang_libcpp_cxxabi1002__
rootdir: /build/work/.../google3/runfiles/google3/third_party/pybind11/tests, configfile: pytest.ini
collected 5 items

test_gil_scoped.py ==10081==ThreadSanitizer: starting new threads after multi-threaded fork is not supported. Dying (set die_after_fork=0 to override)
F==10082==ThreadSanitizer: starting new threads after multi-threaded fork is not supported. Dying (set die_after_fork=0 to override)
F==10083==ThreadSanitizer: starting new threads after multi-threaded fork is not supported. Dying (set die_after_fork=0 to override)
F..

=================================== FAILURES ===================================
___________________ test_python_to_cpp_to_python_from_thread ___________________

    def test_python_to_cpp_to_python_from_thread():
        """Makes sure there is no GIL deadlock when running in a thread.

        It runs in a separate process to be able to stop and assert if it deadlocks.
        """
>       assert _run_in_process(_python_to_cpp_to_python_from_threads, 1) == 0
E       assert 66 == 0
E        +  where 66 = _run_in_process(_python_to_cpp_to_python_from_threads, 1)


test_gil_scoped.py:59: AssertionError
__________ test_python_to_cpp_to_python_from_thread_multiple_parallel __________

    def test_python_to_cpp_to_python_from_thread_multiple_parallel():
        """Makes sure there is no GIL deadlock when running in a thread multiple times in parallel.

        It runs in a separate process to be able to stop and assert if it deadlocks.
        """
>       assert _run_in_process(_python_to_cpp_to_python_from_threads, 8, parallel=True) == 0
E       assert 66 == 0
E        +  where 66 = _run_in_process(_python_to_cpp_to_python_from_threads, 8, parallel=True)


test_gil_scoped.py:68: AssertionError
_________ test_python_to_cpp_to_python_from_thread_multiple_sequential _________

    def test_python_to_cpp_to_python_from_thread_multiple_sequential():
        """Makes sure there is no GIL deadlock when running in a thread multiple times sequentially.

        It runs in a separate process to be able to stop and assert if it deadlocks.
        """
>       assert (
            _run_in_process(_python_to_cpp_to_python_from_threads, 8, parallel=False) == 0
        )
E       assert 66 == 0
E        +  where 66 = _run_in_process(_python_to_cpp_to_python_from_threads, 8, parallel=False)


test_gil_scoped.py:77: AssertionError
- generated xml file: /build/work/5db35a65bbd644542f198253cf4c0968ea07/google3/blaze-out/k8-tsan-fastbuild/testlogs/third_party/pybind11/tests/test_gil_scoped/test.xml -
=========================== short test summary info ============================
FAILED test_gil_scoped.py::test_python_to_cpp_to_python_from_thread - assert ...
FAILED test_gil_scoped.py::test_python_to_cpp_to_python_from_thread_multiple_parallel
FAILED test_gil_scoped.py::test_python_to_cpp_to_python_from_thread_multiple_sequential
========================= 3 failed, 2 passed in 0.55s ==========================

[henryiii]

Did you test this on anything that was supposed to be fixed by it? I copied the contents of #1276 on ubuntu 18.04 (since I was there for something else) and still get:

$ git fetch origin pull/4218/head:MASTER
$ c++ I/usr/include/python3.6m -I/usr/include/python3.6m -I/pybind11/include -Wno-unused-result -Wsign-compare -DNDEBUG -g -fwrapv -O2 -Wall -g -fstack-protector-strong -Wformat -Werror=format-security -g -flto -fuse-linker-plugin -ffat-lto-objects -std=c++11 gil.cpp -Wl,-Bsymbolic-functions -Wl,-z,relro -L/usr/lib/python3.6/config-3.6m-x86_64-linux-gnu -lpython3.6m -lpthread -ldl -lutil -lm -o gil
$ ./gil
Calling C++ code
Segmentation fault

The second example passes, even on the old version (I'm using python3 -m pybind11 --embed --file=gil2.cpp | xargs c++ on my branch with that the extra compile helper). I'm not using a debug version, but the second example is supposed to fail without the debug build.

On macOS, both before and after this patch, the examples pass. On Ubuntu 20.04 and 22.04, both pass as well. So this patch has no visible effect on #1276, which seems to be resolved or at least better on most newer compilers.

# On henryiii:henryiii/feat/clihelper branch
docker run --rm -v$PWD:/pybind11 -w /pybind11 -it ubuntu:22.04
apt update && apt install -y g++ python3-dev
compile_command=$(python3 -m pybind11 --embed --file=gil.cpp)
compile_command2=$(python3 -m pybind11 --embed --file=gil2.cpp)
# git checkout testing version in another window
c++ $compile_command && ./gil
c++ $compile_command2 && ./gil2

[rwgk]

Did you test this on anything that was supposed to be fixed by it?

To answer this question (sorry for delay): I didn't test with the #1276 code and only glanced at that bug and #1322 a little bit before.

My starting point was #4216, the idea here was to try what happens if we only have the simple code: the CI passed, and Google global testing.

#1276 is from 4 yr 8 mo ago. I don't see references to Python 3 in the comments, only Python 2.7. I didn't spot details about platforms and Python versions. With so much uncertainty after such a long time, is this still something useful to look at?

I'm thinking it's best to leave that old issue behind (not reference it anymore) and only focus on #4216.

Considering the uses of the non-simple version you found (comments here), this PR is too radical. I agree we need to keep the non-simple version around for a while. Our only difference seems to be when to flip the default, with me on the "as soon as possible" side, and you on the "let's wait" IIUC.

For Google-internal use, I still want to flip the default as soon as possible. I don't want other Google engineers to find out the hard way (like the PyTorch people) that they need the simple version, when they probably never even became aware of the features of the non-simple version, but just wanted a convenient way to ensure the GIL is held in a section of code.

I could flip the default only on the smart_holder branch, that would achieve my goal for Google's purposes.

I'll convert this PR to draft mode. Rationale: Q: Is it better to just add a define as in #4216, or also fold in the internals changes here? A: I believe purging the two members (tstate, istate) doesn't do anything measurable to compile times and binary sizes, and if we cannot purge the non-simple code, the complication with the #ifdefs here is just distracting. I.e. this PR is mainly useful as a starting point for purging the non-simple code later.

[rwgk]

Closing this PR now, I think it will be completely stale by the time we get to removing the non-simple code. The change to test_embed/test_interpreter.cpp needed to be transferred to #4216, so that the tests pass when PYBIND11_SIMPLE_GIL_MANAGEMENT is defined. I also marked the tstate and istate members of internals with comments under #4216, so that we don't forget to remove them eventually. Everything else is really easy to figure out from scratch, without looking at this PR again.