[pybind11] Switch back to vanilla pybind11

[rpoyner-tri]

This patch drops the special dependency on the RobotLocomotion/pybind11 fork, and instead uses vanilla pybind11 with local patch files.

---

This change is 

[rpoyner-tri]

Toward #21968.

Reviewable status: needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", missing label for release notes (waiting on @rpoyner-tri)

[rpoyner-tri]

I believe the plan is to wait to try to merge this until after this month's release: v1.39.0.

Reviewable status: needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", missing label for release notes (waiting on @rpoyner-tri)

[rpoyner-tri]

Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", missing label for release notes (waiting on @rpoyner-tri)

---

a discussion (no related file):
Looks like macos is sad about the patches because of numpy version differences. Investigating.

[rpoyner-tri]

Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", missing label for release notes (waiting on @rpoyner-tri)

---

a discussion (no related file):

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

Looks like macos is sad about the patches because of numpy version differences. Investigating.

r2 may fix it. Let's see what CI says.

[rpoyner-tri]

-(status: do not review) +(release notes: none) (?) +@jwnimmer-tri for feature review.

Reviewable status: LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge" (waiting on @rpoyner-tri)

---

a discussion (no related file):

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

r2 may fix it. Let's see what CI says.

All good now.

[jwnimmer-tri]

I think probably +(release notes: fix) -(release notes: none) because users who are using our pybind11 to write their own bindings will have some probability of not working anymore.

Reviewed 4 of 5 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: 5 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge" (waiting on @rpoyner-tri)

---

a discussion (no related file):
I see you launched the Anzu test already, thanks! There is some compile problem now, though. I assume we just need one or two more bindings tweaks downstream.

---

tools/workspace/pybind11/patches/shared_ptr_lifetime.patch line 7 at r2 (raw file):

These changes were derived from the PR at pybind/pybind11#2839, with
heavy rebasing for compatibility with more current pybind releases.

BTW Chasing some citations from that starting point, it seems possible that the recently merged pybind/pybind11#5542 was designed to solve this problem, and once we upgrade we could switch to using its annotation to help us. Maybe add a TODO here?

---

tools/workspace/pybind11/patches/eigen_object_matrices.patch line 556 at r2 (raw file):

     m.def(
--- tests/test_eigen_matrix.py
+++ tests/test_eigen_matrix.py

Working

I probably support keeping the test changes rolled into our patches, but want to think a little bit about whether/how we can run them. Doing real CI is probably too much work, but possibly we should give manual instructions for running. Or maybe all we need is to document in the patchsets that the tests in the patches are never run.

---

tools/workspace/pybind11/repository.bzl line 4 at r2 (raw file):

load("//tools/workspace:github.bzl", "github_archive")

# Using the `drake` branch of this repository.

nit Stale comment.

---

tools/workspace/pybind11/repository.bzl line 8 at r2 (raw file):

# When upgrading this commit, check the version header within
#  https://github.com/RobotLocomotion/pybind11/blob/drake/include/pybind11/detail/common.h

nit Stale comment.

[rpoyner-tri]

Reviewable status: 5 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge"

---

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I see you launched the Anzu test already, thanks! There is some compile problem now, though. I assume we just need one or two more bindings tweaks downstream.

Indeed. Working.

---

tools/workspace/pybind11/patches/shared_ptr_lifetime.patch line 7 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW Chasing some citations from that starting point, it seems possible that the recently merged pybind/pybind11#5542 was designed to solve this problem, and once we upgrade we could switch to using its annotation to help us. Maybe add a TODO here?

Without having done any testing, I suspect the smart pointer branch code is much preferred.

[rpoyner-tri]

Reviewable status: 5 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge"

---

a discussion (no related file):

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

Indeed. Working.

https://github.shared-services.aws.tri.global/robotics/anzu/pull/15814 should do the trick.

[jwnimmer-tri]

Reviewed 3 of 3 files at r3.
Reviewable status: 6 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge" (waiting on @rpoyner-tri)

---

tools/workspace/pybind11/repository.bzl line 4 at r3 (raw file):

load("//tools/workspace:github.bzl", "github_archive")

# When upgrading the location of upstream sources, check the version header

nit Clarity

Suggestion:

Any time you change either the _REPOSITORY or _COMMIT below,

[jwnimmer-tri]

feature.

+@SeanCurtis-TRI for platform review per schedule, please.

Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), labeled "do not merge" (waiting on @rpoyner-tri)

---

tools/workspace/pybind11/patches/eigen_object_matrices.patch line 556 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Working

I probably support keeping the test changes rolled into our patches, but want to think a little bit about whether/how we can run them. Doing real CI is probably too much work, but possibly we should give manual instructions for running. Or maybe all we need is to document in the patchsets that the tests in the patches are never run.

I guess I don't really care which route we go, but we should at least do one of them.

Probably fastest for now would be to just put a comment atop this patch file noting that is adds test code which is never run anywhere. Later if we decide we need to run the test, we can add that in later and update the comment.

[SeanCurtis-TRI]

With some superficial stuff in the patches.

