Skip to content

LoadComposableNodes: unload on shutdown #441

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

Open
wants to merge 2 commits into
base: rolling
Choose a base branch
from

Conversation

rayferric
Copy link

Added a new optional unload_on_shutdown argument to LoadComposableNodes.
If True, the action will unload the associated nodes from the container on exit.

@rayferric rayferric force-pushed the rolling-unload-composable-nodes-on-shutdown branch from 7fbc418 to 2a0d453 Compare February 22, 2025 15:16
@alsora alsora self-assigned this Mar 6, 2025
@alsora
Copy link

alsora commented Mar 6, 2025

@rayferric could you please provide some details about the use-case for this?
Shouldn't nodes always be unloaded when we shutdown? Why do we need a parameter for this?

@rayferric
Copy link
Author

@rayferric could you please provide some details about the use-case for this?
Shouldn't nodes always be unloaded when we shutdown? Why do we need a parameter for this?

Of course!

The usecase:

This feature is useful when you split the development workflow into two launch files - one for simulation/rviz and another for the navigation stack. In such setup you can keep restarting the stack quickly without having to wait for gui programs to initialize.

Then if you'd want to have a high end multi-camera robot in the simulation, the obvious choice is to compose the simulator with your image processing pipeline. So in order to keep your simulation in a separate launch file, you'd start the container there and the simulator would be a ComposableNode. Then you could load the rest of your image processing stack from the second launch file using LoadComposableNodes.

The problem with vanilla LoadComposableNodes is that it exits immediately and doesn't allow you to unload the nodes in any way.

My PR adds an optional switch to the action that will make it keep the launch process alive and once the user sends a SIGINT, the action will unload all previously inserted nodes.

Why not the default?

I wanted to keep this feature non-invasive, thus I created an optional flag for it.

@alsora
Copy link

alsora commented Mar 7, 2025

Ok, thanks for the explanation.
I have to admit that I haven't looked at the implementation yet and I had misunderstood the purpose.

This feature absolutely needs to be optional (i.e. controlled by a flag), as keeping the launch process alive is a major change.

I'll do a review in the next days, thanks for the context.

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

Successfully merging this pull request may close these issues.

2 participants