Make planning pipeline respect the allowed_planning_time #2692

TomCC7 · 2024-02-18T06:27:46Z

Description

@sjahr WIP PR to solve #2581. Several notes:

Modified allowed_planning_time directly to change the timeout of each planner
Currently the timeout for each planner is naively evenly distributed as discussed in the issue.
Haven't thoroughly tested the modification. I wonder if is there an easy way to do this. I saw a tutorial here but it seems outdated.

Checklist

Required by CI: Code is auto formatted using clang-format
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

sjahr

Nice work, thank you!

Regarding your comments:

Modified allowed_planning_time directly to change the timeout of each planner

That's fine 👍

Currently the timeout for each planner is naively evenly distributed as discussed in the issue.

👍 Sounds good for now

Haven't thoroughly tested the modification. I wonder if is there an easy way to do this. I saw a tutorial here but it seems outdated.

You could extend the existing unit tests

moveit_ros/planning/planning_pipeline/src/planning_pipeline.cpp

sjahr · 2024-02-19T10:36:18Z

moveit_ros/planning/planning_pipeline/src/planning_pipeline.cpp

@@ -288,11 +295,24 @@ bool PlanningPipeline::generatePlan(const planning_scene::PlanningSceneConstPtr&
                     req_adapter->getDescription().c_str(), status.message.c_str());
        break;
      }
+
+      // check for timeout


Add a note here that it is assumed that the request adapters only consume a very small fraction of the allowed planning time and thus they don't get a budget assigned. The timeout check is mainly a sanity check

sjahr · 2024-02-19T10:41:44Z

moveit_ros/planning/planning_pipeline/src/planning_pipeline.cpp

+
+      // TODO: should this be optional since the plugins already checked for timeout?
+      // check for timeout
+      if (std::chrono::duration<double>(clock::now() - planner_start_time).count() >= max_single_planner_time)


Maybe I would even make sense to use the same check here as for the adapters

if (std::chrono::duration<double>(clock::now() - plan_start_time).count() >= allowed_planning_time)

I think we might not care too much about the individual elements exceeding their allocated individual budget (for now) but for the overall planning process to not exceed the allowed_planning_time 🤔

Agreed. Especially because some planners will take longer, so splitting the timeout evenly seems risky.

codecov · 2024-02-19T11:23:51Z

Codecov Report

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

Project coverage is 43.03%. Comparing base (d962501) to head (167fb5e).
Report is 34 commits behind head on main.

❗ Current head 167fb5e differs from pull request most recent head af3e30e. Consider uploading reports for the commit af3e30e to get more accurate results

Files	Patch %	Lines
...planning_pipeline/test/planning_pipeline_tests.cpp	90.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2692      +/-   ##
==========================================
- Coverage   50.74%   43.03%   -7.71%     
==========================================
  Files         392      692     +300     
  Lines       32553    56351   +23798     
  Branches        0     7277    +7277     
==========================================
+ Hits        16517    24247    +7730     
- Misses      16036    31941   +15905     
- Partials        0      163     +163

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

TomCC7 · 2024-02-19T12:14:43Z

Thanks for the review! Based on your feedback I would propose that we don't enforce and check for timeout after each step but only modify the corresponding remaining time to guide the planners and adapters to follow the time limit.

We can do a single check after response adapters finished and modify the error code accordingly. In this way even if the timeout is reached the function will also finish so that users can still get the planning result. Does that sound good?

sjahr · 2024-02-19T13:51:41Z

Thanks for the review! Based on your feedback I would propose that we don't enforce and check for timeout after each step but only modify the corresponding remaining time to guide the planners and adapters to follow the time limit.

We can do a single check after response adapters finished and modify the error code accordingly. In this way even if the timeout is reached the function will also finish so that users can still get the planning result. Does that sound good?

