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

(moveit_py) Python Bindings for MoveGroupInterface #2574

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

m-elwin
Copy link
Contributor

@m-elwin m-elwin commented Dec 4, 2023

Description

Implements preliminary python bindings for the C++ MoveGroupInterface

Only the minimum needed to create a python version of the Your First C++ MoveIt Project Tutorial, is currently implemented.

A draft of a python version of the tutorial can be found here.

For example, ros2 launch moveit2_tutorials demo.launch.py. Then, with this PR,
running the following python script will move end-effector to a target pose:

import moveit
from moveit import MoveGroupInterface
from geometry_msgs.msg import Pose, Quaternion, Point

# Creates a node called "hello_moveit_interface" 
# (in C++ with a separate executor) and connects to the "manipulator" move_group
move_group_interface = MoveGroupInterface("hello_moveit_interface", "manipulator")

# set the desired end-effector target pose
target_pose = Pose(quaternion=Quaternion(w=1),
               position=Point(x=0.28, y=-0.2, z=0.5))
move_group_interface.setPoseTarget(target_pose)

# Plan the motion
(result, plan) = move_group_interface.plan()

# Execute the motion if planning was successful
if result.val == result.SUCCESS:
    move_group_interface.execute(plan)
else:
    print("Planning Failed")

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
  • While waiting for someone to review your request, please help review another open pull request to support the maintainers

@m-elwin m-elwin marked this pull request as ready for review December 4, 2023 16:18
@m-elwin m-elwin changed the title Python Bindings for MoveGroupInterface (moveit_py) Python Bindings for MoveGroupInterface Dec 4, 2023
@henningkayser henningkayser requested a review from sjahr December 12, 2023 15:34
Copy link

codecov bot commented Dec 12, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 50.73%. Comparing base (87d5945) to head (b0eaa9a).
Report is 67 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2574   +/-   ##
=======================================
  Coverage   50.73%   50.73%           
=======================================
  Files         386      386           
  Lines       32087    32087           
=======================================
  Hits        16276    16276           
  Misses      15811    15811           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@m-elwin
Copy link
Contributor Author

m-elwin commented Dec 12, 2023

Re-based off the latest main branch.

The warning is caused (in this case) by binding a C++ function that
takes a std::vector without including <pybind11/stl.h>
The MoveGroupInterface class can now be imported in python
with from moveit.planning import MoveGroupInterface

Minimal functionality is ported over, but enough to enable
a parallel tutorial to the "Your First C++ MoveIt Project"

This means that the following functions are wrapped for python:

MoveGroupInterface::MoveGroupInterface
MoveGroupInterface::Plan (a struct that holds the motion plan)
MoveGroupInterface::setPoseTarget
MoveGroupInterface::plan
MoveGroupInterface::execute
@sjahr sjahr requested review from peterdavidfagan and removed request for peterdavidfagan December 22, 2023 11:14
Copy link
Contributor

@sjahr sjahr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this contribution!
I am not sure, if it makes sense to expose the move_group_interface with the python bindings. We'd have a python wrapper around the cpp wrapper of the ros interface of moveit which is probably too many layers. I think an approach such as the one taken here where the python code uses the move_group ros api directly might be more appropriate although I am not sure if something like this needs to live in the main repository. Please let me know if you have a good rationale for the MoveGroupInterface python bindings, I am not seeing at the moment!

@m-elwin
Copy link
Contributor Author

m-elwin commented Dec 22, 2023

Thanks for the feedback @sjahr.

There are two primary motivations behind why I decided to work on this:

  1. Keep the beginner python moveit2 experience as close as possible to that of the beginner C++ experience.
  2. Enable every C++ moveit tutorial to have a direct python equivalent and help ensure that what can be done in C++ can also be done in python with similar code.

Here's the main motivating example: The Your First C++ MoveIt Project

I wrote an equivalent python tutorial Your First Python MoveIt Project (which relies on this PR).

The python tutorial could not be written with existing moveit_py, because it would require setting up moveit parameters in a launchfile. Nor could it be written with pymoveit2 because the python user would need to deal with executors, callback groups, and threads (as far as I can tell). Either way, the alternative approaches would have resulted in a python tutorial that was arguably more complicated than the C++ equivalent (which seems wrong).

