[Foxy]: Can't launch a ComposableNodeContainer with the latest Foxy packages (0.10.3)

[clalancette]

Bug report

Required Info:

• Operating System:
  • Ubuntu 20.04
• Installation type:
  • Foxy debian packages
• Version or commit hash:
  • ros-foxy-launch-ros_0.10.3
• DDS implementation:
  • Fast-RTPS
• Client library (if applicable):
  • rclpy

Steps to reproduce issue

I have a fairly straightforward launch file:

from launch import LaunchDescription
from launch_ros.actions import ComposableNodeContainer
from launch_ros.descriptions import ComposableNode

def generate_launch_description():
    params = {
        'target_frame': 'base_footprint',
        'min_height': 0.10,
        'max_height': 2.10,
        'angle_min': -2.356194490192345,
        'angle_max': 2.356194490192345,
        'angle_increment': 0.005759586531581288,
        'range_min': 0.4,
        'range_max': 200.0,
        'use_inf': False,
    }

    return LaunchDescription([
        ComposableNodeContainer(
            name='velodyne_container',
            namespace='',
            package='rclcpp_components',
            executable='component_container',
            composable_node_descriptions=[
                ComposableNode(package='pointcloud_to_laserscan',
                               plugin='pointcloud_to_laserscan::PointCloudToLaserScanNode',
                               name='pointcloud_to_laserscan',
                               remappings=[('cloud_in', 'velodyne_points')],
                               parameters=[params, {'use_sim_time': True}])
            ],
            output='both',
        ),
    ])

Expected behavior

Container successfully launches

Actual behavior

Throws an exception:

$ ros2 launch bug-launch.py 
[INFO] [launch]: All log files can be found below /home/ubuntu/.ros/log/2020-10-19-20-46-35-369504-ros2-focal-1778566
[INFO] [launch]: Default logging verbosity is set to INFO
[INFO] [component_container-1]: process started with pid [1778568]
Task exception was never retrieved
future: <Task finished name='Task-5' coro=<LaunchService._process_one_event() done, defined at /opt/ros/foxy/lib/python3.8/site-packages/launch/launch_service.py:271> exception=InvalidServiceNameException("Invalid service name: topic name must not contain characters other than alphanumerics, '_', '~', '{', or '}':\n  '<node_namespace_unspecified>/velodyne_container/_container/load_node'\n   ^")>
Traceback (most recent call last):
  File "/opt/ros/foxy/lib/python3.8/site-packages/launch/launch_service.py", line 273, in _process_one_event
    await self.__process_event(next_event)
  File "/opt/ros/foxy/lib/python3.8/site-packages/launch/launch_service.py", line 293, in __process_event
    visit_all_entities_and_collect_futures(entity, self.__context))
  File "/opt/ros/foxy/lib/python3.8/site-packages/launch/utilities/visit_all_entities_and_collect_futures_impl.py", line 38, in visit_all_entities_and_collect_futures
    sub_entities = entity.visit(context)
  File "/opt/ros/foxy/lib/python3.8/site-packages/launch/action.py", line 108, in visit
    return self.execute(context)
  File "/opt/ros/foxy/lib/python3.8/site-packages/launch_ros/actions/load_composable_nodes.py", line 200, in execute
    self.__rclpy_load_node_client = get_ros_node(context).create_client(
  File "/opt/ros/foxy/lib/python3.8/site-packages/rclpy/node.py", line 1260, in create_client
    self._validate_topic_or_service_name(srv_name, is_service=True)
  File "/opt/ros/foxy/lib/python3.8/site-packages/rclpy/node.py", line 1079, in _validate_topic_or_service_name
    validate_topic_name(topic_or_service_name, is_service=is_service)
  File "/opt/ros/foxy/lib/python3.8/site-packages/rclpy/validate_topic_name.py", line 41, in validate_topic_name
    raise InvalidServiceNameException(name, error_msg, invalid_index)
rclpy.exceptions.InvalidServiceNameException: Invalid service name: topic name must not contain characters other than alphanumerics, '_', '~', '{', or '}':
  '<node_namespace_unspecified>/velodyne_container/_container/load_node'
   ^

Additional information

If I run the above launch file with the latest sources (on master), everything works fine. If I run the above launch file with the previous version of launch_ros (0.10.2), everything works fine. There is only one patch between 0.10.2 and 0.10.3, and it comes from this PR: #179 . So it looks like something in that backport is either missing or went wrong.

@ivanpauno @hidmic FYI.

[clalancette]

Also @jacobperron FYI.

[ivanpauno]

#153 needed some rework to be backported, and it seems that something went wrong and wasn't caught in the tests.

I can take a look to it.

[hidmic]

Also, the launch_ros.actions.LoadComposableNodes action should verify the container's node name is fully specified. It relies on that to be true.

[clalancette]

Also, on master, if you don't specify any namespace, you get the following error:

[INFO] [launch]: All log files can be found below /home/ubuntu/.ros/log/2020-10-19-21-34-20-249576-ros2-focal-1779158
[INFO] [launch]: Default logging verbosity is set to INFO
Task exception was never retrieved
future: <Task finished name='Task-2' coro=<LaunchService._process_one_event() done, defined at /home/ubuntu/ros2_ws/install/launch/lib/python3.8/site-packages/launch/launch_service.py:271> exception=InvalidServiceNameException("Invalid service name: topic name must not contain characters other than alphanumerics, '_', '~', '{', or '}':\n  '<node_namespace_unspecified>/velodyne_container/_container/load_node'\n   ^")>
Traceback (most recent call last):
  File "/home/ubuntu/ros2_ws/install/launch/lib/python3.8/site-packages/launch/launch_service.py", line 273, in _process_one_event
    await self.__process_event(next_event)
  File "/home/ubuntu/ros2_ws/install/launch/lib/python3.8/site-packages/launch/launch_service.py", line 293, in __process_event
    visit_all_entities_and_collect_futures(entity, self.__context))
  File "/home/ubuntu/ros2_ws/install/launch/lib/python3.8/site-packages/launch/utilities/visit_all_entities_and_collect_futures_impl.py", line 45, in visit_all_entities_and_collect_futures
    futures_to_return += visit_all_entities_and_collect_futures(sub_entity, context)
  File "/home/ubuntu/ros2_ws/install/launch/lib/python3.8/site-packages/launch/utilities/visit_all_entities_and_collect_futures_impl.py", line 45, in visit_all_entities_and_collect_futures
    futures_to_return += visit_all_entities_and_collect_futures(sub_entity, context)
  File "/home/ubuntu/ros2_ws/install/launch/lib/python3.8/site-packages/launch/utilities/visit_all_entities_and_collect_futures_impl.py", line 45, in visit_all_entities_and_collect_futures
    futures_to_return += visit_all_entities_and_collect_futures(sub_entity, context)
  [Previous line repeated 2 more times]
  File "/home/ubuntu/ros2_ws/install/launch/lib/python3.8/site-packages/launch/utilities/visit_all_entities_and_collect_futures_impl.py", line 38, in visit_all_entities_and_collect_futures
    sub_entities = entity.visit(context)
  File "/home/ubuntu/ros2_ws/install/launch/lib/python3.8/site-packages/launch/action.py", line 108, in visit
    return self.execute(context)
  File "/home/ubuntu/ros2_ws/install/launch_ros/lib/python3.8/site-packages/launch_ros/actions/load_composable_nodes.py", line 201, in execute
    self.__rclpy_load_node_client = get_ros_node(context).create_client(
  File "/home/ubuntu/ros2_ws/install/rclpy/lib/python3.8/site-packages/rclpy/node.py", line 1263, in create_client
    self._validate_topic_or_service_name(srv_name, is_service=True)
  File "/home/ubuntu/ros2_ws/install/rclpy/lib/python3.8/site-packages/rclpy/node.py", line 1082, in _validate_topic_or_service_name
    validate_topic_name(topic_or_service_name, is_service=is_service)
  File "/home/ubuntu/ros2_ws/install/rclpy/lib/python3.8/site-packages/rclpy/validate_topic_name.py", line 41, in validate_topic_name
    raise InvalidServiceNameException(name, error_msg, invalid_index)
rclpy.exceptions.InvalidServiceNameException: Invalid service name: topic name must not contain characters other than alphanumerics, '_', '~', '{', or '}':
  '<node_namespace_unspecified>/velodyne_container/_container/load_node'
   ^

There are 2 problems with that:

1. That error message is hard to follow; if we don't want to allow no namespace, we should throw an exception before passing a made-up string to the rcl layer.
2. It feels to me like ComposableNodeContainer should act as much like Node as possible. That is, if having a namespace is required, it should be required for both. If we want to assume a namespace of /, then we should assume it for both. Either way, consistency would make this easier for users to use.

[ivanpauno]

I'm not sure what's the correct fix.

Before, passing namespace='' to Node meant that no remapping rule was passed (i.e. the namespace hard-coded in the node executable was used), same as passing namespace=None.
That was a mistake, and fixed on master (None still mean no remapping rule).

The property node_name was giving always a valid fqn, even when the namespace wasn't known at launch time (i.e.: it can be giving an incorrect namespace).
#179 fixed that issue, and now node_name property uses the prefix <node_namespace_unspecified> when the namespace isn't known at launch time.

So maybe my comment here was incorrect, and it was more logical to fix the meaning of '' on foxy (i.e. '' remaps the namespace to /).

@jacobperron @hidmic opinions?

[ivanpauno]

Also, on master, if you don't specify any namespace, you get the following error:

Does that mean passing namespace=None?

That error message is hard to follow; if we don't want to allow no namespace, we should throw an exception before passing a made-up string to the rcl layer.

Agreed.

It feels to me like ComposableNodeContainer should act as much like Node as possible. That is, if having a namespace is required, it should be required for both. If we want to assume a namespace of /, then we should assume it for both. Either way, consistency would make this easier for users to use.

In Node, passing namespace=None means that no remapping rule is passed for the namespace.
That means you cannot know the node namespace at launch time, and then you cannot know the services names to load a component in the container.
We use in all containers "/" as the harcoded namespace, so maybe making that assumption isn't bad.
But I think that in ComposableNodeContainer the namespace should be mandatory (and not necessarily in Node).

[clalancette]

Does that mean passing namespace=None?

I mean in the above example, if you comment out the namespace='' line, you get the exception above. Also, if you set namespace=None, you get the same behavior.

[hidmic]

That error message is hard to follow; if we don't want to allow no namespace, we should throw an exception before passing a made-up string to the rcl layer.

Agreed. But IMHO, that's something that launch_ros.actions.LoadComposableNodes has to do, as it is that action that requires the name to be fully qualified to derive the load service fully qualified name.

It feels to me like ComposableNodeContainer should act as much like Node as possible. That is, if having a namespace is required, it should be required for both. If we want to assume a namespace of /, then we should assume it for both. Either way, consistency would make this easier for users to use.

But I think that in ComposableNodeContainer the namespace should be mandatory (and not necessarily in Node).

It's important to bear in mind that it is not required for ComposableNodeContainer to be given a node namespace unless you expect to load components using launch.

[jacobperron]

I've haven't looked in detail, but I suspect that there are other patches related to composable nodes that need to be backported following #179. I had made a bunch of bug fixes related to launching composable nodes for Rolling that I wanted to backport to Foxy, but was blocked by backporting #153 (now backported in #179).

[hidmic]

I think we have three separate problems:

1. launch_ros.actions.LoadComposableNodes is making assumptions without checking. We need to fix that.
2. Whether launch_ros.actions.ComposableNodeContainer should assume/force/require a namespace. Considering we control the binary, we could either assume root or require namespaces for the sake of UX. But otherwise I think we have to be careful with blanket remappings. Processes can hold many nodes, and a blanket remapping can break a carefully crafted intra-process hierarchy.
3. Whether launch_ros.actions.Node should accept empty namespaces or not. Personally, I think we shouldn't be accepting node names or namespaces that after substitution resolution remain invalid. Period. But if folks like that "empty means root" exception that's fine.

I think 1. and 2. are enough to "solve" this issue.

[jacobperron]

See #186 for a proposed fix. We applied the same logic in LifecycleNode in #179 to maintain backwards compatibility, but missed ComposableNodeContainer since there is a lack of unit tests.

[jacobperron]

Whether launch_ros.actions.ComposableNodeContainer should assume/force/require a namespace.

FWIW, the namespace parameter of ComposableNodeContainer is mandatory for Rolling. But, for Foxy it is optional (defaulting to / if unspecified) and it should remain that way, IMO, to avoid breaking user-code.

#186 should fix the regression introduced in #179.

launch_ros.actions.LoadComposableNodes is making assumptions without checking. We need to fix that.

I think with namespace being mandatory for ComposableNodeContainer, then the assumption about the node name being valid is okay. Correct me if I'm wrong.

Whether launch_ros.actions.Node should accept empty namespaces or not. Personally, I think we shouldn't be accepting node names or namespaces that after substitution resolution remain invalid. Period. But if folks like that "empty means root" exception that's fine.

It seems like it might be easiest to make the namespace parameter mandatory (and valid). I'm not sure though.

[clalancette]

FWIW, the namespace parameter of ComposableNodeContainer is mandatory for Rolling.

Huh, interesting. I guess it is mandatory at the API level, but it is not checked that it is given in Rolling. That is, if you don't give one in Rolling, you don't find out until the rcl layer fails:

rclpy.exceptions.InvalidServiceNameException: Invalid service name: topic name must not contain characters other than alphanumerics, '_', '~', '{', or '}':
  '<node_namespace_unspecified>/velodyne_container/_container/load_node'

But, for Foxy it is optional (defaulting to / if unspecified) and it should remain that way, IMO, to avoid breaking user-code.

Agreed 100% on this one.

I think with namespace being mandatory for ComposableNodeContainer, then the assumption about the node name being valid is okay. Correct me if I'm wrong.

I think we should validate whether that is the case at the launch_ros level, rather than just assuming it. Otherwise users will get difficult-to-debug errors later on (as in this case).

It seems like it might be easiest to make the namespace parameter mandatory (and valid). I'm not sure though.

In my mind, we have three things that a user would consider "similar":

• Node
• ComposableNodeContainer
• LifecycleNode

I know that they all do different things, but I think that from a user perspective they should all act similarly. That is, if we decide to make namespace mandatory, we should make it mandatory on all of them. If we decide to assume the root namespace when no namespace is given, we should do that on all of them. Having them act differently makes it harder for a user to use them, in my opinion.

[ivanpauno]

That is, if we decide to make namespace mandatory, we should make it mandatory on all of them

I think that making them mandatory in Node isn't a bad idea.
We could also take / as the default, instead of the one harcoded in the node (i.e. actually passing a remapping rule __ns:=/).

FWIW, the namespace parameter of ComposableNodeContainer is mandatory for Rolling. But, for Foxy it is optional (defaulting to / if unspecified) and it should remain that way, IMO, to avoid breaking user-code.

I have opened #189 to make it actually mandatory.

[jacobperron]

AFAIK, this issue was resolved in #186. Please re-open or comment if I'm mistaken.

[hidmic]

I think with namespace being mandatory for ComposableNodeContainer, then the assumption about the node name being valid is okay. Correct me if I'm wrong.

In most cases, it'll be OK. But the ability to compose other nodes isn't exclusive to the typical containers nodes that rclcpp_components provides. So I think we should always check.

[clalancette]

@jacobperron Looks like we still need a release of launch_ros to get this out to users. Would you like me to do it, or are you still waiting for additional backports?

[hidmic]

I know that they all do different things, but I think that from a user perspective they should all act similarly.

I agree with that statement.

I think that making them mandatory in Node isn't a bad idea.

I disagree. If some executable contains N different nodes, with their own names and namespaces carefully crafted, and launch_ros.actions.Node starts remapping namespaces for the sake of consistency, I won't have an option but to fall back to ExecuteProcess to launch it.

Ultimately, if an action B uses an action A and has a requirement on it, then action B should be enforcing that action A fulfills that requirement instead of over-constraining action A.

[ivanpauno]

I disagree. If some executable contains N different nodes, with their own names and namespaces carefully crafted, and launch_ros.actions.Node starts remapping namespaces for the sake of consistency, I won't have an option but to fall back to ExecuteProcess to launch it.

Sounds reasonable to me.

Ultimately, if an action B uses an action A and has a requirement on it, then action B should be enforcing that action A fulfills that requirement instead of over-constraining action A.

#189 will fix that then.