Add force set parameter functionality

[alsora]

Signed-off-by: Alberto Soragna [email protected]

This PR contains the force-set-parameter functionality first introduced in https://github.com/ros2/rclcpp/pull/1824. It addresses #1762

We can use this separate PR to discuss whether we want this API or not.

@wjwwood wrote

I actually don't think we should have "set" functions that override the read_only flag. One of the reasons for introducing the read_only flag was so that the storage could be static (imagine a read_only sequence stored as a std::array). We're not taking advantage of this in rclcpp but I know of people using it in their own forks in that way. It was one of the purposes. Undeclaring it would just be removing it from the list of declared parameters or marking it as inaccessible, but changing the value, especially if you ignore the dynamic_typing flag and/or let the size of a sequence change would be a problem.

Can you elaborate on why this is a problem? We all agree that a node should be always allowed to undeclare a parameter it already declared. Nothing prevents the node from declaring this parameter again with a different size.

IMO this function is just a very convenient utility that allows to do in a single atomic operation what users would otherwise do in two separate operations (undeclare + declare different), thus reducing complexity in user code and saving them from having to handle concurrency issues.

[fujitatomoya]

Just leaving my previous comments from here

I am just saying can overwrite the read-only value does not make sense. if it is allowed, what is that supposed to mean read-only? (for me, it sounds like updating contents in the read-only file system... but we can mount/unmount the file system.)

for me, overwriting and inaccessible is totally different meaning.

[wjwwood]

Sorry for the delay in replying.

I actually think @fujitatomoya's analogy with the filesystem is pretty appropriate, though it's more like a read-only file on a read-write filesystem... but still it's illustrative.

I think without getting into having access control (ACL) kind of things, the argument you make here @alsora:

IMO this function is just a very convenient utility that allows to do in a single atomic operation what users would otherwise do in two separate operations (undeclare + declare different), thus reducing complexity in user code and saving them from having to handle concurrency issues.

rather than convincing me the "force set" methods are a good idea, this is making me thing that forced undeclaring is not such a good idea. Because while, yes, the user can undeclare and re-declare, I would recommend against it as it breaks the semantic meaning of "read-only". Originally, I had both "read-only" and "immutable" as concepts, but kept it to just "read-only" for simplicity.

Immutable would be "value, and therefore type, cannot change, therefore safe to read once and only once", and I would say probably should also be impossible to undeclare.

Then "read only" could become clearly a semantic thing for users that don't have direct access to the node (maybe even protected methods), meaning "it can change value or type and can be removed or re-declared, and therefore is unsafe to read once and only once, but you (as the observer) can only read it and not set it".

---

The problem I have with these methods, especially them being public methods rather than protected methods, is that someone can create an instance of a Node that sets a read-only parameter and expects them to stay set and not change, and then call these methods (from outside the class) and subtly break the assumptions made by the class.

If we make these separate methods which are protected (rather than new optional parameters to the existing public methods), then at least the logic is self-contained in the class, the class can know that if it sets a read only parameter that it is safe to assume it will stay there and not change unless done by some other part of the class. Obviously someone can inherit from the class and mess with those parameters then, but that's ok in my opinion because I don't consider deriving a class as part of the public API. If a naive user comes along and sees that they can update the value of parameters in the node with a public API, both they and the node need to assume the value can change at any time and may be changed to something that doesn't make sense.

Put another way, with this pull request and/or https://github.com/ros2/rclcpp/pull/1824, a class can no longer safely "set and forget" a read-only parameter, instead every time it uses that parameter it will need to make sure it is set and that the right type/value is there, therefore significantly changing the original semantic meaning of a read-only parameter, in my opinion.

For an example, imagine that we started setting a read-only parameter for the domain ID that the node is on (I'm not suggesting this, just as an example). The purpose would be so that external observers could get access to this information, but both the node and observers would never expect this parameter's value or type to change, nor would they expect it to be undeclared, and yet with these proposed changes you could do that, which would be surprising and weird for both the observers out there and the node itself. Tools might also (understandably) assume that read-only parameters only ever emit one event when created and one event when destroyed (node is shutdown).

These changes both change the current behavior of read only parameters (which is not a reason, in and of itself, to reject the changes imo) and make them more surprising. Making the behavior more surprising is what bothers me about this.

---

I'm inclined to 👎 this pull request as-is, and encourage https://github.com/ros2/rclcpp/pull/1824 to be changed so that re-declaring is not allowed (really just hiding parameters previously declared as read-only), to close the loop hole around undeclare/re-declare as a more verbose "forced set". But in generally I'm feeling less and less good about even the forced undeclare in spite of my previous comment (https://github.com/ros2/rclcpp/pull/1824#pullrequestreview-818040368).

I'll return to my original take (https://github.com/ros2/rclcpp/issues/1762#issuecomment-981809522) that we need two concepts, read-only and immutable.

But I'm open to persuasion and/or to be overruled. @fujitatomoya @ivanpauno @jacobperron

[ros-discourse]

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/ros-2-tsc-meeting-2022-02-17/24435/1

[jacobperron]

Force setting parameters (or force re-declaring parameters) seem to defeat the purpose of read-only parameters. I think there's a confusion over the semantic meaning of "read-only". IIUC, the current meaning is immutable, i.e. once a parameter has been declared, it cannot be changed, which to me includes not being able to undeclare it. I think @wjwwood might be right that we need two different concepts. The current, immutable parameter type and another type of parameter that can be changed locally, but not remotely.

Or, maybe there's another way to satisfy the use-case motivating the original issue, that doesn't use parameters. I'm not sure if there's a concrete use-case in mind.

[clalancette]

@alsora I've assigned you to this to get your thoughts on what the plan going forward is, and if you are planning on working on it in the near future. Feel free to unassign yourself if you are not going to work on it, thanks.

[alsora]

I think I should close both this PR and https://github.com/ros2/rclcpp/pull/1824. We discussed about them in the client library WG meeting and the consensus is that we should have both a read_only and immutable flag.