Skip to content

Test PlanningRequestAdapterChain call sequence#2234

Closed
henningkayser wants to merge 1 commit intomoveit:mainfrom
henningkayser:pr/adapter_chain
Closed

Test PlanningRequestAdapterChain call sequence#2234
henningkayser wants to merge 1 commit intomoveit:mainfrom
henningkayser:pr/adapter_chain

Conversation

@henningkayser
Copy link
Copy Markdown
Member

@henningkayser henningkayser commented Jun 7, 2023 •
edited
Loading

Motivation
I'm currently debugging a confusing issue where failing adapters are being called in the wrong sequence. In particular, it's the FixStartStateBounds adapter failing right AFTER TimeOptimalTrajectoryGeneration complains about path tolerance. Both failures/warnings have their own reason and need to be addressed in the setup, however the planning pipeline should never put FixStartStateBounds (which should be called before the planner) after TOTG (which should always be called after the planner).

With this test I wanted to figure out what's really going on in an AdapterChain, and was already surprised multiple times. The sequence of adapters is not respected correctly, and failing adapters or plans don't result in an aborted chain (why should we trigger a motion plan if there are already fixup adapters failing before it?). To me this seems pretty serious, already the happy path.

Problem
Let's build a chain with OMPL + STOMP + TOTG in that order, according to the happy path (as implemented in my test case) the AdapterChain already switches up STOMP and TOTG, producing {OMPL + TOTG + STOMP}? (I'll test that exact use case next)

Maybe I'm getting something really wrong here, but to me the implementation of the adapter chain and the adapters themselves just ask for a reimplementation, where:

  • BEFORE and AFTER (PreAdapter, PostAdapter) are explicitly typed
  • Failing adapters or planner will actually abort the chain

With the current implementation it's almost impossible to know what's going on.

Test log from original commit

build/moveit_core/test_results/moveit_core/test_planning_request_adapter.gtest.xml: 1 test, 0 errors, 1 failure, 0 skipped
- moveit_core.TestPlanningRequestAdapterChain SequenceOK
  <<< failure message
    /home/ubuntu/ws_ros2/src/moveit2/moveit_core/planning_request_adapter/test/test_planning_request_adapter.cpp:194
    Value of: called_adapters
    Expected: has 5 elements where
    element #0 is equal to "A",
    element #1 is equal to "B",
    element #2 is equal to "PLANNER",
    element #3 is equal to "C",
    element #4 is equal to "D"
      Actual: { "A", "B", "PLANNER", "D", "C" }, whose element #3 doesn't match
    /home/ubuntu/ws_ros2/src/moveit2/moveit_core/planning_request_adapter/test/test_planning_request_adapter.cpp:201
    Value of: bool(res)
      Actual: true
    Expected: false
    /home/ubuntu/ws_ros2/src/moveit2/moveit_core/planning_request_adapter/test/test_planning_request_adapter.cpp:202
    Value of: called_adapters
    Expected: has 3 elements where
    element #0 is equal to "A",
    element #1 is equal to "B",
    element #2 is equal to "PLANNER"
      Actual: { "A", "B", "PLANNER", "D", "C" }, which has 5 elements
    /home/ubuntu/ws_ros2/src/moveit2/moveit_core/planning_request_adapter/test/test_planning_request_adapter.cpp:212
    Value of: bool(res)
      Actual: true
    Expected: false
    /home/ubuntu/ws_ros2/src/moveit2/moveit_core/planning_request_adapter/test/test_planning_request_adapter.cpp:213
    Value of: called_adapters
    Expected: has 6 elements where
    element #0 is equal to "A",
    element #1 is equal to "B",
    element #2 is equal to "PLANNER",
    element #3 is equal to "C",
    element #4 is equal to "D",
    element #5 is equal to "fails_after"
      Actual: { "A", "B", "PLANNER", "E", "fails_after", "D", "C" }, which has 7 elements
    /home/ubuntu/ws_ros2/src/moveit2/moveit_core/planning_request_adapter/test/test_planning_request_adapter.cpp:222
    Value of: bool(res)
      Actual: true
    Expected: false
    /home/ubuntu/ws_ros2/src/moveit2/moveit_core/planning_request_adapter/test/test_planning_request_adapter.cpp:223
    Value of: called_adapters
    Expected: has 3 elements where
    element #0 is equal to "A",
    element #1 is equal to "B",
    element #2 is equal to "fails_before"
      Actual: { "A", "B", "fails_before", "PLANNER", "E", "fails_after", "D", "C" }, which has 8 elements
  >>>

@henningkayser henningkayser marked this pull request as draft June 7, 2023 16:11
@codecov
Copy link
Copy Markdown

codecov bot commented Jun 7, 2023

Codecov Report

Patch coverage: 66.67% and project coverage change: -0.05 ⚠️

Comparison is base (b98bb6b) 50.53% compared to head (1b4303a) 50.47%.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #2234      +/-   ##
==========================================
- Coverage   50.53%   50.47%   -0.05%     
==========================================
  Files         386      391       +5     
  Lines       31736    31874     +138     
==========================================
+ Hits        16034    16085      +51     
- Misses      15702    15789      +87
Impacted Files Coverage Δ
moveit_ros/moveit_servo/src/servo_calcs.cpp 71.14% <0.00%> (ø)
...anning_scene_monitor/src/current_state_monitor.cpp 74.32% <100.00%> (ø)

... and 6 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@sjahr sjahr mentioned this pull request Jul 13, 2023
Merged
6 tasks
@github-actions
Copy link
Copy Markdown

github-actions bot commented Jul 24, 2023

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 label Jul 24, 2023
@sjahr sjahr added persistent bug Something isn't working and removed stale labels Aug 5, 2023
@github-actions
Copy link
Copy Markdown

github-actions bot commented Oct 2, 2023

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 label Oct 2, 2023
@sjahr sjahr mentioned this pull request Oct 6, 2023
Closed
9 tasks
@mergify
Copy link
Copy Markdown

mergify bot commented Oct 22, 2023

This pull request is in conflict. Could you fix it @henningkayser?

@github-actions github-actions bot removed the stale label Nov 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@sjahr sjahr

Labels

bug Something isn't working

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@henningkayser @sjahr