Skip to content

Refactor component containers + Add option for CBG Executor#3134

Merged
skyegalaxy merged 20 commits intorollingfrom
skyegalaxy/component-container-refactor-cbg
Apr 20, 2026
Merged

Refactor component containers + Add option for CBG Executor#3134
skyegalaxy merged 20 commits intorollingfrom
skyegalaxy/component-container-refactor-cbg

Conversation

@skyegalaxy
Copy link
Copy Markdown
Member

@skyegalaxy skyegalaxy commented Apr 19, 2026

Description

Refactors component_container.cpp to be the single entrypoint for launching component managers of all types.

  • CLI parsing of executor type and isolated mode
  • Added new constructor to ComponentManagerIsolated which consumes an ExecutorOptions and num_threads. these are stored in the class and used when spawning new nodes in isolated executors
  • Defaults to SingleThreadedExecutor in non-isolated mode
  • Adds support for the new Callback Group Events Executor introduced in feat: Added callback group events executor #3097
  • Adds a debug message detailing what executor the component container was spawned with, if it's in isolated mode, and the number of threads if applicable

Is this user-facing behavior change?

When users use component_container_mt/component_container_event/component_container_isolated, they will see a warning message
When using component_container without any arguments, it will default to non isolated + single threaded, so users will probably not notice this change
Users will be able to use the Callback Group Events Executor in component containers
Users will be able to specify the number of threads used for MT / CBG executors in isolated component containers, making the refactored executable more featureful than component_container_isolated.cpp which just uses the max possible threads for each isolated executor

Did you use Generative AI?

@mini-1235 's changes were generated by github copilot. My subsequent changes were initially generated by Claude Sonnet 4.6, and then manually cleaned up afterwards.

Additional Information

Depends on #3097
Successor PR to #3129
Supersedes #3055
Companion PR which adds frontend tests to launch_ros: ros2/launch_ros#536

Copy link
Copy Markdown
Contributor

@mini-1235 mini-1235 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much @skyegalaxy, this looks good to me!

