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

Find correct python3 version in pybind11-config #22333

Merged

Conversation

nicolecheetham
Copy link
Contributor

@nicolecheetham nicolecheetham commented Dec 18, 2024

Improves logic so Python3 will not be set by pybind11-config file unless a user has not already set the python version

Resolves #22051


This change is Reviewable

Copy link
Contributor Author

@nicolecheetham nicolecheetham left a comment

Choose a reason for hiding this comment

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

+@BetsyMcPhail for feature review please

Reviewable status: LGTM missing from assignee BetsyMcPhail, needs platform reviewer assigned, needs at least two assigned reviewers, missing label for release notes (waiting on @nicolecheetham)

@nicolecheetham nicolecheetham added the release notes: none This pull request should not be mentioned in the release notes label Dec 19, 2024
Copy link
Contributor

@BetsyMcPhail BetsyMcPhail 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 1 files at r1, all commit messages.
Reviewable status: needs platform reviewer assigned, needs at least two assigned reviewers

@jwnimmer-tri jwnimmer-tri self-assigned this Dec 19, 2024
@jwnimmer-tri
Copy link
Collaborator

I'll take on the platform review here.

@jwnimmer-tri
Copy link
Collaborator

The claim in the overview is "Resolves 22051". What evidence do we have to prove it is resolved?

The pre-merge tests do not seem to have any leverage to over the resolution (since no test code or configs were changed here), so in that case it is incumbent upon the PR author to explain in the pull review text somwhere what testing they performed to confirm the fix, presumably either as links to online test runs against this branch, or list(s) of manual commands that were run locally to confirm what happens.

If 22051 had a reproduction recipe clearly outlined then I could try it myself, but I couldn't tease out any reproducer from the posting.

@nicolecheetham
Copy link
Contributor Author

@jwnimmer-tri I was thinking of procuring evidence by testing the drake-ros repo as Betsy brought to my attention it had the pybind11 issue in the past. Is there a way to locally test the drake-ros repository? Or do I have create an PR to test?

@jwnimmer-tri
Copy link
Collaborator

jwnimmer-tri commented Dec 19, 2024

There is run_all_tests.sh but per the TODOs it doesn't actually run everything. (It's also possible that it has bitrotted, since I don't think it has any CI coverage.) But you probably don't need to test everything anyway -- the git repo has a bunch of different modules (kinda like drake-external-examples) and I imagine only one or two of them are relevant to this change. If you can find even one build or test that is failing when run vs Drake master, but passing vs this pull request, that would be sufficient.

Or, it's certainly fine to open a pull request against drake-ros if that is helpful for CI testing. Just mark it as "draft" and comment that it's only for some testing so nobody starts accidentally reviewing it. However, beware that some parts of drake-ros CI are probably known to be broken (unrelated to this Python3 problem), so also beware of false negatives.

In any case, if you had a bug reproducer that didn't involved drake-ros, that would probably be the easiest thing to test. I would hope that some project in drake-external-examples could demonstrate the bug, probably with a few tweaks to its CMakeLists.txt to be able to hit the problem.

@nicolecheetham
Copy link
Contributor Author

Local testing was performed to confirm the fix.
Steps Taken (tri mac mini 1):

  • cd drake-external-examples/drake_cmake_installed

  • ./setup/install_prereqs

  • ./.github/ci_build_test #Check to make sure tests work properly as is

  • Change to home directory:

  • cd /opt/drake/lib/cmake/pybind11

  • vim pybind11-config.cmake #Made change to find_package for python3

  • Change directory back to drake-external-examples/drake_cmake_installed

  • ./.github/ci_build_test #Check to see tests still work after change

  • Edit drake_cmake_installed Cmake code so find_package(pybind11) command occurred prior find_package(Python3)

  • ./.github/ci_build_test #Check to see if tests pass meaning known pybind11-config error was resolved

All three times, the ci_build_test succeeded
Aside: the build_test failed when pybind11 was prior to find_package(Python3) and no edit had been made to the local version of pybind11-config.cmake. This was the expected result

@jwnimmer-tri
Copy link
Collaborator

jwnimmer-tri commented Jan 20, 2025

Okay, me say that back to you to make sure I got it.

  • You have a list of manual commands to run to test this PR.
  • When you run those commands against the master branch, then the example doesn't work correctly.
  • When you run those commands against this PR branch, then the example does work correctly.

Assuming I've got that right, then :lgtm:.

@nicolecheetham nicolecheetham changed the title Find Python3 if not set in pybind11-config Find correct python3 version in pybind11-config Jan 21, 2025
Copy link
Contributor Author

@nicolecheetham nicolecheetham left a comment

Choose a reason for hiding this comment

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

Yes, that is correct

Reviewable status: commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @nicolecheetham)

@jwnimmer-tri jwnimmer-tri merged commit 9ef3e21 into RobotLocomotion:master Jan 21, 2025
9 of 10 checks passed
@jwnimmer-tri jwnimmer-tri added status: squashing now https://drake.mit.edu/reviewable.html#curated-commits release notes: fix This pull request contains fixes (no new features) and removed release notes: none This pull request should not be mentioned in the release notes labels Jan 21, 2025
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) status: squashing now https://drake.mit.edu/reviewable.html#curated-commits
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pybind11 python modes aren't supported as expected
3 participants