Reviewed 2 of 5 files at r1, 1 of 1 files at r2, 2 of 3 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status: 4 unresolved discussions, labeled "do not merge" (waiting on @rpoyner-tri)

---

tools/workspace/pybind11/patches/eigen_object_matrices.patch line 556 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I guess I don't really care which route we go, but we should at least do one of them.

Probably fastest for now would be to just put a comment atop this patch file noting that is adds test code which is never run anywhere. Later if we decide we need to run the test, we can add that in later and update the comment.

THe shared ptr patch likewise does tests -- so a similar comment there as well?

---

tools/workspace/pybind11/patches/eigen_object_matrices.patch line 83 at r4 (raw file):

+            a = array({ src.rows(), src.cols() }, { elem_size * src.rowStride(), elem_size * src.colStride() },
+                      src.data(), base);
+    }

BTW Not incredibly important, but I'm somewhat surprised by the formatting. It doesn't match the formatting it's replacing, nor Drake's standards. (By this, I mean the else { on a trailing line from the closing }.

This occurs here and below.

---

tools/workspace/pybind11/patches/eigen_object_matrices.patch line 91 at r4 (raw file):

+        throw cast_error(
+            "dtype=object does not permit array referencing. "
+            "(NOTE: this generally not be reachable, as upstream APIs "

BTW Quibbling about the grammar of an ostensibly unreachable error message, but I wouldn't be me if I didn't at least point it out.

And as we're delaying merging until after the next release, choosing to correct it doesn't hurt us.

[rpoyner-tri]

Reviewable status: 1 unresolved discussion, labeled "do not merge" (waiting on @rpoyner-tri)

---

tools/workspace/pybind11/patches/eigen_object_matrices.patch line 83 at r4 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW Not incredibly important, but I'm somewhat surprised by the formatting. It doesn't match the formatting it's replacing, nor Drake's standards. (By this, I mean the else { on a trailing line from the closing }.

This occurs here and below.

I had speculated that this formatting exercise might be a waste of time. However, I learned something missed before. Pybind's array type has a mutable_unchecked template method for fast element access.

[rpoyner-tri]

Reviewable status: 1 unresolved discussion, labeled "do not merge" (waiting on @rpoyner-tri)

---

a discussion (no related file):

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

https://github.shared-services.aws.tri.global/robotics/anzu/pull/15814 should do the trick.

Done.

[rpoyner-tri]

Reviewable status: 1 unresolved discussion, labeled "do not merge" (waiting on @rpoyner-tri)

---

tools/workspace/pybind11/patches/eigen_object_matrices.patch line 83 at r4 (raw file):

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

I had speculated that this formatting exercise might be a waste of time. However, I learned something missed before. Pybind's array type has a mutable_unchecked template method for fast element access.

FTR, the pybind code seems to limit lines to around 100 characters. The patches in RobotLocomotion/pybind11 have tended toward 80, but are inconsistent.

[jwnimmer-tri]

Reviewed 1 of 2 files at r5, all commit messages.
Reviewable status: labeled "do not merge" (waiting on @rpoyner-tri)

---

a discussion (no related file):

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

Done.

Confirmed with rebuild Anzu-master-Drake-PR/299.

---

tools/workspace/pybind11/patches/eigen_object_matrices.patch line 83 at r4 (raw file):

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

FTR, the pybind code seems to limit lines to around 100 characters. The patches in RobotLocomotion/pybind11 have tended toward 80, but are inconsistent.

I can't find my way through the reformatting mixed with functional changes, so I guess I'll just trust CI to tell us if all is well.

[rpoyner-tri]

Reviewable status: labeled "do not merge" (waiting on @rpoyner-tri)

---

tools/workspace/pybind11/patches/eigen_object_matrices.patch line 83 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I can't find my way through the reformatting mixed with functional changes, so I guess I'll just trust CI to tell us if all is well.

I essentially copied the HEAD version of matrix.h from the RL fork, reviewed the changes (specifically the diferences from my first iteration), and reconstructed the patch.

[rpoyner-tri]

r6 reapplies grammar fix lost in reformatting changes.

Reviewable status: labeled "do not merge" (waiting on @rpoyner-tri)

[jwnimmer-tri]

Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: labeled "do not merge" (waiting on @rpoyner-tri)

[SeanCurtis-TRI]

Reviewed 1 of 2 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status: labeled "do not merge" (waiting on @rpoyner-tri)

---

tools/workspace/pybind11/patches/eigen_object_matrices.patch line 83 at r4 (raw file):

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

I essentially copied the HEAD version of matrix.h from the RL fork, reviewed the changes (specifically the diferences from my first iteration), and reconstructed the patch.

I'm taking the same view as Jeremy.

[rpoyner-tri]

-(status: do not merge) since the release has gone out.

Reviewable status: labeled "do not merge" (waiting on @rpoyner-tri)

[rpoyner-tri]

Note to release notes authors: This PR effectively removes some of the fancy support for unique_ptr (both explicit and implicit) in bindings. Downstream projects that rely on drake's pybind may need to adjust either bindings or their dependencies.

Reviewable status: complete! all discussions resolved, LGTM from assignees jwnimmer-tri(platform),SeanCurtis-TRI(platform)