Skip to content

Fuzzy-matching Trajectory Cache Injectable Traits refactor 🔥🔥 (backport #2941)#3408

Merged
sea-bass merged 1 commit intojazzyfrom
mergify/bp/jazzy/pr-2941
Mar 30, 2025
Merged

Fuzzy-matching Trajectory Cache Injectable Traits refactor 🔥🔥 (backport #2941)#3408
sea-bass merged 1 commit intojazzyfrom
mergify/bp/jazzy/pr-2941

Conversation

@mergify
Copy link

@mergify mergify bot commented Mar 30, 2025

As requested in the original PR, this PR refactors the TrajectoryCache to allow users to inject their own behaviors (which will allow them to cache on and sort by any arbitrary feature, as long as it can be represented as a series of warehouse_ros columns).

Depends on:

Builds on top of:

TODOs:

  • Fix integration-test
  • Formatting pass (will be done after review)
  • Fix tutorials
  • Fix bugs

Preamble

I apologize that this PR is even larger than the one it builds on top of. Most of the added lines are docstrings or tests, and boilerplate to support the behavior injection pattern this refactor is concerned with.

On the other hand, the average file length has decreased, so the code is MUCH more modular and hopefully easy to digest.

I can't split up this PR into multiple smaller ones since technically speaking, in order to preserve cache functionality, all the feature extractors and cache insert policies introduced in this PR will need to go in together.

I would suggest looking at the tests to aid in review (they run the gamut of unit and integration tests).

You can also build and run the example:

PS: If the size is still prohibitive, and we are okay with having partial implementations live in moveit2 while reviews are pending, let me know and I will split up the PR into smaller PRs (though I suspect at that point, that a logical number of splits might end up being somewhere close to 5-10 PRs.)

Navigating This PR

Since this PR is so large, here is a proposed order for comprehending the PR.

  • Fully read this PR description and the example code in the description
  • Build and run the demo to convince yourself that the cache works (instructions in that PR)
  • Look at the new interfaces introduced
    • features/features_interface.hpp, cache_insert_policies/cache_insert_policy_interface.hpp
  • Then look at their implementations and tests
    • features/, cache_insert_policies/
  • Then look at the main TrajectoryCache class
    • trajectory_cache.hpp, trajectory_cache.cpp
  • Then tie it all together by looking at the example usage of the classes in this PR in the demo code in Add Trajectory Cache Example For Refactor moveit2_tutorials#940

Additionally, all docstrings are filled, including file ones, as appropriate. So hopefully any clarificatory questions would have already been pre-empted and answered.

Description

This PR builds on top of the fuzzy-matching Trajectory Cache introduced in:

The implementation in that PR coupled the cache tightly with assumptions about what features to extract and sort by (i.e., a specific set of start/goal constraints, and pruning by execution time.)

This means that users who might want to store different features or a subset of those features, or who might want to fetch and prune on different features (e.g., minimum jerk, path length, etc.) will be unable to.

This PR refactors the cache to allow users to inject their own feature extractors and pruning/insertion logic!

This is done by introducing two new abstract classes that can be injected into the cache methods, acting a lot like class "traits":

  • FeaturesInterface<>: Governs what query/metadata items to extract and append to the warehouse_ros query.
  • CacheInserterInterface<>: Governs the pruning and insertion logic.

For more details on FeaturesInterface, see the Docstrings: https://github.com/moveit/moveit2/blob/cc0feb37cf423076e133523ccdbbf3038b84a01e/moveit_ros/trajectory_cache/include/moveit/trajectory_cache/features/features_interface.hpp

Some notes:

  • I decided to not go with lambdas, because there should be tight coupling between the query and metadata insertion logic for a particular feature.
  • Similarly, cache insertion logic heavily benefits from being stateful, and coupling those chunks of logic together.

Example

In other words, before. the cache was used like this:

auto traj_cache = std::make_shared<TrajectoryCache>(node);
traj_cache->init(/*db_host=*/":memory:", /*db_port=*/0, /*exact_match_precision=*/1e-6);

auto fetched_trajectory =
    traj_cache->fetchBestMatchingTrajectory(*move_group_interface, robot_name, motion_plan_req_msg,
                                            _cache_start_match_tolerance, _cache_goal_match_tolerance,
                                            /*sort_by=*/"execution_time_s", /*ascending=*/true);

if (fetched_trajectory)
{
  // Great! We got a cache hit
  // Do something with the plan.
}
else
{
  // Plan... And put it for posterity!
  traj_cache->insertTrajectory(
      *interface, robot_name, std::move(plan_req_msg), std::move(res->result.trajectory),
      rclcpp::Duration(res->result.trajectory.joint_trajectory.points.back().time_from_start).seconds(),
      res->result.planning_time, /*delete_worse_trajectories=*/true);
}

Now the cache is used like this:

auto traj_cache = std::make_shared<TrajectoryCache>(node);
traj_cache->init(/*db_host=*/":memory:", /*db_port=*/0, /*exact_match_precision=*/1e-6);

std::vector<std::unique_ptr<FeaturesInterface<MotionPlanRequest>>> features;
features.emplace_back(std::make_unique<WorkspaceFeatures>());
features.emplace_back(std::make_unique<StartStateJointStateFeatures>(start_tolerance));
// ...

auto fetched_trajectory =
    traj_cache->fetchBestMatchingTrajectory(move_group, robot_name, motion_plan_req_msg,
                                            /*features=*/features,
                                            /*sort_by=*/TrajectoryCache::getDefaultSortFeature(),
                                            /*ascending=*/true);

// Or more simply, if you want the same feature set as before the refactor, instead of painfully listing the features one by one:
// Type: std::vector<std::unique_ptr<FeaturesInterface<MotionPlanRequest>>>
auto default_features = TrajectoryCache::getDefaultFeatures(_cache_start_match_tolerance, _cache_goal_match_tolerance);

if (fetched_trajectory)
{
  // Great! We got a cache hit
  // Do something with the plan.
}
else
{
  // Plan... And put it for posterity!
  //
  // NOTE: Now instead of passing a trajectory, pass the plan result,
  // it'll contain the execution time and planning time we need!
  //
  // cache_inserter is a CacheInserterInterface<MotionPlanRequest, MotionPlanResponse, msg::RobotTrajectory>
  // It will tell the trajectory cache:
  //   - how to fetch "matching entries"
  //   - how to determine if they should be pruned,
  //   - how to determine when to insert the candidate cache entry
  //   - and what metadata to attach 
  //
  // additional_features allows a user to further add more metadata features for use with fetching
  // though they will not be considered by the cache_inserter
  traj_cache->insertTrajectory(
      move_group, robot_name, std::move(plan_req_msg), std::move(plan),
      /*cache_inserter=*/BestSeenExecutionTimePolicy(),
      /*prune_worse_trajectories=*/true, /*additional_features=*/{});
}

See the motion plan request features here: 79b7f95

The Feature Contract

Users may use an instance of FeaturesInterface<> to fetch a cache entry only if it was supported by the CacheInserterInterface<> instance that they used (or on insert, the feature was added in additional_features).

I could not think of a way to create a coupling between uses of the cache inserters and the features. This is the cost of generality and allowing users to inject arbitrary logic into the cache.

As such, users must take care to look at the docs of the cache inserter to see what features can be used with them.

(This can be mitigated by adding helper methods to get "standard" bundles of features and a "standard" CacheInserter.)

Bonus

I added new features to the default feature extractors set and cleaned up some utilities!

There are now FeaturesInterface<> implementations that can handle path and trajectory constraints!
Multiple goal constraints are also handled (at the cost of increased cardinality.)

I also added "atomic" features that wrap the basic ops you can do with warehouse_ros, to allow users to specify their own metadata to tag cache entries with.

Here: cc0feb3

Pre-Existing Support

The package now provides some starter implementations that covers most general cases of motion planning.

For more information, see the implementations of:

  • FeaturesInterface
  • CacheInsertPolicyInterface

Cache Keying Features

The following are features of the plan request and response that you can key the cache on.

These support separately configurable fuzzy lookup on start and goal conditions!
Additionally, these features "canonicalize" the inputs to reduce the cardinality of the cache, increasing the chances of cache hits. (e.g., restating poses relative to the planning frame).

Supported Features:

  • "Start"
    • WorkspaceFeatures: Planning workspace
    • StartStateJointStateFeatures: Starting robot joint state
  • "Goal"
    • MaxSpeedAndAccelerationFeatures: Max velocity, acceleration, and cartesian speed limits
    • GoalConstraintsFeatures: Planning request goal_constraints
      • This includes ALL joint, position, and orientation constraints (but not constraint regions)!
    • PathConstraintsFeatures: Planning request path_constraints
    • TrajectoryConstraintsFeatures: Planning request trajectory_constraints

Additionally, support for user-specified features are provided for query-only or cache metadata tagging constant features.

Similar support exists for the cartesian variants of these.

Cache Insert and Pruning Policies

The following are cache insertion and pruning policies to govern when cache entries are inserted, and how they are (optionally) pruned.

Supported Cache Insert Policies:

  • BestSeenExecutionTimePolicy: Only insert best seen execution time, optionally prune on best execution time.
  • AlwaysInsertNeverPrunePolicy: Always insert, never prune

Caveat

The increased functionality is now no longer 100% covered. But I tried adding tests where I had time to. I am unfortunately running out of time to iterate on this, so let's be targeted with the improvements!

Build/Test

source /opt/ros/rolling/setup.bash
sudo rosdep init
rosdep update

mkdir -p traj_cache_ws/src
cd traj_cache_ws/src
git clone https://github.com/methyldragon/moveit2.git -b ch3/trajectory-cache-refactor
for repo in moveit2/moveit2.repos $(f="moveit2/moveit2_rolling.repos"; test -r $f && echo $f); do vcs import < "$repo"; done

cd ..
rosdep install --from-paths src --ignore-src --rosdistro rolling -y -r
colcon build --packages-up-to moveit_ros_trajectory_cache --cmake-args -DCMAKE_BUILD_TYPE=Release

source install/setup.bash
colcon test --packages-select moveit_ros_trajectory_cache

Precommit with:

pre-commit run --all
```<hr>This is an automatic backport of pull request #2941 done by [Mergify](https://mergify.com).

* Implement trajectory cache

Signed-off-by: methylDragon <methylDragon@gmail.com>

* Add README

Signed-off-by: methylDragon <methylDragon@gmail.com>

* Move test cpp to test directory

Signed-off-by: methylDragon <methylDragon@gmail.com>

* Clean up logging and comments

Signed-off-by: methylDragon <methylDragon@gmail.com>

* Use move_group node for time

Signed-off-by: methylDragon <methylDragon@gmail.com>

* Add and use logger

Signed-off-by: methylDragon <methylDragon@gmail.com>

* Use new move_group accessors

Signed-off-by: methylDragon <methylDragon@gmail.com>

* Coerce variable and method names to style

Signed-off-by: methylDragon <methylDragon@gmail.com>

* Formatting pass

Signed-off-by: methylDragon <methylDragon@gmail.com>

* Add docstrings

Signed-off-by: methylDragon <methylDragon@gmail.com>

* Add ability to sort in descending order

Signed-off-by: methylDragon <methylDragon@gmail.com>

* Add RFE for custom cost functions

Signed-off-by: methylDragon <methylDragon@gmail.com>

* Formatting pass

Signed-off-by: methylDragon <methylDragon@gmail.com>

* Fix build for downstream packages

Signed-off-by: methylDragon <methylDragon@gmail.com>

* Always get some workspace frame ID

Signed-off-by: methylDragon <methylDragon@gmail.com>

* Always get some cartesian path request frame ID

Signed-off-by: methylDragon <methylDragon@gmail.com>

* Fix tests

Signed-off-by: methylDragon <methylDragon@gmail.com>

* Add const qualifiers as appropriate

Signed-off-by: methylDragon <methylDragon@gmail.com>

* Add accessors, and support for preserving K plans

Signed-off-by: methylDragon <methylDragon@gmail.com>

* Edit docs and rename puts to inserts

Signed-off-by: methylDragon <methylDragon@gmail.com>

* Make clang tidy happy

Signed-off-by: methylDragon <methylDragon@gmail.com>

* Fix CMakeLists.txt

Signed-off-by: methylDragon <methylDragon@gmail.com>

* Make getters const

Signed-off-by: methylDragon <methylDragon@gmail.com>

* Clarify frame ID utils docstrings

Signed-off-by: methylDragon <methylDragon@gmail.com>

* Elaborate on trajectory cache benefits

Signed-off-by: methylDragon <methylDragon@gmail.com>

* Fix CHANGELOG, and make library shared

Signed-off-by: methylDragon <methylDragon@gmail.com>

* Add utils library with test fixtures

Signed-off-by: methylDragon <methylDragon@gmail.com>

* Add features interface with constant features

Signed-off-by: methylDragon <methylDragon@gmail.com>

* Add constraint feature extractor utils

Signed-off-by: methylDragon <methylDragon@gmail.com>

* Add RobotState.joint_state feature extractor utils

Signed-off-by: methylDragon <methylDragon@gmail.com>

* Add MotionPlanRequest features

Signed-off-by: methylDragon <methylDragon@gmail.com>

* Add GetCartesianPlanRequest features

Signed-off-by: methylDragon <methylDragon@gmail.com>

* Use namespace declarations and do cleanups

Signed-off-by: methylDragon <methylDragon@gmail.com>

* Add CacheInsertPolicyInterface and AlwaysInsertNeverPrunePolicy

Signed-off-by: methylDragon <methylDragon@gmail.com>

* Add CartesianAlwaysInsertNeverPrunePolicy

Signed-off-by: methylDragon <methylDragon@gmail.com>

* Init policy features on construction

Signed-off-by: methylDragon <methylDragon@gmail.com>

* Add execution time extraction util

Signed-off-by: methylDragon <methylDragon@gmail.com>

* Add BestSeenExecutionTimePolicy and rename methods

Signed-off-by: methylDragon <methylDragon@gmail.com>

* Add CartesianBestSeenExecutionTimePolicy

Signed-off-by: methylDragon <methylDragon@gmail.com>

* Return reason string from cache insert policy methods

Signed-off-by: methylDragon <methylDragon@gmail.com>

* Refactor TrajectoryCache to use the interfaces

Signed-off-by: methylDragon <methylDragon@gmail.com>

* Move test fixtures to their own directory

Signed-off-by: methylDragon <methylDragon@gmail.com>

* Fix bugs and build

Signed-off-by: methylDragon <methylDragon@gmail.com>

* Fix formatting and clang-tidy

Signed-off-by: methylDragon <methylDragon@gmail.com>

* Update CHANGELOG

Signed-off-by: methylDragon <methylDragon@gmail.com>

* Make clang-tidy happy

Signed-off-by: methylDragon <methylDragon@gmail.com>

* Update README

Signed-off-by: methylDragon <methylDragon@gmail.com>

* Enable unrelated query matching test

Signed-off-by: methylDragon <methylDragon@gmail.com>

* Make libraries shared

Signed-off-by: methylDragon <methylDragon@gmail.com>

* Sidestep deprecation warning for computeCartesianPath

Signed-off-by: methylDragon <methylDragon@gmail.com>

* Fix typo in trajectory cache utils test

Signed-off-by: methylDragon <methylDragon@gmail.com>

* Exclude test on humble

* Add missing header

* Use precrement for for loops

Signed-off-by: methylDragon <methylDragon@gmail.com>

* Use constref in range-based for loops in utils where possible

Signed-off-by: methylDragon <methylDragon@gmail.com>

* Reserve vectors in getSupportedFeatures

Signed-off-by: methylDragon <methylDragon@gmail.com>

* Fix README and CHANGELOG

Signed-off-by: methylDragon <methylDragon@gmail.com>

* Add and use restateInNewFrame util function

Signed-off-by: methylDragon <methylDragon@gmail.com>

* Attempt to fix policy test

Signed-off-by: methylDragon <methylDragon@gmail.com>

* Use .hpp instead of .h

Signed-off-by: methylDragon <methylDragon@gmail.com>

* Undo CHANGELOG changes

Signed-off-by: methylDragon <methylDragon@gmail.com>

* Use const ref strings for restateInNewFrame

Signed-off-by: methylDragon <methylDragon@gmail.com>

* Add clarificatory comment to tests and fix formatting

Signed-off-by: methylDragon <methylDragon@gmail.com>

* Fix compile error

* Use constref shared_ptr in restateInNewFrame

Signed-off-by: methylDragon <methylDragon@gmail.com>

* Mitigate cartesian path tests

Signed-off-by: methylDragon <methylDragon@gmail.com>

* Mitigate test flakiness

Signed-off-by: methylDragon <methylDragon@gmail.com>

* Allow -11 for move_group gtest fixture

Signed-off-by: methylDragon <methylDragon@gmail.com>

* Make execution times in test deterministic

Signed-off-by: methylDragon <methylDragon@gmail.com>

---------

Signed-off-by: methylDragon <methylDragon@gmail.com>
Co-authored-by: Sebastian Jahr <sebastian.jahr@picknik.ai>
Co-authored-by: Sebastian Jahr <sebastian.jahr@tuta.io>
Co-authored-by: Sebastian Castro <4603398+sea-bass@users.noreply.github.com>
(cherry picked from commit a12f327)
Copy link
Contributor

@sea-bass sea-bass left a comment

Choose a reason for hiding this comment

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

Decided to backport this so the main branch of tutorials works on Jazzy

@sea-bass sea-bass requested review from sjahr and stephanie-eng March 30, 2025 15:28
@codecov
Copy link

codecov bot commented Mar 30, 2025

Codecov Report

Attention: Patch coverage is 83.29646% with 302 lines in your changes missing coverage. Please review.

Project coverage is 46.23%. Comparing base (355cd6b) to head (bf72152).
Report is 1 commits behind head on jazzy.

Files with missing lines Patch % Lines
...veit_ros/trajectory_cache/src/trajectory_cache.cpp 0.00% 123 Missing ⚠️
moveit_ros/trajectory_cache/src/utils/utils.cpp 76.48% 64 Missing ⚠️
...nsert_policies/best_seen_execution_time_policy.cpp 85.58% 30 Missing ⚠️
...c/features/get_cartesian_path_request_features.cpp 83.14% 28 Missing ⚠️
...ajectory_cache/test/fixtures/warehouse_fixture.cpp 0.00% 19 Missing ⚠️
...sert_policies/always_insert_never_prune_policy.cpp 93.01% 10 Missing ⚠️
...ache/src/features/motion_plan_request_features.cpp 93.71% 8 Missing ⚠️
...est_seen_execution_time_policy_with_move_group.cpp 97.33% 5 Missing ⚠️
...ways_insert_never_prune_policy_with_move_group.cpp 97.61% 4 Missing ⚠️
...jectory_cache/test/fixtures/move_group_fixture.cpp 85.00% 1 Missing and 2 partials ⚠️
... and 4 more
Additional details and impacted files
@@            Coverage Diff             @@
##            jazzy    #3408      +/-   ##
==========================================
+ Coverage   44.78%   46.23%   +1.45%     
==========================================
  Files         701      717      +16     
  Lines       61903    62600     +697     
  Branches     7499     7572      +73     
==========================================
+ Hits        27718    28936    +1218     
+ Misses      34017    33499     -518     
+ Partials      168      165       -3     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@sea-bass sea-bass merged commit 8f9603b into jazzy Mar 30, 2025
8 checks passed
@sea-bass sea-bass deleted the mergify/bp/jazzy/pr-2941 branch March 30, 2025 17:10
@github-project-automation github-project-automation bot moved this to ✅ Done in MoveIt Mar 30, 2025
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.

3 participants