Add option for the events executor to the isolated component container #2541

Timple · 2024-05-27T09:30:45Z

No description provided.

Timple · 2024-05-27T09:31:49Z

rclcpp_components/src/component_container_isolated.cpp

  std::vector<std::string> args = rclcpp::remove_ros_arguments(argc, argv);
  for (auto & arg : args) {
-    if (arg == std::string("--use_multi_threaded_executor")) {
+    if (arg == std::string("--use_multi_threaded_executor") ||


Since using underscores in command flags is very uncommon. I added the new option with dashes and allowed for the same here.

Perhaps in the future we can deprecate the original flag based on discussion here.

fujitatomoya

a couple of questions,

EventsExecutor is still experimental, do we want to bring experimental executor into ComponentManager? I am not sure about this, any thoughts?
Before adding EventsExecutor to ComponentManagerIsolated, probably we would want to support ComponentManager?

IMO, this feature is really nice to have. but i think EventsExecutor should support simulation time before this support, see #2480

audrow · 2024-06-06T17:31:32Z

Before we move forward with this, I think it'd be good to do @fujitatomoya's comment of adding this to the ComponentManager as well.

alsora · 2024-06-07T02:35:56Z

EventsExecutor is still experimental

we should have a discussion on what would be the steps necessary to bring it out of experimental, but IMO this shouldn't prevent from using it here (especially because it's not the default).

Before adding EventsExecutor to ComponentManagerIsolated, probably we would want to support ComponentManager?

I agree.
Moreover, I wonder if we really need all these separate executables (component_container_mt.cpp, component_container.cpp, component_container_isolated.cpp). IMO we could just have 1 executable with a bunch of arguments: --isolated (true if present) and --executor-type (second argument is the name of the executor)

Timple · 2024-06-07T06:27:11Z

Your last arguments (--isolated & --executor-type) did cross my mind. But I was afraid about acceptance of larger PR's and backportability to Jazzy. But they can actually easily be extended without bracking api. So I'll happy refactor to that syntax if you agree 👍

ros-discourse · 2024-06-24T05:57:44Z

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/the-ros-2-c-executors/38296/5

alsora · 2024-08-27T02:42:12Z

@Timple the PR looks good.
Would you mind merging / rebasing your branch with rolling so that I can start the CI?

Timple · 2024-08-27T05:41:25Z

Rebased

SteveMacenski · 2024-12-13T01:07:59Z

Why is this blocked?

fujitatomoya

previous comments #2541 (review) and #2541 (comment), probably we can do this kind of refactoring in rolling without backporting. so i think those do not need to be addressed with this PR.

rclcpp_components/src/component_container_isolated.cpp

fujitatomoya · 2024-12-13T02:00:43Z

rclcpp_components/src/component_container_isolated.cpp

+    exec = std::make_shared<executor>();
    node = std::make_shared<ComponentManagerIsolated>(exec);
+  } else {
+    std::cerr << "Invalid executor type: " << executor_type << std::endl;


this is actually behavior change compared to before. before, even if user specify the wrong option, that falls back to SingleThreadedExecutor, but now it fails and user does not really know what can be set. it would be probably nice to show what and how executor types are set? or command line helper function can be introduced?

this is actually behavior change compared to before.

Same as before, if nothing is supplied the single-threaded is used.

Only if previously the user supplied a non-existent option with a non-existent executor this is a behavior change:

ros2 run rclcpp_components component_container --executor-type something-wrong

But that would be a great coincidence I guess. As the --executor-type never existed.

fujitatomoya · 2024-12-13T02:06:10Z

rclcpp_components/src/component_container_isolated.cpp

+    using executor = rclcpp::executors::MultiThreadedExecutor;
+    using ComponentManagerIsolated = rclcpp_components::ComponentManagerIsolated<executor>;
+    exec = std::make_shared<executor>();
+    node = std::make_shared<ComponentManagerIsolated>(exec);


this is also behavior change compared to before. before, it creates ComponentManagerIsolated node with SingleThreadedExecutor always. but now creating ComponentManagerIsolated node with MultiThreadedExecutor, i do not think this is needed since component node is expected to do the simple task like load/unload the components. is this intentional change?

This was the old code:

if (use_multi_threaded_executor) { using ComponentManagerIsolated = rclcpp_components::ComponentManagerIsolated<rclcpp::executors::MultiThreadedExecutor>;

So I do not recognise this:

before, it creates ComponentManagerIsolated node with SingleThreadedExecutor always

Can you elaborate?

fujitatomoya · 2024-12-13T02:06:47Z

rclcpp_components/src/component_container_isolated.cpp

+        break;
+      }
+    } else if (args[i] == "--use_multi_threaded_executor") { // backward compatibility
+      executor_type = "multi-threaded";


it would be probably nice to add message for user, that this option is deprecated?

Timple · 2025-08-15T09:33:20Z

So it seems that I got bypassed here: #2885

If more hardcoded nodes is the way to go, this can be closed I guess?

fujitatomoya · 2025-08-16T02:32:52Z

@Timple thanks for circling back to this.

this PR is the another enhancement to add the option to isolated container, but #2885 only adds the another executable with EventsExecutor. where i can see, these two enhancement is really different.

but that is up to you. if you are good to go with #2885 and please go ahead to close this PR. when someone finds and takes over this PR, that is when we can come back to this enhancement?

Signed-off-by: Tim Clephas <[email protected]>

Timple · 2025-08-18T08:13:12Z

I think I never saw your review and was under the impression this was good to go.
I'll implement your requested changes!

Timple requested review from hidmic, ivanpauno and wjwwood as code owners May 27, 2024 09:30

Timple commented May 27, 2024

View reviewed changes

Timple force-pushed the component-container-isolated-events-executor branch from 5deb093 to 2c75845 Compare May 27, 2024 09:32

fujitatomoya reviewed May 28, 2024

View reviewed changes

audrow added the more-information-needed Further information is required label Jun 6, 2024

Timple force-pushed the component-container-isolated-events-executor branch 3 times, most recently from 9bcecb7 to b790207 Compare June 25, 2024 07:55

alsora approved these changes Aug 27, 2024

View reviewed changes

Timple force-pushed the component-container-isolated-events-executor branch from b790207 to b2c64b0 Compare August 27, 2024 05:40

fujitatomoya requested changes Dec 13, 2024

View reviewed changes

This was referenced Dec 13, 2024

add explanation about component container types and options ros2/ros2_documentation#4901

Closed

add explanation about component containers. ros2/ros2_documentation#4902

Merged

Timple added 4 commits August 18, 2025 10:06

Add option for the events executor to the isolated component container

7a3d3e3

Signed-off-by: Tim Clephas <[email protected]>

Parse '--executor-type'

d8bfc56

Signed-off-by: Tim Clephas <[email protected]>

No need for backward compatibility on self introduced option

a92ce2f

Signed-off-by: Tim Clephas <[email protected]>

Add deprecation log

45898cc

Signed-off-by: Tim Clephas <[email protected]>

Timple force-pushed the component-container-isolated-events-executor branch from b2c64b0 to 45898cc Compare August 18, 2025 08:19

Uh oh!

Add option for the events executor to the isolated component container #2541

Are you sure you want to change the base?

Add option for the events executor to the isolated component container #2541

Uh oh!

Conversation

Timple commented May 27, 2024

Uh oh!

Timple May 27, 2024

Choose a reason for hiding this comment

Uh oh!

fujitatomoya left a comment

Choose a reason for hiding this comment

Uh oh!

audrow commented Jun 6, 2024

Uh oh!

alsora commented Jun 7, 2024

Uh oh!

Timple commented Jun 7, 2024

Uh oh!

ros-discourse commented Jun 24, 2024

Uh oh!

alsora commented Aug 27, 2024

Uh oh!

Timple commented Aug 27, 2024

Uh oh!

SteveMacenski commented Dec 13, 2024

Uh oh!

fujitatomoya left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fujitatomoya Dec 13, 2024

Choose a reason for hiding this comment

Uh oh!

Timple Aug 18, 2025

Choose a reason for hiding this comment

Uh oh!

fujitatomoya Dec 13, 2024

Choose a reason for hiding this comment

Uh oh!

Timple Aug 18, 2025

Choose a reason for hiding this comment

Uh oh!

fujitatomoya Dec 13, 2024

Choose a reason for hiding this comment

Uh oh!

Timple commented Aug 15, 2025

Uh oh!

fujitatomoya commented Aug 16, 2025

Uh oh!

Timple commented Aug 18, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants