Setting certain extra-arguments to components has no effect

[fabmene]

Bug report

Required Info:

• Operating System:
  • Ubuntu 18.04
• Installation type:
  • Binaries
• Version or commit hash:
  • Eloquent
• DDS implementation:
  • Fast-RTPS
• Client library (if applicable):
  • rclcpp

Steps to reproduce issue

Minimal component example:

#include "rclcpp/rclcpp.hpp"
class MyNode : public rclcpp::Node
{
public:
  explicit MyNode(const rclcpp::NodeOptions& options) : rclcpp::Node("my_node", options){
    setvbuf(stdout, NULL, _IONBF, BUFSIZ);
    if (options.use_intra_process_comms())
      RCLCPP_INFO(this->get_logger(), "Using IPC");
    if (options.allow_undeclared_parameters())
      RCLCPP_INFO(this->get_logger(), "Allowing undeclared params");
    if (options.use_global_arguments())
      RCLCPP_INFO(this->get_logger(), "Using global arguments");
};

#include "rclcpp_components/register_node_macro.hpp"
RCLCPP_COMPONENTS_REGISTER_NODE(my_pkg::MyNode)

Start a component container and load above component with extra arguments, either as launch, or via cli:

container = ComposableNodeContainer(
            node_name='my_container',
            node_namespace='',
            package='rclcpp_components',
            node_executable='component_container',
            composable_node_descriptions=[
                ComposableNode(
                    package='my_pkg',
                    node_plugin='my_node::MNode',
                    node_name='my_node1',
                    extra_arguments=[{'use_intra_process_comms': True}, \
                      {'allow_undeclared_parameters': True}, \
                      {'use_global_arguments': True}])
            ], 
            output='screen',

Expected behavior

[component_container-1] [INFO] [my_node]: Using IPC
[component_container-1] [INFO] [my_node]: Allowing undeclared params
[component_container-1] [INFO] [my_node]: Using global arguments

Actual behavior

[component_container-1] [INFO] [my_node]: Using IPC

Additional Information

Does not seem to depend on order or number of arguments.

[ivanpauno]

If I'm not mistaken, it's supposed only to work with use_intra_process_comms.
Maybe @hidmic can clarify

[hidmic]

Correct. But I don't see any fundamental reason why these other options should not be available. @wjwwood @mjcarroll thoughts? If everyone agrees, @fabmene a PR adding support for those options would be much appreciated.

[ivanpauno]

I don't think that allowing configuration through the command line of allow_undeclared_parameters is a good idea.
Same about use_global_arguments.

[hidmic]

What command line @ivanpauno? Extra arguments only appear in component loading service calls. Having said, I do have a hard time thinking of a use case for dynamically changing allow_undeclared_parameters on load.

[ivanpauno]

What command line @ivanpauno? Extra arguments only appear in component loading service calls. Having said, I do have a hard time thinking of a use case for dynamically changing allow_undeclared_parameters on load.

Ups, sorry. But the same comment applies to launch files.
If the original program wasn't planned to be used with allow_undeclared_parameters=true or use_global_arguments=true, passing them from a launch file can just generate problems. Specially the first one, the second one might have an use-case I'm not seeing.

[fabmene]

Thank you for providing inside on this matter. To clarify, I do not have a use case for the aforementioned arguments either, but was merely experimenting with components and their limitations.

[ivanpauno]

@hidmic Does any tutorial mentions extra_arguments and how it can be used. I think that mentioning it in a tutorial would clarify the situation.

[wjwwood]

Thinking about this more, it's like we have three categories of settings for nodes:

• settings that do not significantly affect behavior of the node APIs
  • by "do not significantly affect behavior" I mean can be changed without violating any important assumptions the node developer is making, e.g. doesn't change blocking behavior or hard coded names of resources, etc...
  • for me this includes use_intra_process
• settings that affect behavior of the node APIs and cannot be a parameter
  • for me this includes use_global_arguments and allow_undeclared_parameters
• settings that affect behavior of the node APIs and can be a parameter
  • for me this includes things like "publisher_X_should_use_reliable" or something like that

For the second case, options that affect the behavior of the node and therefore the original developer should be aware of them, I'd normally say "use a parameter if you want to make it configurable", but in these cases like use_global_arguments and allow_undeclared_parameters happen before parameters are available or affect parameters.

Maybe the components interface in rclcpp should only accept a subset of rclcpp::NodeOptions (see:

rclcpp/rclcpp_components/include/rclcpp_components/node_factory.hpp

Lines 40 to 42 in f30329f

	virtual
	NodeInstanceWrapper
	create_node_instance(const rclcpp::NodeOptions & options) = 0;

), limiting it to the first class of node options in my list above.

This limited set of node options would dictate what can be part of the "extra options" for each client library:

https://github.com/ros2/rcl_interfaces/blob/93cedce2dacd25fa4f2969d022ccbe3f2903e3fe/composition_interfaces/srv/LoadNode.srv#L19

[ivanpauno]

Maybe the components interface in rclcpp should only accept a subset of rclcpp::NodeOptions (see: ...), limiting it to the first class of node options in my list above.

Yes, I agree with that. That method could still take rclcpp::NodeOptions as a parameter, but arguments to modify its attributes should be exposed only for a subset of them.

Other arguments that I believe enter in the first category:

• enable_rosout.
• start_parameter_services.
• start_parameter_event_publisher.

The three of them can usually be configured by the user without the need to change how the node was implemented (in some corner cases, maybe not, but I think they should still be exposed).

We could add them to be configurable when loading a component, like here:

rclcpp/rclcpp_components/src/component_manager.cpp

Line 170 in f30329f

if (extra_argument.get_name() == "use_intra_process_comms") {

.

We could also add some ros arguments to make them configurable in manually composed executables.

[hidmic]

Maybe the components interface in rclcpp should only accept a subset of rclcpp::NodeOptions

Hmm but then that'd apply to dynamic loading only -- statically constructing a node would still allow full access to all options. I think that if we establish that some options are not to be modified by client code, then maybe those should not be accessible in rclcpp::NodeOptions to begin with. And of course, when writing a component one may still enable client code to change these private options in a variety of non-standard, application-specific ways.

---

I'm inclined to think that putting restrictions doesn't do us any good, even more so if restrictions are partial. All options that @ivanpauno mentioned make sense to be exposed, same for use_intra_process, same for use_global_arguments and even allow_undeclared_parameters if e.g. a user ever wants to extend a composable node through inheritance. IMHO having a standard way to pass these common node attributes will always be better than each component rolling out their own.

[wjwwood]

I wasn't suggesting that we change NodeOptions or change nodes to take something other than NodeOptions in their constructor, instead I was saying that we should have a new options struct that is a subset (most likely) of NodeOptions that is passed to the "node factory":

rclcpp/rclcpp_components/include/rclcpp_components/node_factory.hpp

Lines 40 to 42 in f30329f

	virtual
	NodeInstanceWrapper
	create_node_instance(const rclcpp::NodeOptions & options) = 0;

Something like this:

class NodeOptions;
class ComposableNodeOptions
{
public:
  NodeOptions to_node_options() const;
};

class NodeFactory
{
public:
  virtual
  NodeInstanceWrapper
  create_node_instance(const ComposableNodeOptions & options) = 0;
};

class Node
{
public:
  Node(..., const NodeOptions &, ...);
};

// ...

class MyNode : public Node
{
public:
  explicit MyNode(const NodeOptions & no) : Node("my_node", ..., no, ...) {...};
};

class MyNodeFactory : public NodeFactory
{
public:
  virtual
  NodeInstanceWrapper
  create_node_instance(const ComposableNodeOptions & cno)
  {
    return NodeInstanceWrapper(std::make_shared<MyNode>(cno.to_node_options()));
  }
};

In this way your custom node (MyNode above) can still be fully configured with NodeOptions, but when being loaded dynamically using this package (rclcpp_components) it would be restricted to only the ComposableNodeOptions.

EDIT: I changed it so that NodeOptions would not need to depend on ComposableNodeOptions.

[hidmic]

I wasn't suggesting that we change NodeOptions or change nodes to take something other than NodeOptions in their constructor, instead I was saying that we should have a new options struct that is a subset (most likely) of NodeOptions that is passed to the "node factory":

Oh yeah, I got that. I still stand by what I said above. We're restricting options because we think they are not meant to be changed by a user, but we only do so for the dynamic composition case. Code composing MyNode manually could still affect all those options you'd rather not.

So instead of introducing an asymmetry, I'd be fine with proper documentation. People will always find ways of shooting themselves in the foot if they are willing to. You can make a mess in rclpy too if you start playing with object attributes you shouldn't be playing with.

[wjwwood]

Ok, well I don't feel strongly about it. Documentation is fine with me too.

[hidmic]

Circling back! It seems we were discussing whether to restrict options for composable nodes only OR just allow everything and document potential side-effects. I was and still am inclined towards the latter. Patch is small if you guys @wjwwood @ivanpauno agree.

[wjwwood]

Documentation is ok for me.

[gezp]

Hi, what's the status of this issue right now?

i'm trying to implement dynamically composed bringup for Nav2, and Nav2 has a large param file for multiple nodes (also contain some internal node's parameters), when use_global_arguments is false (https://github.com/ros2/rclcpp/blob/master/rclcpp_components/src/component_manager.cpp#L151), the internal nodes (e.g. local_costmap, global_costmap) couldn't load parameters from container's param file. i couldn't solve this problem by other ways.

@wjwwood , in your opinion, use_global_arguments cannot be a parameter, how about adding a parameter for container, so that users could set use_global_arguments of all component nodes true? or do you have another idea to load parameters for internal nodes of component node? could you give me some suggestions ? thank you!

[SteveMacenski]

I don't think that allowing configuration through the command line of allow_undeclared_parameters is a good idea. Same about use_global_arguments.

Specially the first one [allow_undeclared_parameters], the second one [use_global_arguments] might have an use-case I'm not seeing.

I agree (somewhat) with allow_undeclared_parameters.

The use_global_arguments does have a legitimate use case, based on @gezp's analysis -- unless there's a way around the situation described below?

For composition nodes that have internally another node object, we need to be able to pass in the parameters to that secondary internal node. The example in Nav2 we're trying to work around, where each server has a single node, but the servers that contain costmaps have another independent node for the costmaps. This is because a costmap is a sufficiently complex unique entity in its own right that needs control over its flow of sensor data and parameters separately from the server that uses it. However, it is not appropriate to compose the costmap as a separate node since it needs to be statically tied to the task servers for ultra-fast access by the planning and control algorithms regardless of composition strategies employed. Further, if a node is an organizational unit of interfaces, it would be nonsensical to tie the costmap into the server's implementation since they are really serving different purposes that is important to be able to debug independently.

I'm only concerned by being able to set it for the container or by a particular component (I don't really care which) so that it can be used on the odd situation it is required. We only see this issue in dynamic composition in the component container but not elsewhere due to the global setting of that is default to true (e.g. works fine for manual composition, non-composed bringup with each server in another process), which makes it even more odd that this behaves the opposite of the default behavior in every other situation.

https://github.com/ros2/rclcpp/blob/master/rclcpp/include/rclcpp/node_options.hpp#L391

Below assumes that changing use_global_arguments is the right / cleanest solution:

It seems arbitrary that all node types can use it, but the container of components restricts it by statically false. I can definitely appreciate the point that it is probably not a good idea in general to do this, but the point of ROS 2 is to meet the needs of commercial users, and there are many non-general times where things that might generally be considered a bad idea need to be done for specific situations in commercial applications. I believe this is one of them.

It would be silly if Nav2 or other projects in the ecosystem had to create its own implementation of a component container simply to remove 1 line for the odd use-case which does appear.

I think a decent middle ground is to allow it but then add a RCLCPP_WARN when its set (right around here

rclcpp/rclcpp_components/src/component_manager.cpp

Line 170 in f30329f

if (extra_argument.get_name() == "use_intra_process_comms") {

) indicating that this is probably a bad idea unless you actually know what you're doing.

Would that be a decent compromise? Or is there another way to get our parameters to the internal node? We currently use a single file for all parameters https://github.com/ros-planning/navigation2/blob/main/nav2_bringup/params/nav2_params.yaml that is passed to the nodes to grab their particular values. I'm not particularly excited about setting use_global_arguments=True either but we're in a bit of a pickle that might be self-inflicted. I'm open for suggestions if what I'm saying is illinformed.

[fujitatomoya]

Circling back! It seems we were discussing whether to restrict options for composable nodes only OR just allow everything and document potential side-effects. I was and still am inclined towards the latter.

I am with @hidmic with this.

Code composing MyNode manually could still affect all those options you'd rather not.

if the custom node implementation wasn't planned to be used with some arguments, probably node implementation needs to be responsible for that?

[gezp]

the internal nodes (e.g. local_costmap, global_costmap) couldn't load parameters from container's param file.

sorry, the internal nodes (e.g. local_costmap, global_costmap) create their own rclcpp::NodeOptions (with default use_global_arguments(true)) , so they could load parameters from container node's argument --params-file. only component Node can't load parameters from container node.

[ros-discourse]

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

https://discourse.ros.org/t/ros-2-tsc-meeting-minutes-2021-9-16/22372/1

[SteveMacenski]

Still feels like a niche we should have resolved

[fujitatomoya]

closing in favor of #2685