moveit_commander.MoveGroupInterface plan() to better align with C++ MoveGroup::plan() #790

bmagyar · 2018-03-06T23:04:05Z

Description

The MoveGroupInterface.plan() was out of line in its behaviour from the C++ class: now it returns a tuple of 4 as such: (success flag : boolean, trajectory message : RobotTrajectory, planning time : float, error code : MoveitErrorCodes).

Up to now, users of this function could only tell if a plan() call was successful by processing the trajectory message (some planners return trajectories regardless of a failed plan!).

It would be nice to add some tests too.

Checklist

Required: Code is auto formatted using clang-format
Extended the tutorials / documentation, if necessary reference
Optional: Created tests, which fail without this PR reference
Optional: Decide if this should be cherry-picked to other current ROS branches (Indigo, Jade, Kinetic)

v4hn · 2018-03-08T21:31:29Z


I would propose to merge this into L+ only.
Also I would like to propose to return a new class that can be cast to bool to simplify transition from the previous API.

~~~Concerning this request: Could you please elaborate on how you interpret and use the `trajectory_constraints` field? I am not aware of any official documentation on what exactly it means (especially in contrast to `path_constraints`).~~~

format check failed.

v4hn · 2018-03-09T14:17:14Z

I updated my comment above to account for your changes.

bmagyar · 2018-03-09T18:07:26Z

Didn't we just remove the bool casting from MoveItErrorCodes recently?
Casting to bool is a very C++ way of thinking, tuples are more Python-esque.

v4hn · 2018-03-10T14:21:12Z

Didn't we just remove the bool casting from MoveItErrorCodes recently?

We removed implicit bool casts. In that case we had to tell users to change
bool success = group.move() to bool success = static_cast<bool>(group.move()).

Casting to bool is a very C++ way of thinking, tuples are more Python-esque.

With this PR we would have to tell them to change group.plan() to group.plan()[0] which I find rather unintuitive. I would believe bool(group.plan()) or group.plan().success would be preferred over this.
That being said, I don't write a lot of Python code at the moment, so if you disagree we should wait for more comments from others..

bmagyar · 2018-03-11T09:13:04Z

The current Python API only returns a moveit_msgs.msg.RobotTrajectory object which doesn't have bool casting options either. The way deserialize() runs on a Python ROS message object there is no possibility for that to become a None object which removes boolean conversion also for the edge-case of a bad message.

rhaschke

I generally approve this. I don't share the concerns of @v4hn who asked to introduce a bool cast. This is not neccessary in python, where you simply write
err, trajectory, time = plan()

rhaschke · 2018-03-11T21:13:47Z

moveit_commander/src/moveit_commander/move_group.py

        plan = RobotTrajectory()
-        plan.deserialize(self._g.compute_plan())
-        return plan
+        return (error_code.val == MoveItErrorCodes.SUCCESS,


Please update the doc string of this function to reflect the new return value.

While I like the idea to match the C++ interface, this change essentially changes API of the plan() function in Python.
Do we want to accept this for Melodic as well? Or should this be limited to the master branch? Please also add a note to MIGRATION.md.

I received the very same comment from a year ago where it was proposed to only add it to Lunar, not Kinetic.

I think it's ok to put this on melodic too.

rhaschke · 2018-03-11T21:20:14Z

moveit_commander/src/moveit_commander/move_group.py

        plan = RobotTrajectory()
-        plan.deserialize(self._g.compute_plan())


compute_plan() isn't used anywhere else. Why do you introduce a new function plan() / planPython in the wrapper? Wouldn't it make sense to replace or deprecate the old function?

I'm using plan to reflect the MoveGroupInterface better.

MoveItErrorCode plan(Plan& plan)

I can remove compute_plan

Please do so. Am I correct, that these names are internal only?

So internal, some of them - including compute_plan() - only exist as a string in the wrapping rules.

rhaschke · 2018-03-12T07:54:40Z

@bmagyar Please fix clang-format issues.

bmagyar · 2018-03-12T11:43:25Z

Sorry about the rebase, got a conflict with #788 I had to resolve.

rhaschke · 2018-03-12T12:38:56Z

We have failing tests now. Is that due to the rebase?

bmagyar · 2018-03-12T13:28:40Z

What a pleasant surprise!
It comes from the tests is related to the removed compute_plan() function. I'll update them now.

rhaschke · 2018-03-12T13:34:02Z

It comes from the tests is related to the removed compute_plan() function. I'll update them now.

Hm. If compute_plan() was exposed and intended for use, we should only deprecate it for now (and pointing to the implementation of plan()). That's why I asked for it being internal only.

bmagyar · 2018-03-12T13:39:24Z

It is internal only. This test may look familiar to you :)

