[pydrake] Centralize pybind11 includes for ODR safety #19571

jwnimmer-tri · 2023-06-10T16:13:13Z

For convenience and easier ODR-safety, consolidate the common pybind11 includes (especially the includes that specialize trait structs) into our central pydrake_pybind.h header. This avoids hazards where some modules forget to include the proper headers and experience compile-time or runtime failures as a result.

Towards #19250.

Tested locally for possible performance regressions (see below).

Checking for performance regression of a full rebuild:

Build time on master:

git checkout master
bazel build //bindings/...
patch pybind11/package.BUILD.bazel to force a bindings rebuild [1]
time bazel build //bindings/...

Result:

INFO: Elapsed time: 170.486s, Critical Path: 135.65s
INFO: 99 processes: 1 internal, 98 linux-sandbox.

Build time on pull request:

git checkout pydrake-canonical-includes
bazel build //bindings/...
patch pybind11/package.BUILD.bazel to force a bindings rebuild [1]
time bazel build //bindings/...

Result:

INFO: Elapsed time: 151.502s, Critical Path: 150.37s
INFO: 99 processes: 1 internal, 98 linux-sandbox.

I believe the critical path changes are just the luck of when multibody_tree_py.cc starts compiling.

[1] the patch:

--- a/tools/workspace/pybind11/package.BUILD.bazel
+++ b/tools/workspace/pybind11/package.BUILD.bazel
@@ -43,6 +43,7 @@ cc_library(
         "include/pybind11/stl_bind.h",
     ],
     includes = ["include"],
