ros2 / rclpy

rclpy (ROS Client Library for Python)
Apache License 2.0
268 stars 221 forks source link

[design question] get_parameter exception choice #1222

Closed haianos closed 4 months ago

haianos commented 4 months ago

Hi,

Not a bug report but a rationale request behind the choice of raising a ParameterUnderinitializedException when get_parameter, see here .

I personally found this choice annoying because the parameter is declared, hence it exists, simply not init (hence its value is None). However I can still get the Parameter object and doing other operations instead of the exception, including check if the value is None or getting the descriptor. So I am confused about this choice, which enforces to catch an exception for a legit request.

Note: I claim the request is legit as the return value of declare_parameter is a Parameter and it doesn't throw if originally no initial value was passed, hence I don't know why this Parameter is legit and one obtained with get_parameter would not be :smile:

Note 2 using get_parameter_or without providing an alternative could do the job, however it seems to return a new Parameter detached from the description passed when declared_parameter first..

fujitatomoya commented 4 months ago

because the parameter is declared, hence it exists, simply not init (hence its value is None).

this should have not been able to be declared by design. addressed by https://github.com/ros2/rclpy/pull/1216.

haianos commented 4 months ago

Thanks for the answer @fujitatomoya . However, enforcing a init value at declaration sounds a bad idea to me, or at least not as a minor change as it seems. It sort of defeat the purpose of declaring -- why the need of declaring then? why not set directly instead?

More food for thoughts: If you consider that a node is a software app which requires its own configuration, and the configuration is owned by the software app (this is good), can you give suggestions how to implement a required configuration, that is a configuration that must be indicated explicitly prior to start the software.

When initial values are defined, then attaching a configuration to the software component is optional, and not required. By declaring only, without an initial value, enforces external configuration to be required. Enforcing this now seems to break this pattern.

What do you think?

fujitatomoya commented 4 months ago

@haianos

It sort of defeat the purpose of declaring -- why the need of declaring then? why not set directly instead?

with allow_undeclared_parameters flag for the node, we can control the parameter operation for undeclared parameters.

https://docs.ros.org/en/rolling/Concepts/Basic/About-Parameters.html#declaring-parameters

When initial values are defined, then attaching a configuration to the software component is optional, and not required. By declaring only, without an initial value, enforces external configuration to be required. Enforcing this now seems to break this pattern.

for this use case, how about using dynamic_typing? https://docs.ros.org/en/rolling/Concepts/Basic/About-Parameters.html#parameter-types

haianos commented 4 months ago

Thanks a lot for the input @fujitatomoya, much appreciated !

I am afraid the suggestion would be too permissive for my use-case:

In my use-case, when declaring a parameter, I know exactly which type the parameter should be (and restricted to that type only), while other types that are not defined should not be accepted.

In other words, I am trying to validate the configuration I pass from --params-file and, whenever a initial value is expected and not provided, prevent the node to run further.

I don't know if this use-case was considered on the design phase. I can surely work-around using the two options you mentioned, and apply the constraints for validating a configuration somewhere else, i.e., write an external tool that validate this prior launching. However it feels a missed occasion as ROS2 parameters have already everything in place to do that...

Let me know if I can contribute further in this direction, otherwise feel free to close this issue (the question itself has been addressed :smile: )

fujitatomoya commented 4 months ago

@haianos thanks for sharing the use case, i will go ahead to close this one.