Capture generated autopybind11 bindings using command #3
Conversation
|
@josephsnyder for review please |
There was a problem hiding this comment.
Reviewable status: 0 of 6 files reviewed, 2 unresolved discussions
tmp/autopybind11/drake_py.cpp, line 13 at r1 (raw file):
.value("absolute", ::drake::ToleranceType::kAbsolute, "") .value("relative", ::drake::ToleranceType::kRelative, "") .export_values();
I would like this to not export values.
Is this possible?
tmp/autopybind11/RandomGenerator_py.cpp, line 7 at r1 (raw file):
namespace py = pybind11; void apb11_pydrake_RandomGenerator_py_register(py::module &m) { static bool called = false;
I'm not sure if I understand why these guards are here. The concern is that if this is not called the second time, it may return an empty module in Python 3.
Can you point me to the autopybind11 unittest that exercises this guard?
FWIW This is the macro we use in Drake for similar purposes, but generally this is only for semi-unavoidable cyclic dependencies:
https://github.com/RobotLocomotion/drake/blob/v0.26.0/bindings/pydrake/pydrake_pybind.h#L139-L155
|
Capturing looks correct to me. Merging now |
|
Ah, no! Please don't merge! |
|
Sorry - this is just to review against something concrete; not meant to be merged in. |
|
Haha, I saw the other comments before I hit the button. No merging |
There was a problem hiding this comment.
Reviewable status: 0 of 6 files reviewed, 2 unresolved discussions (waiting on @EricCousineau-TRI)
tmp/autopybind11/drake_py.cpp, line 13 at r1 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
I would like this to not export values.
Is this possible?
We don't have a way to turn this off right now. It wouldn't be hard to add a customization to do so.
My play will be to default it to not exporing and supplying some export_enum_values flag will append the function call
tmp/autopybind11/RandomGenerator_py.cpp, line 7 at r1 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
I'm not sure if I understand why these guards are here. The concern is that if this is not called the second time, it may return an empty module in Python 3.
Can you point me to the
autopybind11unittest that exercises this guard?FWIW This is the macro we use in Drake for similar purposes, but generally this is only for semi-unavoidable cyclic dependencies:
https://github.com/RobotLocomotion/drake/blob/v0.26.0/bindings/pydrake/pydrake_pybind.h#L139-L155
I don't know if we have a test for this at the moment. It shouldn't be hard to test though, as I think we'll just have to import a module twice to see if it crashes when trying to load the module again.
There was a problem hiding this comment.
Reviewable status: 0 of 6 files reviewed, 3 unresolved discussions (waiting on @EricCousineau-TRI and @josephsnyder)
a discussion (no related file):
I'm porting some of your stuff from this branch:
https://github.com/josephsnyder/drake-external-examples/tree/add_binding_example_2/drake_cmake_bindings/pydrake
However, I don't understand how the geometry binding generation works:
RobotLocomotion/drake-external-examples@master...josephsnyder:add_binding_example_2#diff-88377387f1f75c3c059a8a3001ed1880d9597e5cbf4401d1498f0265ab27ee6a
Will briefly review autopybind11 docs to see if that is explained...
tmp/autopybind11/drake_py.cpp, line 13 at r1 (raw file):
Previously, josephsnyder (Joseph Snyder) wrote…
We don't have a way to turn this off right now. It wouldn't be hard to add a customization to do so.
My play will be to default it to not exporing and supplying someexport_enum_valuesflag will append the function call
You should also be able to distinguish between a C-syle enum (uscoped enumerations) and a C++11 enum class (scoped enumerations) via pygccxml, right?
https://en.cppreference.com/w/cpp/language/enum#Scoped_enumerations
There was a problem hiding this comment.
Reviewable status: 0 of 6 files reviewed, 3 unresolved discussions (waiting on @josephsnyder)
tmp/autopybind11/RandomGenerator_py.cpp, line 7 at r1 (raw file):
Previously, josephsnyder (Joseph Snyder) wrote…
I don't know if we have a test for this at the moment. It shouldn't be hard to test though, as I think we'll just have to import a module twice to see if it crashes when trying to load the module again.
Sounds good. You should also make sure that the re-imported module still has some integrity (e.g. the definitions up to that point, or whatever).
I can point to places in our code which fail if we don't have our PYDRAKE_PREVENT_PYTHON3_MODULE_REIMPORT workaround.
There was a problem hiding this comment.
Reviewable status: 0 of 6 files reviewed, 3 unresolved discussions (waiting on @josephsnyder)
tmp/autopybind11/RandomGenerator_py.cpp, line 7 at r1 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Sounds good. You should also make sure that the re-imported module still has some integrity (e.g. the definitions up to that point, or whatever).
I can point to places in our code which fail if we don't have our
PYDRAKE_PREVENT_PYTHON3_MODULE_REIMPORTworkaround.
See also:
pybind/pybind11#1559
There was a problem hiding this comment.
Reviewable status: 0 of 6 files reviewed, 3 unresolved discussions (waiting on @EricCousineau-TRI and @josephsnyder)
a discussion (no related file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
I'm porting some of your stuff from this branch:
https://github.com/josephsnyder/drake-external-examples/tree/add_binding_example_2/drake_cmake_bindings/pydrakeHowever, I don't understand how the geometry binding generation works:
RobotLocomotion/drake-external-examples@master...josephsnyder:add_binding_example_2#diff-88377387f1f75c3c059a8a3001ed1880d9597e5cbf4401d1498f0265ab27ee6aWill briefly review autopybind11 docs to see if that is explained...
Geometry is well behind all the others. I cannot remember why I stopped the attempted binding of it, though. I'll have to revisit it.
tmp/autopybind11/drake_py.cpp, line 13 at r1 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
You should also be able to distinguish between a C-syle
enum(uscoped enumerations) and a C++11enum class(scoped enumerations) viapygccxml, right?
https://en.cppreference.com/w/cpp/language/enum#Scoped_enumerations
That's an upcoming feature. CastXML passes the type of the enumeration in the latest version but pygccxml doesn't handle it yet. I have a branch to make it do so which is still a WIP.
There was a problem hiding this comment.
Reviewable status: 0 of 6 files reviewed, 3 unresolved discussions (waiting on @EricCousineau-TRI and @josephsnyder)
a discussion (no related file):
Previously, josephsnyder (Joseph Snyder) wrote…
Geometry is well behind all the others. I cannot remember why I stopped the attempted binding of it, though. I'll have to revisit it.
Ah, so I should ignore that config file for now?
That's fine, just wasn't sure if there were features I was unaware of (like "autodisocover where exactly this belongs).
There was a problem hiding this comment.
Reviewable status: 0 of 6 files reviewed, 3 unresolved discussions (waiting on @EricCousineau-TRI and @josephsnyder)
a discussion (no related file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Ah, so I should ignore that config file for now?
That's fine, just wasn't sure if there were features I was unaware of (like "autodisocover where exactly this belongs).
Yes, please ignore it. I'll revisit it and see why I left it in such a poor state.
There was a problem hiding this comment.
Reviewable status: 0 of 6 files reviewed, 4 unresolved discussions (waiting on @EricCousineau-TRI and @josephsnyder)
a discussion (no related file):
Hit the error I think you alluded to last week:
bazel-out/k8-opt/bin/tools/install/libdrake/_virtual_includes/drake_headers/drake/lcm/drake_lcm.h:7:10: fatal error: 'lcm/lcm-cpp.hpp' file
not found
#include "lcm/lcm-cpp.hpp"
There was a problem hiding this comment.
Reviewable status: 0 of 7 files reviewed, 4 unresolved discussions (waiting on @EricCousineau-TRI and @josephsnyder)
a discussion (no related file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Hit the error I think you alluded to last week:
bazel-out/k8-opt/bin/tools/install/libdrake/_virtual_includes/drake_headers/drake/lcm/drake_lcm.h:7:10: fatal error: 'lcm/lcm-cpp.hpp' file not found #include "lcm/lcm-cpp.hpp"
I guess I didn't add the LCM as a data dependency to the rule. Whoops!
There was a problem hiding this comment.
Reviewable status: 0 of 7 files reviewed, 4 unresolved discussions (waiting on @EricCousineau-TRI and @josephsnyder)
a discussion (no related file):
Previously, josephsnyder (Joseph Snyder) wrote…
Yes, please ignore it. I'll revisit it and see why I left it in such a poor state.
OK Sounds good!
a discussion (no related file):
For the current revision, I'm doing a naive stress-test. It does seem to be very slow (it's been 5 minutes on r3, and still haven't seen any output).
I'm also only seeing one core doing work on my 48 core machine.
FWIW, mkdoc.py is able to trawl through all of Drake's symbols and generate the docstring tree in about 50-60s, and I believe that is on a single processor.
I will split things up to manually do "parallelization".
Relates https://gitlab.kitware.com/autopybind11/autopybind11/-/issues/158
tmp/autopybind11/drake_py.cpp, line 13 at r1 (raw file):
Previously, josephsnyder (Joseph Snyder) wrote…
That's an upcoming feature. CastXML passes the type of the enumeration in the latest version but pygccxml doesn't handle it yet. I have a branch to make it do so which is still a WIP.
Gotcha. Yeah, I would recommend not exporting values when you can detect a scoped enum.
Is there a GitLab issue for that already?
There was a problem hiding this comment.
Reviewable status: 0 of 7 files reviewed, 4 unresolved discussions (waiting on @EricCousineau-TRI and @josephsnyder)
tmp/autopybind11/drake_py.cpp, line 13 at r1 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Gotcha. Yeah, I would recommend not exporting values when you can detect a scoped enum.
Is there a GitLab issue for that already?
Per VC: https://gitlab.kitware.com/autopybind11/autopybind11/-/issues/60
There was a problem hiding this comment.
Reviewable status: 0 of 7 files reviewed, 4 unresolved discussions (waiting on @EricCousineau-TRI and @josephsnyder)
a discussion (no related file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
For the current revision, I'm doing a naive stress-test. It does seem to be very slow (it's been 5 minutes on
r3, and still haven't seen any output).I'm also only seeing one core doing work on my 48 core machine.
FWIW,mkdoc.pyis able to trawl through all of Drake's symbols and generate the docstring tree in about 50-60s, and I believe that is on a single processor.I will split things up to manually do "parallelization".
Relates https://gitlab.kitware.com/autopybind11/autopybind11/-/issues/158
Can point `autopybind11 repository to use this commit:
https://gitlab.kitware.com/autopybind11/autopybind11/-/merge_requests/121
Per VC, can speed up MBP gen from 50min-ish down to 8min.
Also, got an error when trying to generate bindings with the current config file at r3:
$ bazel run --run_under=time //bindings/pydrake:autopybind11_example -- --output_dir=${PWD}/tmp/autopybind11
...
pygccxml.declarations.runtime_errors.multiple_declarations_found_t: Multiple declarations have been found. Matcher: [(decl type==constructor_t)]
...
real 13m18.134s
user 13m2.113s
sys 0m15.936s
There was a problem hiding this comment.
Reviewable status: 0 of 7 files reviewed, 4 unresolved discussions (waiting on @EricCousineau-TRI and @josephsnyder)
a discussion (no related file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Can point `autopybind11 repository to use this commit:
https://gitlab.kitware.com/autopybind11/autopybind11/-/merge_requests/121
Per VC, can speed up MBP gen from 50min-ish down to 8min.Also, got an error when trying to generate bindings with the current config file at r3:
$ bazel run --run_under=time //bindings/pydrake:autopybind11_example -- --output_dir=${PWD}/tmp/autopybind11 ... pygccxml.declarations.runtime_errors.multiple_declarations_found_t: Multiple declarations have been found. Matcher: [(decl type==constructor_t)] ... real 13m18.134s user 13m2.113s sys 0m15.936s
Same error, but faster! (with https://gitlab.kitware.com/autopybind11/autopybind11/-/tree/bf45caec52c65d2bf6d8d493393e2fbd408c1544)
pygccxml.declarations.runtime_errors.multiple_declarations_found_t: Multiple declarations have been found. Matcher: [(decl type==constructor_t)]
...
real 2m50.211s
user 2m43.947s
sys 0m6.192s
|
a discussion (no related file): Previously, EricCousineau-TRI (Eric Cousineau) wrote…
I was able to track the error to line 564-566 in main.py of AutoPyBind11. Changing those lines to look like this: will get you past the error. I'm trying to figure out which dep_class_t object that we reached which has multiple constructors and how to see if we need to stick with the 0th of all of the constructors. |
There was a problem hiding this comment.
Reviewable status: 0 of 8 files reviewed, 4 unresolved discussions (waiting on @EricCousineau-TRI and @josephsnyder)
a discussion (no related file):
Previously, josephsnyder (Joseph Snyder) wrote…
I was able to track the error to line 564-566 in main.py of AutoPyBind11. Changing those lines to look like this:
extra_includes.add( dep_class_t.constructors()[0].location.file_name )will get you past the error. I'm trying to figure out which dep_class_t object that we reached which has multiple constructors and how to see if we need to stick with the 0th of all of the constructors.
Seems to be related to Eigen::Matrix<double, -1, 1, 0, -1, 1>. Not surprising that that has a >1 constructors.
The zero-eth constructor route seems to be the best way forward.
There was a problem hiding this comment.
Reviewable status: 0 of 12 files reviewed, 5 unresolved discussions (waiting on @EricCousineau-TRI and @josephsnyder)
tmp/autopybind11/RenderCameraCore_py.cpp, line 7 at r5 (raw file):
namespace py = pybind11; void apb11_pydrake_RenderCameraCore_py_register(py::module &m) {
@josephsnyder This seems to break, silently? On this revision (r5, 75828fe), I do the following to generate this:
rm -rf ./tmp/autopybind11/
bazel run //bindings/pydrake:autopybind11_example -- --output_dir=${PWD}/tmp/autopybind11
Nothing useful is generated?
There was a problem hiding this comment.
Reviewable status: 0 of 12 files reviewed, 5 unresolved discussions (waiting on @EricCousineau-TRI and @josephsnyder)
a discussion (no related file):
Previously, josephsnyder (Joseph Snyder) wrote…
Seems to be related to
Eigen::Matrix<double, -1, 1, 0, -1, 1>. Not surprising that that has a >1 constructors.
The zero-eth constructor route seems to be the best way forward.
Gotcha. I will wait until a fix is ready for autopybind11 proper; until then, I will see if I can avoid this hot path.
Do you know which class this came up for?
There was a problem hiding this comment.
Reviewable status: 0 of 12 files reviewed, 5 unresolved discussions (waiting on @EricCousineau-TRI)
a discussion (no related file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Gotcha. I will wait until a fix is ready for
autopybind11proper; until then, I will see if I can avoid this hot path.Do you know which class this came up for?
It was AutoDiffXd. We have a fix but not a test yet. I'm considering just merging it as is and dealing with the test later.
There was a problem hiding this comment.
Reviewable status: 0 of 12 files reviewed, 6 unresolved discussions (waiting on @EricCousineau-TRI)
bindings/pydrake/autopybind11_example.yaml, line 1 at r5 (raw file):
# For additional examples, see the README.rst or
Working: I will uncomment the classes I would like to inspect/customize per upstream issue.
Given silent failures etc, will then pass to Joe to ensure that it generates the corresponding code.
tmp/autopybind11/RenderCameraCore_py.cpp, line 7 at r5 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
@josephsnyder This seems to break, silently? On this revision (r5, 75828fe), I do the following to generate this:
rm -rf ./tmp/autopybind11/ bazel run //bindings/pydrake:autopybind11_example -- --output_dir=${PWD}/tmp/autopybind11Nothing useful is generated?
(silent: no stdout/stderr, exit status 0)
There was a problem hiding this comment.
Reviewable status: 0 of 12 files reviewed, 5 unresolved discussions (waiting on @EricCousineau-TRI)
bindings/pydrake/autopybind11_example.yaml, line 1 at r5 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Working: I will uncomment the classes I would like to inspect/customize per upstream issue.
Given silent failures etc, will then pass to Joe to ensure that it generates the corresponding code.
OK I've updated this file with what I would expect to work. (There may still be typos).
@josephsnyder Handing off to you!
There was a problem hiding this comment.
Reviewable status: 0 of 12 files reviewed, 5 unresolved discussions (waiting on @EricCousineau-TRI)
tmp/autopybind11/RenderCameraCore_py.cpp, line 7 at r5 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
(silent: no stdout/stderr, exit status 0)
Is it possible the fact that inst isn't defined? e.g. RenderCameraCore: {} vs. RenderCameraCore: {inst: []}?
|
tmp/autopybind11/RenderCameraCore_py.cpp, line 7 at r5 (raw file): Previously, EricCousineau-TRI (Eric Cousineau) wrote…
It shouldn't be. The absence of |
There was a problem hiding this comment.
Reviewable status: 0 of 5 files reviewed, 6 unresolved discussions (waiting on @EricCousineau-TRI)
a discussion (no related file):
Working: I should review the current bindings being generated.
Also fix some markup glitches.
…RobotLocomotion#14679) * [tools:drake_visualizer] Simple vector menu for hydroelastic contact.
…14693) Google Colab changed their default version to 3.7.
Update the YAML file to add a template value for BarycentricMesh and to add the internal namespace for the SystemMessageInterface Update autopybind11 to pull a merge request which allows the drake::AutoDiffXd class to be properly found as a template class.
Propagate some rst diffs from 2290ae0 back into the md copy of the site. Remove the stale "serves an empty page" disclaimer.
…vent object. (RobotLocomotion#14663) * Make sure overridden DoCalcNextUpdateTime() is properly implemented.
…tion#14698) * Enable simple caching for FEM state dependent quantities
…n#14700) * fixed two links on the homepage that were not correct
Introduces: Alphabetical ordering of data in each class: https://gitlab.kitware.com/autopybind11/autopybind11/-/merge_requests/143 "Raw" string format for comments: https://gitlab.kitware.com/autopybind11/autopybind11/-/merge_requests/144
Adds update which will replace long template params with shorter alias names if used.
Following example in Drake, use a tuple of (name, filename, line number) to sort the objects in the output code.
josephsnyder
force-pushed
the
test_autopybind_integration-eric-gen-review
branch
from
e95a8ed
to
dd297dd
Compare
There was a problem hiding this comment.
Reviewable status: 0 of 500 files reviewed, 8 unresolved discussions
a discussion (no related file):
Previously, josephsnyder (Joseph Snyder) wrote…
Per F2F with Jamie, we want to move forward with this to try and get the main pull request (RobotLocomotion#14265) merged before end of contract.
@jamiesnape I think it's inefficient to have merged this for the sake of moving forward without concretely addressing my comments.
There was a problem hiding this comment.
Reviewable status: 0 of 500 files reviewed, 8 unresolved discussions
a discussion (no related file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
@jamiesnape I think it's inefficient to have merged this for the sake of moving forward without concretely addressing my comments.
OK The generated bindings are still in the Drake PR.
Defects can be re-filed, but it's having to be replayed, and it makes Drake PR noisy. Oh well.
Cookie crumb: https://docs.reviewable.io/tips.html#easy-local-revision-checkout
Will comment using Reviewable
This change is