Batch parameter events on node startup

[Aposhian]

Feature request

Feature description

When a node starts up, most nodes call declare_parameter several times, each time publishing an rcl_interfaces::msg::ParameterEvent if start_parameter_event_publisher is true on NodeOptions (which it is by default). For large systems with many parameters, this can flood the system with many parameter events on startup. The rcl_interfaces::msg::ParameterEvent message supports sending multiple parameter changes per message, but currently rclcpp only sends one event per message in this case, which is less efficient.

It would be desirable to batch all parameter events from a node startup into one ParameterEvent message.

Implementation considerations

This batching could be implemented in declare_parameters, refactoring it to not directly call declare_parameter in a std::transform, but to at least change the publishing logic.

Also, while the interface supports this, I'm not sure if this would break any assumptions by the ros2 cli.

[christophebedard]

+1

I've seen the effect of multiple ParameterEvent messages being published (for example using this), and it really overloads the executor instances on the other end. It's only for a relatively short amount of time on startup, but surely this can be improved.

[fujitatomoya]

this sounds reasonable to me.

some notes,

• NodeParameters::set_parameters_atomically does send batch parameter event just once.
• This optimization can be applied to rclpy as well.

[llapx]

I found set_parameters does not send parameter event in batch,

std::vector<rcl_interfaces::msg::SetParametersResult>
NodeParameters::set_parameters(const std::vector<rclcpp::Parameter> & parameters)
{
  std::vector<rcl_interfaces::msg::SetParametersResult> results;
  results.reserve(parameters.size());

  for (const auto & p : parameters) {
    auto result = set_parameters_atomically({{p}});
    results.push_back(result);
  }

  return results;
}

see https://github.com/ros2/rclcpp/blob/b953bdddf8de213b4a051ecf2d668dad65ff9f89/rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp#L674 for detail, in my option, this may also lead to flood.

[clalancette]

Generally I think this would be a good thing to do. I also don't think it will have any impacts on the ros2 cli interface; you can already declare parameters at "any time", so batching them up shouldn't change much of that.

That said, I'm not sure how to achieve this in a nice way for the user. One thing we could do is to have a start_transaction/stop_transaction type of interface, like:

MyNode::MyNode() : rclcpp::Node("my_node")
{
start_collecting_parameters();
declare_parameter("foo", 5);
declare_parameter("bar", "hello");
stop_collecting_parameters();

This gives us an obvious place to batch them up, but this makes it "opt-in" for users. I'd rather try to come up with a way that we can do it by default with the current interfaces (or with small modifications to the current interfaces), but nothing is coming to mind right now.

@llapx The issue with set_parameters is related to, but orthogonal from this. You are right that set_parameters sends out events one at a time, but a) users already have an option to batch in that situation with set_parameters_atomically, and b) in my experience, you aren't often changing tons of parameters at the same time dynamically, so it hurts a lot less than in the startup case.

[llapx]

@clalancette

Many thanks for your reply.

[llapx]

In my option, declare_parameters maybe reasonable for user, bellow are the declaration:

  std::vector<rclcpp::ParameterValue>
  declare_parameters(
    const std::string & namespace_,
    const std::vector<rclcpp::Parameter> & parameters,
    bool ignore_override);

  std::vector<rclcpp::ParameterValue>
  declare_parameters(
    const std::string & namespace_,
    const std::vector<std::pair<rclcpp::Parameter, rcl_interfaces::msg::ParameterDescriptor>> & parameters,
    bool ignore_override);

the usage like bellow:

// without descriptor
        auto parameters_results = this->declare_parameters(
            {
                rclcpp::Parameter("foo", 2),
                rclcpp::Parameter("bar", "hello"),
            });

// with descriptor

        rcl_interfaces::msg::ParameterDescriptor foo_desc;
        foo_desc.description = "Foo";
        rcl_interfaces::msg::ParameterDescriptor bar_desc;
        bar_desc.description = "Bar";
        auto parameters_results = this->set_parameters(
            {
                {rclcpp::Parameter("foo", 2), foo_desc},
                rclcpp::Parameter("bar", "hello"), bar_desc},
            });

start_transaction/stop_transaction type of interface also a good way for user to migrate their code with little pain, so any tips?

[Barry-Xu-2018]

For existing code, the way by start_collecting_parameters/stop_collecting_parameters is better. It can help user easy to update existing code for batch parameter event.
For new code, it is better to use updated declare_parameters() since interface declare_parameters() for same type already exists.
Is it necessary to implement both? or which one should be selected?
Any thoughts?

[Aposhian]

I think no matter what, this optimization will have to be "opt-in" without doing some serious magic. My first reaction was that users who want/need batching should use declare_parameters and set_parameters. I don't think you would need to batch more than that: why would you set a parameter right after you declared it with a default? I think the transaction idea is interesting but might be overkill. Also it makes migration slightly easier at the cost of obfuscating the code. I don't think migrating to the plural forms of these methods is too big of an ask, and then maybe the singular forms should just be deprecated, to push for a sensible (optimized) default in the future?

[clalancette]

So the issue is what overload of the singular declare_parameter people are using. Looking at https://github.com/ros2/rclcpp/blob/rolling/rclcpp/include/rclcpp/node.hpp , there are 4 of them:

1. declare_parameter that takes in an rclcpp::ParameterValue & default_value which is used as the default value and to set the type.
2. declare_parameter that takes in an rclcpp::ParameterType type, which sets the type (the default value is whatever is default constructed for that type).
3. Templated declare_parameter that takes in a ParameterT & default_value which is used as the default value and to set the type.
4. Templated declare_parameter that takes in no additional arguments, but uses uses the templated type to set the type (the default value is whatever is default constructed for that type).

I can easily see an argument where we deprecate the first 2 of these overloads, and make the plural declare_parameters the default. It's a pretty natural change from one to the other.

But overloads 3 and 4 are very convenient, and are what most people are using. There really can't be an equivalent in the plural form, since the types that different parameters will use will often be different. It is certainly possible to convert overloads 3 and 4 to overloads 1 and 2 (and from there to the plurals), but it is a lot less ergonomic.

So with that said, I see two paths forward here:

1. A purely documentation change, where we update all of the tutorials and examples to use the plural form. This is the least disruptive.
2. The transaction-based thing like I outlined above.

[fujitatomoya]

You are right that set_parameters sends out events one at a time, but a) users already have an option to batch in that situation with set_parameters_atomically

correct. I believe tha set_parameters intentionally sets each parameter so that it can get the result on each parameter. and user can call set_parameters_atomically if user want to batch the request.

since the types that different parameters will use will often be different

agree.

How about having atomic operation like declare_parameters_atomically which takes array of rcl_interfaces::msg::ParameterDescriptor?

[clalancette]

It also occurs to me that we could use a Factory style pattern here, where we have convenience functions like overloads 3 and 4 above which just construct and return an rclcpp::ParameterValue or rclcpp::ParameterType (without doing the actual declaration), and then make the user make a separate call to declare_parameters. It still keeps a lot of the ergonomics, and only forces the user to make one additional call. So I'd call that option 3.

How about having atomic operation like declare_parameters_atomically which takes array of rcl_interfaces::msg::ParameterDescriptor?

Yeah, as part of making this more efficient we'd have to add another method like this (as the current declare_parameters just iterates over the map, calling declare_parameter on each one).

[llapx]

It also occurs to me that we could use a Factory style pattern here, where we have convenience functions like overloads 3 and 4 above

good idea.

[Aposhian]