+    defines = ["TRIGGER_REBUILD=YES_PLEASE_master"],
     deps = [
         "@eigen",
         "@python",

Checking performance of a single-file incremental rebuild:

Neither of these two targets show any performance slow-down of a rebuild with the extra included headers:

bazel build //bindings/pydrake/common:schema_py
bazel build //bindings/pydrake/common:eigen_geometry_py

This change is

jwnimmer-tri · 2023-06-12T14:31:53Z

+@xuchenhan-tri for both reviews per schedule, please.

\CC @EricCousineau-TRI in case you'd like to weigh in

xuchenhan-tri

with one question:

Can you clarify the criterion for what includes make the cut to pydrake_pybind.h? For example, I see that "pybind11/eval.h" is excluded and not sure why. Is it not common enough? Does that undermines ODR-safety goal that we are trying to achieve here?

Reviewed 100 of 100 files at r1, all commit messages.
Reviewable status: complete! all discussions resolved, LGTM from assignee xuchenhan-tri(platform)

jwnimmer-tri

Reviewable status: 1 unresolved discussion

a discussion (no related file):
Working

Can you clarify the criterion for what includes make the cut to pydrake_pybind.h? For example, I see that "pybind11/eval.h" is excluded and not sure why. Is it not common enough? Does that undermines ODR-safety goal that we are trying to achieve here?

For convenience and easier ODR-safety, consolidate the common pybind11 includes (especially the includes that specialize trait structs) into our central pydrake_pybind header. This avoids hazards where some modules forget to include the proper headers and experience compile- time or runtime failures as a result.

jwnimmer-tri

+@rpoyner-tri for platform review of bindings/pydrake/pydrake_pybind.h only, please -(status: single reviewer ok), to check the clarity there with >1 person.

Reviewable status: LGTM missing from assignee rpoyner-tri(platform)

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Working

Can you clarify the criterion for what includes make the cut to pydrake_pybind.h? For example, I see that "pybind11/eval.h" is excluded and not sure why. Is it not common enough? Does that undermines ODR-safety goal that we are trying to achieve here?

Done.

xuchenhan-tri

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: LGTM missing from assignee rpoyner-tri(platform)

rpoyner-tri

with some follow-up questions

Reviewed 98 of 100 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: 1 unresolved discussion

bindings/pydrake/pydrake_pybind.h line 5 at r2 (raw file):

#include <utility>

// Here we include a lot of the pybind11 API, to ensure that all code in pydrake

btw I'm satisfied that this patch does the things the comment describes:

includes type_caster headers
includes ADL headers
does not include headers we don't need (yet?)
does not include headers that not involved in specializations

It looks then to me like I should verify that this header is seen by every pydrake C++ translation unit.

Are there any correctness invariants that I've missed?

jwnimmer-tri

Reviewable status: 1 unresolved discussion

bindings/pydrake/pydrake_pybind.h line 5 at r2 (raw file):

Are there any correctness invariants that I've missed?

Those seem like the right ones to me.

It looks then to me like I should verify that this header is seen by every pydrake C++ translation unit.

Every TU that includes pybind11 stuff, yes. If there is any C++ code in pydrake that doesn't use pybind11 at all, it would be safe without including this.

rpoyner-tri

Reviewable status: 1 unresolved discussion

bindings/pydrake/pydrake_pybind.h line 5 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Are there any correctness invariants that I've missed?

Those seem like the right ones to me.

It looks then to me like I should verify that this header is seen by every pydrake C++ translation unit.

Every TU that includes pybind11 stuff, yes. If there is any C++ code in pydrake that doesn't use pybind11 at all, it would be safe without including this.

There is one TU that might be on the bubble, and maybe has escaped attention so far: bindings/bazel_workaround_4594_libdrake_py.cc

Clearly it's an outlier and maybe doesn't need this header reform. I'm not quite far enough in the headspace to make a judgment. WDYT?

jwnimmer-tri

Reviewable status: 1 unresolved discussion

bindings/pydrake/pydrake_pybind.h line 5 at r2 (raw file):

Previously, rpoyner-tri (Rick Poyner (rico)) wrote…

There is one TU that might be on the bubble, and maybe has escaped attention so far: bindings/bazel_workaround_4594_libdrake_py.cc

Clearly it's an outlier and maybe doesn't need this header reform. I'm not quite far enough in the headspace to make a judgment. WDYT?

Since it doesn't define any types (only a module doc), I think it poses no risk.

My guess is that trying to capture that in a comment somewhere would cause more confusion that help. WDYT?

rpoyner-tri

Reviewable status: 1 unresolved discussion

bindings/pydrake/pydrake_pybind.h line 5 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Since it doesn't define any types (only a module doc), I think it poses no risk.

My guess is that trying to capture that in a comment somewhere would cause more confusion that help. WDYT?

It's on the bubble -- a pretty trivial module. I was thinking to just use the drake_pybind header for consistency across the board, and not comment on it.

jwnimmer-tri

Reviewable status: 1 unresolved discussion

bindings/pydrake/pydrake_pybind.h line 5 at r2 (raw file):

Previously, rpoyner-tri (Rick Poyner (rico)) wrote…

It's on the bubble -- a pretty trivial module. I was thinking to just use the drake_pybind header for consistency across the board, and not comment on it.

Given that the point of that module is to not be infected with pydrake (see bindings/BUILD.bazel docs), I am skeptical about using a pydrake header there. The compiler might allow it, but I still come down on more-confusing-than-helpful for that change as well.

rpoyner-tri

Reviewable status: complete! all discussions resolved, LGTM from assignees rpoyner-tri(platform),xuchenhan-tri(platform)

bindings/pydrake/pydrake_pybind.h line 5 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Given that the point of that module is to not be infected with pydrake (see bindings/BUILD.bazel docs), I am skeptical about using a pydrake header there. The compiler might allow it, but I still come down on more-confusing-than-helpful for that change as well.

I'm good then.

rpoyner-tri · 2023-06-13T15:34:39Z

@drake-jenkins-bot linux-focal-clang-bazel-experimental-everything-release please.

…n#19571) For convenience and easier ODR-safety, consolidate the common pybind11 includes (especially the includes that specialize trait structs) into our central pydrake_pybind header. This avoids hazards where some modules forget to include the proper headers and experience compile- time or runtime failures as a result.

jwnimmer-tri added priority: low status: single reviewer ok https://drake.mit.edu/reviewable.html release notes: none This pull request should not be mentioned in the release notes labels Jun 10, 2023

jwnimmer-tri mentioned this pull request Jun 10, 2023

[workspace] Switch pybind11 to upstream #19250

Closed

5 tasks

jwnimmer-tri assigned xuchenhan-tri Jun 12, 2023

jwnimmer-tri force-pushed the pydrake-canonical-includes branch from 2b5211e to 463e6d2 Compare June 12, 2023 17:23

xuchenhan-tri reviewed Jun 12, 2023

View reviewed changes

jwnimmer-tri commented Jun 12, 2023

View reviewed changes

jwnimmer-tri force-pushed the pydrake-canonical-includes branch from 463e6d2 to ef58f8f Compare June 12, 2023 20:42

jwnimmer-tri removed the status: single reviewer ok https://drake.mit.edu/reviewable.html label Jun 12, 2023

jwnimmer-tri assigned rpoyner-tri Jun 12, 2023

jwnimmer-tri commented Jun 12, 2023

View reviewed changes

xuchenhan-tri reviewed Jun 12, 2023

View reviewed changes

rpoyner-tri reviewed Jun 12, 2023

View reviewed changes

jwnimmer-tri commented Jun 12, 2023

View reviewed changes

rpoyner-tri reviewed Jun 13, 2023

View reviewed changes

jwnimmer-tri commented Jun 13, 2023

View reviewed changes

rpoyner-tri reviewed Jun 13, 2023

View reviewed changes

jwnimmer-tri commented Jun 13, 2023

View reviewed changes

rpoyner-tri reviewed Jun 13, 2023

View reviewed changes

jwnimmer-tri merged commit efff8ba into RobotLocomotion:master Jun 13, 2023

jwnimmer-tri deleted the pydrake-canonical-includes branch June 13, 2023 16:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[pydrake] Centralize pybind11 includes for ODR safety #19571

[pydrake] Centralize pybind11 includes for ODR safety #19571

jwnimmer-tri commented Jun 10, 2023 •

edited

Loading

jwnimmer-tri commented Jun 12, 2023

xuchenhan-tri left a comment

jwnimmer-tri left a comment

jwnimmer-tri left a comment

xuchenhan-tri left a comment

rpoyner-tri left a comment

jwnimmer-tri left a comment

rpoyner-tri left a comment

jwnimmer-tri left a comment

rpoyner-tri left a comment

jwnimmer-tri left a comment

rpoyner-tri left a comment

rpoyner-tri commented Jun 13, 2023

[pydrake] Centralize pybind11 includes for ODR safety #19571

[pydrake] Centralize pybind11 includes for ODR safety #19571

Conversation

jwnimmer-tri commented Jun 10, 2023 • edited Loading

jwnimmer-tri commented Jun 12, 2023

xuchenhan-tri left a comment

Choose a reason for hiding this comment

jwnimmer-tri left a comment

Choose a reason for hiding this comment

jwnimmer-tri left a comment

Choose a reason for hiding this comment

xuchenhan-tri left a comment

Choose a reason for hiding this comment

rpoyner-tri left a comment

Choose a reason for hiding this comment

jwnimmer-tri left a comment

Choose a reason for hiding this comment

rpoyner-tri left a comment

Choose a reason for hiding this comment

jwnimmer-tri left a comment

Choose a reason for hiding this comment

rpoyner-tri left a comment

Choose a reason for hiding this comment

jwnimmer-tri left a comment

Choose a reason for hiding this comment

rpoyner-tri left a comment

Choose a reason for hiding this comment

rpoyner-tri commented Jun 13, 2023

jwnimmer-tri commented Jun 10, 2023 •

edited

Loading