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.

Sign up for GitHub

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

Jump to bottom

Plan cache code review fixes (sans unit tests) #26

Merged
merged 13 commits into from
Nov 27, 2023
Merged

Plan cache code review fixes (sans unit tests) #26

merged 13 commits into from
Nov 27, 2023

Conversation

methylDragon
Copy link
Collaborator

@methylDragon methylDragon commented Nov 20, 2023 •
edited
Loading

Description

This PR addresses code review comments across the following PRs:

It's done as a separate PR so the pending PRs don't need to be constantly rebased (and changes duplicated.)

The git history now looks like:

# [[ THIS PR ]]
d1847ba (HEAD -> ch3/plan-cache-code-review-fixes, origin/ch3/plan-cache-code-review-fixes)
...

# [[ PR 18 ]]
b2cda90 (origin/ch3/cartesian-plan-cache, ch3/cartesian-plan-cache) Fix move bug
...

# [[ PR 16 ]]
ca0f123 (origin/ch3/motion-plan-cache, ch3/motion-plan-cache) Remove internal cache node
...

dcede0e (tag: 0.1.1, origin/main, origin/HEAD) Bump versions to 0.1.1 (#25)

Changes

  • Remove usage of query appending macro
  • Default to warehouse_ros plugin if parameter is not set
  • Return and check cache init result
  • Add explanatory comments
  • Refactor PutPlan and update variable names
  • Split out motion plan cache into its own library

Testing

A unit test will be added in a later PR.

@methylDragon methylDragon marked this pull request as draft November 20, 2023 19:39
@methylDragon methylDragon force-pushed the ch3/plan-cache-code-review-fixes branch from 0e1e9e3 to 6ebd330 Compare November 20, 2023 19:41
@methylDragon methylDragon mentioned this pull request Nov 20, 2023
Merged
4 tasks
@methylDragon
Remove query appending macro
6ad592e 
Signed-off-by: methylDragon <methylDragon@gmail.com>
@methylDragon methylDragon force-pushed the ch3/plan-cache-code-review-fixes branch from 6ebd330 to 6ad592e Compare November 20, 2023 19:47
@methylDragon
Default to warehouse_ros plugin if warehouse plugin isn't set
5563f3c 
Signed-off-by: methylDragon <methylDragon@gmail.com>
@methylDragon methylDragon force-pushed the ch3/plan-cache-code-review-fixes branch from 76f5faf to 5563f3c Compare November 20, 2023 19:57
@methylDragon
Return and use init result
03afb8e 
Signed-off-by: methylDragon <methylDragon@gmail.com>
@methylDragon methylDragon force-pushed the ch3/plan-cache-code-review-fixes branch from 4e76d71 to 03afb8e Compare November 20, 2023 19:59
@methylDragon
Add todo for catching exceptions
0e84dd4 
Signed-off-by: methylDragon <methylDragon@gmail.com>
@methylDragon
Implement plan fetching with configurable key
6004f96 
Signed-off-by: methylDragon <methylDragon@gmail.com>
@methylDragon
Add comments for exact match tolerance
ccbf91c 
Signed-off-by: methylDragon <methylDragon@gmail.com>
@methylDragon
Slightly refactor put plan
06cdcf9 
Signed-off-by: methylDragon <methylDragon@gmail.com>
@methylDragon methylDragon changed the base branch from main to ch3/cartesian-plan-cache November 20, 2023 22:56
@methylDragon
Rename overwrite to delete_worse_plans
d1847ba 
Signed-off-by: methylDragon <methylDragon@gmail.com>
@methylDragon methylDragon force-pushed the ch3/plan-cache-code-review-fixes branch from f5713a1 to d1847ba Compare November 20, 2023 23:03
@methylDragon methylDragon changed the title (WIP) Plan cache code review fixes Plan cache code review fixes Nov 20, 2023
@methylDragon methylDragon marked this pull request as ready for review November 20, 2023 23:06
@methylDragon methylDragon changed the title Plan cache code review fixes Plan cache code review fixes (sans unit tests) Nov 20, 2023
@methylDragon
Split out motion plan cache into its own library
6e5b963 
Signed-off-by: methylDragon <methylDragon@gmail.com>
@methylDragon methylDragon force-pushed the ch3/plan-cache-code-review-fixes branch from f166c89 to 6c29eae Compare November 21, 2023 03:34
@methylDragon
Sort constraints for reduced cardinality
636b4d5 
Signed-off-by: methylDragon <methylDragon@gmail.com>
@methylDragon methylDragon force-pushed the ch3/plan-cache-code-review-fixes branch from 6c29eae to 636b4d5 Compare November 21, 2023 03:39
@methylDragon
Rename util function
5ea5ad5 
Signed-off-by: methylDragon <methylDragon@gmail.com>
@methylDragon
Add todo for is_diff
bbc07ce 
Signed-off-by: methylDragon <methylDragon@gmail.com>
This was referenced Nov 22, 2023
Closed
Merged
@methylDragon
Add unit tests for motion plan cache (#28)
1af731b 
* Add count methods

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

* Enable shared from this for cache class

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

* Add unit test build rules

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

* Add tests for motion plan cache (but not cartesian)

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

* Fix bugs in cartesian caching

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

* Add tests for cartesian plan cache

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

* Exit if a test fails

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

* Remove gtest import

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

* Remove enable_shared_from_this

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

* Only check for failure

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

* Test half in-tolerance

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

* Test different robot cache

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

* Add force_cache_mode_execute_read_only (#29)

* Add force_cache_mode_execute_read_only

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

* Add force_cache_mode_execute_read_only input port

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

---------

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

---------

Signed-off-by: methylDragon <methylDragon@gmail.com>
@methylDragon methylDragon merged commit 66864b9 into ch3/cartesian-plan-cache Nov 27, 2023
1 of 2 checks passed
Yadunund pushed a commit that referenced this pull request Dec 4, 2023
@methylDragon
Implement cartesian plan cache (#18)
e4d5dd6 
* Add cartesian plan caching interfaces

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

* Add construct_get_cartesian_plan_request

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

* Add goal query and metadata

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

* Add start query and metadata

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

* Implement top level cache ops

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

* Use motion plan cache for cartesian plans

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

* Allow mismatched plan frames since we coerce anyway

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

* Fix move bug

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

* Plan cache code review fixes (sans unit tests) (#26)

* Remove query appending macro

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

* Default to warehouse_ros plugin if warehouse plugin isn't set

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

* Return and use init result

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

* Add todo for catching exceptions

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

* Implement plan fetching with configurable key

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

* Add comments for exact match tolerance

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

* Slightly refactor put plan

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

* Rename overwrite to delete_worse_plans

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

* Split out motion plan cache into its own library

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

* Sort constraints for reduced cardinality

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

* Rename util function

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

* Add todo for is_diff

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

* Add unit tests for motion plan cache (#28)

* Add count methods

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

* Enable shared from this for cache class

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

* Add unit test build rules

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

* Add tests for motion plan cache (but not cartesian)

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

* Fix bugs in cartesian caching

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

* Add tests for cartesian plan cache

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

* Exit if a test fails

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

* Remove gtest import

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

* Remove enable_shared_from_this

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

* Only check for failure

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

* Test half in-tolerance

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

* Test different robot cache

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

* Add force_cache_mode_execute_read_only (#29)

* Add force_cache_mode_execute_read_only

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

* Add force_cache_mode_execute_read_only input port

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

---------

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

---------

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

---------

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

---------

Signed-off-by: methylDragon <methylDragon@gmail.com>
Yadunund pushed a commit that referenced this pull request Jan 22, 2024
@methylDragon
Implement motion plan cache (#16)
edce5e4 
* Add motion plan cache

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

* Switch to snake case for function names

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

* Print cache fetch time and key plans on planned execution time

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

* Add overwrite_worse_plans param

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

* Fix plan fetching printout and don't recache fetched plans

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

* Update only use cached plans parameter name

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

* Add copyright headers and motion_planner namespace

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

* Make motion_plan_cache a unique_ptr instead of shared_ptr

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

* Set numerical precision

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

* Use enum for planner database mode

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

* Rename cache methods to prepare for cartesian caching

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

* Add very important warning to cache class

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

* Fix access after move bug

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

* Remove internal cache node

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

* Implement cartesian plan cache (#18)

* Add cartesian plan caching interfaces

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

* Add construct_get_cartesian_plan_request

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

* Add goal query and metadata

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

* Add start query and metadata

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

* Implement top level cache ops

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

* Use motion plan cache for cartesian plans

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

* Allow mismatched plan frames since we coerce anyway

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

* Fix move bug

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

* Plan cache code review fixes (sans unit tests) (#26)

* Remove query appending macro

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

* Default to warehouse_ros plugin if warehouse plugin isn't set

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

* Return and use init result

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

* Add todo for catching exceptions

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

* Implement plan fetching with configurable key

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

* Add comments for exact match tolerance

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

* Slightly refactor put plan

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

* Rename overwrite to delete_worse_plans

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

* Split out motion plan cache into its own library

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

* Sort constraints for reduced cardinality

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

* Rename util function

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

* Add todo for is_diff

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

* Add unit tests for motion plan cache (#28)

* Add count methods

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

* Enable shared from this for cache class

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

* Add unit test build rules

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

* Add tests for motion plan cache (but not cartesian)

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

* Fix bugs in cartesian caching

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

* Add tests for cartesian plan cache

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

* Exit if a test fails

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

* Remove gtest import

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

* Remove enable_shared_from_this

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

* Only check for failure

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

* Test half in-tolerance

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

* Test different robot cache

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

* Add force_cache_mode_execute_read_only (#29)

* Add force_cache_mode_execute_read_only

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

* Add force_cache_mode_execute_read_only input port

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

---------

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

---------

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

---------

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

---------

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

* Fix nullptr dereference

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

* Fix and add motion planner server tests (#30)

* Fix and add motion planner server tests

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

* Add test deps

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

* Add motion planner server test to CI

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

---------

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

---------

Signed-off-by: methylDragon <methylDragon@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@koonpeng koonpeng koonpeng approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@methylDragon @koonpeng