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

Add arguments to configure number of threads #1693

Conversation

wep21
Copy link
Contributor

@wep21 wep21 commented Jun 13, 2021

This PR enables to configure number of threads in component_container_mt as below.

    container = ComposableNodeContainer(
            name='my_container',
            namespace='',
            package='rclcpp_components',
            executable='component_container_mt',
            composable_node_descriptions=[component],
            output='screen',
            arguments=['-t', '5'],
    )

Signed-off-by: wep21 <border_goldenmarket@yahoo.co.jp>
Copy link
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.

I am not really inclined to have this option. instead, how about providing documents how to support the custom component container? because this kind of extra option and arguments would be required and dependent on use cases?

@fujitatomoya
Copy link
Collaborator

CC: @clalancette @ivanpauno what would you think?

Signed-off-by: wep21 <border_goldenmarket@yahoo.co.jp>
@wep21 wep21 force-pushed the feature/configurable_thread_num_for_container_mt branch from 15640fb to a1c3d44 Compare June 13, 2021 16:09
@wep21
Copy link
Contributor Author

wep21 commented Jun 13, 2021

@fujitatomoya

this kind of extra option and arguments would be required and dependent on use cases?

I think it's better for component_container_mt to have the same option or arguments as MultiThreadedExecutor.

@wep21 wep21 requested a review from fujitatomoya June 13, 2021 16:26
Copy link
Member

@ivanpauno ivanpauno left a comment

Choose a reason for hiding this comment

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

Adding this options sounds reasonable to me.
It would also be great to provide a --help option.

wep21 added 4 commits June 19, 2021 05:05
* Add help option

* Return if invalid argument is passed

Signed-off-by: wep21 <border_goldenmarket@yahoo.co.jp>
Signed-off-by: wep21 <border_goldenmarket@yahoo.co.jp>
Signed-off-by: wep21 <border_goldenmarket@yahoo.co.jp>
Signed-off-by: wep21 <border_goldenmarket@yahoo.co.jp>
@wep21 wep21 requested review from ivanpauno and fujitatomoya June 20, 2021 05:32
Copy link
Member

@ivanpauno ivanpauno left a comment

Choose a reason for hiding this comment

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

@wep21 could you add tests?

Maybe, you can move the function doing argument processing to anoter cpp file and unit test that.

@wjwwood
Copy link
Member

wjwwood commented Jun 22, 2021

Have we considered using ROS parameters to configure the number of threads instead (I'm slightly hesitant to add custom argument parsing)? The container is already a node (so that it can provide ROS Services). It's slightly more tedious on the command line, but from launch it should be somewhat straightforward (may require changes to ComposableNodeContainer, but I didn't check).

@wep21
Copy link
Contributor Author

wep21 commented Jun 23, 2021

@wjwwood For current implementation, it is difficult to pass ros parameter to executor argument because ComponentManager node constructor obtains executor from argument.

@wjwwood
Copy link
Member

wjwwood commented Jun 23, 2021

@wjwwood For current implementation, it is difficult to pass ros parameter to executor argument because ComponentManager node constructor obtains executor from argument.

Interesting, but we could/maybe should change that. There's no strong reason that the executor is needed in the constructor as far as I know. It could be passed in via a method that is called after the constructor.

Also, we could look at the parameter overrides before creating the node to extract the setting, though the API to do so is a bit cumbersome if I remember correctly, but this is also an opportunity to fix that up while we do it.

Anyway, I still think custom argument parsing here is fragile and redundant.

@ivanpauno
Copy link
Member

Using a parameter sounds fine to me 👍

@ivanpauno
Copy link
Member

Replaced by #1708, closing

@ivanpauno ivanpauno closed this Jul 8, 2021
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.

4 participants