Implementing a unit test of a C++ class through it's Python-wrapper is risky, especially if the wrapping is done on non-standard ways like MoveIt does it. Normally Python wrapping of functions should keep the name and not allow for custom ones to be declared ( compute_plan() vs plan()). For instance, you can get all kinds of issues such as this one :)

bmagyar · 2018-03-12T15:21:35Z

I've added checking on the error codes too. Unfortunately it's a bit ugly now since these tests are using the raw C++ to Python interface I had to use deserialize on the returned values to get a MoveItErrorCode object. Let me know what you think :)

rhaschke · 2018-03-12T15:53:27Z

Feel free to modify the test to use the python-only interface. Obviously I was not aware of this interface, when writing the tests...

v4hn · 2018-03-14T18:55:30Z

The current Python API only returns a moveit_msgs.msg.RobotTrajectory object which doesn't have bool casting options either

in python, where you simply write
err, trajectory, time = plan()

Fair enough. It's actually err, trajectory, time, errcode = plan() in the proposed code.
In my opinion that looks like someone really wants matlab's variable-returns support in Python..

I take back my comments though 👍.

rhaschke · 2018-03-18T10:23:41Z

@bmagyar Do you still plan to adapt the unittest to use the recommended API, not requiring manual deserialization?

bmagyar · 2018-03-19T17:19:34Z

Yes! I am planning to do that next weekend.

henningkayser · 2019-03-04T13:24:41Z

@bmagyar Do you think you could finish this PR and target it to the new master branch?

bmagyar · 2019-03-06T08:52:41Z

@rhaschke yes I can bundle that into this one.
@henningkayser Sure no problem.

I hope it's ok if I only get to this on the weekend

bmagyar · 2019-03-10T23:39:30Z

I think this is ready to be merged.

@rhaschke I am not getting test failures locally with these changes. I went back to see what we discussed exactly about those tests and I have to say that I think I've changed my mind since then. I think there is justification for any sort of coverage no matter which side of a binding it is.

There is a TODO issue I'd like to add to follow this PR up. We should merge the two Python test implementations, python_move_group_ns.py and python_move_group.py as they only differ in a single parameter. It could be parametrized using 2 .test files passing in different arguments to the .py file or adding a 3-rd file which contains a parametrized Python class, the two individual test nodes instantiating it with different parameter values. This simple refactoring would remove 100 lines of code and ease maintaining. I'd prefer todo as I won't have time to do it during next week. What do you think?

rhaschke · 2019-03-11T00:25:39Z

I went back to see what we discussed ...

I'm lost. Could you please add links for the mentioned "back" reference(s).

There is a TODO issue.

Could you also provide a (source code) link to this TODO. Or is the paragraph in #790 (comment) describing the todo, i.e. merging python_move_group_ns.py and python_move_group.py? A big 👍 for this idea!

bmagyar · 2019-03-11T07:56:42Z

I'm lost. Could you please add links for the mentioned "back" reference(s).

Sorry for the brevity of that. I was referring to this comments, more specifically to this test. While using deserialize may not be the nicest, I think any means of providing coverage for code is a good means, and it can only get better from there :)

Could you also provide a (source code) link to this TODO. Or is the paragraph in #790 (comment) describing the todo, i.e. merging python_move_group_ns.py and python_move_group.py? A big +1 for this idea!

Yes, I meant merging those two files to be added to a TODO. I'm not a big fan of in-code todo-s in general, we have an issue tracker for that :)

bmagyar · 2019-03-12T21:29:08Z

@rhaschke I think your comments may be resolved now.
@mlautman any comments on this?

rhaschke

I do have some final comments. Generally this looks good to me.

rhaschke · 2019-03-13T09:04:16Z

moveit_commander/src/moveit_commander/move_group.py

@@ -316,10 +317,6 @@ def set_random_target(self):
        """ Set a random joint configuration target """
        self._g.set_random_target()

-    def get_named_targets(self):


I don't see a reason to remove this. I think it's by mistake.

Correct, I had added it back (in a new commit, the MR should be squashed anyways

rhaschke · 2019-03-13T09:10:17Z

moveit_commander/src/moveit_commander/move_group.py

        plan = RobotTrajectory()
-        plan.deserialize(self._g.compute_plan())
-        return plan
+        return (error_code.val == MoveItErrorCodes.SUCCESS,


While I like the idea to match the C++ interface, this change essentially changes API of the plan() function in Python.
Do we want to accept this for Melodic as well? Or should this be limited to the master branch? Please also add a note to MIGRATION.md.

moveit_ros/planning_interface/move_group_interface/src/wrap_python_move_group.cpp

rhaschke · 2019-03-13T09:14:40Z

moveit_ros/planning_interface/move_group_interface/src/wrap_python_move_group.cpp

@@ -642,7 +648,7 @@ static void wrap_move_group_interface()
                              &MoveGroupInterfaceWrapper::setMaxAccelerationScalingFactor);
  MoveGroupInterfaceClass.def("set_planner_id", &MoveGroupInterfaceWrapper::setPlannerId);
  MoveGroupInterfaceClass.def("set_num_planning_attempts", &MoveGroupInterfaceWrapper::setNumPlanningAttempts);
-  MoveGroupInterfaceClass.def("compute_plan", &MoveGroupInterfaceWrapper::getPlanPython);


Please note this API change in MIGRATION.md.

rhaschke · 2019-03-13T09:19:26Z

moveit_ros/planning_interface/test/python_move_group.py

@@ -53,8 +62,11 @@ def test_validation(self):
        self.assertFalse(self.group.execute(plan2))

        # newly planned trajectory should execute again
-        plan3 = self.plan(current)
+        error_code3, plan3, time = self.plan(current)
        self.assertTrue(self.group.execute(plan3))


Execution should follow the plan-result assertions, shouldn't it?

rhaschke · 2019-03-13T09:20:39Z

moveit_ros/planning_interface/test/robot_state_update.py

+            error_code1, plan, time = self.plan(target)
+            error_code = MoveItErrorCodes()
+            error_code.deserialize(error_code1)
+            if self.group.execute(plan) and (error_code.val == MoveItErrorCodes.SUCCESS):


Change order of checking planning result and execution here as well.

henningkayser

Looks good to me! This should get squash merged once the changes requested by @rhaschke are addressed.

Do we want to accept this for Melodic as well? Or should this be limited to the master branch?

I think it's fine keeping it in the master branch only since it changes the API while the added feature is not too critical.

rhaschke · 2019-03-14T08:46:44Z

moveit_ros/planning_interface/move_group_interface/src/wrap_python_move_group.cpp

@@ -425,9 +425,9 @@ class MoveGroupInterfaceWrapper : protected py_bindings_tools::ROScppInitializer
    return asyncExecute(plan) == MoveItErrorCode::SUCCESS;
  }

-  const char* getPlannerIdCStr() const


Oops. This became an infinite recursive function call now. Where is this function (or the previous getPlannerIdCStr) used at all?

I think we tried to fix something that git already did, or perhaps it was on an outdated diff. This conversation seems to be on an outdated diff as well... :/

The current diff shows me adding this function which is absolutely not intended. Big of a jump from a PR intended for Kinetic a year ago :)

The github diff view seems to be cached and I discovered a refresh button on the top... That may be the trick. I've added a new commit to remove the function, compilation seems fine, let's see travis with the tests.

henningkayser · 2019-03-14T14:04:50Z

@rhaschke good to go?

rhaschke

LGTM, but: While the two of us argued for merging this into master only, @bmagyar votes for Melodic.
This needs to be decided and MIGRATION.md adapted correspondingly.

davetcoleman · 2019-03-14T21:13:55Z

+1 to not changing melodic beyond bug fixes.

…time, error_code] Update test of C++ MoveGroupInterface through Python wrappers Adjust unit test

bmagyar · 2019-03-15T07:48:50Z

That makes it 3 vs 1 for not going to Melodic, it's decided then :)

I've rebased the branch to resolve the conflict, dropped the migration notes for Melodic and pre-squashed the commits, all ready for merging as soon as the CI turns green.

rhaschke · 2019-03-15T08:13:46Z

@bmagyar, please re-add the MIGRATION notes, but below section Noetic.

bmagyar · 2019-03-15T15:22:45Z

updated

rhaschke

Sorry, small nitpick in the wording of the migration note.

MIGRATION.md

Co-Authored-By: bmagyar <[email protected]>

bmagyar · 2019-03-16T10:30:38Z

That took a while, thanks for sticking around! 💪

bmagyar force-pushed the moveit-commander-move-group-interface-fixup branch from 4900174 to e70ecd1 Compare March 9, 2018 12:16

bmagyar mentioned this pull request Mar 9, 2018

MoveitCommander support for TrajectoryConstraints #793

Merged

4 tasks

bmagyar changed the title ~~moveit_commander.MoveGroupInterface fixup~~ moveit_commander.MoveGroupInterface plan() to better align with C++ MoveGroup::plan() Mar 9, 2018

