get_parameters_service_ should return empty if allow_undeclared_ is false

[fujitatomoya]

address #1510

[ivanpauno]

If you now request:

ros2 service call /talker/get_parameters rcl_interfaces/srv/GetParameters "names: [use_sim_time, not_exist]"

and the answer is:

rcl_interfaces.srv.GetParameters_Response(values=[rcl_interfaces.msg.ParameterValue(type=1, bool_value=False, integer_value=0, double_value=0.0, string_value='', byte_array_value=[], bool_array_value=[], integer_array_value=[], double_array_value=[], string_array_value=[])])

How do you know if the given value corresponds to use_sim_time or not_exist?

[fujitatomoya]

How do you know if the given value corresponds to use_sim_time or not_exist?

in this case of #1514 (review), with this PR it will return empty.

root@5c7a784cba7f:~/ros2_ws/colcon_ws# ros2 service call /talker/get_parameters rcl_interfaces/srv/GetParameters "names: [use_sim_time, not_exist]"
requester: making request: rcl_interfaces.srv.GetParameters_Request(names=['use_sim_time', 'not_exist'])

response:
rcl_interfaces.srv.GetParameters_Response(values=[])

this behavior is consistent with get_parameter_types and describe_parameters when allow_undeclare_parameter is false(default).

[Barry-Xu-2018]

I discussed with @iuhilnehc-ynos.

Share our thought.

From user perspective, it is better to reply something even if non-existent parameters is included in request parameters.
If a user request many parameters (only one parameter doesn't exist), return empty may make user confused what does service happen.
So we think current behavior of get_parameter_types and describe_parameters are not good.

If the parameter exist, related information should be returned to the caller.
If the parameter doesn't exist, should return initialized default information (type is 'Not Set') to the caller.

[fujitatomoya]

@Barry-Xu-2018

thanks for sharing opinion. i thought that as well, that would be a direction we would take also.
but at the moment, i was thinking that user can see the parameters using list_parameters then request the operation via services. besides, i expect that is what the allow_undeclare_parameter flag is for.

[ivanpauno]

in this case of #1514 (review), with this PR it will return empty.

ah, so if one name is wrong the result is completely empty (?).
I guess that's ok, particularly if it's consistent with the other service calls, but I'm not sure what the intention was.

[fujitatomoya]

i pushed a new commit which is fujitatomoya@0b31523 for this PR. but it does not appear to be here, i am not sure why.

[ivanpauno]

i pushed a new commit which is fujitatomoya@0b31523 for this PR. but it does not appear to be here, i am not sure why.

I can see it in the PR, sometimes github responds slowly ...

[wjwwood]

I see the issue you guys have identified. But I wonder if, in addition to returning empty, we should also return a string that explains why? My thinking is that returning empty is not the same as throwing an exceptions (which is what the C++ API does), and if try to map that to the data centric response message I think we should have a failure_reason string field which is empty when it succeeds.

Aside from that, the change in behavior which is that when allow_undeclared = false and a non existing parameter is requested, nothing is returned (ideally in the future with a reason why as I suggested), and when allow_undeclared = true and a non existing parameter is requested, they are all returned with the non existing ones type as "not set".

[ivanpauno]

But I wonder if, in addition to returning empty, we should also return a string that explains why?

That sounds fine, but couldn't the same be argued for the "describe parameters" and "get parameters types" services?
IMHO, this is at least making things consistent.

Note to self: review the tests

[hidmic]

Looks reasonable to me, though it needs CI.

[hidmic]

My thinking is that returning empty is not the same as throwing an exceptions (which is what the C++ API does), and if try to map that to the data centric response message I think we should have a failure_reason string field which is empty when it succeeds.

I agree with not conflating return values and errors. But I think we can (and probably should) do better than that. I'd suggest we propagate exceptions implicitly. Implicit fields (like goal IDs in action messages) could carry exception type and reason as strings. The receiver can then throw a generic exception or map the exception type to a concrete type.

It's an ABI breaking change, but so is manually adding custom fields to message definitions.

[ivanpauno]

Running CI, including downstream packages tests just in case:

• Linux
• Linux-aarch64
• macOS
• Windows

[wjwwood]

But I think we can (and probably should) do better than that. I'd suggest we propagate exceptions implicitly. Implicit fields (like goal IDs in action messages) could carry exception type and reason as strings. The receiver can then throw a generic exception or map the exception type to a concrete type.

That sounds nice, but it's a lot more work. As it involves changes in all the rmw implementations (and each is a bit different in how they implement services) as well as necessary changes in rmw/rcl/rclcpp/rclpy to expose the exception path.

[fujitatomoya]

thanks for leaving the comment.

But I wonder if, in addition to returning empty, we should also return a string that explains why?

That sounds fine, but couldn't the same be argued for the "describe parameters" and "get parameters types" services?
IMHO, this is at least making things consistent.

yes, that is the purpose of this PR. probably we can discuss on how to return the failure with explanation string as next step.

[fujitatomoya]

CI is unstable because system_test/test_rclcpp needs to be updated accordingly.

[fujitatomoya]

@ros-pull-request-builder retest this please

[fujitatomoya]

CI:

• Linux
• Linux-aarch64
• macOS
• Windows

[fujitatomoya]

CI:

• Linux
• Linux-aarch64
• macOS
• Windows

[fujitatomoya]

CI green, this must go with ros2/system_tests#466 which waits for at least one review.

[ivanpauno]

@ros-pull-request-builder retest this please

[hidmic]

That sounds nice, but it's a lot more work. As it involves changes in all the rmw implementations (and each is a bit different in how they implement services) as well as necessary changes in rmw/rcl/rclcpp/rclpy to expose the exception path.

It is certainly more work. Though not that much if we do the same we did for actions i.e. tweak rosidl generators to add an extra field and just use it in client libraries. It'd be transparent to both rmw and rcl. It's not the prettiest approach, but it's simple.

[ivanpauno]

Going in, I will merge this and ros2/system_tests#466 and then re-release rclcpp to make the system_tests PR job pass.

[fujitatomoya]

@ivanpauno thanks 👍