Add composable node launch actions #197
Conversation
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
| from .composable_node import ComposableNode | ||
|
|
||
| __all__ = [ | ||
| 'ComposableNode', |
There was a problem hiding this comment.
@wjwwood mentioned making this launch_ros.ComposableNode instead of having a descriptions module. #173 (comment)
I'm fine with either if @wjwwood is on board.
There was a problem hiding this comment.
Oh, I see. The stubs did not match the examples so I chose to go with the later. But I can back up.
There was a problem hiding this comment.
I don't have a strong opinion about it, I'll leave it up to you guys based on what you think and my previous comment.
| @property | ||
| def parameters(self) -> Optional[Parameters]: | ||
| """Get node parameter YAML files or dicts with substitutions to be performed.""" | ||
| return self.__parameters |
There was a problem hiding this comment.
Since everything is a reference and this can contain dictionaries which are mutable, I had been thinking about the description doing a copy.deepcopy to guard against a buggy action modifying a description that was used multiple places. I'm don't think it needs to be done, but thought I'd mention it in case that makes someone think of something.
There was a problem hiding this comment.
+1 to keep descriptions immutable, though the same could apply to actions in general and we don't try to enforce it elsewhere.
There was a problem hiding this comment.
Well, we do try by using properties, but that's as far as we go.
| for params_set_or_path in evaluated_parameters: | ||
| if isinstance(params_set_or_path, pathlib.Path): | ||
| with open(str(params_set_or_path), 'r') as f: | ||
| params_set = yaml.load(f) # type: Dict[str, EvaluatedParameterValue] |
There was a problem hiding this comment.
I think if loaded from a yaml file there can be nested dictionaries, so the name should be joined with a . from each level. A hack might be to call normalize_parameters/evaluate_parameters on the loaded yaml to flatten it
There was a problem hiding this comment.
Hmm, that's a very good point. I'll flatten it.
|
@sloretz is there an implementation to test this against? |
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
| """ | ||
| for composable_node_description in composable_nodes_description: | ||
| context.add_completion_future( | ||
| context.asyncio_loop.run_in_executor( |
There was a problem hiding this comment.
I think asyncio loops are always single threaded, so this will still load the nodes sequentially. I'm not sure about the order they'll be run, but I imagine it's the same order that run_in_executor is called.
There was a problem hiding this comment.
That's true as long as _load_node() remains synchronous, or in other words, as long as we keep using services synchronously. And of course all this is assuming that once the service response comes along, the composed node has been fully brought up already.
IMHO the neatest solution would be to turn rclpy service methods into coroutines. Alternatively, we could spin up a pool of threads, but I'm less fond of it. What do you think?
There was a problem hiding this comment.
neatest solution would be to turn rclpy service methods into coroutines
Agreed that would be the neatest. I think the future returned from call_async could be await'd in an asyncio loop, but that future expects an rclpy executor to be spinning somewhere. I recommend removing the parallel option for now, with a plan to add it later.
There was a problem hiding this comment.
Fair enough. Want me to comment it out w/ a TODO or drop it altogether?
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
|
Sorry for disrupting this pr, but since we moved several packages to ros2/launch_ros this pr will need to be moved over there. However, I cannot move pr's (unfortunately) like I did the issues. @hidmic I assume you'll be able to make the move yourself? |
|
I'll take care. I'll close this one and re-open @sloretz. I won't forget about your comments! :) |
Connected to #160. This pull request adds
ComposableNodeContainerandLoadComposableNodesactions tolaunch_ros.