It would definitely be possible to do all this with a third-party python wrapper for the ROS API (such as pymoveit2) but there are a few disadvantages of this approach that I can see:

  1. These API's being third-party makes them seem less official and less integrated and supported.
  2. There is potentially a lot of duplicated code in maintaining a separate pure-python API wrapper, especially because moveit_py already provides the infrastructure for making python bindings.
  3. It is easier to ensure feature-parity and similar usage patterns between C++ and python by using bindings rather than a separate ROS API wrapper.

I agree that there are advantages to the approach pymoveit2 takes, including avoiding "wrapping the wrapper", but I think of both approaches as complimentary rather than mutually exclusive and targeted at slightly different users and use-cases.

@MatthijsBurgh
Copy link
Contributor

I think I agree with @sjahr.

The move group interface is some cpp code to talk to ROS API of moveit. I don't think we should wrap that with pybind. I do think it is a good idea to create a move group interface for python, but written in python.

If that library is close to the cpp interfaces, it could live in this repo, maybe even be part of moveit_py.

@m-elwin
Copy link
Contributor Author

m-elwin commented Jan 3, 2024

Thanks for the feedback @MatthijsBurgh.

It seems like the main reluctance here is that MoveGroupInterface is already a wrapper around the ROS API so this is "wrapping a wrapper".

My concern is that MoveGroupInterface does not seem to be just a simple ROS API wrapper: it uses threads, futures, and matrix math,all of which a pure python approach needs to re-implement and maintain. And the existing python API wrappers (such as pymoveit2) are non-trivial.

Anyway, thanks for taking the time to consider this. I'd like to help contribute to making the python and C++ moveit experiences as similar as possible (especially in terms of ROS 2 knowledge required for a given task) regardless of the outcome of this PR, so I'd appreciate a bit more feedback on how to proceed.

Is the pybind11 approach in this PR ruled out completely (seems like it might be)? If so I'll close the PR (and can make a new one removing the currently unused python wrapping code in the C++ MoveGroupInterface).

If there is a possibility of acceptance down the road, I'm happy to attempt writing a full wrapper implementation with tests and tutorials to prove the concept further.

@sjahr
Copy link
Contributor

sjahr commented Jan 8, 2024

@m-elwin We really appreciate your effort here!

Is the pybind11 approach in this PR ruled out completely (seems like it might be)? If so I'll close the PR (and can make a new one removing the currently unused python wrapping code in the C++ MoveGroupInterface).

Maybe this would be a good step forward. Furthermore, you could try writing a minimal python move_group interface (e.g. as a tutorial for moveit and we'll see where to go from there). If you're interested you could join the public maintainer meeting later this month to discuss any further ideas

Copy link

mergify bot commented Feb 13, 2024

This pull request is in conflict. Could you fix it @m-elwin?

Copy link

github-actions bot commented Apr 1, 2024

This PR is stale because it has been open for 45 days with no activity. Please tag a maintainer for help on completing this PR, or close it if you think it has become obsolete.

@github-actions github-actions bot added the stale Inactive issues and PRs are marked as stale and may be closed automatically. label Apr 1, 2024
Copy link

mergify bot commented Apr 1, 2024

This pull request is in conflict. Could you fix it @m-elwin?

@github-actions github-actions bot removed the stale Inactive issues and PRs are marked as stale and may be closed automatically. label Apr 2, 2024
Copy link

This PR is stale because it has been open for 45 days with no activity. Please tag a maintainer for help on completing this PR, or close it if you think it has become obsolete.

@github-actions github-actions bot added the stale Inactive issues and PRs are marked as stale and may be closed automatically. label Nov 20, 2024
@codecov-commenter
Copy link

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 50.73%. Comparing base (87d5945) to head (b0eaa9a).
Report is 189 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2574   +/-   ##
=======================================
  Coverage   50.73%   50.73%           
=======================================
  Files         386      386           
  Lines       32087    32087           
=======================================
  Hits        16276    16276           
  Misses      15811    15811           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@github-actions github-actions bot removed the stale Inactive issues and PRs are marked as stale and may be closed automatically. label Nov 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants