Refactor component containers + Add option for CBG Executor#3134
Refactor component containers + Add option for CBG Executor#3134skyegalaxy merged 20 commits intorollingfrom
Conversation
mini-1235
left a comment
There was a problem hiding this comment.
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< |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
|
|
||
| if (args.executor_type == ExecutorType::SingleThreaded && | ||
| num_threads > 0 && | ||
| num_threads < std::thread::hardware_concurrency()) |
There was a problem hiding this comment.
If user sets the number of threads more than hardware_concurrency, the check will fail. Why would someone do that? IDK, hilarious.
There was a problem hiding this comment.
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
|
Hope this is the correct way? Pulls: #3134, ros2/launch_ros#536 |
@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 |
|
ah oke then they will probably fail. I'll stop them to offload ci |
cd1309d to
80629f1
Compare
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>
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>
0bd014e to
f39a5a2
Compare
|
Pulls: #3134, ros2/launch_ros#536 |
fujitatomoya
left a comment
There was a problem hiding this comment.
@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 👍
Description
Refactors
component_container.cppto be the single entrypoint for launching component managers of all types.ComponentManagerIsolatedwhich consumes anExecutorOptionsandnum_threads. these are stored in the class and used when spawning new nodes in isolated executorsIs 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.cppwhich just uses the max possible threads for each isolated executorDid 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