Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ComposableNodeContainer and respawn=True #361

Open
simulacrus opened this issue May 29, 2023 · 14 comments
Open

ComposableNodeContainer and respawn=True #361

simulacrus opened this issue May 29, 2023 · 14 comments

Comments

@simulacrus
Copy link

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:

  • Operating System:
    • Ubuntu 20.04
  • Installation type:
    • apt
  • Version or commit hash:
    • foxy
  • DDS implementation:
    rmw_cyclonedds_cpp

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
Copy link
Member

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
Copy link
Author

@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
Copy link
Member

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
Copy link
Author

@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
Copy link
Member

@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
Copy link
Author

@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
Copy link

tonynajjar commented Jun 14, 2023

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 😄

@tonynajjar
Copy link

tonynajjar commented Aug 4, 2023

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 #373

@tonynajjar
Copy link

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
Copy link

tonynajjar commented Aug 25, 2023

@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
Copy link
Member

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
Copy link

tonynajjar commented Sep 7, 2023

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
Copy link

Aposhian commented Nov 9, 2023

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
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants