Python Bindings for some of moveit_core

[PeterMitrano]

Description

Add python bindings for PlanningScene, complete enough to make most of the PlanningScene tutorial work in python. See #900 for earlier discussion. I can make a PR with the new tutorial, but for that to make sense we'd need to figure out how/where to generate docs for these bindings. For now you can just run the tutorial here to test the bindings: https://github.com/UM-ARM-Lab/moveit_tutorials/blob/python-bindings/doc/planning_scene_python/src/planning_scene_tutorial.py

This hopefully sets the stage for an overhaul of moveit_commander as well.

Checklist

• Required by CI: Code is auto formatted using clang-format
• Extend the tutorials / documentation reference
• Document API changes relevant to the user in the MIGRATION.md notes
• Create tests, which fail without this PR reference
• Include a screenshot if changing a GUI
• While waiting for someone to review your request, please help review another open pull request to support the maintainers

[rhaschke]

Thanks a lot for this effort. I have a few inline remarks (see below) that apply across the whole PR:

• Add a copyright disclaimer to all files
• Avoid extra definitions of overload_casts, where it doesn't improve readability

[rhaschke]

Couldn't this be handled by an appropriate type caster?

[PeterMitrano]

true, although actually I think we need to somehow explicitly prevent/delete that overload. It doesn't work, and in general we should prohibit calling function that takes a ros message by mutable reference, because the mutation won't make it back to the python caller.

[rhaschke]

Yeah, that's another concern. Generally, of course, it is possible to modify passed arguments.

[PeterMitrano]

Do you know if we can make sure this is not accidentally allowed? I thought I could just change the enable_if statement, so that the specialization didn't match if it was a ref to non-const, but I didn't work. This is what I tried:

struct type_caster<T, std::enable_if_t<ros::message_traits::IsMessage<T>::value &&
                                      (std::is_const<std::remove_reference<T>>::value || !std::is_reference<T>::value)>>

but this still allows me to call a function that takes a ros message by ref to non-const, which is what I want to avoid.

[PeterMitrano]

A few points we should discuss in general:

1. Which python version are we supporting? Here I've written with/for python 3.8 because that's the default on 20.04 and I think most noetic users use 3.8
2. How do we generate documentation?
3. What next? I'm happy to continue (slowly) implementing this stuff, are there any strong opinions on where to go next?

[rhaschke]

1. Which python version are we supporting? Here I've written with/for python 3.8 because that's the default on 20.04 and I think most noetic users use 3.8

pybind11 is rather agnostic regarding the python version. So, why do you see a need to restrict to a specific version?
If really needed, I would target 20.04's python version as well.

2. How do we generate documentation?

Do you want to have a doxygen-like API doc?

[PeterMitrano]

1. Which python version are we supporting? Here I've written with/for python 3.8 because that's the default on 20.04 and I think most noetic users use 3.8

pybind11 is rather agnostic regarding the python version. So, why do you see a need to restrict to a specific version?
If really needed, I would target 20.04's python version as well.

was thinking about this for any pure python code we include, which so far was just the tutorial I was working on. So actually for this PR we don't need to worry about this. I'll make a PR to the tutorials repo and there it may be relevant.

1. How do we generate documentation?

Do you want to have a doxygen-like API doc?

Sure, something like that. moveit_commander API docs, and those are fine. I just think we need some kind of online docs people can browse, and that we can link to in whatever tutorials may be written for using the python bindings.

[rhaschke]

I think moveit_commander's API docs are generated by rosdoc from the python code. sphinx can also auto-generate docs from pybind11 bindings:
https://pybind11.readthedocs.io/en/stable/advanced/misc.html#generating-documentation-using-sphinx

[PeterMitrano]

I think moveit_commander's API docs are generated by rosdoc from the python code. sphinx can also auto-generate docs from pybind11 bindings:
https://pybind11.readthedocs.io/en/stable/advanced/misc.html#generating-documentation-using-sphinx

Thanks for the tip. I've got this working ok, but I'm not sure where this will go or how to PR it. It's going to involve putting a handful of config/text files + Makefile somewhere, and then there will be build folders where HTML is generated. So where should that go, and how would that get hosted somewhere? We can leave all this out for this PR, which is already big, but just asking for next steps.

[rhaschke]

To (automatically) install API docs via rosdoc_lite, please consult the wiki http://wiki.ros.org/rosdoc_lite. Can you link the config files you used?

[PeterMitrano]

To (automatically) install API docs via rosdoc_lite, please consult the wiki http://wiki.ros.org/rosdoc_lite. Can you link the config files you used?

Here's what I've got:

https://github.com/UM-ARM-Lab/moveit/tree/docs/moveit_core/doc

