[Preview 3] Merge smart_holder into master (squash merge) #5339

rwgk · 2024-08-26T19:11:34Z

Description

For those who would like to see this PR merged: Please read #5339 (comment)

This PR is in [Preview] mode, for other maintainers to give feedback/support:

Is there agreement on starting the pybind11 v3 series with this PR?

If not, what should we do?

Tagging all people that are listed in the pybind11-scipy2023 slides:

@ax3l @wjakob @Skylion007 @henryiii @EthanSteinberg

Additionally @EricCousineau-TRI who was involved earlier and has a fork with overlapping functionality.

Updates after [Preview 1]:

[Preview 2] Add [smart_holder] Introduce PYBIND11_SMART_HOLDER_DISABLE option. #5348
[Preview 3] Apply [smart_holder] git merge master #5349

This is used as production code at Google. The first deployment was in February 2021. I.e. this code has been extremely thoroughly tested. Additionally, testing via PyCLIF-pybind11 provided thousands of diverse use cases that needed to satisfy many million unit tests (the PyCLIF-pybind11 success rate is/was about 99.999%).

git diff counts under include/:

14 lines removed (git diff master include/ | grep '^-' | grep -v '^---' | wc -l)
1621 lines added (git diff master include/ | grep '^+' | grep -v '^+++' | wc -l)

I.e., the existing production code is barely touched.

Similarly for tests/:

2 lines removed
4047 lines added

Existing tests are unchanged, except for a small change in test_class.cpp (to explicitly use unique_ptr as holder) and test_class.py (to add a test).

For more detail, results produced by cloc --diff for include/ between the master branch and this PR:

-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
C/C++ Header
 same                           26           1336           2994          14574
 modified                        9              0              1             13
 added                           5            209            166           1232
 removed                         0              0              0              0
-------------------------------------------------------------------------------

I.e., this PR increases the production code size by 8.4%. — But please see the description of PR #5348, which added the PYBIND11_SMART_HOLDER_DISABLE option, to effectively remove all complexity/overhead added with this PR, if desired.

All code was formally reviewed at least once. Much of the code was formally reviewed multiple times (as it was moved around between branches).

To be decided: How do we want to manage pybind11 versioning?

Concretely:

Public version number (v3?)
PYBIND11_INTERNALS_VERSION

Note the change near the top of

pybind11/detail/common.h:

-#define PYBIND11_VERSION_MAJOR 2
-#define PYBIND11_VERSION_MINOR 14
+#define PYBIND11_VERSION_MAJOR 3
+#define PYBIND11_VERSION_MINOR 0
 #define PYBIND11_VERSION_PATCH 0.dev1

 // Similar to Python's convention: https://docs.python.org/3/c-api/apiabiversion.html
 // Additional convention: 0xD = dev
-#define PYBIND11_VERSION_HEX 0x020E00D1
+#define PYBIND11_VERSION_HEX 0x030000D1

Without this, all smart_holder functionality is disabled:

pybind11/detail/internals.h:

+#    if PYBIND11_VERSION_MAJOR >= 3
+#        define PYBIND11_INTERNALS_VERSION 6
...
+#if PYBIND11_INTERNALS_VERSION >= 6
+
+#    define PYBIND11_HAS_INTERNALS_WITH_SMART_HOLDER_SUPPORT

In theory, this PR could be merged without the version number change in pybind11/detail/common.h, and users could activate the smart_holder functionality by overriding PYBIND11_INTERNALS_VERSION as needed.

TODO this PR:

Minimal update of README_smart_holder.rst docs/, .github/workflows (clean out branches: smart_holder)

Beyond this PR:

Before this PR is merged: EDIT: DONE but this PR still needs to be updated
- Merge PR feat: enable type-safe interoperability between different independent Python/C++ bindings systems. #5296 for the pybind11 v2 series.
After this PR is merged (which starts the pybind11 v3 series):
- Pick up // SMART_HOLDER_BAKEIN_FOLLOW_ON work. This is intentionally deferred because this PR (the already very large diff) is much easier to understand as-is.
- Work on full docs/ updates and remove README_smart_holder.rst.
- We could/should piggy-back PR [smart_holder] Conversions between Python's native (stdlib) enum types and C++ enums. #5280 asap to avoid another PYBIND11_INTERNALS_VERSION bump.