Can someone explain why you would want a "non-atomic" set_parameters or declare_parameters? It seems like people would gravitate towards using those by default rather than an _atomically version. I would like to see the default be atomic, but at the minimum, I think the parameter yaml loader should definitely call the atomic version (so it doesn't flood the system with messages).

[clalancette]

Can someone explain why you would want a "non-atomic" set_parameters or declare_parameters? It seems like people would gravitate towards using those by default rather than an _atomically version.

I didn't originally write it, but I think the idea is that with an atomic update, either every single parameter in the transaction succeeds, or they all fail. With the non-atomic ones, if your 4th parameter fails to set because it is of the wrong type (for instance), the first 3 will have been set. Whether that is a desirable property, I'm not entirely sure. I guess maybe someone like @wjwwood might be able to fill us in on the historical details.

Personally, I'd be for removing the non-atomic versions; that would also reduce bandwidth in other ways by reducing the number of services we are offering on every node. But that is a fairly large change and would require a bunch of work across rcl, rclcpp, rclpy, the tests, and the documentation.

[llapx]

Hi All,

I have create/refactory declare_paramters/_atomically staff, see #1991 for detail, as decribed in https://github.com/ros2/rclcpp/issues/1970#issuecomment-1204859142, when calling declare_parameters/_atomically, if failed(some parameter are invalid), the declare_parameters/_atomically will recover to original status (status before calling it).

[wjwwood]

We were talking about this in our weekly "triage" meeting and I suggested a "builder" pattern approach, something like this:

node->atomic_parameter_declarer()
  .declare_parameter("an_int", 42)
  .declare_parameter("a_double", 3.14)
  .declare_parameter("a_string", "default value")
  .build();

Under the hood it can use the newly proposed declare_parameters_atomically, which I think is the right thing to add to the interface.

Grouping by atomic change is the right thing in this case, grouping by any other metric (arbitrary batching or coalescing based on time) is probably not desirable, imo.

[wjwwood]

Also, sorry for not following this thread initially, but catching up...

Can someone explain why you would want a "non-atomic" set_parameters or declare_parameters? It seems like people would gravitate towards using those by default rather than an _atomically version. I would like to see the default be atomic, but at the minimum, I think the parameter yaml loader should definitely call the atomic version (so it doesn't flood the system with messages).

You would use atomic versions of the function if you wanted to change a group of parameters or none at all, e.g. you want to change P, I, and D for a controller, but only if all the parameters are set successfully, avoiding an inconsistent state if one of them fails to be set.

So, atomically will only set any of the parameters if all of them are successfully set. So that's another trade-off to consider. If you are setting many parameters and one of them might fail to be set, then none of them will be set. They can fail to be set if someone has set up a callback to validate the parameters and rejects the value being set, e.g. trying to assign a string to a parameter that is supposed to be an integer, or trying to assign a 0 when that's not a valid value.

So with that in mind, maybe the declare_parameters_atomically() will not work for everyone, and the API I suggested above should be something like atomic_parameter_declarer...

[wjwwood]

Even with that draw back of the "atomically" signatures, I think it is best to have one ParameterEvent msg per transaction. That's why it was designed to have multiple events in a single message, not so that you could batch it. If we want to use it that way, then I think we need to have a way to distinguish between batched transactions and transactions that have multiple events in them.

[Aposhian]

Yes, I'm not so much concerned about the "atomic" portion in the sense of whether or not the parameters are applied or not, but that we should be efficient with ParameterEvent messages either way.

With that in mind, it seems like there should be a declare_parameters that allows for declaring multiple parameters in a non-atomic way, but only sends one ParameterEvent. Alternatively, there is declare_parameters_atomically, which gives you batching and atomicity.

@wjwwood the builder pattern is nifty, but what does it concretely add that a simple method that takes a vector of parameters does not?

[clalancette]

@wjwwood the builder pattern is nifty, but what does it concretely add that a simple method that takes a vector of parameters does not?

I think it is more ergonomic, and thus more likely for people to use. That is, the simpler vector of parameters works, but you have to construct each Parameter separately, add them to a vector, and then call declare_parameters with that vector. Something like:

auto param1 = rclcpp::Parameter("an_int", 42);
auto param2 = rclcpp::Parameter("a_double", 3.14);
auto param3 = rclcpp::Parameter("a_string", "default value");

std::vector<rclcpp::Parameter> parameters{param1, param2, param3};
node->declare_parameters(parameters);

Compare that to the Builder pattern:

node->atomic_parameter_declarer()
  .declare_parameter("an_int", 42)
  .declare_parameter("a_double", 3.14)
  .declare_parameter("a_string", "default value")
  .build();

That latter seems nicer, and I'd argue it is an easier migration for downstream users. Ultimately, they end up doing the same thing, it's just a matter of taste in the API. Particularly if we want people to use this API instead of the default of this->declare_parameter, it is important to make it ergonomic.

[Aposhian]

There's nothing to stop you from doing something like this though:

node->declare_parameters({
  rclcpp::Parameter("an_int", 42),
  rclcpp::Parameter("a_double", 3.14),
  rclcpp::Parameter("a_string", "default value")
});

[nlamprian]

Related discussion in ros2/ros2_documentation#3399. See here.

In combination with the set parameters callback, I would argue this behavior breaks the API; parameters are evaluated individually. If one of the parameters in a set_parameters call is rejected, the rest are accepted as normal.

You can extend the parameter_events_async demo node to reproduce the issue (in humble).

[clalancette]

In combination with the set parameters callback, I would argue this behavior breaks the API; parameters are evaluated individually. If one of the parameters in a set_parameters call is rejected, the rest are accepted as normal.

I really don't think that is the case, and if that is, that itself is the bug. Rejecting any parameter should reject the entire batch of them.