Setting certain extra-arguments to components has no effect

fabmene commented 4 years ago

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 commented 4 years ago

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

hidmic commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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: https://github.com/ros2/rclcpp/blob/f30329fbec79cb699f41a37cc27c0c475de3edc1/rclcpp_components/include/rclcpp_components/node_factory.hpp#L40-L42), 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 commented 4 years ago

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:

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: https://github.com/ros2/rclcpp/blob/f30329fbec79cb699f41a37cc27c0c475de3edc1/rclcpp_components/src/component_manager.cpp#L170.

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

hidmic commented 4 years ago

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 commented 4 years ago

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":

https://github.com/ros2/rclcpp/blob/f30329fbec79cb699f41a37cc27c0c475de3edc1/rclcpp_components/include/rclcpp_components/node_factory.hpp#L40-L42

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 commented 4 years ago

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 commented 4 years ago

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

hidmic commented 4 years ago

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 commented 4 years ago

Documentation is ok for me.

gezp commented 3 years ago

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 commented 3 years ago

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 https://github.com/ros2/rclcpp/blob/f30329fbec79cb699f41a37cc27c0c475de3edc1/rclcpp_components/src/component_manager.cpp#L170) 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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

Still feels like a niche we should have resolved

ros2 / rclcpp