Nanobind transition#24749
Conversation
For nanobind, the __setstate__ constructor must be bound in C++, not overridden in Python.
rpoyner-tri
left a comment
rpoyner-tri
left a comment
There was a problem hiding this comment.
@rpoyner-tri partially reviewed 1 file.
Reviewable status: 15 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on jwnimmer-tri).
rpoyner-tri
left a comment
rpoyner-tri
left a comment
There was a problem hiding this comment.
@rpoyner-tri reviewed all commit messages.
Reviewable status: 15 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on jwnimmer-tri).
Python subclasses should be able to override DoDeclareCacheEntries and DoDeclareInputPorts, but previously they could not.
jwnimmer-tri
left a comment
jwnimmer-tri
left a comment
There was a problem hiding this comment.
@jwnimmer-tri reviewed 30 files and all commit messages, made 11 comments, and resolved 6 discussions.
Reviewable status: 10 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on rpoyner-tri).
bindings/pydrake/geometry/geometry_py_render.cc line 138 at r7 (raw file):
This code has no tests.
Correction: has no tests of omitting the method in the Python subclass.
In-progress now at #24763.
bindings/pydrake/geometry/geometry_py_render.cc line 142 at r7 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Working
This code has no tests.
Done. Removed in #24762.
bindings/pydrake/multibody/tree_py.cc line 1287 at r7 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Working
This override (and the one below) are not covered by any unit tests. It was missed in #23255.
Even if I amend the current unit test to call
plant.mutable_deformable_model().AddExternalForce(dut), the override is not ever called (even on the C++ side).
In-progress now at #24764.
bindings/pydrake/systems/analysis_py.cc line 298 at r7 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Working
Cherry-picked in #24751.
Done.
bindings/pydrake/systems/framework_py_systems.cc line 340 at r7 (raw file):
This code has no tests.
Correction: has no tests of omitting the method in the Python subclass.
(The new test actually passes on this branch already, because for this particular NVI method, we bind the Do... method to be callable by Python. Its implementation is not a stub -- the derived class is always supposed to call the base class.)
In-progress now at #24763.
bindings/pydrake/systems/framework_py_systems.cc line 365 at r7 (raw file):
This code has no tests.
Correction: has no tests of omitting the method in the Python subclass.
(The new test actually passes on this branch already, because for this particular NVI method, we bind the Do... method to be callable by Python. Its implementation is not a stub -- the derived class is always supposed to call the base class.)
In-progress now at #24763.
bindings/pydrake/multibody/inverse_kinematics_py_differential.cc line 338 at r7 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Working
Cherry-picked in #24751.
Done.
bindings/pydrake/multibody/plant_py.cc line 1694 at r7 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Working
Cherry-picked in #24757.
Done
solvers/solver_options.cc line 65 at r1 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Working
Cherry-picked in #24750.
Done.
systems/analysis/simulator.h line 277 at r1 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Working
Cherry-picked in #24751.
Done.
a discussion (no related file):
Working
We need to do something about DRAKE_PYBIND11_CHECK_SIGNATURE_INFECTION. Probably just marking a TODO and circling back later.
rpoyner-tri
left a comment
rpoyner-tri
left a comment
There was a problem hiding this comment.
@rpoyner-tri made 1 comment.
Reviewable status: 10 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on jwnimmer-tri).
a discussion (no related file):
Previously, rpoyner-tri (Rick Poyner (rico)) wrote…
I'm having a hard time linking the failures back to change of compiler. Nanobind makes some interesting choices for type maps. There is a "fast map" which is just doing some standard arithmetic hash on void pointer values. I could see that failing if we have ODR violations for type
type_info. Then there is a "slow map" which actually extracts the string name of the type and computesstd::hash<std::string_view>. Seems to me that the slow map should pretty much always work. And yet...Aside: is there a reason to implement a full run-time string hash, and not rely on
std::type_indexorstd::type_info::hash_code? Maybe MSVC is broken?
So, the unalias map mechanism in pydrake_pybind.h is not "the" under clang; the singleton implementation is failing and we get map-per-TU. This causes the bound_type lookup in the module_py.cc nice type override to be useless, etc., etc. I can think of fixes, but not sure what spellings are permissible. Let me get something working at all and then we can tweak it.
jwnimmer-tri
left a comment
jwnimmer-tri
left a comment
There was a problem hiding this comment.
@jwnimmer-tri made 1 comment.
Reviewable status: 10 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on rpoyner-tri).
a discussion (no related file):
Previously, rpoyner-tri (Rick Poyner (rico)) wrote…
So, the unalias map mechanism in pydrake_pybind.h is not "the" under clang; the singleton implementation is failing and we get map-per-TU. This causes the bound_type lookup in the module_py.cc nice type override to be useless, etc., etc. I can think of fixes, but not sure what spellings are permissible. Let me get something working at all and then we can tweak it.
Ah, yeah, the ODR of that class was on my list of cleanups for common before we could land it. I was thinking to try to put its single definition in common/module_py.cc but I'm not sure the other modules would be able to resolve against that. We might need to put its storage into libdrake somehow (perhaps with type erasure), perhaps near the nice_type_name seam.
rpoyner-tri
left a comment
rpoyner-tri
left a comment
There was a problem hiding this comment.
@rpoyner-tri made 1 comment.
Reviewable status: 10 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on jwnimmer-tri).
a discussion (no related file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Ah, yeah, the ODR of that class was on my list of cleanups for common before we could land it. I was thinking to try to put its single definition in
common/module_py.ccbut I'm not sure the other modules would be able to resolve against that. We might need to put its storage into libdrake somehow (perhaps with type erasure), perhaps near the nice_type_name seam.
Still investigating. I'm tempted to steal an idea from cpp_param and store a global pointer in Python, perhaps via PyCapsule.
jwnimmer-tri
left a comment
jwnimmer-tri
left a comment
There was a problem hiding this comment.
@jwnimmer-tri reviewed 5 files and all commit messages, and made 1 comment.
Reviewable status: 10 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on rpoyner-tri).
a discussion (no related file):
Previously, rpoyner-tri (Rick Poyner (rico)) wrote…
Still investigating. I'm tempted to steal an idea from
cpp_paramand store a global pointer in Python, perhaps via PyCapsule.
Sounds plausible!
rpoyner-tri
left a comment
rpoyner-tri
left a comment
There was a problem hiding this comment.
@rpoyner-tri made 1 comment and resolved 1 discussion.
Reviewable status: 9 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on jwnimmer-tri).
a discussion (no related file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Sounds plausible!
Pushed. WorksOnMyMachine(tm)!
|
Time for the YOLO test! ---------- Provisioned ---------- ---------- Unprovisioned ---------- |
|
The YOLO tests all passed! (Except for wheel builds, which are known to be unfinished.) |
jwnimmer-tri
left a comment
jwnimmer-tri
left a comment
There was a problem hiding this comment.
@jwnimmer-tri reviewed 7 files and all commit messages, made 4 comments, and resolved 4 discussions.
Reviewable status: 5 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes.
bindings/pydrake/geometry/geometry_py_render.cc line 138 at r7 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
This code has no tests.
Correction: has no tests of omitting the method in the Python subclass.
In-progress now at #24763.
Done
bindings/pydrake/multibody/tree_py.cc line 1287 at r7 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
In-progress now at #24764.
Done
bindings/pydrake/systems/framework_py_systems.cc line 340 at r7 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
This code has no tests.
Correction: has no tests of omitting the method in the Python subclass.
(The new test actually passes on this branch already, because for this particular NVI method, we bind the
Do...method to be callable by Python. Its implementation is not a stub -- the derived class is always supposed to call the base class.)In-progress now at #24763.
Done
bindings/pydrake/systems/framework_py_systems.cc line 365 at r7 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
This code has no tests.
Correction: has no tests of omitting the method in the Python subclass.
(The new test actually passes on this branch already, because for this particular NVI method, we bind the
Do...method to be callable by Python. Its implementation is not a stub -- the derived class is always supposed to call the base class.)In-progress now at #24763.
Done
rpoyner-tri
left a comment
rpoyner-tri
left a comment
There was a problem hiding this comment.
@rpoyner-tri reviewed 4 files.
Reviewable status: 5 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on jwnimmer-tri).
2 participants
This is a review refresh from work done in #24513 towards #21572.
This change is