Skip to content
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.

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

[workspace] Bump pybind11 fork to latest commit (merge w/ upstream v2.11.1) #20362

Conversation

EricCousineau-TRI
Copy link
Contributor

@EricCousineau-TRI EricCousineau-TRI commented Oct 13, 2023

Requires RobotLocomotion/pybind11#64


This change is Reviewable

Copy link
Contributor Author

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

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

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

a discussion (no related file):
Working: Failures due to bad implicit conversion (I think)
//bindings/pydrake/common:py/type_safe_index_pybind_test


a discussion (no related file):
Working: Failures due to something going awry with rvalue casting, e.g.

$ bazel test //bindings/pydrake/multibody:py/plant_test
...
RuntimeError: Unable to cast Python <class 'plant_test.AppliedForceTestSystem_𝓣AutoDiffXd𝓤'> instance to C++ rvalue: instance has multiple references (#define PYBIND11_DETAILED_ERROR_MESSAGES or compile in debug mode for details)
...

@EricCousineau-TRI

This comment was marked as resolved.

Copy link
Contributor Author

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

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

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 @EricCousineau-TRI)

a discussion (no related file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Working: Failures due to bad implicit conversion (I think)
//bindings/pydrake/common:py/type_safe_index_pybind_test

Done.


a discussion (no related file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Working: Failures due to something going awry with rvalue casting, e.g.

$ bazel test //bindings/pydrake/multibody:py/plant_test
...
RuntimeError: Unable to cast Python <class 'plant_test.AppliedForceTestSystem_𝓣AutoDiffXd𝓤'> instance to C++ rvalue: instance has multiple references (#define PYBIND11_DETAILED_ERROR_MESSAGES or compile in debug mode for details)
...

Done.


a discussion (no related file):
Working: Requires testing for Anzu.


Copy link
Contributor Author

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

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

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

a discussion (no related file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Working: Requires testing for Anzu.

Done. See Anzu PR 10993.


Copy link
Contributor Author

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

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

+@jwnimmer-tri and +@rpoyner-tri for review, please

Reviewable status: LGTM missing from assignees jwnimmer-tri(platform),rpoyner-tri(platform), missing label for release notes (waiting on @EricCousineau-TRI)

Copy link
Contributor Author

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 unresolved discussion, LGTM missing from assignees rpoyner-tri(platform),jwnimmer-tri(platform), missing label for release notes (waiting on @EricCousineau-TRI)

a discussion (no related file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Done. See Anzu PR 10993.

Update PR once more to doubly confirm.


Copy link
Contributor Author

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

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

Reviewable status: LGTM missing from assignees rpoyner-tri(platform),jwnimmer-tri(platform), missing label for release notes (waiting on @EricCousineau-TRI)

a discussion (no related file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Update PR once more to doubly confirm.

Done. Same PR - Anzu 10993


@EricCousineau-TRI EricCousineau-TRI added the release notes: fix This pull request contains fixes (no new features) label Nov 2, 2023
Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 3 files at r1, 1 of 2 files at r2, 7 of 7 files at r3, all commit messages.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee rpoyner-tri(platform)

a discussion (no related file):
Working

To be cautious, we should fire off a wheel build before landing this (macOS Arm Wheel is the best choice). I can do that after pushes / review are finished.



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

#  https://github.com/RobotLocomotion/pybind11/blob/drake/include/pybind11/detail/common.h
# and if it has changed, then update the version number in the two
# pybind11-*.cmake files in the current directory to match.

We're at 2.11.1 now in the RLG/pybind11 fork, so this cmake stuff mentioned in the comment here needs to bump up.

@EricCousineau-TRI EricCousineau-TRI force-pushed the feature-drake-pybind-pr64 branch from 4b82525 to 14da014 Compare November 2, 2023 16:13
Copy link
Contributor Author

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

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

Reviewable status: 2 unresolved discussions, LGTM missing from assignee rpoyner-tri(platform)


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

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

We're at 2.11.1 now in the RLG/pybind11 fork, so this cmake stuff mentioned in the comment here needs to bump up.

Done.

Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

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

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

@jwnimmer-tri
Copy link
Collaborator

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Working

To be cautious, we should fire off a wheel build before landing this (macOS Arm Wheel is the best choice). I can do that after pushes / review are finished.

@drake-jenkins-bot mac-arm-monterey-unprovisioned-clang-wheel-experimental-release please

Copy link
Contributor Author

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

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

Reviewable status: 2 unresolved discussions, LGTM missing from assignee rpoyner-tri(platform)

a discussion (no related file):
Working: I also have an updated PR to try and make the pybind11 fork CI pass:
RobotLocomotion/pybind11#67

Once CI passes there, I will request (rubber stamp) review then merge it.
For us, functionally, it may not affect things, but may good to use pin with all CI green.

WDYT?


Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

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

Reviewable status: 2 unresolved discussions, LGTM missing from assignee rpoyner-tri(platform)

a discussion (no related file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Working: I also have an updated PR to try and make the pybind11 fork CI pass:
RobotLocomotion/pybind11#67

Once CI passes there, I will request (rubber stamp) review then merge it.
For us, functionally, it may not affect things, but may good to use pin with all CI green.

WDYT?

Let's discuss at RobotLocomotion/pybind11#67. If that PR merges, then we'll need to bump Drake to use it.


Copy link
Contributor

@rpoyner-tri rpoyner-tri left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 3 files at r1, 1 of 2 files at r2, 7 of 7 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status: 2 unresolved discussions

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

@drake-jenkins-bot mac-arm-monterey-unprovisioned-clang-wheel-experimental-release please

:lgtm: pending tests


Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 unresolved discussion

a discussion (no related file):

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

:lgtm: pending tests

OK wheel smoke build passed.


Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 unresolved discussion

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Let's discuss at RobotLocomotion/pybind11#67. If that PR merges, then we'll need to bump Drake to use it.

If you'd like to merge this PR as-is, and then re-bump again later, that's also OK by me.


Copy link
Contributor Author

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 unresolved discussion

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

If you'd like to merge this PR as-is, and then re-bump again later, that's also OK by me.

Done. Will avoid the churn for now.


a discussion (no related file):
Sorry, one more: RobotLocomotion/pybind11#66
This is a small bugfix only really relevant to the fork pybind11 CI.
I'd like to merge it, though, just to keep things somewhat aligned.

It should also not affect testing that has already been done (beyond nominal PR testing)


@EricCousineau-TRI EricCousineau-TRI force-pushed the feature-drake-pybind-pr64 branch from 14da014 to 64bb3d7 Compare November 2, 2023 17:17
Copy link
Contributor Author

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

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

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

a discussion (no related file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Sorry, one more: RobotLocomotion/pybind11#66
This is a small bugfix only really relevant to the fork pybind11 CI.
I'd like to merge it, though, just to keep things somewhat aligned.

It should also not affect testing that has already been done (beyond nominal PR testing)

Done.


Copy link
Contributor Author

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 unresolved discussion


tools/workspace/pybind11/repository.bzl line 11 at r5 (raw file):

# and if it has changed, then update the version number in the two
# pybind11-*.cmake files in the current directory to match.

Working: Oops, extra newline.

This merges in pybind11 upstream's v2.11.1

Removes need for monostate_pybind.h
Changes error types for certain tests
@EricCousineau-TRI EricCousineau-TRI force-pushed the feature-drake-pybind-pr64 branch from 64bb3d7 to 977574d Compare November 2, 2023 17:19
Copy link
Contributor Author

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

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

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


tools/workspace/pybind11/repository.bzl line 11 at r5 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Working: Oops, extra newline.

Done.

Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: :shipit: complete! all discussions resolved, LGTM from assignees rpoyner-tri(platform),jwnimmer-tri(platform)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: fix This pull request contains fixes (no new features)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants