WIP: Rework Python API

[rhaschke]

This is an attempt to modernize the Python API from boost::python to pybind11 and reduce boilerplate code for type conversions based on initial work in #2547. See individual commits for details.

• [x] Move py_bindings_tools from moveit_ros_planning_interface to moveit_core (and some cleanup)
• [ ] Feed rospy.names.get_mappings() into ROScppInitializer to use python remappings for C++ as well?
• [x] Check whether ros_msg_typecasters.h requires PyBytes handling as introduced by @gleichdick in #1870
• [x] Migrate existing wrappers to pybind11 and its automatic type conversions. Remove serialize_msg.h.
• [x] Replace py_conversions.h with pybind11/stl.h
• [x] Replace gil_releaser.h with pybind11::gil_scoped_release
• [x] Replace eigenpy with pybind11/eigen.h
• [ ] Fix fundamental catkin issue https://github.com/ros/catkin/pull/1153
• [ ] Check backwards compatibility of moveit_commander
• [ ] Host python docs somewhere
  • #2606
  • #2914

@gleichdick, @PeterMitrano: I would be happy if you could contribute to this work, filing PRs against my PR branch. What do you think of the cleanup so far?

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 15.09434% with 45 lines in your changes are missing coverage. Please review.

Project coverage is 14.49%. Comparing base (c174715) to head (0d4248a). Report is 10 commits behind head on master.

:exclamation: Current head 0d4248a differs from pull request most recent head 509ee49. Consider uploading reports for the commit 509ee49 to get more accurate results

Files	Patch %	Lines
...oveit_core/python/tools/src/roscpp_initializer.cpp	0.00%	41 Missing :warning:
...veit_core/python/tools/src/ros_msg_typecasters.cpp	0.00%	4 Missing :warning:

Additional details and impacted files

@@             Coverage Diff             @@
##           master    #2910       +/-   ##
===========================================
- Coverage   47.15%   14.49%   -32.65%     
===========================================
  Files         603      281      -322     
  Lines       58999    24731    -34268     
  Branches     6969        0     -6969     
===========================================
- Hits        27814     3583    -24231     
+ Misses      30773    21148     -9625     
+ Partials      412        0      -412     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[rhaschke]

I'm not sure why you remove the calls to initialize roscpp in the demo scripts.

Ideally, those explicit roscpp_init calls shouldn't be necessary if all python-exported classes requiring this functionality derive from ROScppInitializer, which calls roscpp_init behind the scenes.

Would you consider changing how the C++ node is named? the default argument of "moveit_python_wrappers" means if you run multiple python nodes using moveit you get lots of confusing ros node names. Perhaps have a look at the ros init python bindings we use in my lab, which names nodes as "my_name" and "my_name_cpp".

Generally, I like this idea. It's easily implemented for moveit_commander.roscpp_initialize(), but not so trivial (but possible) for ROScppInitializer. Any implementation should consider the case that rospy.init_node() wasn't called yet (and then fallback to the current default and issue an info message?).

I'm not sure why you added the string <--> PoseStameped converter but seems fine.

It allows creating a PoseStamped type from a string.

[PeterMitrano]

Hm ok I guess that's reasonable. So MoveGroupInterfaceWrapper inhereits from it, so that should stlil work, interesting. I prefer an explicit initialize at the beginning of my script, because often use multiple C++ ROS bindings, not just moveit, but that's still compatible with this I think

[rhaschke]

I prefer an explicit initialize at the beginning of my script ... but that's still compatible with this I think.

Sure. The first call wins.

[gleichdick]

Always happy to help :smile:

ros_msg_typecasters.h does not need ByteString, as the buffer does not cross the boundary between pybind11/boost::python and another python module. pybind11::bytes could be used to get rid of the plain CPython function calls, but that'd be just for readability.

If we use ros_msg_typecasters.h in planning_interface, the interface of the wrapper changes. Do we take in accout users which use these wrappers directly?

Side note: I tried to enable coverage for moveit_commander, but some problems arised: ros/ros_comm#2199

[rhaschke]

If we use ros_msg_typecasters.h in planning_interface, the interface of the wrapper changes. Do we take in account users which use these wrappers directly?

Using the old library wrappers directly isn't very useful because you would need to handle serialized byte strings. I was expecting nobody to do that and thus not provide backwards compatibility to this awkward API. Of course, backward compatibility should be maintained for all the existing python API operating on python ROS messages.

[rhaschke]

@ros-planning/moveit-maintainers, @gleichdick, @PeterMitrano: These python wrapper improvements are lying around (nearly ready) for over a year now. The (only) road blocker was and is a major design flaw in catkin's devel-space layout (https://github.com/ros/catkin/pull/1153), which renders multiple namespace packages installing into the same devel-space location impossible. Specifically, moveit.core introduced in #2547, moveit.planning_interface renamed from moveit_ros_planning_interface in this PR, and moveit.task_constructor cannot be used together within the same devel-space workspace. Essentially only the first installed package (i.e. moveit.core) will be accessible (see CI failure). Install-space workspaces don't have that issue. MTC within a devel-space workspace that is overlaying an install-space workspace comprising MoveIt (e.g. /opt/ros) works as well.

As we won't fix catkin anymore, I only see the following options:

• Drop this PR and focus on python for MoveIt2 only (not really an option)
• Drop the (nice) idea of namespace package and rename packages from moveit.* to moveit_*
• Ignore the issue and recommend using install spaces, particularly symlinked installs with colcon to keep the benefits of a devel space. As releases are not affected, I think that is a viable option.

What do you think? @henningkayser: We should discuss that in the next maintainer meeting.

[PeterMitrano]

Seems like the 2nd option, i.e. change the package naming, is by far the least work and honestly not that bad. Moveit 1 doesn't seem to be getting much core support anymore, and python for moveit 1 isn't high on the priorities (although I really want it). So seems like the second option is best?

As for #2613 I think we're still blocked on docs not generating/being hosted somewhere? #2606

[PeterMitrano]

I've done some more investigating on how to get import moveit.planning_interface to work despite the devel space limitations. There are one or two ways to make it work using some tricks, but they require centralizing the python bindings to some extent, instead of distributing them completely across each package. Specifically, we could:

1. Put all the python bindings in one package, this means the .cpp files defining the bindings as well as the init.py files and such. In my opinion, this is a very clean & simple solution, but it means we have to maintain python bindings in a separate place
2. Put some import magic in the init.py files in a centralized location. The actual binding definitions can live in each specific package, and each package will build a file like pymoveit_core.so or pymoveit_planning_interface.so, and we would have one package that lets you do import moveit.[core|planning_interface]

Thoughts?

[PeterMitrano]

Alternatively, we can just not have in centralized imports, and stick with import moveit_core and import moveit_planning_interface