-
Notifications
You must be signed in to change notification settings - Fork 525
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
Parallel planning pipelines #1420
Conversation
Codecov ReportBase: 51.00% // Head: 50.97% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #1420 +/- ##
==========================================
- Coverage 51.00% 50.97% -0.02%
==========================================
Files 378 378
Lines 31649 31649
==========================================
- Hits 16138 16131 -7
- Misses 15511 15518 +7
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
moveit_core/metrics/include/moveit/metrics/robot_trajectory_metrics.hpp
Outdated
Show resolved
Hide resolved
moveit_ros/planning/moveit_cpp/include/moveit/moveit_cpp/planning_component.h
Outdated
Show resolved
Hide resolved
moveit_ros/planning/moveit_cpp/include/moveit/moveit_cpp/planning_component.h
Outdated
Show resolved
Hide resolved
moveit_ros/planning/moveit_cpp/include/moveit/moveit_cpp/planning_component.h
Outdated
Show resolved
Hide resolved
{ | ||
auto planning_thread = std::thread([&]() { | ||
auto solution = plan(plan_request_parameter); | ||
planning_solutions.pushBack(solution); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some planners in OMPL (asymptotically optimal ones) can produce more than one solution before exiting. These could also be retrieved and added to planning_solutions
in case the criteria the callback uses for selecting the best solution are different than the optimization objective used by planner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know if multiple solutions are accessible through the interface that is provided by
PlanningComponent::PlanSolution PlanningComponent::plan(const PlanRequestParameters& parameters)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know. My comment isn't that important. The way to get to these solutions is first check that dynamic_cast<OMPLPlanningPipelinePtr>(planning_pipeline_ptr> != nullptr
(or something like that). Next, you have to get the ModelBasedPlanningContextPtr. Next, you loop over each solution in getOMPLSimpleSetup()->getProblemDefinition()->getSolutions()
. This can be done at some later point.
9a19320
to
f3092b8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here are some general comments on this. Do you have a minimal example of using this somewhere so it can be tested?
moveit_ros/planning/moveit_cpp/include/moveit/moveit_cpp/planning_component.h
Outdated
Show resolved
Hide resolved
eab6255
to
7c55b81
Compare
a77aea4
to
7bdabcb
Compare
99aa505
to
82901cc
Compare
moveit_ros/planning/planning_pipeline/include/moveit/planning_pipeline/planning_pipeline.h
Show resolved
Hide resolved
I think a good default planner setup would be: Priority 1. Pilz Select Pilz by default, if it succeeded. Because it follows straight lines that are nice and predictable RRTConnect is kind of a last-ditch method. Nobody wants that solution, unless the other planners fail |
afb1836
to
3427631
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for letting this get stale for so long. I'm generally approving this implementation with some smaller requests.
Long-term, I would really like to iterate on this feature to make it more of a default. Some ideas:
- All plan requests (even single ones) should run in a thread with futures for synchronization
- Multi-plan-requests would simply run multiple plan requests and keep track of futures
- Things like
terminate()
shouldn't require another pipeline lookup. The pipeline or terminate function should be stored with the requested plan instance/thread. - If possible, building of requests and parsing of responses should be in separate (free?) functions for improved readability
moveit_ros/planning/moveit_cpp/include/moveit/moveit_cpp/planning_component.h
Outdated
Show resolved
Hide resolved
moveit_ros/planning/moveit_cpp/include/moveit/moveit_cpp/planning_component.h
Outdated
Show resolved
Hide resolved
auto const& planning_pipeline = planning_pipelines_.at(pipeline_name); | ||
if (planning_pipeline->isActive()) | ||
{ | ||
planning_pipeline->terminate(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
terminate()
returns true, even if the planner is not running. https://github.com/ros-planning/moveit2/blob/main/moveit_core/planning_interface/include/moveit/planning_interface/planning_interface.h#L128
We should check for false
, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, is it enough to just check the activity of the planner instance itself or do we need to make the whole pipeline more interrupt-able, for example how long can planning_scene->isPathValid(...)
take? The isActive() function checks on the pipeline level while terminate() only checks on the planner level & only aborts the planner
moveit_ros/planning/planning_pipeline/include/moveit/planning_pipeline/planning_pipeline.h
Show resolved
Hide resolved
3427631
to
55b2cb6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here are some mostly-nitpicky comments, apologies for that. I'll give it a test now.
We've been using this in ROS1 and it's AWESOME!
moveit_core/planning_interface/include/moveit/planning_interface/planning_response.h
Outdated
Show resolved
Hide resolved
moveit_core/planning_interface/include/moveit/planning_interface/planning_response.h
Outdated
Show resolved
Hide resolved
moveit_core/planning_interface/include/moveit/planning_interface/planning_response.h
Outdated
Show resolved
Hide resolved
...t_ros/hybrid_planning/global_planner/global_planner_plugins/src/moveit_planning_pipeline.cpp
Outdated
Show resolved
Hide resolved
moveit_ros/planning/moveit_cpp/include/moveit/moveit_cpp/planning_component.h
Outdated
Show resolved
Hide resolved
moveit_ros/planning/moveit_cpp/include/moveit/moveit_cpp/planning_component.h
Outdated
Show resolved
Hide resolved
moveit_ros/planning/moveit_cpp/include/moveit/moveit_cpp/planning_component.h
Outdated
Show resolved
Hide resolved
moveit_ros/planning/moveit_cpp/include/moveit/moveit_cpp/planning_component.h
Show resolved
Hide resolved
moveit_ros/planning/moveit_cpp/include/moveit/moveit_cpp/planning_component.h
Outdated
Show resolved
Hide resolved
moveit_ros/planning/planning_pipeline/include/moveit/planning_pipeline/planning_pipeline.h
Outdated
Show resolved
Hide resolved
moveit_core/planning_interface/include/moveit/planning_interface/planning_response.h
Outdated
Show resolved
Hide resolved
moveit_core/planning_interface/include/moveit/planning_interface/planning_response.h
Show resolved
Hide resolved
moveit_core/planning_interface/include/moveit/planning_interface/planning_response.h
Show resolved
Hide resolved
Add plan(const MultiPipelinePlanRequestParameters& parameters) Add mutex to avoid segfaults Add optional stop_criterion_callback and solution_selection_callback Remove stop_criterion_callback Make default solution_selection_callback = nullptr Remove parameter handling copy&paste code in favor of a template Add TODO to refactor pushBack() method into insert() Fix selection criteria and add RCLCPP_INFO output Changes due to rebase and formatting Fix race condition and segfault when no solution is found Satisfy clang tidy Remove mutex and thread safety TODOs Add stopping functionality to parallel planning Remove unnecessary TODOs
0407091
to
5c73e31
Compare
Co-authored-by: AndyZe <andyz@utexas.edu>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I gave it a final test with the WIP tutorial. LGTM!
…3244) This is a backport of: - moveit/moveit2#1420 - moveit/moveit2#1710 Co-authored-by: Robert Haschke <rhaschke@users.noreply.github.com>
Description
This PR adds a first implementation of @mamoll proposal in #1200 to run multiple planning pipelines in parallel to improve the path quality by creating multiple solutions and choosing the most suitable one. I extended the MoveItCpp interface with an additional function that allows processing multiple planning requests in parallel in separate threads. Additionally, I've refactored the kinematics_metrics package in moveit_core to a more general package called metrics which contains several metric functions for robot trajectories. These implementations were mostly extracted out of the benchmarks package to become more visible and usable at runtime outside of the benchmarking context. I'll probably separate this change in a second PR once this is not WIP anymore.
Tutorial
→ https://moveit.picknik.ai/main/doc/how_to_guides/parallel_planning/parallel_planning_tutorial.html
Usage example:
config.yaml
demo_node.cpp
TODO
Checklist