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: Provide Python API docstrings. #7914

Closed
EricCousineau-TRI opened this Issue Jan 31, 2018 · 25 comments

Comments

Projects
None yet
5 participants
@EricCousineau-TRI
Contributor

EricCousineau-TRI commented Jan 31, 2018

Presently, most of the bindings of pydrake does not provide documentation.

My ideal solution would be to have some sort of link between the Python code and the C++ code, so that we don't waste time duplicating documentation. I assume this could be automated, and applied automatically (either at compile-time, or at run-time), such that help(...) in Python / IPython is informative to users.

The alternative is to manually duplicate all documentation.
This is much less preferable for me, and seems less maintainable.

\cc @naveenoid

@EricCousineau-TRI EricCousineau-TRI self-assigned this Jan 31, 2018

@EricCousineau-TRI EricCousineau-TRI changed the title from pydrake: Provide documentation. to pydrake: Provide Python API docstrings. Jan 31, 2018

@jwnimmer-tri

This comment has been minimized.

Collaborator

jwnimmer-tri commented Jan 31, 2018

FWIW I agree that mostly the python documentation should (semi-?)automatically come over from the C++ documentation. However, we should still encourage people to add (or supplement) that with python-specific help strings where it makes sense. (And pure-python non-private code should always have docstrings, per the style guide -- a few small places in bindings are currently missing it.)

@jwnimmer-tri

This comment has been minimized.

Collaborator

jwnimmer-tri commented Mar 17, 2018

The GENERATE_XML = Yes output of Doxygen seems plausibly parse-able at runtime via Python. The first approach I'd try exploring is to have the help() query lazily load the Doxygen XML output and find the relevant section(s) to show, with only a loose dependency so Doxygen isn't in the critical path for locally running and testing the bindings.

@EricCousineau-TRI

This comment has been minimized.

Contributor

EricCousineau-TRI commented Mar 17, 2018

Ah, didn't think about lazy evaluation!
From a quick search, though, and from what I've dealt with with descriptors and __dict__ entries (which __doc__ is), it seems that this would require additional effort:
https://stackoverflow.com/a/2694358/7829525
We could monkey patch the pybind11 metaclass, but then we still have functions, methods, and modules to patch. We could wrap those with wrapt or what not to proxy (everything, but namely) __doc__, which should work -- but I smell a bit of storm brewing in that solution. Will still keep my eyes peeled, though!

Overall, I think that this approach would require a lot more fine-tuning in implementation, whereas the non-lazy scanning approach could be a first pass that, if implemented well, could then pave the way for dynamic docstrings if it's apparent they're necessary for performance / usability.

[...] with only a loose dependency so Doxygen isn't in the critical path for locally running and testing the bindings.

For the non-lazy patching approach, my intent is to definitely do a loose dependency at runtime: have some method, like pydrake::UseDoxygenIfAvailable(m), which, if the doxygen XML was available, would scan the entries in the current module, find the relevant entry, and patch them.

Side note:
When perusing the pybind11 project, seems like there's a codegen-related option of sorts already out there (if the other options are not viable):
pybind/pybind11#1321

@EricCousineau-TRI

This comment has been minimized.

Contributor

EricCousineau-TRI commented Mar 17, 2018

Also, I'd probably end up using breathe or importing a portion of it - at least for a first pass - since they've already done the work of parsing Doxygen XML for mapping to Sphinx / rst:
https://github.com/michaeljones/breathe

@EricCousineau-TRI

This comment has been minimized.

Contributor

EricCousineau-TRI commented Jul 31, 2018

pybind does have a utility for creating C++ #define statements with the docstrings:
https://github.com/pybind/pybind11/blob/master/tools/mkdoc.py

Haven't looked much deeper into it, though.

@fbudin69500

This comment has been minimized.

Contributor

fbudin69500 commented Aug 16, 2018

@EricCousineau-TRI As a follow up of our conversation this morning, ITK does basically what @jwnimmer-tri suggested to add its doxygen documentation to its Python bindings:

  • https://github.com/InsightSoftwareConsortium/ITK/blob/master/Wrapping/Generators/Doc/CMakeLists.txt
  • The generated XML files are converted to *.i files that SWIG, which is used by ITK to create its Python wrapping, understands.
  • In the past, the second step was not performed, and the documentation was loaded at runtime if it was available (if the files were found in a specific directory). One could indeed use this solution, at least at first, and install the documentation files during drake installation process.
@EricCousineau-TRI

This comment has been minimized.

Contributor

EricCousineau-TRI commented Aug 29, 2018

Per Slack chat with @m-chaturvedi, he's trying out the mkdoc.py approach, and will see what this approach yields.

For testing, we made a simple trimmed-down branch:
https://github.com/EricCousineau-TRI/drake/tree/feature/py_just_common

@m-chaturvedi

This comment has been minimized.

Contributor

m-chaturvedi commented Sep 5, 2018

The "mkdoc.py approach" is outlined here by a contributor of Pybind11:
pybind/pybind11#807 (comment)

mkdoc.py is calling clang internally to parse the header files so it needs the include directories just like we need while building.

The two main problems it presents us are the following:

  1. Given a .cc file, for example math_py.cc, get the header files from it (non-transitive, example #include "drake/math/barycentric.h") on which mkdoc.py needs to be run.
  2. On the header files run mkdoc.py. Running mkdoc.py needs the -I similar to how gcc needs -Is while building.
@jamiesnape

This comment has been minimized.

Contributor

jamiesnape commented Sep 5, 2018

On the header files run mkdoc.py. Running mkdoc.py needs the -I similar to how gcc needs -Is while building.

More precisely, it is running clang under the hood so it needs include directories.

@jwnimmer-tri

This comment has been minimized.

Collaborator

jwnimmer-tri commented Sep 5, 2018

Since pydrake is using libdrake.so for basically all of the C++ code that's going to be wrapped, I'm not sure why we'd need fine-grained parsing of math_py.cc's includes like barycentric.h. There could be a single build rule that processed all of the //:drake_shared_library headers into a single docstring *.inc file, which any math_py.cc that wanted to access the comments could use.

@jamiesnape

This comment has been minimized.

Contributor

jamiesnape commented Sep 5, 2018

We certainly discussed having one doc header per module, but that ends up being a small implementation detail. Neither of what @m-chaturvedi mentions are really "problems" in the sense that they are difficult.

@jamiesnape

This comment has been minimized.

Contributor

jamiesnape commented Sep 17, 2018

When processing all the //:drake_shared_library headers, clang is choking on the various includes of drake/common/autodiff.h and drake/common/symbolic.h.

@EricCousineau-TRI

This comment has been minimized.

Contributor

EricCousineau-TRI commented Sep 18, 2018

What way is it choking? Is there visible output on it, or does it just hang forever as it expands a massive amount of template instantiations?

@jamiesnape

This comment has been minimized.

Contributor

jamiesnape commented Sep 18, 2018

Numerous compilation errors. Not sure if it is because no-one compiles those headers in isolation now, or some other reason.

@jamiesnape

This comment has been minimized.

Contributor

jamiesnape commented Sep 18, 2018

I have a feeling the choice of #pragma once is part of the issue since I am seeing -Wpragma-once-outside-header warnings and multiple definitions of various types. I will see if adding some (redundant) include guards helps.

@jamiesnape

This comment has been minimized.

Contributor

jamiesnape commented Sep 18, 2018

I have a feeling the choice of #pragma once is part of the issue [...]

Yes, that one of the problems. Since I presume #2104 is irreversible, I will add redundant guards (at least in the short term).

Meanwhile, some of the drake/common/autodiff.h are not compilable without adding some includes in the #ifndef DRAKE_COMMON_AUTODIFF_HEADER ... #endif that contains the #warning.

@jamiesnape

This comment has been minimized.

Contributor

jamiesnape commented Sep 18, 2018

Finding a lot of headers that do not IWYU (or even include-what-you-need). Some clearly deliberate, but not all. I guess this is a signal that I should be dusting off my branch for running IWYU (easiest when we switch to Bionic).

@jwnimmer-tri

This comment has been minimized.

Collaborator

jwnimmer-tri commented Sep 18, 2018

What is this header reprocessing tool doing, that diverges from what the compiler (with preprocesser + etc) would be doing while parsing a header? The compilers seem to like the headers; why is this tool different? Of course if there are code defects we should fix them anyway, but I worry a bit about adopting a tool which diverges from what compilers do, in terms of ongoing upkeep.

@jamiesnape

This comment has been minimized.

Contributor

jamiesnape commented Sep 18, 2018

It is a compiler, but it only processes headers.

@jamiesnape

This comment has been minimized.

Contributor

jamiesnape commented Sep 18, 2018

(When headers are treated like source files, #pragma once does not work. Otherwise, everything else is what I would consider a defect.)

@jwnimmer-tri

This comment has been minimized.

Collaborator

jwnimmer-tri commented Sep 18, 2018

... but doesn't process include statements? Otherwise, how do IWYU violations cause it to fail? (Presumably, some transitively included header got the job done, for regular compilers?)

@jamiesnape

This comment has been minimized.

Contributor

jamiesnape commented Sep 18, 2018

It processes #include statements, but we have some headers that apparently are never normally used in isolation.

@jwnimmer-tri

This comment has been minimized.

Collaborator

jwnimmer-tri commented Sep 18, 2018

Ah, okay. Most headers have a matching cc file or unit test that includes them standalone as the first line. But yes, some may not.

@jamiesnape

This comment has been minimized.

Contributor

jamiesnape commented Sep 18, 2018

Exactly.

@EricCousineau-TRI

This comment has been minimized.

Contributor

EricCousineau-TRI commented Oct 21, 2018

Resolved since #9578.

Other todo items tracked by #9599 and related issues.

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