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

pydrake has C++ class names in the documentation #17520

Open
mwoehlke-kitware opened this issue Jul 7, 2022 · 6 comments
Open

pydrake has C++ class names in the documentation #17520

mwoehlke-kitware opened this issue Jul 7, 2022 · 6 comments
Assignees
Labels
component: pydrake Python API and its supporting Starlark macros priority: high type: bug

Comments

@mwoehlke-kitware
Copy link
Contributor

Several bindings have C++ class names in their signatures, e.g. in pydrake.geometry.AddContactMaterial.

Besides that we'd rather have the correct Python type names in the documentation, this may be an issue for IDE completion (n.b. #16987), at least if using pybind11-stubgen to generate the .pyis.

@mwoehlke-kitware mwoehlke-kitware added type: bug component: pydrake Python API and its supporting Starlark macros labels Jul 7, 2022
@jwnimmer-tri
Copy link
Collaborator

jwnimmer-tri commented Jul 7, 2022

Discussed in slack thread.

The problem stems from an incorrect execution while loading our pydrake native definitions. That can happen either because we're missing py::import(...) statements to help guide the loading order, or else because there is a circular dependency between modules in which case no loading order will succeed.

In the simple case of missing imports, @mwoehlke-kitware and/or myself will probably just add the imports ourselves.

The example above of AddContactMaterial is the latter case. There's a circular dependency between geometry <=> multibody/plant. That is a C++ physical dependency layering violation, even aside from it's consequences for pydrake. For that example, the best answer is probably that the file drake/multibody/plant/coulomb_friction.h should move into either the geometry/ folder or somewhere even lower, like math/. The other possible answer is to move the stanza m.def("AddContactMaterial", ...) from geometry_py_common.cc to plant_py.cc, possibly also moving the AddContactMaterial function itself from its current header file into coulomb_friction.h.

As we find further specific instances of cycles, we'll figure out some way to loop in the feature area owners to help decide how to break the cycles.

mwoehlke-kitware added a commit to mwoehlke-kitware/drake that referenced this issue Jul 7, 2022
Add pybind11-stubgen as a new external. Add a rule to generate `.pyi`s
for `pydrake` using the same using a small helper script.

For now, this must be manually invoked, and some functions are being
skipped (see RobotLocomotion#17520). However, this will give us the ability to manually
test the generated `.pyi` files. Adding logic to install them and/or
otherwise bundle them can come latr.
mwoehlke-kitware added a commit to mwoehlke-kitware/drake that referenced this issue Jul 7, 2022
Add pybind11-stubgen as a new external. Add a rule to generate `.pyi`s
for `pydrake` using the same and a small helper script.

For now, this must be manually invoked, and some functions are being
skipped (see RobotLocomotion#17520). However, this will give us the ability to manually
test the generated `.pyi` files. Adding logic to install them and/or
otherwise bundle them can come later.
mwoehlke-kitware added a commit to mwoehlke-kitware/drake that referenced this issue Jul 7, 2022
Add pybind11-stubgen as a new external. Add a rule to generate `.pyi`s
for `pydrake` using the same and a small helper script.

For now, this must be manually invoked, and some functions are being
skipped (see RobotLocomotion#17520). However, this will give us the ability to manually
test the generated `.pyi` files. Adding logic to install them and/or
otherwise bundle them can come later.
@mwoehlke-kitware
Copy link
Contributor Author

Also, once #17521 lands, generating the .pyis will hopefully report (as warnings) any such issues. (It will report at least some of them; whether it reliably finds all, I am less confident in asserting. 🙂)

mwoehlke-kitware added a commit to mwoehlke-kitware/drake that referenced this issue Jul 8, 2022
Add mypy (and dependency typing-extensions) as new externals. Add a rule
to generate `.pyi`s for `pydrake` using the same and a tiny wrapping
helper script.

For now, this must be manually invoked, and some functions are being
skipped (see RobotLocomotion#17520). However, this will give us the ability to manually
test the generated `.pyi` files. Adding logic to install them and/or
otherwise bundle them can come later.
mwoehlke-kitware added a commit to mwoehlke-kitware/drake that referenced this issue Jul 8, 2022
Add mypy (and dependencies) as new externals. Add a rule to generate
`.pyi`s for `pydrake` using the same and a tiny wrapping helper script.

For now, this must be manually invoked, and some functions have degraded
type information (see RobotLocomotion#17520). However, this will give us the ability to
manually test the generated `.pyi` files. Adding logic to install them
and/or otherwise bundle them can come later.
mwoehlke-kitware added a commit to mwoehlke-kitware/drake that referenced this issue Jul 8, 2022
Add mypy (and dependencies) as new externals. Add a rule to generate
`.pyi`s for `pydrake` using the same and a tiny wrapping helper script.

For now, this must be manually invoked, and some functions have degraded
type information (see RobotLocomotion#17520). However, this will give us the ability to
manually test the generated `.pyi` files. Adding logic to install them
and/or otherwise bundle them can come later.
mwoehlke-kitware added a commit to mwoehlke-kitware/drake that referenced this issue Jul 8, 2022
Add mypy (and dependencies) as new externals. Add a rule to generate
`.pyi`s for `pydrake` using the same and a tiny wrapping helper script.

For now, this must be manually invoked, and some functions have degraded
type information (see RobotLocomotion#17520). However, this will give us the ability to
manually test the generated `.pyi` files. Adding logic to install them
and/or otherwise bundle them can come later.
mwoehlke-kitware added a commit to mwoehlke-kitware/drake that referenced this issue Jul 8, 2022
Add mypy (and dependencies) as new externals. Add a rule to generate
`.pyi`s for `pydrake` using the same and a tiny wrapping helper script.

For now, this must be manually invoked, and some functions have degraded
type information (see RobotLocomotion#17520). However, this will give us the ability to
manually test the generated `.pyi` files. Adding logic to install them
and/or otherwise bundle them can come later.
mwoehlke-kitware added a commit to mwoehlke-kitware/drake that referenced this issue Jul 11, 2022
Add mypy (and dependencies) as new externals. Add a rule to generate
`.pyi`s for `pydrake` using the same and a tiny wrapping helper script.

For now, this must be manually invoked, and some functions have degraded
type information (see RobotLocomotion#17520). However, this will give us the ability to
manually test the generated `.pyi` files. Adding logic to install them
and/or otherwise bundle them can come later.
jwnimmer-tri pushed a commit that referenced this issue Jul 11, 2022
Add mypy (and dependencies) as new externals. Add a rule to generate
`.pyi`s for `pydrake` using the same and a tiny wrapping helper script.

For now, this must be manually invoked, and some functions have degraded
type information (see #17520). However, this will give us the ability to
manually test the generated `.pyi` files. Adding logic to install them
and/or otherwise bundle them can come later.
@jwnimmer-tri
Copy link
Collaborator

In #18674, I have a patch that removes any C++ types from the *.pyi stub files.

What remains here is to fix the documentation so that it doesn't have C++ class names in API signatures.

Do to that, we need to define our bindings in a dependency-topological order, so that anything used as an argument or return type is already bound (and imported!) prior to defining more bindings.

@jwnimmer-tri
Copy link
Collaborator

The next step is #20491. After that, we can return here to work on loading / definition order.

@jwnimmer-tri
Copy link
Collaborator

jwnimmer-tri commented Sep 15, 2024

Actually, doing #20491 without CI checking for docstring / mypy regressions is too dangerous. We need to make progress on this issue (and add regression tests) before all other pydrake refactoring.

@jwnimmer-tri
Copy link
Collaborator

jwnimmer-tri commented Sep 17, 2024

My WIP branch is here. I'm carving out the simpler parts into a PR train. Some of the harder parts might need to wait until pydrake re-modularization happens first. The goal for now is to get a regression test onto master relatively quickly, even if that means it has an allow-list of known bugs -- at least then we won't make things too much worse.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: pydrake Python API and its supporting Starlark macros priority: high type: bug
Projects
Status: In Progress
Development

No branches or pull requests

2 participants