If you run make html in this folder, it should generate everything into _build and _autosummary folders, perhaps a few others.

Thanks for the link, I had no idea ros how the rosdoc stuff worked. I added a rosdoc.yaml file listing doxygen and sphinx and it seems to work locally! This is on the doc branch I linked to, so check it out and let me know if I should include the docs configuration stuff in this PR.

[PeterMitrano]

I want to add the copyright statements now, but I'm not sure what to put. Do I use my name? or PickNik? or something else?

/*********************************************************************
 * Software License Agreement (BSD License)
 *
 *  Copyright (c) 2021, Peter Mitrano
 *  All rights reserved.
 *
 *  Redistribution and use in source and binary forms, with or without
 *  modification, are permitted provided that the following conditions
 *  are met:
 *
 *   * Redistributions of source code must retain the above copyright
 *     notice, this list of conditions and the following disclaimer.
 *   * Redistributions in binary form must reproduce the above
 *     copyright notice, this list of conditions and the following
 *     disclaimer in the documentation and/or other materials provided
 *     with the distribution.
 *   * Neither the name of Peter Mitrano nor the names of its
 *     contributors may be used to endorse or promote products derived
 *     from this software without specific prior written permission.
 *
 *  THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
 *  "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
 *  LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS
 *  FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE
 *  COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT,
 *  INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING,
 *  BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
 *  LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER
 *  CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
 *  LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN
 *  ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
 *  POSSIBILITY OF SUCH DAMAGE.
 *********************************************************************/

/* Author: Peter Mitrano */

[rhaschke]

Of course, you should use your name. It's your work! Please use my name for the ROS msg type caster, as this is copied 1:1 from MTC, isn't it?

[PeterMitrano]

Ok, and yes I will put your name on those files.

…

On Sun, Mar 14, 2021, 14:20 Robert Haschke ***@***.***> wrote: Of course, you should use your name. It's your work! Please use my name for the ROS msg type caster, as this is copied 1:1 from MTC, isn't it? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2547 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA6TGET76EYZ7AT5YWP3RFDTDT47BANCNFSM4Y6WO2GA> .

[codecov]

Codecov Report

Merging #2547 (55070f8) into master (a8326da) will decrease coverage by 0.03%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2547      +/-   ##
==========================================
- Coverage   60.51%   60.49%   -0.02%     
==========================================
  Files         402      402              
  Lines       29606    29606              
==========================================
- Hits        17913    17907       -6     
- Misses      11693    11699       +6     

Impacted Files	Coverage Δ
...on/pick_place/src/approach_and_translate_stage.cpp	73.24% <0.00%> (-0.70%)	⬇️
...nning_scene_monitor/src/planning_scene_monitor.cpp	68.13% <0.00%> (-0.54%)	⬇️
...dl_kinematics_plugin/src/kdl_kinematics_plugin.cpp	81.00% <0.00%> (-0.41%)	⬇️
...meterization/work_space/pose_model_state_space.cpp	83.34% <0.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a8326da...55070f8. Read the comment docs.

[rhaschke]

This looks quite good already.

I added a rosdoc.yaml file listing doxygen and sphinx and it seems to work locally!

Great. I tried myself for MTC, but it doesn't yet work: the API summaries for my pybind11 modules are empty.

Let me know if I should include the docs configuration stuff in this PR.

Yes, please. But, I think you don't need to have a Makefile for the docs. rosdoc_lite (which is run by the ROS buildfarm) should do the job.

[rhaschke]

One more nitpick. Otherwise lgtm.

[rhaschke]

Note, that there is still a clang-format issue reported from Travis.
Also, do you already install pymoveit_core.so?

[rhaschke]

I found a more severe issue: local (python) extensions don't become visible right now.

[rhaschke]

I'm inclined to slightly restructure the package layout. What about placing everything into a moveit namespace and have below that packages for each moveit_* ROS package, but dropping the (redundant) moveit_? That is:

from moveit.core import robot_model, robot_state
m = robot_model.RobotModel()

Any opinions, @felixvd, @mikeferguson?

[felixvd]

That looks perfectly fine to me. As long as the convention we choose is easy to follow and remember, I don't mind which one it ends up being. I am hoping to read and try this out on the weekend.

[PeterMitrano]

I'm inclined to slightly restructure the package layout. What about placing everything into a moveit namespace and have below that packages for each moveit_* ROS package, but dropping the (redundant) moveit_? That is:

from moveit.core import robot_model, robot_state
m = robot_model.RobotModel()

I'm glad you bring this up. I started looking into the other packages, and yea the naming gets weird. I'm a bit confused by the directory structure of moveit, because folder and namespace names don't match the library names (why is the package called moveit_ros_planning_interface, but the namespace is moveit::planning_interface, but the path is moveit_ros/planning_interface?) ... so take what I say here with a lot of salt.