The check in every iteration makes sense to me, so I wouldn't change that. If the first out of ten pipeline elements uses the full time budget, there is no point in running the other elements before reporting the error. I think we should always check if the full time budget is exhausted but we don't care so much if a planner uses the allocated fraction of the budget or a bit more as long as the overall planning time remains within the budget

TomCC7 · 2024-02-28T21:37:09Z

@sjahr sorry for the late update. I revised my implementation based on your feedback. Here's the update:

after checking the timeout a moveit::core::MoveItErrorCode::TIMED_OUT error code is sent, which will be catched by error code checks later
now the unused time by previous planner will be automatically released for later planners to use
added timeout check in the unit test files and verified that it's working

I think if you are good with the current time distribution strategy this pull request is ready for review and merge!

sjahr · 2024-03-04T20:03:06Z

moveit_ros/planning/planning_pipeline/src/planning_pipeline.cpp

+      std::string message = status.message;
+


Suggested change

std::string message = status.message;

sjahr

Thanks, I reviewed your changes

moveit_ros/planning/planning_pipeline/src/planning_pipeline.cpp

sjahr · 2024-03-04T20:10:55Z

moveit_ros/planning/planning_pipeline/src/planning_pipeline.cpp

-      const auto& planner = planner_map_.at(planner_name);
+      // modify planner request to notice plugins of their corresponding max_allowed_time
+      // NOTE: currently just evenly distributing the remaining time among the remaining planners
+      double max_single_planner_time = std::chrono::duration<double>(clock::now() - plan_start_time).count() /


I don't understand this time calculation. Can you explain it? Why are we giving the planner the time since the planning started as time budget? Maybe I am missing something. Also can you make this const?

sorry made a mistake, I changed it to

mutable_request.allowed_planning_time = (allowed_planning_time - std::chrono::duration<double>(clock::now() - plan_start_time).count()) / (pipeline_parameters_.planning_plugins.size() - i);

moveit_ros/planning/planning_pipeline/src/planning_pipeline.cpp

moveit_ros/planning/planning_pipeline/test/planning_pipeline_test_plugins.cpp

moveit_ros/planning/planning_pipeline/test/planning_pipeline_tests.cpp

TomCC7 · 2024-03-06T15:45:33Z

moveit_ros/planning/planning_pipeline/test/planning_pipeline_tests.cpp

