Append ros namespace to target container if available. #429

mbharatheesha · 2024-12-13T22:49:45Z

This PR introduces a change to fix #428.

With this change, when a composable node is loaded into a target container that is running under a ros_namespace set using <push_ros_namespace>, the target container name gets prefixed with ros_namespace using the LaunchContext. This enables the composable nodes to load into a namespaced target container.

How did I test this:

apt install ros-rolling-image-tools
Create a python launch file

from launch import LaunchDescription
from launch_ros.actions import LoadComposableNodes, Node, PushROSNamespace
from launch_ros.descriptions import ComposableNode

def generate_launch_description():

    set_ns = PushROSNamespace("test_ns")
    container = Node(
        name='image_container',
        package='rclcpp_components',
        executable='component_container',
        output='both',
    )

    load_composable_nodes = LoadComposableNodes(
        target_container='image_container',
        composable_node_descriptions=[
            ComposableNode(
                package='image_tools',
                plugin='image_tools::Cam2Image',
                name='cam2image',
                remappings=[('/image', '/burgerimage')],
                parameters=[{'width': 320, 'height': 240, 'burger_mode': True, 'history': 'keep_last'}],
                extra_arguments=[{'use_intra_process_comms': True}],
            ),
            ComposableNode(
                package='image_tools',
                plugin='image_tools::ShowImage',
                name='showimage',
                remappings=[('/image', '/burgerimage')],
                parameters=[{'history': 'keep_last'}],
                extra_arguments=[{'use_intra_process_comms': True}]
            ),
        ],
    )

    return LaunchDescription([set_ns, container, load_composable_nodes])

Confirm that the composable nodes are loaded into the namespaced target container:

[INFO] [launch_ros.actions.load_composable_nodes]: Loaded node '/test_ns/cam2image' in container '/test_ns/image_container'
[component_container-1] [INFO] [1734128809.737064712] [test_ns.image_container]: Found class: rclcpp_components::NodeFactoryTemplate<image_tools::Cam2Image>
[component_container-1] [INFO] [1734128809.737089661] [test_ns.image_container]: Found class: rclcpp_components::NodeFactoryTemplate<image_tools::ShowImage>
[component_container-1] [INFO] [1734128809.737094571] [test_ns.image_container]: Instantiate class: rclcpp_components::NodeFactoryTemplate<image_tools::ShowImage>
[component_container-1] [INFO] [1734128809.742501542] [test_ns.showimage]: Subscribing to topic 'image'
[INFO] [launch_ros.actions.load_composable_nodes]: Loaded node '/test_ns/showimage' in container '/test_ns/image_container'
[component_container-1] [INFO] [1734128809.768871705] [test_ns.cam2image]: Publishing image #1
[component_container-1] [INFO] [1734128809.769063393] [test_ns.showimage]: Received image #camera_frame

Create an xml launch file

<launch>
    <group>
        <push_ros_namespace namespace="test_ns"/>

        <node_container pkg="rclcpp_components" exec="component_container" name="container" namespace=""/>
            <load_composable_node target="container">
                <composable_node name="cam2image" pkg="image_tools" plugin="image_tools::Cam2Image">
                    <remap from="image" to="burger_image" />
                    <param name="width" value="320" />
                    <param name="height" value="240" />
                    <param name="burger_mode" value="True" />
                    <param name="history" value="keep_last" />
                </composable_node>
                <composable_node name="showimage" pkg="image_tools" plugin="image_tools::ShowImage">
                    <remap from="image" to="burger_image" />
                    <param name="history" value="keep_last" />
                </composable_node>
            </load_composable_node>
    </group>
</launch>

Confirm that the composable nodes are loaded into the namespaced target container:

[INFO] [launch_ros.actions.load_composable_nodes]: Loaded node '/test_ns/cam2image' in container '/test_ns/container'
[component_container-1] [INFO] [1734130914.070237617] [test_ns.container]: Found class: rclcpp_components::NodeFactoryTemplate<image_tools::Cam2Image>
[component_container-1] [INFO] [1734130914.070265706] [test_ns.container]: Found class: rclcpp_components::NodeFactoryTemplate<image_tools::ShowImage>
[component_container-1] [INFO] [1734130914.070270385] [test_ns.container]: Instantiate class: rclcpp_components::NodeFactoryTemplate<image_tools::ShowImage>
[component_container-1] [INFO] [1734130914.073884311] [test_ns.showimage]: Subscribing to topic 'image'
[INFO] [launch_ros.actions.load_composable_nodes]: Loaded node '/test_ns/showimage' in container '/test_ns/container'
[component_container-1] [INFO] [1734130914.101434113] [test_ns.cam2image]: Publishing image #1
[component_container-1] [INFO] [1734130914.102238122] [test_ns.showimage]: Received image #camera_frame

ros2 node list

/launch_ros_780940
/test_ns/cam2image
/test_ns/container
/test_ns/showimage

Without the changes in this PR, the output from the launch would be:

[INFO] [launch]: Default logging verbosity is set to INFO
[INFO] [component_container-1]: process started with pid [779037]

And the composable nodes wouldn't show up with ros2 node list

/launch_ros_779454
/test_ns/container

Signed-off-by: Mukunda Bharatheesha <[email protected]>

mbharatheesha · 2025-01-03T16:17:45Z

@ahcorde @sloretz A friendly ping on this.

