refactor: make ros2param use rclpy.parameter_client

[ihasdapie]

Use rclpy.parameter_client from instead of internal implementation (Related PR: ros2/rclpy#945) to avoid duplicate logic. Certain functions (e.g. get_parameter_value) are moved over to rclpy and ros2param. Behavior of ros2param.api is left unchanged.

I'd like some review on the proposed architecture & implementation before I clean things up and write tests etc.

[ihasdapie]

Just a sanity check:

• Linux
• Linux-aarch64
• Windows

[jacobperron]

The refactor makes sense to me 👍

[ihasdapie]

There are still large swathes of code like this) scattered throughout which look like they should be replaced with get_parameter_value but are all slightly different.

[ihasdapie]

build fail due to gh actions not using related branch. See:

• Linux
• Linux-aarch64
• Windows

Edit: build may fail due to unrelated linting nitpick in rclpy; it is now fixed.

[ihasdapie]

CI:

• Linux
• Linux-aarch64
• Windows

[ihasdapie]

See: ros2/rclpy#959 (comment)

Running again with increased timeout and more --retest-until-pass

• Linux

[ihasdapie]

Hopefully removing the busy wait and streamlining the dump verb helps those tests pass...

• Linux
• Linux-aarch64
• Windows

[ihasdapie]

CI is passing for this PR, failures unrelated

[clalancette]

CI is passing for this PR, failures unrelated

Just to be clear, that does not seem to be the case. All of the nightly builds on Linux and Windows are green, and the last CI was not, so it does look like there is breakage here.

[ihasdapie]

• Linux
• Linux-aarch64
• Windows

@clalancette Windows error seemed to be a weird flaky test. Yellow because of downstream linting error on one of my related PRs I'm still working on.

[ihasdapie]

• Linux
• Linux-aarch64
• Windows

[jacobperron]

The behavior for ros2 param list is much better now, thanks!

[jacobperron]

LGTM with green CI

[ihasdapie]

• Linux
• Linux-aarch64
• Windows

[Kaju-Bubanja]

I was using load_parameter_dict and was wondering where it went? I noticed commit fc02ffe removes it, but without a note to where it went. I checked rclpy but couldn't find it. Was there a rational why this was removed?

[ihasdapie]

@Kaju-Bubanja This internal api was deprecated in favor of rclpy's AsyncParameterClient's set_parameter method:

from rclpy.parameter_client import AsyncParameterClient

client = AsyncParameterClient(node, node_name)
future = client.set_parameters(list(param_dict.values()))
rclpy.spin_until_future_complete(node, future)
response = future.result()
# ... and so forth

Reference:

• https://github.com/ros2/rclpy/blob/1ee5dff998d0e34cfbe0897b66952d5b270ada79/rclpy/rclpy/parameter_client.py#L169
• https://github.com/ros2/rclpy/blob/1ee5dff998d0e34cfbe0897b66952d5b270ada79/rclpy/rclpy/parameter_client.py#L289
• https://github.com/ros2/rclpy/blob/1ee5dff998d0e34cfbe0897b66952d5b270ada79/rclpy/rclpy/parameter.py#L265-L314

[Kaju-Bubanja]

@ihasdapie Any reason for that? Did something not work with the function or cause problems? Otherwise I don't see why a perfectly fine function should be removed.

[ihasdapie]

The methods in ros2param duplicated code and had inconsistent implementations which this refactor addressed. Plus, we didn't anticipate external users using the internal ros2param methods. If you think that this method should be put back into ros2param feel free to open a PR and we can discuss there, but in my opinion the current design consolidating the parameter management logic in rclpy and calling out to it in ros2param makes more sense.

[Kaju-Bubanja]

Hmmm, I found the parse_parameter_dict function very convenient and copy pasted it into my file, in case somebody in the future searches for this issue here you go, important is to also allow_undeclared_parameters:

def parse_parameter_dict(*, namespace, parameter_dict):
    parameters = []
    for param_name, param_value in parameter_dict.items():
        full_param_name = namespace + param_name
        # Unroll nested parameters
        if type(param_value) == dict:
            parameters += parse_parameter_dict(
                    namespace=full_param_name + PARAMETER_SEPARATOR_STRING,
                    parameter_dict=param_value)
        else:
            parameter = Parameter(full_param_name, value=param_value)
            parameters.append(parameter)
    return parameters

class Foo(Node):
    def __init__(self):
        self.name = "foo"
        # allow_undeclared_parameters implicitly declares parameters when setting them, basically syntax sugar
        super().__init__(self.name, allow_undeclared_parameters=True)
    def load_parameter_dict(self, d):
        call_set_parameters(node=self, node_name=self.name, parameters=parse_parameter_dict(namespace="", parameter_dict=d))