@@ -119,7 +119,7 @@ TEST_F(TestPlanningPipeline, NoPlannerPluginConfigured)

 TEST_F(TestPlanningPipeline, TestTimeout)
 {
-  // construct pipline
+  // construct pipeline


didn't know that checker also checks for spelling lol

TomCC7 · 2024-03-11T18:22:39Z

hi @sjahr, just checking in, is there anything else I need to do on this PR?

henningkayser · 2024-03-14T13:55:28Z

moveit_ros/planning/planning_pipeline/src/planning_pipeline.cpp

@@ -335,6 +363,13 @@ bool PlanningPipeline::generatePlan(const planning_scene::PlanningSceneConstPtr&
        RCLCPP_INFO(node_->get_logger(), "Calling PlanningResponseAdapter '%s'", res_adapter->getDescription().c_str());
        res_adapter->adapt(planning_scene, mutable_request, res);
        publishPipelineState(mutable_request, res, res_adapter->getDescription());
+
+        // check for timeout


Maybe I'm missing it, but it looks like we will hit a timeout here if the planners use up the allowed planning time. Optimizing planners usually do that, so I think we should either ignore response adapters or use a separate budget

Yeah I think the ultimate solution to this is to give user more control over the timeout. Maybe let them decide timeout of each section (and each planner) separately? But that would require changing the MotionPlanRequest message so I'm not sure. What's your suggestion on this? Should we make the PR more complex or just ignore the response adapters for now?

Can we ensure for now that a fraction of the time will be reserved for the planners? Something like 100ms in any case?

But in that case the planner might still use up all the reserved time. Or are you saying that giving the request adapter at least 100ms?

the response adapter should have a reserved time budget which is deducted from the planner's allowed planning time.

hi, I add an extra check to ensure that the response adapter has 10ms left in the latest commit

EzraBrooks · 2024-05-01T15:47:11Z

@TomCC7, I apologize for the delay, we've been pretty absorbed with bugfixes for multi-arm and mobile manipulator configurations lately.

@sjahr @henningkayser is there anything else we need changed on this PR before we give it a last round of testing and merge it?

github-actions · 2024-06-28T12:38:16Z

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.

Make planning pipeline respect the allowed_planning_time

3930be6

sjahr reviewed Feb 19, 2024

View reviewed changes

sjahr linked an issue Feb 19, 2024 that may be closed by this pull request

[Planning Pipeline] Make planning pipeline respect the max. planning time #2581

Open

sjahr requested review from marioprats, henningkayser and sea-bass February 19, 2024 10:47

TomCC7 added 3 commits February 28, 2024 18:37

feat: timeout check set error_code instead of throw

e37e314

feat: evenly distribute the planning time among remaining planners

69b6de8

test: add timeout handling test for planningpipeline

06377f7

sjahr reviewed Mar 4, 2024

View reviewed changes

sjahr marked this pull request as ready for review March 5, 2024 07:36

sjahr changed the title ~~WIP: Make planning pipeline respect the allowed_planning_time~~ Make planning pipeline respect the allowed_planning_time Mar 5, 2024

TomCC7 added 2 commits March 6, 2024 03:06

chore, fix: formatting and minor fixes for planning pipeline timeout

167fb5e

fix spelling

99bf09c

TomCC7 commented Mar 6, 2024

View reviewed changes

henningkayser reviewed Mar 14, 2024

View reviewed changes

TomCC7 and others added 2 commits March 28, 2024 15:32

feat: allowing extra 10ms for response adapter

cffe188

Merge branch 'main' into main

af3e30e

github-actions bot added the stale Inactive issues and PRs are marked as stale and may be closed automatically. label Jun 28, 2024

Merge branch 'main' into main

f903bd0

github-actions bot removed the stale Inactive issues and PRs are marked as stale and may be closed automatically. label Jul 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make planning pipeline respect the allowed_planning_time #2692

Make planning pipeline respect the allowed_planning_time #2692

TomCC7 commented Feb 18, 2024 •

edited

Loading

sjahr left a comment •

edited

Loading

sjahr Feb 19, 2024

sjahr Feb 19, 2024

sea-bass Feb 19, 2024

codecov bot commented Feb 19, 2024 •

edited

Loading

TomCC7 commented Feb 19, 2024

sjahr commented Feb 19, 2024

TomCC7 commented Feb 28, 2024

sjahr Mar 4, 2024

sjahr left a comment

sjahr Mar 4, 2024

TomCC7 Mar 6, 2024

TomCC7 Mar 6, 2024

TomCC7 commented Mar 11, 2024

henningkayser Mar 14, 2024

TomCC7 Mar 14, 2024

sjahr Mar 15, 2024

TomCC7 Mar 15, 2024

henningkayser Mar 26, 2024

TomCC7 Mar 28, 2024

EzraBrooks commented May 1, 2024

github-actions bot commented Jun 28, 2024

Make planning pipeline respect the allowed_planning_time #2692

Are you sure you want to change the base?

Make planning pipeline respect the allowed_planning_time #2692

Conversation

TomCC7 commented Feb 18, 2024 • edited Loading

Description

Checklist

sjahr left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 19, 2024 • edited Loading

Codecov Report

TomCC7 commented Feb 19, 2024

sjahr commented Feb 19, 2024

TomCC7 commented Feb 28, 2024

Choose a reason for hiding this comment

sjahr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomCC7 commented Mar 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EzraBrooks commented May 1, 2024

github-actions bot commented Jun 28, 2024

TomCC7 commented Feb 18, 2024 •

edited

Loading

sjahr left a comment •

edited

Loading

codecov bot commented Feb 19, 2024 •

edited

Loading