launch_ros/launch_ros/actions/load_composable_nodes.py

fujitatomoya

i think this is correct fix, i will start the CI.

CC: @sloretz @ahcorde

fujitatomoya · 2025-01-03T19:54:00Z

Pulls: #429
Gist: https://gist.githubusercontent.com/fujitatomoya/c181a9c225d235c70f1dc8fc33c7e8c5/raw/c90c650b79c5dd5c861551173a24442c69d68d04/ros2.repos
BUILD args: --packages-above-and-dependencies launch_ros
TEST args: --packages-above launch_ros
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/15028

Linux
Linux-aarch64
Linux-rhel
Windows

mbharatheesha · 2025-01-13T09:40:59Z

Thanks @fujitatomoya . All builds succeeded it seems ☺️

fujitatomoya · 2025-01-13T16:46:46Z

@ahcorde @sloretz can we have a review for this?

sloretz · 2025-01-24T00:54:06Z

Thank you for the PR!

I'm not too familiar with the launch internals. Would you be willing to add a test to https://github.com/ros2/launch_ros/tree/rolling/launch_ros/test that shows how this fixes the issue? 🧇

MartinCornelis2 · 2025-01-27T14:00:23Z

@mbharatheesha I'm wondering if containers should automatically get the ros namespace pushed onto them. What if you have a higher level abstract container in which you want to put a bunch of namespaced nodes and topics. This would make a setup like this much harder to achieve, because you would have to namespace everything going into the container separately.

Example:
container use-case 1:
{lidar container} : [lidar1 driver] [lidar1 processing] [lidar2 driver][lidar2 processing]

container use-case 2:
{lidar1 container}: [lidar1 driver] [lidar1 processing]
{lidar2 container}: [lidar2 driver][lidar2 processing]

This change will make it such that you can no longer push ros namespace in use-case 1, but it makes use-case 2 easier to achieve.

So the only question for me is, how often do people use use-case 1 vs use-case 2, if use-case 1 never happens then this is an improvement, otherwise it makes use-case 1 cumbersome.

Yadunund · 2025-02-07T02:50:43Z

We've decided to discuss the approaches further in next week's ROS PMC meeting.

mbharatheesha · 2025-02-07T12:06:21Z

@MartinCornelis2 Fair point. The rationale for the changes in this PR was based on the idea that if the container gets the namespace from the current context then perhaps it is likely that the nodes that get loaded into that container are also started in that same namespace. So, we could consider appending the namespace to the target container.

But I understand your point regarding the example1 usecase. It indeed is a bit of a grey area in deciding the scope of containers. Reading the phrase "generic container process" in the documentation seems to incline towards the usecase1 you mention. And in the context of usecase1, one has to do the book-keeping of target containers manually, which could also be cumbersome, but it is explicit.

The question would then be is it desirable to do the target container book-keeping manually or let it be handled implicitly by appending the namespace to the target container based on the context set by push_ros_namespace?

And then there are also two levels of namespacing that you get to do with containers:

a namespace= option within the <node_container> tag
and a push_ros_namespace which is somewhat like a "blanket" namespacing to everything that starts after that.

It could also be a convention consideration that if the namespace tag is used in the <node_container> tag then, the push_ros_namespace can be ignored for that container and hence the namespace tag defines the "generic" level at which the container is set up.

@Yadunund Thanks! Looking forward to hear what the outcome will be!

mbharatheesha mentioned this pull request Dec 13, 2024

Namespace doesn't get applied to target container when using load_composable_node tag #428

Open

Append ros namespace to target container if available.

f5671d3

Signed-off-by: Mukunda Bharatheesha <[email protected]>

mbharatheesha force-pushed the append-ns-to-target-container branch from ae1920c to f5671d3 Compare December 13, 2024 22:59

fujitatomoya reviewed Jan 3, 2025

View reviewed changes

launch_ros/launch_ros/actions/load_composable_nodes.py Show resolved Hide resolved

fujitatomoya requested review from ahcorde and sloretz January 3, 2025 19:52

fujitatomoya reviewed Jan 3, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Append ros namespace to target container if available. #429

Append ros namespace to target container if available. #429

mbharatheesha commented Dec 13, 2024 •

edited

Loading

mbharatheesha commented Jan 3, 2025

fujitatomoya left a comment

fujitatomoya commented Jan 3, 2025

mbharatheesha commented Jan 13, 2025

fujitatomoya commented Jan 13, 2025

sloretz commented Jan 24, 2025

MartinCornelis2 commented Jan 27, 2025

Yadunund commented Feb 7, 2025

mbharatheesha commented Feb 7, 2025

Append ros namespace to target container if available. #429

Are you sure you want to change the base?

Append ros namespace to target container if available. #429

Conversation

mbharatheesha commented Dec 13, 2024 • edited Loading

Without the changes in this PR, the output from the launch would be:

mbharatheesha commented Jan 3, 2025

fujitatomoya left a comment

Choose a reason for hiding this comment

fujitatomoya commented Jan 3, 2025

mbharatheesha commented Jan 13, 2025

fujitatomoya commented Jan 13, 2025

sloretz commented Jan 24, 2025

MartinCornelis2 commented Jan 27, 2025

Yadunund commented Feb 7, 2025

mbharatheesha commented Feb 7, 2025

mbharatheesha commented Dec 13, 2024 •

edited

Loading