But extending your proposal to another package, I think that would mean

from moveit import planning_interface 
planning_interface.MoveGroupInterface
planning_interface.PlanningSceneInterface

That looks nice, but then it turns everything into one module, which means one pymoveit.so file and the call to pybind_add_module in the top level CMakeLists, I think? Should be fine, but there may be some surprises.

If we stick with the current approach, which I do think is ok, then we'd be writing

from moveit_ros_planning_interface import planning_interface 
planning_interface.MoveGroupInterface
planning_interface.PlanningSceneInterface

I think it's fine either way, but I'm not totally sure implementing what you propose is straightforward. I'm happy to prototype it and see though!

[rhaschke]

That looks nice, but then it turns everything into one module, which means one pymoveit.so file and the call to pybind_add_module in the top level CMakeLists, I think? Should be fine, but there may be some surprises.

No, you can have separate C++ extension libs for each moveit package. I drafted the restructuring in UM-ARM-Lab@4a75ab6
The appeal of the proposed approach is to have a global, unifying moveit package.

[PeterMitrano]

I've updated it to use moveit.core, the install works, and it is compatible with other packages, so when we add bindings for moveit_ros_planning for example, it should become something like from moveit.planning import robot_model_loader

[felixvd]

Thanks for this! I tried to get this latest version to run in Melodic, but had issues. The commits to reproduce the errors are on this branch:

• random_numbers isn't a part of moveit.core
• If I comment out the rng lines, I get errors about the robot_model module not being loaded correctly, it seems:

File "/root/linked_ws_moveit/src/moveit_tutorials/doc/planning_scene_python/src/planning_scene_tutorial.py", line 34, in main
    kinematic_model = robot_model.load_robot_model(urdf_path, srdf_path)
AttributeError: 'module' object has no attribute 'load_robot_model'

I left minor comments for now. Some other thoughts:

• What is missing for Tests 5 and 11 in the tutorial, which display the contact points?
• Is there a reason to keep the RobotCommander class? Can we replace it with the new RobotState?
• Do we accept that the Python functions are camelCase and not snake_case? I would say yes.

[tylerjw]

Deleted my comment, looks like you got it figured out. I'm sorry this was so much trouble. The whole point of this new formatter system was to make it much easier to run the auto-formatters so people wouldn't comment on PRs about them and instead focus on reviewing the substance of the PR. This was also because people constantly seemed confused about how to run clang-format manually.

[PeterMitrano]

oh I agree it will be easier! it's just because I started this PR before you made that change, and didn't realize it was a new thing and I had to rebase. I didn't read your first commend carefully enough

[PeterMitrano]

Still hoping for feedback on how to resolve the catkin linting errors -- here's my understanding:

We get the first one because I have this listed as an optional dependency, and apparently that doesn't count in the linter. We can (1) make it an exec_depend does fix the problem or (2) make these bindings required and not optional ?

The second one is because the target pymoveit_core is defined in a macro and so the linter can't see it. We could add --ignore undefined_target for moveit_core, to suppress the problem. Not sure if there's a better way to fix it.

[tylerjw]

I'm sorry if this has been explained before but why would we want the python bindings to be optional? Does it greatly increase the build time?

[PeterMitrano]

It hasn't been discussed, I was just trying to be cautious with introducing it

[rhaschke]

I have added two more commits: UM-ARM-Lab#2.

• moveit should be declared as a proper namespace package to allow merging of sub packages from different sources
  While this works nicely in install space, it fails in devel space. To let this work in devel space, we would need to adapt the __init__.py files created by catkin_python_setup() to extend the sys.path to the various src folders.
  Note that the namespace package conflicts with from . import core to allow

  import moveit
  moveit.core.xxx

• Making the python bindings non-optional also silences catkin_lint warnings.

[PeterMitrano]

yay all the checks have passed! I think this might be ready to go?

[rhaschke]

@PeterMitrano, do you mind squash-merging this as a single commit? If not, the long history of commits should be cleaned up...
@felixvd, could you have a look and provide a 2nd review?

[PeterMitrano]

I believe github has an option to squash on merge? that way we keep the history in my branch, but avoid having it on master

[rhaschke]

Sure. I just wanted to know if you accept squashing into a single commit. Seems so.

[PeterMitrano]

oh yea I would highly prefer that

[felixvd]

Love it. Almost the only thing that's missing for me is setFromIK. Cannot wait to see this and the moveit_tutorials branch merged.

I saw that you have a branch with the octomap bindings. That's also a good next step. With both, these bindings will actually be really useful.

edit: I'm fine with squash merging after you have a last look at the empty fields in conf.py I left a comment about.