Suggested changelog entry:

…0700)

petersteneteg · 2024-09-01T08:29:33Z

I would love to see this merged. Have been using it for years and it is great! But being able to use the default package in various package managers would be very nice!

rwgk · 2024-09-01T16:06:57Z

I would love to see this merged. Have been using it for years and it is great! But being able to use the default package in various package managers would be very nice!

Thanks @petersteneteg!

The only reason that this PR has been just sitting here for a week is a complete absence of feedback.

I'll try working on it some more to hopefully make it acceptable to everyone, even the most efficiency/optimization-minded users:

The idea is to introduce a PYBIND11_SMART_HOLDER_DISABLE macro, but in such a way that the PYBIND11_INTERNALS_ID is identical with and without that define. That way the feature would be available by default, but people worried about compile-time or runtime overhead could use that macro to essentially eliminate that completely. — I believe the extra time the preprocessor needs to strip out the smart_holder code is virtually unmeasurable; although I didn't try to time it. The extra compile time for the tiny internals addition is probably also unmeasurable.

[EDIT 10 hours later: This is done now: #5348]

…ind#5348) * Step 1: Establish new `PYBIND11_SMART_HOLDER_ENABLED` macro, but only under include/pybind11/ At the stage `PYBIND11_HAS_INTERNALS_WITH_SMART_HOLDER_SUPPORT` and `PYBIND11_SMART_HOLDER_ENABLED` are still equivalent. * Systematically replace all `PYBIND11_HAS_INTERNALS_WITH_SMART_HOLDER_SUPPORT` with `PYBIND11_SMART_HOLDER_ENABLED` under tests/ and ubench/ As at the previous stage, `PYBIND11_HAS_INTERNALS_WITH_SMART_HOLDER_SUPPORT` and `PYBIND11_SMART_HOLDER_ENABLED` are still equivalent. * Introduce `PYBIND11_SMART_HOLDER_DISABLE` option. * `#ifdef` out entire `wrap()` function to avoid `unused-parameter` warning-as-error under macos-13 ``` /Users/runner/work/pybind11/pybind11/tests/test_class_sh_trampoline_basic.cpp:67:23: error: unused parameter 'm' [-Werror,-Wunused-parameter] void wrap(py::module_ m, const char *py_class_name) { ^ /Users/runner/work/pybind11/pybind11/tests/test_class_sh_trampoline_basic.cpp:67:38: error: unused parameter 'py_class_name' [-Werror,-Wunused-parameter] void wrap(py::module_ m, const char *py_class_name) { ^ 2 errors generated. ```

…iew_1

rhaschke

Great that a merge of smart_holder into master is tackled now.
To convince users concerned about compile-time or runtime overhead, it would be helpful to link to the results of your benchmark tests.

rhaschke · 2024-09-02T08:27:54Z

.github/workflows/ci.yml

@@ -7,12 +7,13 @@ on:
    branches:
      - master
      - stable
+      - smart_holder


I'm surprised to see the smart_holder branch listed here (and several other workflow definitions).
Isn't the goal of this PR to merge that branch into master, making the former branch superfluous?

rhaschke · 2024-09-02T08:33:13Z

.github/workflows/ci.yml

      - v*

 permissions: read-all

 concurrency:
-  group: test-${{ github.ref }}
+  group: test-sh-avl${{ github.ref }}


Is that addition needed?

No, thanks for catching this, that's an oversight. I forgot to remove it on the smart_holder branch. Will do that now.

Removing it on the smart_holder branch didn't pan out:

Tried: 4cc11dc

Reverted: f0d2da3

Comment so it's not forgotten: c31e79a

(I couldn't explain why it didn't work. Another thing I don't want to spend my time on.)

With addition is meant suffix. You must not remove the whole line.

Suggested change

group: test-sh-avl${{ github.ref }}

group: test-${{ github.ref }}

With addition is meant suffix. You must not remove the whole line.

Sorry I messed up this simple change so badly. I think I got it right now. (I had it in my mind wrongly that the line isn't on master, but it is.)

rhaschke · 2024-09-02T08:43:10Z

README_smart_holder.rst

+the smart_holder functionality is fully baked into pybind11.
+Prior to PR #5257 the smart_holder implementation was an "add-on", which made
+it necessary to use a ``PYBIND11_SMART_HOLDER_TYPE_CASTERS`` macro. This macro
+still exists for backward compatibility, but is now a no-op. The trade-off


This is a severe limitation for me. I frequently need interoperability between different extension modules, compiled both with and without smart_holder support.
Will this limitation be lifted with #5296?

Yes, #5296 would fix this, if it gets merged.

Yes, #5296 would fix this, if it gets merged.

@wjakob rejected it. See there. (I didn't see any evidence that @wjakob actually reviewed the PR, only the description; please correct me if I'm wrong.)

But to give a more nuanced answer to your question:

If you use the smart_holder branch, and

you compile one extension withOUT defining PYBIND11_SMART_HOLDER_DISABLE, and

another extension WITH PYBIND11_SMART_HOLDER_DISABLE defined,

they will interoperate (because the PYBIND11_SMART_HOLDER_DISABLE define does not alter the PYBIND11_INTERNALS_ID).

I didn't see any evidence that @wjakob actually reviewed the PR, only the description; please correct me if I'm wrong

This is not a good summary of my response, it is a bit more nuanced than that.

I didn't summarize anything!

auscompgeek · 2024-09-02T09:53:26Z

README.rst

@@ -13,6 +13,10 @@

 .. start

+.. Note::
+
+   This is the pybind11 **smart_holder** branch. Please refer to


This won't be true when merged, right?

Correct, that's what I want to clean out last minute, "TODO this PR" in the PR description.

rwgk · 2024-09-02T17:23:54Z

Great that a merge of smart_holder into master is tackled now. To convince users concerned about compile-time or runtime overhead, it would be helpful to link to the results of your benchmark tests.

Yes that might be interesting, but is not where I want to spend my very limited time left. In large part because I believe it's inconsequential: micro benchmarks of the bindings layer have very little practical value for drawing conclusions about real-world applications.

I'd be happy to participate in discussing results if someone else wants to run benchmarks. Also happy to answer questions about using the ubench/ code that's included here (I haven't run it myself for at least two years).

…iew_1

rwgk · 2024-09-07T17:00:15Z

For those who would like to see this PR merged:

(You may want to quickly skip to the bottom of this comment first and then come back here.)

The work behind this PR was done as part of a Google project, to unify PyCLIF and pybind11. As of yesterday (2024-09-06), I'm no longer a Google employee. I no longer have access to any Google-internal resources. For this weekend I'm technically unemployed. I will start with a new employer next week. — The full story behind this is long and twisty. I cannot freely share a lot of it publicly, and some of it not even privately. It is public information though that the "roles" of all US members of my team (and roles in many other teams) were abruptly "eliminated" in a batch of layoffs in April this year. Please get in touch with me if you are interested in learning more about how this affected my team, my work, and me personally.

I'm very grateful that another division of Google quickly and proactively offered a new role to me. They offered me the opportunity to properly close out projects and do handover work, before moving on to new projects. This PR was created under that arrangement.

When my role was eliminated in April, Google was running what was then named pybind11k (renamed to pybind11clif in the meantime), a fork of pybind11 first deployed internally in May 2023. Unfortunately the fork was inevitable, to not stall the progress of the PyCLIF-pybind11 unification, but I consistently spent effort to maximize compatibility with upstream pybind11 (most notably PR #30008), and I offered several PRs for merging back into upstream master, which were often ignored, like this PR.

The pybind11k fork was based on the smart_holder branch. At the time of the role eliminations in April, I was the only person familiar with keeping the pybind11 master branch, smart_holder branch, and the pybind11k fork in sync. Merging the pybind11 master branch into the smart_holder branch, and then the smart_holder branch into the pybind11k fork, was messy and laborious. I was very concerned that the fork would inevitably begin to diverge and eventually fragment the pybind11 ecosystem. I saw this as a personal defeat, because I had set out to unify, but at the time of the role eliminations there was only the tangle of the branch and the fork.

After assessing the new realities, I first convinced myself, then my (then) colleagues, that it is best to prioritize compatibility with upstream pybind11 and ramp back the PyCLIF-pybind11 unification work, although it was at a very advanced stage. My ramp-back plan was to move the Google codebase back to the smart_holder branch, after first doing major work on the smart_holder branch itself. That goal was achieved in early August this year. The second part of the plan was to create and then merge this PR, so that the Google codebase can go back to the pybind11 master branch before I was expected to move on to new projects.

Diving one level deeper, into the "major work on the smart_holder branch itself": When the smart_holder branch was created (and deployed internally) in 2021, I chose a messy route to implementing the smart_holder functionality, trading off simplicity of implementation and ease of use for not having to change the PYBIND11_INTERNALS_VERSION. I made this choice to keep the smart_holder branch ABI-compatible with the master branch. I accepted the messiness because I expected that the work on the bigger PyCLIF-pybind11 unification project would take a significant amount of time. I never felt comfortable offering that state of the smart_holder branch for merging into master. When my situation changed drastically in April, the original tradeoff no longer made sense. Therefore I reworked the smart_holder branch (PR #5257) to have a much cleaner implementation, so that I'm comfortable offering it for merging. The price to pay is a PYBIND11_INTERNALS_VERSION bump.

Note: The PYBIND11_INTERNALS_VERSION change means that extensions built with pybind11 master no longer interoperate with extensions built with the smart_holder branch.

To solve that problem, I created PR #5296. It is the distillation of several similar but more ad-hoc solutions cooked up over the years, to enable SWIG/PyCLIF/pybind11 interoperability as required for internal migration projects.

I'm very disappointed about the inattention to this PR, and the thinly veiled death-by-committee suggestions under PR #5296, but sadly I'm not surprised. Without going into details, my subjective experience can be described as a long and consistent string of similar frustrations, big and small.

What's the root problem, in my view, as a momentarily unemployed person, mostly freed from the constraints that come with the benefits of working for Google, and not yet bound to anyone else?

The "because I said so" governance. — It brings out the worst, in the followers, and the dissenters. Based on what I see here, it is not the first time that the detached but controlling governance style has led to frustration and stagnation.

I want to remind everyone that most of the work on pybind11 was done by a very large community.

Before I log more complaints, I want to point out that I was heavily involved in the development of Boost.Python, and that I was the Boost.Python maintainer from about 2002 to 2012, at which time I handed over that role to someone else because I was joining Google. To not give a wrong impression: apart from the maintainer role, my Boost.Python involvement is accurately characterized as assisting, largely as a sort of early adopter, power user, and partner in discussions about priorities. All credit for the design and almost all of the implementation is due to David Abrahams.

Around 2002 David left his work to the community. Similarly, I saw my contributions as community work. I was very happy to see that the Boost.Python design was picked up and reimplemented in C++11. I'm also very happy to see that pybind11 is thriving, and that today the pybind11 contributor count on GitHub is >350.

Ignoring this PR, and blocking PR #5296 without any evidence of an actual code review (at the time of this writing) is irresponsible towards the large community that has contributed to Boost.Python, the pybind11 rewrite, and many of the features added in pybind11. I made it very easy to opt out of this PR (-DPYBIND11_SMART_HOLDER_DISABLE). PR #5296 adds so little code (essentially no templates), it would be nonsensical and very counterproductive to add an opt-out feature.

I'd love to bring this work to the general community as soon as possible, but I also want to be sure whatever I do here in the future is in full agreement with the goals of my new employer, and my new team.

What does that mean concretely?

Unless someone else surprisingly picks up the work here (see TODOs in the PR description), this PR will not be worked on for an unknown length of time. It very much depends on the needs of my new employer and team. I'm very sorry it is this way. Please keep using the smart_holder branch for the time being; I'll keep maintaining it, it's very low effort now.

While I very strongly disagree with the current governance, I respect it as the status quo. I've never merged a PR without consent, although I technically could. — Note though that I never had admin access to this repo. — Note also that I'll continue to review PRs here as I've done consistently over the past four years, although I've now lost my super power of being able to run Google global testing for incoming PRs.

I want to thank my ex-teams, my ex-managers, and many ex-colleagues for their unwavering support of my pybind11 contributions between June 2020 and now. (I believe it would be inappropriate to call them out by name, unfortunately.)

Everything I wrote here reflects my own experience and opinions. There is nothing "official" here, in any form or shape.

wjakob · 2024-09-09T14:44:00Z

Hi @rwgk,

I appreciate your hard work on pybind11 in the last years, and I am disappointed how Google treated you and the rest of the Python team. Your last message feels quite accusatory towards me in particular. I'm not quite sure where this is all coming from, so I want to clarify a few things:

I haven't been involved with pybind11 development for years now (the message you post as an example of my governance style is back from 2019). I still comment occasionally or clarify old design decisions when someone pings me here or on a separate pybind11 Slack instance. But I am not reviewing PRs with a maintainer hat (or merging them, for that matter). The tiny bit of oversight that I still do here is to give commit/tool access to people, and I always do so quickly (usually within hours).

I have given this project very much of my time and energy over the years, but don't feel that I owe it continuing to do so for the rest of my life. Refactoring pybind11 to fix its many design flaws while supporting the many existing users with their stringent API/ABI stability requirements seems to me like an almost impossible task. I gave up and instead started a new project.

After you pinged me, I commented on your PR #5296 with my nanobind hat on and read it as an interop proposal between pybind11 and other binding generators (which I think is a great idea and definitely needed). However, this kind of ABI contract is tricky and needs a careful specification. I did not read the code because there was no specification. It needs a specification if that is how you want the PR to be used. But reading your message above, I think you want to merge this PR mainly to allow pybind11 interop with itself to fix an issue in this PR, with other binding generators not being the main consideration. In this case, I don't think I am a good person to review the change. I certainly don't have the power to reject it, as you put it.

Back in April 2020, we had a nice meeting with a team of power-users from industry and academia, who stepped up to take on the job of maintaining pybind11. Over the years, you invited a number of additional people to help out in providing reviews. So when you criticize my governance, I would actually put this around and say that it is the governance of the team that your initiative created. I don't feel like I am to blame for this obviously frustrating situation.

henryiii · 2024-09-09T15:22:53Z

@rwgk As far as I know, of the original team, only a few of use are still actively maintaining pybind11 - primarily you and me. I think almost everyone else tagged is not active here at the moment. I'd be happy if they want to return, but priorities change and that's okay.

Reviewing a PR of this magnitude and providing feedback is not something that I can reasonably do - I have not been following the development of smart-holder at all, so I have very little idea of what the benefits and drawbacks are. I've also just started teaching last week, and still have a young baby at home, so didn't have time to do anything on a short deadline.

IMO, if you want feedback, you'll have to find someone invested in smartholder already and willing to give it. This could be current users, for example, I don't think it needs to be a "maintainer" here at all.

If you just need signoff from another "maintainer", I'm happy to give that usually, and especially for this one since I have asked for smartholder to be merged in the past, but this PR literally states it's a "preview" (the third preview). There are TODOs here too. There are also no docs in this PR, other than a note saying the docs are wrong for smartholder. I didn't think you were ready for or asking for a sign-off?

If you want a sign-off, I'd want a few high-level things: How does this affect compile time, binding size, and runtime for an extension? With that info, I think we would be able to answer the long-term question: do we want to support an opt-out and the classic system? If it's less than 10% worse, I'd say no, let's plan for long-term smartholder only. For short term, we can merge this as default-on, and see how it goes; quite a few people use the master branch, so if there's fallout, we might see it before releasing, even if we go with with the most conservative 2.x and default-off for the next release.

If we go with 3.0 and default-on, I think we can also start stripping out the older internals numbers. We should go with the last supported 2.x ABI and the new 3.0 smart-holder ABI, for example. We could even make 3.0 smartholder only and only the new ABI, I think a 3.0 release allows some freedom; 3.0 could even be non ABI compatible with the 2.x series?

If the fastest way forward is to hop on a Zoom call and you describe your plans and thoughts, I'm happy to do that.

rwgk · 2024-09-12T18:04:58Z

Hi @wjakob and @henryiii, thanks a lot for the feedback! I'll pick up working on this PR, to address the TODO's, and to get formal approvals from stakeholders at Google. Almost everything was reviewed already, and this code has been in production use at Google ever since spring 2021 (and still is; I found it impossible/impractical to operate it out).

@henryiii wrote:

If the fastest way forward is to hop on a Zoom call and you describe your plans and thoughts, I'm happy to do that.

I'll get in touch with you after working on this PR more. Might be a week or two (I still don't have a Linux workstation).

Snapshot of smart_holder branch @ f99ffd7 (Tue Aug 27 01:56:00 2024 +…

6d7e47a

…0700)

rwgk marked this pull request as ready for review August 27, 2024 16:42

rwgk requested a review from henryiii as a code owner August 27, 2024 16:42

This was referenced Aug 27, 2024

Smart Pointers for non custom types #1985

Open

feat: enable type-safe interoperability between different independent Python/C++ bindings systems. #5296

Merged

Attach python lifetime to shared_ptr passed to C++ #2839

Open

rwgk mentioned this pull request Sep 1, 2024

[smart_holder] Introduce PYBIND11_SMART_HOLDER_DISABLE option. #5348

Merged

Ralf W. Grosse-Kunstleve added 2 commits September 1, 2024 18:29

Merge branch 'master' into squash_merge_smart_holder_into_master_prev…

ba62fcd

…iew_1

rwgk changed the title ~~[Preview 1] Merge smart_holder into master (squash merge)~~ [Preview 2] Merge smart_holder into master (squash merge) Sep 2, 2024

rhaschke reviewed Sep 2, 2024

View reviewed changes

auscompgeek reviewed Sep 2, 2024

View reviewed changes

Ralf W. Grosse-Kunstleve added 2 commits September 2, 2024 11:48

Merge branch 'master' into squash_merge_smart_holder_into_master_prev…

36c319a

…iew_1

Apply ci.yml diff piggy-backed in pybind#5349

1baa98b

rwgk changed the title ~~[Preview 2] Merge smart_holder into master (squash merge)~~ [Preview 3] Merge smart_holder into master (squash merge) Sep 2, 2024

rwgk mentioned this pull request Sep 12, 2024

Added exception translator specific mutex used with try_translate_exceptions #5362

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Preview 3] Merge smart_holder into master (squash merge) #5339

[Preview 3] Merge smart_holder into master (squash merge) #5339

rwgk commented Aug 26, 2024 •

edited

Loading

petersteneteg commented Sep 1, 2024

rwgk commented Sep 1, 2024 •

edited

Loading

rhaschke left a comment

rhaschke Sep 2, 2024

rhaschke Sep 2, 2024

rwgk Sep 2, 2024

rwgk Sep 2, 2024

rhaschke Sep 2, 2024

rwgk Sep 2, 2024

rhaschke Sep 2, 2024

rwgk Sep 2, 2024

rwgk Sep 3, 2024

wjakob Sep 3, 2024

rwgk Sep 3, 2024

auscompgeek Sep 2, 2024

rwgk Sep 2, 2024

rwgk commented Sep 2, 2024

rwgk commented Sep 7, 2024

wjakob commented Sep 9, 2024

henryiii commented Sep 9, 2024

rwgk commented Sep 12, 2024 •

edited

Loading

	group: test-sh-avl${{ github.ref }}
	group: test-${{ github.ref }}

[Preview 3] Merge smart_holder into master (squash merge) #5339

Are you sure you want to change the base?

[Preview 3] Merge smart_holder into master (squash merge) #5339

Conversation

rwgk commented Aug 26, 2024 • edited Loading

Description

Suggested changelog entry:

petersteneteg commented Sep 1, 2024

rwgk commented Sep 1, 2024 • edited Loading

rhaschke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwgk commented Sep 2, 2024

rwgk commented Sep 7, 2024

wjakob commented Sep 9, 2024

henryiii commented Sep 9, 2024

rwgk commented Sep 12, 2024 • edited Loading

rwgk commented Aug 26, 2024 •

edited

Loading

rwgk commented Sep 1, 2024 •

edited

Loading

rwgk commented Sep 12, 2024 •

edited

Loading