Constraint samplers with seed #3112

tahsinkose · 2022-04-07T22:04:50Z

Description

This PR adds the ability to specify a seed for constraint samplers through the parameter server, which would be useful in deterministic testing and benchmarking of these objects.

Checklist

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

codecov · 2022-04-08T04:05:30Z

Codecov Report

Merging #3112 (52d8387) into master (0f66b99) will decrease coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #3112      +/-   ##
==========================================
- Coverage   61.60%   61.58%   -0.02%     
==========================================
  Files         376      376              
  Lines       33309    33317       +8     
==========================================
- Hits        20518    20516       -2     
- Misses      12791    12801      +10

Impacted Files	Coverage Δ
.../constraint_samplers/default_constraint_samplers.h	`79.32% <100.00%> (+1.54%)`	⬆️
...raint_samplers/src/default_constraint_samplers.cpp	`79.77% <100.00%> (+0.36%)`	⬆️
.../ompl_interface/src/detail/constrained_sampler.cpp	`43.91% <0.00%> (-17.07%)`	⬇️
...meterization/work_space/pose_model_state_space.cpp	`80.00% <0.00%> (-1.76%)`	⬇️
...on/pick_place/src/approach_and_translate_stage.cpp	`75.16% <0.00%> (-0.60%)`	⬇️
...dl_kinematics_plugin/src/kdl_kinematics_plugin.cpp	`77.91% <0.00%> (-0.37%)`	⬇️
...g_scene_interface/src/planning_scene_interface.cpp	`52.20% <0.00%> (+1.10%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0f66b99...52d8387. Read the comment docs.

rhaschke

Thanks for this contribution. Please adjust your code for a minimal changeset. The comments provided for JointConstraintSampler apply to IKConstraintSampler as well.

rhaschke · 2022-04-08T06:36:43Z

...it_core/constraint_samplers/include/moveit/constraint_samplers/default_constraint_samplers.h

+    int rng_seed;
+    if (ros::param::get("~constraint_sampler_random_seed", rng_seed))
+    {
+      ROS_WARN_STREAM_NAMED("constraint_samplers",


Why did you decided for a warning here? This is an info at most, preferably a debug message, isn't it?

Suggested change

ROS_WARN_STREAM_NAMED("constraint_samplers",

ROS_INFO_STREAM_NAMED("constraint_samplers",

Well, I had a bias from a production view. Generally, debug messages are not enabled in normal runs, which may cause this to slip away from the eyes. Anyways a simple rosparam lookup would still give the information needed, thus reducing to debug 👍🏼

rhaschke · 2022-04-08T06:37:47Z

...it_core/constraint_samplers/include/moveit/constraint_samplers/default_constraint_samplers.h

+    if (ros::param::get("~constraint_sampler_random_seed", rng_seed))
+    {
+      ROS_WARN_STREAM_NAMED("constraint_samplers",
+                            "Creating random number generator with seed " << std::to_string(rng_seed));


No need to convert the int to a string. The streaming operator supports int.

Suggested change

"Creating random number generator with seed " << std::to_string(rng_seed));

"Creating random number generator with seed " << rng_seed);

...it_core/constraint_samplers/include/moveit/constraint_samplers/default_constraint_samplers.h

rhaschke · 2022-04-08T06:44:56Z

...it_core/constraint_samplers/include/moveit/constraint_samplers/default_constraint_samplers.h

@@ -191,7 +203,8 @@ class JointConstraintSampler : public ConstraintSampler

  void clear() override;

-  random_numbers::RandomNumberGenerator random_number_generator_; /**< \brief Random number generator used to sample */
+  std::unique_ptr<random_numbers::RandomNumberGenerator>


Why do you need to change API? Please revert this.

Without this, random_number_generator_ gets default constructed and the assignment operator of random_numbers::RandomNumberGenerator is deleted, hence I can't do the following:

random_number_generator_ =random_numbers::RandomNumberGenerator(rng_seed);

This way it enables us to construct it once. This API change is thus necessary.

It would be nicer if you explain why you disagree @v4hn. I'm genuinely curious about how this can be done without API* change.

*random_number_generator_ is fully internal, so it is not an API. This change will have zero effects from the users' perspective. (unless they are using a compute-board that has very limited heap access)

tahsinkose · 2022-04-08T07:53:23Z

@rhaschke I wanted this capability for both constraint samplers as IKConstraintSampler might need that too. Addressed your review, PTAL 🙏🏼

tahsinkose · 2022-04-08T09:08:24Z

Added IKConstraintSampler test.

tahsinkose · 2022-04-12T10:05:30Z

@rhaschke PTAL 🙏🏼

tahsinkose · 2022-04-13T15:59:12Z

@v4hn Please have a look. As time passes, it becomes much more difficult to contribute.

v4hn

looks good after addressing the open issues.
Determinism is sure important! (and sadly by design MoveIt cannot give a lot of that)

...it_core/constraint_samplers/include/moveit/constraint_samplers/default_constraint_samplers.h

rhaschke

@tahsinkose, looks like your new tests are never finishing.
Could you please have another look and fix them?

tahsinkose · 2022-04-18T18:32:22Z

@rhaschke I just checked. The parameter server manipulation inevitably led to the existence of ROS master, hence the test needed to be updated to be a rostest. PTAL 🙏🏼

tahsinkose · 2022-04-18T18:38:17Z

moveit_core/constraint_samplers/CMakeLists.txt

+  add_rostest_gtest(test_constraint_samplers
+    test/constraint_samplers.test


@rhaschke This seems like an unorthodox addition to moveit_core as I didn't see any instances of rostest in the package. I guess all rostest tests are categorized strictly differently than unit tests according to MoveIt conventions, but that's just a hunch regarding the status quo.

If you think this is a serious violation, then I need to move the newly added tests to a convenient place in moveit_ros.

@v4hn, could you review as well and comment on this point? IMO, moveit_core isn't ROS-agnostic for a while already, e.g. using ROS logging in a few places. However, ROS parameters are actually not yet used in moveit_core.

I fully agree. If we allow ros parameters here (and we seem to agree we want to do so) then we also need to allow tests for them, so both reading the param as well as adding the ros test are fine by me.

Maybe the ros2 sync will split it out into a separate framework structure, but for this repo I will just merge this as is now.

tahsinkose · 2022-04-18T18:54:21Z

The failing checks seem to be related with https://github.blog/2022-04-12-git-security-vulnerability-announced/. Also see actions/checkout#760

rhaschke · 2022-04-19T12:19:55Z

Hi @tylerjw,
did you run into git issues like these for MoveIt2 as well? If so, how did you solve them?

setup_upstream_workspace
$ sudo apt-get -qq install -y --no-upgrade --no-install-recommends python3-vcstool | grep -E 'Setting up' 
Setting up python3-vcstool (0.3.0-1) ...
EEE
=== /home/runner/work/moveit/moveit/.work/upstream_ws/src/geometric_shapes (git) ===
Could not determine remotes: fatal: unsafe repository ('/home/runner/work/moveit/moveit/.work/upstream_ws/src/geometric_shapes' is owned by someone else)

The problem only arises if the (upstream) cache was restored.

tahsinkose · 2022-04-22T06:39:46Z

Any update on this @rhaschke @tylerjw? I think this issue should be affecting all PRs right now..

v4hn · 2022-04-28T15:13:40Z

@JafarAbdi

rhaschke · 2022-04-29T06:29:13Z

Closing and reopening to trigger CI.

rhaschke · 2022-05-05T10:21:55Z

@tahsinkose, could you please rebase this onto the latest master (which has fixed CI).
@v4hn, I believe I have seen you rebasing and force-pushing other user's PRs in the past. However, I cannot find a corresponding github interface. How did you achieve that?

v4hn · 2022-05-05T19:24:23Z

if the user allowed edits by maintainers I believe you can simply force-push to their PR branch. I wouldn't know how else I would have done that.

rhaschke · 2022-05-05T19:28:57Z

If the user allowed edits by maintainers I believe you can simply force-push to their PR branch.

No, that doesn't work. So, @tahsinkose, please rebase yourself. Thanks.

tahsinkose · 2022-05-05T19:33:01Z

If the user allowed edits by maintainers I believe you can simply force-push to their PR branch.

No, that doesn't work. So, @tahsinkose, please rebase yourself. Thanks.

Rebased @rhaschke 👍🏼

rhaschke

The tests pass now. Thanks.

v4hn · 2022-05-09T08:39:20Z

Thanks for your patience with this @tahsinkose 👍

tahsinkose · 2022-05-09T08:45:18Z

No worries @v4hn I got used to the elaborate review process of MoveIt maintainers 😝

v4hn · 2022-05-09T09:23:48Z

I'm not sure that's a good thing. ;) But we are volunteers after all 😛
Do you have any idea why your new tests now fails upstream right away?

tahsinkose · 2022-05-09T09:25:39Z

I'm not sure that's a good thing. ;) But we are volunteers after all stuck_out_tongue Do you have any idea why your new tests now fails upstream right away?

Maybe just bad luck. The tests passed in this PR, not sure why they would fail in upstream 🤔

rhaschke · 2022-05-10T13:38:59Z

Looks like the test is flaky (a manually triggered second run succeeded). @tahsinkose, could you please have another look and make the test more robust?

tahsinkose · 2022-05-10T13:45:22Z

It seems, when the true randomness is restored with rosparam deletion, it starts to be flaky 😄 Adding a different seed for the 3rd sampler should solve this issue @rhaschke.

tahsinkose requested a review from v4hn as a code owner April 7, 2022 22:04

rhaschke reviewed Apr 8, 2022

View reviewed changes

tahsinkose force-pushed the constraint-samplers-with-seed branch from 29a2c4f to 9cb0edc Compare April 8, 2022 09:07

tahsinkose force-pushed the constraint-samplers-with-seed branch from 9cb0edc to 2d4a092 Compare April 8, 2022 09:10

v4hn reviewed Apr 13, 2022

View reviewed changes

...it_core/constraint_samplers/include/moveit/constraint_samplers/default_constraint_samplers.h Outdated Show resolved Hide resolved

...it_core/constraint_samplers/include/moveit/constraint_samplers/default_constraint_samplers.h Outdated Show resolved Hide resolved

rhaschke approved these changes Apr 18, 2022

View reviewed changes

rhaschke requested changes Apr 18, 2022

View reviewed changes

tahsinkose force-pushed the constraint-samplers-with-seed branch from 3c2ab31 to ea5fc24 Compare April 18, 2022 18:35

tahsinkose commented Apr 18, 2022

View reviewed changes

tahsinkose requested a review from rhaschke April 18, 2022 18:39

tahsinkose force-pushed the constraint-samplers-with-seed branch from ea5fc24 to 3f0eb04 Compare April 18, 2022 18:41

tahsinkose requested a review from v4hn April 22, 2022 06:41

rhaschke closed this Apr 29, 2022

rhaschke reopened this Apr 29, 2022

tahsinkose added 2 commits May 5, 2022 22:32

Allow passing seed to default constraint samplers from parameter server.

4603cdd

Apply formatting fix in default_constraint_samplers.h

4066e19

tahsinkose and others added 5 commits May 5, 2022 22:32

Add unit test for seeded constraint samplers.

d54ad4b

Address review.

db71524

Restore old API

22dd510

Make test_constraint_samplers a rostest.

817d163

clang-tidy: Fix variable naming

52d8387

tahsinkose force-pushed the constraint-samplers-with-seed branch from 42b6a2e to 52d8387 Compare May 5, 2022 19:32

rhaschke approved these changes May 5, 2022

View reviewed changes

v4hn merged commit dd0fb2a into moveit:master May 9, 2022

tahsinkose mentioned this pull request May 10, 2022

Remove flakiness from constraint samplers test. #3135

Merged

2 tasks

Abishalini mentioned this pull request Jun 28, 2022

Constraint samplers with seed moveit/moveit2#1401

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Constraint samplers with seed #3112

Constraint samplers with seed #3112

tahsinkose commented Apr 7, 2022

codecov bot commented Apr 8, 2022 •

edited

Loading

rhaschke left a comment

rhaschke Apr 8, 2022

tahsinkose Apr 8, 2022

rhaschke Apr 8, 2022

rhaschke Apr 8, 2022

tahsinkose Apr 8, 2022

tahsinkose Apr 13, 2022 •

edited

Loading

tahsinkose commented Apr 8, 2022

tahsinkose commented Apr 8, 2022

tahsinkose commented Apr 12, 2022

tahsinkose commented Apr 13, 2022

v4hn left a comment

rhaschke left a comment

tahsinkose commented Apr 18, 2022 •

edited

Loading

tahsinkose Apr 18, 2022 •

edited

Loading

rhaschke May 5, 2022

v4hn May 9, 2022

tahsinkose commented Apr 18, 2022

rhaschke commented Apr 19, 2022

tahsinkose commented Apr 22, 2022

v4hn commented Apr 28, 2022

rhaschke commented Apr 29, 2022

rhaschke commented May 5, 2022

v4hn commented May 5, 2022

rhaschke commented May 5, 2022

tahsinkose commented May 5, 2022

rhaschke left a comment

v4hn commented May 9, 2022

tahsinkose commented May 9, 2022

v4hn commented May 9, 2022

tahsinkose commented May 9, 2022

rhaschke commented May 10, 2022 •

edited

Loading

tahsinkose commented May 10, 2022

	ROS_WARN_STREAM_NAMED("constraint_samplers",
	ROS_INFO_STREAM_NAMED("constraint_samplers",

	"Creating random number generator with seed " << std::to_string(rng_seed));
	"Creating random number generator with seed " << rng_seed);

		add_rostest_gtest(test_constraint_samplers
		test/constraint_samplers.test

Constraint samplers with seed #3112

Constraint samplers with seed #3112

Conversation

tahsinkose commented Apr 7, 2022

Description

Checklist

codecov bot commented Apr 8, 2022 • edited Loading

Codecov Report

rhaschke left a comment

Choose a reason for hiding this comment

rhaschke Apr 8, 2022

Choose a reason for hiding this comment

tahsinkose Apr 8, 2022

Choose a reason for hiding this comment

rhaschke Apr 8, 2022

Choose a reason for hiding this comment

rhaschke Apr 8, 2022

Choose a reason for hiding this comment

tahsinkose Apr 8, 2022

Choose a reason for hiding this comment

tahsinkose Apr 13, 2022 • edited Loading

Choose a reason for hiding this comment

tahsinkose commented Apr 8, 2022

tahsinkose commented Apr 8, 2022

tahsinkose commented Apr 12, 2022

tahsinkose commented Apr 13, 2022

v4hn left a comment

Choose a reason for hiding this comment

rhaschke left a comment

Choose a reason for hiding this comment

tahsinkose commented Apr 18, 2022 • edited Loading

tahsinkose Apr 18, 2022 • edited Loading

Choose a reason for hiding this comment

rhaschke May 5, 2022

Choose a reason for hiding this comment

v4hn May 9, 2022

Choose a reason for hiding this comment

tahsinkose commented Apr 18, 2022

rhaschke commented Apr 19, 2022

tahsinkose commented Apr 22, 2022

v4hn commented Apr 28, 2022

rhaschke commented Apr 29, 2022

rhaschke commented May 5, 2022

v4hn commented May 5, 2022

rhaschke commented May 5, 2022

tahsinkose commented May 5, 2022

rhaschke left a comment

Choose a reason for hiding this comment

v4hn commented May 9, 2022

tahsinkose commented May 9, 2022

v4hn commented May 9, 2022

tahsinkose commented May 9, 2022

rhaschke commented May 10, 2022 • edited Loading

tahsinkose commented May 10, 2022

codecov bot commented Apr 8, 2022 •

edited

Loading

tahsinkose Apr 13, 2022 •

edited

Loading

tahsinkose commented Apr 18, 2022 •

edited

Loading

tahsinkose Apr 18, 2022 •

edited

Loading

rhaschke commented May 10, 2022 •

edited

Loading