rhaschke requested changes Mar 11, 2018

View reviewed changes

bmagyar force-pushed the moveit-commander-move-group-interface-fixup branch from 71e0569 to 534a473 Compare March 12, 2018 11:42

bmagyar changed the base branch from kinetic-devel to master March 10, 2019 17:01

bmagyar force-pushed the moveit-commander-move-group-interface-fixup branch 2 times, most recently from 423e873 to b809736 Compare March 10, 2019 23:25

bmagyar force-pushed the moveit-commander-move-group-interface-fixup branch from b809736 to a771fba Compare March 11, 2019 08:22

bmagyar force-pushed the moveit-commander-move-group-interface-fixup branch 2 times, most recently from c182fb1 to 6763854 Compare March 11, 2019 09:35

rhaschke requested changes Mar 13, 2019

View reviewed changes

henningkayser approved these changes Mar 13, 2019

View reviewed changes

rhaschke reviewed Mar 14, 2019

View reviewed changes

rhaschke approved these changes Mar 14, 2019

View reviewed changes

bmagyar force-pushed the moveit-commander-move-group-interface-fixup branch from 7f7a920 to 2d708f1 Compare March 15, 2019 07:45

MoveGroupCommander.plan() now returns [success, trajectory, planning_…

f7855cf

…time, error_code] Update test of C++ MoveGroupInterface through Python wrappers Adjust unit test

bmagyar force-pushed the moveit-commander-move-group-interface-fixup branch from 2d708f1 to f7855cf Compare March 15, 2019 07:48

Update MIGRATION.md

db70148

rhaschke requested changes Mar 15, 2019

View reviewed changes

MIGRATION.md Outdated Show resolved Hide resolved

Update MIGRATION.md

9339f34

Co-Authored-By: bmagyar <[email protected]>

rhaschke approved these changes Mar 16, 2019

View reviewed changes

rhaschke merged commit d649771 into moveit:master Mar 16, 2019

JafarAbdi mentioned this pull request Aug 22, 2019

Handle the updated plan() function of MoveGroupCommander #1640

Merged

6 tasks

		plan = RobotTrajectory()
		plan.deserialize(self._g.compute_plan())

moveit_commander.MoveGroupInterface plan() to better align with C++ MoveGroup::plan() #790

moveit_commander.MoveGroupInterface plan() to better align with C++ MoveGroup::plan() #790

Conversation

bmagyar commented Mar 6, 2018 • edited Loading

Description

Checklist

v4hn commented Mar 8, 2018 • edited Loading

v4hn commented Mar 9, 2018

bmagyar commented Mar 9, 2018

v4hn commented Mar 10, 2018

bmagyar commented Mar 11, 2018

rhaschke 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmagyar Mar 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmagyar Mar 12, 2018 • edited Loading

Choose a reason for hiding this comment

rhaschke commented Mar 12, 2018

bmagyar commented Mar 12, 2018

rhaschke commented Mar 12, 2018

bmagyar commented Mar 12, 2018

rhaschke commented Mar 12, 2018

bmagyar commented Mar 12, 2018 • edited Loading

bmagyar commented Mar 12, 2018

rhaschke commented Mar 12, 2018

v4hn commented Mar 14, 2018

rhaschke commented Mar 18, 2018

bmagyar commented Mar 19, 2018

henningkayser commented Mar 4, 2019

bmagyar commented Mar 6, 2019

bmagyar commented Mar 10, 2019 • edited Loading

rhaschke commented Mar 11, 2019

bmagyar commented Mar 11, 2019 • edited Loading

bmagyar commented Mar 12, 2019

rhaschke 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

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

henningkayser 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

henningkayser commented Mar 14, 2019

rhaschke left a comment

Choose a reason for hiding this comment

davetcoleman commented Mar 14, 2019

bmagyar commented Mar 15, 2019 • edited Loading

rhaschke commented Mar 15, 2019

bmagyar commented Mar 15, 2019

rhaschke left a comment

Choose a reason for hiding this comment

bmagyar commented Mar 16, 2019

bmagyar commented Mar 6, 2018 •

edited

Loading

v4hn commented Mar 8, 2018 •

edited

Loading

bmagyar Mar 12, 2018 •

edited

Loading

bmagyar Mar 12, 2018 •

edited

Loading

bmagyar commented Mar 12, 2018 •

edited

Loading

bmagyar commented Mar 10, 2019 •

edited

Loading

bmagyar commented Mar 11, 2019 •

edited

Loading

henningkayser left a comment •

edited

Loading

bmagyar commented Mar 15, 2019 •

edited

Loading