ros2 / launch_ros

Tools for launching ROS nodes and for writing tests involving ROS nodes.
Apache License 2.0
60 stars 74 forks source link

ComposableNodeContainer and respawn=True #361

Open simulacrus opened 1 year ago

simulacrus commented 1 year ago

Bug report

There is a non-working combination of a non-empty parameter "composable_node_descriptions" and "respawn=True" in the ComposableNodeContainer class. Required Info:

Steps to reproduce issue

ros2 launch ./<launch_name>


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

def generate_launch_description():
    """Generate launch description with multiple components."""
    container = ComposableNodeContainer(
            name='my_container',
            namespace='',
            package='rclcpp_components',
            executable='component_container',
            composable_node_descriptions=[
                ComposableNode(
                    package='composition',
                    plugin='composition::Talker',
                    name='talker'),
                ComposableNode(
                    package='composition',
                    plugin='composition::Listener',
                    name='listener')
            ],
            output='screen',
            respawn=True
    )

    return launch.LaunchDescription([container])

Expected behavior

After the container is killed, it respawns with its components

Actual behavior

After the container is killed, it respawns without components.

Additional information


Feature request

Feature description

Implementation considerations

mjcarroll commented 1 year ago

This was probably an oversight in the implementation. I think that the correct thing is happening in that respawn applies to the container itself. As the container doesn't track what has been loaded/unloade, this could get quite tricky (especially if things are changing at runtime).

I would suggest that we maybe add a respawn flag to the ComposableNode description to indicate that it should be respawned with the container, but this will introduce more bookkeeping that doesn't currently exist.

simulacrus commented 1 year ago

@mjcarroll Is there a working solution for the container auto-restart task? I tried to do it through RegisterEventHandler and it didn't work.

import launch
from launch_ros.actions import ComposableNodeContainer, LoadComposableNodes
from launch_ros.descriptions import ComposableNode
from launch.event_handlers import (OnExecutionComplete, OnProcessExit,
                                OnProcessIO, OnProcessStart, OnShutdown)
from launch.actions import RegisterEventHandler, LogInfo
def generate_launch_description():
    """Generate launch description with multiple components."""
    comps =[
                ComposableNode(
                    package='composition',
                    plugin='composition::Talker',
                    name='talker'),
                ComposableNode(
                    package='composition',
                    plugin='composition::Listener',
                    name='listener')
            ]
    container = ComposableNodeContainer(
            name='my_container',
            namespace='',
            package='rclcpp_components',
            executable='component_container',
            output='screen',
            respawn=True
    )
    event = RegisterEventHandler(
                OnProcessStart(
                    target_action=container,
                    on_start=[LogInfo(msg='Container start'),LoadComposableNodes(composable_node_descriptions=comps, target_container=container)]
                ))

    return launch.LaunchDescription([container, event])
mjcarroll commented 1 year ago

Is there a working solution for the container auto-restart task?

Not that I'm aware of, I will take a look as well.

simulacrus commented 1 year ago

@mjcarroll I found a working way for this task

import launch
from launch_ros.actions import ComposableNodeContainer, LoadComposableNodes
from launch_ros.descriptions import ComposableNode
from launch.event_handlers import (OnExecutionComplete, OnProcessExit,
                                OnProcessIO, OnProcessStart, OnShutdown)
from launch.actions import RegisterEventHandler, LogInfo, OpaqueFunction

import time

def func(context, *args, **kwargs):
    comps =[
                ComposableNode(
                    package='composition',
                    plugin='composition::Talker',
                    name='talker'),
                ComposableNode(
                    package='composition',
                    plugin='composition::Listener',
                    name='listener')
            ]
    time.sleep(1) # <--- lmao
    return [LoadComposableNodes(composable_node_descriptions=comps, target_container='my_container')]

def generate_launch_description():

    container = ComposableNodeContainer(
        name='my_container',
        namespace='',
        package='rclcpp_components',
        executable='component_container',
        output='screen',
        respawn=True
    )
    event = RegisterEventHandler(
        OnProcessStart(
            target_action=container,
            on_start=[LogInfo(msg='Container start'),OpaqueFunction(function=func)]
        ))

    return launch.LaunchDescription([event, container])
mjcarroll commented 1 year ago

@mjcarroll I found a working way for this task

Great! Would you like to contribute an example to the documentation so that others may easily reference?

simulacrus commented 1 year ago

@mjcarroll The code example above is more of a hack than a reference working example. This solution works, but the LoadComposableNodes class gives a warning "there are now at least 2 nodes with the name ...". It looks like I'm doing something wrong in terms of launch_ros architecture

tonynajjar commented 1 year ago

I came to create exactly the same issue, glad to see I'm not the only one needing this. I'll try out the hack :smile:

tonynajjar commented 1 year ago

Some feedback about the proposed workaround, it worked well for me until I ran it for a simulation. Basically I set use_sim_true globally from a parent launch file using SetParameter("use_sim_time", True) and for some reason that does not propagate into the Actions in on_start from OnProcessStart. Created separate issue https://github.com/ros2/launch_ros/issues/373

tonynajjar commented 1 year ago

this could get quite tricky (especially if things are changing at runtime).

@mjcarroll I can see how it can get tricky if things are changing at runtime, however do you see a straightforward implementation for respawning the container and loading what was first inputted in composable_node_descriptions? I assume that would already unblock 90% of applications. If you have some guidance there I could make a PR

tonynajjar commented 1 year ago

@mjcarroll any updates/insights here? I'm willing to do the contribution here as I need this pretty urgently but I need some guidance. @clalancette Maybe you have an idea?

mjcarroll commented 1 year ago

Yeah @tonynajjar sorry to leave you in the lurch here.

I think that your initial idea of respawning with the description that was used at launch is generally the safest choice.

To prevent any sort of surprise side effects, I think we should make this an opt in flag on the ComposableNodeContainer such that you have respawn=True and respawn_components=True would cause the desired behavior here.

We could consider making that the default further down the road, but it could be surprising right now.

tonynajjar commented 1 year ago

Sure, can you give me some guidance on how to implement the logic behind respawn_components? I'm not very familiar with this package and it seems pretty complex with many abstractions.

As far as I see the respawning logic of the process is in the launch repo and there we don't have any access to the loaded components. Vice-versa, in launch_ros where we do have access to the loaded components, we don't have any respawning logic 🤔.

Aposhian commented 1 year ago

What is a case in which you would want respawn_components=False? Just to slowly roll in this feature? It seems like it will primarily just break people who have hacks for this specific issue.

TheMarksniper commented 6 months ago

Any updates on this? I think respawn_components could be very useful flag to have when we are launching nodes from other developers. In my example i am trying to use depthai oakd camera, that sometimes crashes when ping misses and respawn_components would be i think the correct solution