exec = std::make_shared<rclcpp::executors::SingleThreadedExecutor>();
switch (args.executor_type) {
case ExecutorType::MultiThreaded:
node = std::make_shared<
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we have already created a node to read the thread_num param. In this case, std::make_shared will make another node before moving it into node. There could be a very small window where two component manager nodes exist here. Can we just defer here somehow?

Copy link
Copy Markdown
Member Author

@skyegalaxy skyegalaxy Apr 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude originally tried to get around this by creating a --num-threads cli arg which was shadowed by the ROS param, but I felt like having the two ways of configuring threads left component_container.cpp feeling cluttered and that it may be another source of confusion. I figured the cost of destroying and rebuilding the node was negligible for the sake of cleaner reading code

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mjcarroll - how about 0bd014e ?


if (args.executor_type == ExecutorType::SingleThreaded &&
num_threads > 0 &&
num_threads < std::thread::hardware_concurrency())
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If user sets the number of threads more than hardware_concurrency, the check will fail. Why would someone do that? IDK, hilarious.

Copy link
Copy Markdown
Member Author

@skyegalaxy skyegalaxy Apr 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The intention here was basically "if the user is manually specifying thread_num and they're using the SingleThreadedExecutor, give a warning". Since the thread_num ROS param ranges from 1 to hardware_concurrency and defaults to hardware_concurrency, specifying 0 as a default wouldn't work (even though for any executors with multiple threads, 0 resolves to hardware_concurrency anyway) and since the param is always defaulted to hardware_concurrency, it would give the warning every time you use the SingleThreadedExecutor regardless of if thread_num was specified.

I can modify the check to num_threads != std::thread::hardware_concurrency() and that ought to achieve the same effect though

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Member

@mjcarroll mjcarroll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two small nits here

@knmcguire
Copy link
Copy Markdown

Hope this is the correct way?

Pulls: #3134, ros2/launch_ros#536
Gist: https://gist.githubusercontent.com/knmcguire/324f76b97c2a23fbcce926452befc3f9/raw/39fc1c78ff8c571f56bef07d4c6f241912c63f93/ros2.repos
BUILD args: --continue-on-error --packages-above-and-dependencies rclcpp launch_ros
TEST args: --packages-above rclcpp launch_ros
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/19001

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

@skyegalaxy
Copy link
Copy Markdown
Member Author

skyegalaxy commented Apr 19, 2026

Hope this is the correct way?

Pulls: #3134, ros2/launch_ros#536 Gist: https://gist.githubusercontent.com/knmcguire/324f76b97c2a23fbcce926452befc3f9/raw/39fc1c78ff8c571f56bef07d4c6f241912c63f93/ros2.repos BUILD args: --continue-on-error --packages-above-and-dependencies rclcpp launch_ros TEST args: --packages-above rclcpp launch_ros ROS Distro: rolling Job: ci_launcher ci_launcher ran: https://ci.ros2.org/job/ci_launcher/19001

* Linux [![Build Status](https://camo.githubusercontent.com/f8c647fe9ec2e755107378607f523b07a5abe581e7b4b32e5ba2dd38594c4c15/687474703a2f2f63692e726f73322e6f72672f6275696c645374617475732f69636f6e3f6a6f623d63695f6c696e7578266275696c643d3238373635)](http://ci.ros2.org/job/ci_linux/28765/)

* Linux-aarch64 [![Build Status](https://camo.githubusercontent.com/b277bb25541e829f326a09ae9fa6c9e464b9cc5250fea6ef45303801238d0f7f/687474703a2f2f63692e726f73322e6f72672f6275696c645374617475732f69636f6e3f6a6f623d63695f6c696e75782d61617263683634266275696c643d3231353834)](http://ci.ros2.org/job/ci_linux-aarch64/21584/)

* Linux-rhel [![Build Status](https://camo.githubusercontent.com/23655a2be9fc7cee6c625f96b86f42186795cda7730a5f881d086ca764ec7853/687474703a2f2f63692e726f73322e6f72672f6275696c645374617475732f69636f6e3f6a6f623d63695f6c696e75782d7268656c266275696c643d38363037)](http://ci.ros2.org/job/ci_linux-rhel/8607/)

* Windows [![Build Status](https://camo.githubusercontent.com/c9ba0e3a02930c32a4acf843e3dfc78dd560faf85ef39befe1ef5842e6af4c9b/687474703a2f2f63692e726f73322e6f72672f6275696c645374617475732f69636f6e3f6a6f623d63695f77696e646f7773266275696c643d3237363332)](http://ci.ros2.org/job/ci_windows/27632/)

@knmcguire - that is the correct way, but I'm waiting on @jmachowinski 's CBG Executor PR to merge first because this one is dependent on it

@knmcguire
Copy link
Copy Markdown

ah oke then they will probably fail. I'll stop them to offload ci

mini-1235 and others added 15 commits April 19, 2026 12:21
Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>
Signed-off-by: mini-1235 <mauricepurnawan@gmail.com>
…er_isolated

Signed-off-by: Skyler Medeiros <skye@polymathrobotics.com>
Signed-off-by: Skyler Medeiros <skye@polymathrobotics.com>
Signed-off-by: Skyler Medeiros <skye@polymathrobotics.com>
Signed-off-by: Skyler Medeiros <skye@polymathrobotics.com>
Signed-off-by: Skyler Medeiros <skye@polymathrobotics.com>
…ram for backwards compatibility

Signed-off-by: Skyler Medeiros <skye@polymathrobotics.com>
Signed-off-by: Skyler Medeiros <skye@polymathrobotics.com>
Signed-off-by: Skyler Medeiros <skye@polymathrobotics.com>
Signed-off-by: Skyler Medeiros <skye@polymathrobotics.com>
Signed-off-by: Skyler Medeiros <skye@polymathrobotics.com>
Signed-off-by: Skyler Medeiros <skye@polymathrobotics.com>
Signed-off-by: Skyler Medeiros <skye@polymathrobotics.com>
Signed-off-by: Skyler Medeiros <skye@polymathrobotics.com>
Skyler Medeiros added 5 commits April 19, 2026 12:21
Signed-off-by: Skyler Medeiros <skye@polymathrobotics.com>
Signed-off-by: Skyler Medeiros <skye@polymathrobotics.com>
Signed-off-by: Skyler Medeiros <skye@polymathrobotics.com>
Signed-off-by: Skyler Medeiros <skye@polymathrobotics.com>
Signed-off-by: Skyler Medeiros <skye@polymathrobotics.com>
@skyegalaxy skyegalaxy force-pushed the skyegalaxy/component-container-refactor-cbg branch from 0bd014e to f39a5a2 Compare April 19, 2026 19:21
@skyegalaxy skyegalaxy requested a review from jmachowinski April 19, 2026 19:21
@skyegalaxy
Copy link
Copy Markdown
Member Author

Pulls: #3134, ros2/launch_ros#536
Gist: https://gist.githubusercontent.com/knmcguire/324f76b97c2a23fbcce926452befc3f9/raw/39fc1c78ff8c571f56bef07d4c6f241912c63f93/ros2.repos
BUILD args: --continue-on-error --packages-above-and-dependencies rclcpp launch_ros
TEST args: --packages-above rclcpp launch_ros
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/19005/

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

@skyegalaxy skyegalaxy merged commit 72c5209 into rolling Apr 20, 2026
3 of 4 checks passed
Copy link
Copy Markdown
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@skyegalaxy thanks a lot for this PR! this has been off the top of my head for a long time, but now it is much cleaner 👍

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.

6 participants