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

Pr14878+rico #15205

Closed
wants to merge 5 commits into from
Closed

Conversation

rpoyner-tri
Copy link
Contributor

@rpoyner-tri rpoyner-tri commented Jun 17, 2021

Relates #7889, #14265, and autopybind11/autopybind11#140 (gitlab.kitware.com). Replaces #14878.

This is a messy experiment branch that tracks my early experiments with autopybind. I didn't get as far as I might have liked before vacation. Another thread in this PR will capture some of my thoughts so far.


This change is Reviewable

Copy link
Contributor Author

@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.

Reviewable status: needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @rpoyner-tri)

a discussion (no related file):
FYI @jamiesnape in case this experience is useful for kitware
FYI @ToffeeAlbina-TRI @jwnimmer-tri @EricCousineau-TRI use as you will
FYI @SeanCurtis-TRI @joemasterjohn @DamrongGuoy @sherm1 maybe some hints here if you want to experiment

I only had a few hours to fiddle with autopybind integration, so really only got to study the input side of things. I experimented with adding various elements to the set of bindings to generate, and emitted output to a tmp dir, following the instructions in tools/autopybind/README.md.

First obstacle: couldn't build
temporary solution: revert #15069

Second obstacle: missing dependencies can lead to mystifying CastXML error trains
solution: add a dependency to generate_autopybind11_rsp_file

Unsorted thoughts:

Build times are long; maybe that has to be. Can we support splitting the to-generate spec into multiple files?

User input errors in the yaml can lead to python stack traces from autopybind11; I'm not sure there is a better way to handle that, but maybe worth a bit of thought.

The error messages don't have line numbers; is that even possible with yaml input?

Error messages have inconsistent usage of ':' which leads to odd coloring choices in IDEs and editors

It is possible to respond to most of the errors and warnings with some thought. It could be more reviewers could lead to better wording.

Usage of blank lines and indent in a tall stack of errors seems a bit uncurated.

It is possible to reproduce existing examples of keep-alive usage. It took me a couple of tries to understand the input data layout.

The yaml gets very repetitive. Any prospects for exploiting anchors and aliases, or maybe some other tooling strategy? In a pinch, we could generate yaml from python or some such; perhaps that is overkill.

I didn't find examples of how to handle enums and structs nested inside classes. I worked out something that parsed and produced some useful output. It wasn't clear how to get methods that used those types in their signatures to pick them up; warnings persisted even after there were generated bindings in the output.

I could never quite figure out how to explain AutoDiffXd (an alias of a specialization of an Eigen type) to the tool. Output for templates that referred to it seemed ok, but I'm not sure we could explain the type itself fully.

The output side I left completely unexplored.

Here is my first guess as to a process:

  • copy an emitted file into the tree
  • add a build rule for it
  • get the generated function to be called with te right module object arguemnt
  • add symbols for generated documentation strings from Drake's tooling
  • run clang_format over the new files
  • hand-write and executed tests for the new bindings
  • maybe iterate and add customizations, or insert customizations by hand

Deeper questions:

I'm not sure if it is possible get customize the tool to help with typical Drake deprecation tasks? Perhaps this will always be a manual task.

I wonder if it is possible (didn't try) to fiddle with the generation templates to make the output a bit closer to the Drake by-hand style. In particular, defining useful aliases in the code like cls, which is used to good effect almost everywhere in the manual bindings.

Thanks for doing all the work that led to this point! I look forward to being able to use the tool. I'll be on vacation through July 5, but wanted to make some early feedback available and encourage others to try it out.


@jamiesnape
Copy link
Contributor

FYI @billhoffman @josephsnyder.

@rpoyner-tri
Copy link
Contributor Author

This example no longer needs to be an open PR. Still around for study.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants