search

ros2 / design

Design documentation for ROS 2.0 effort
http://design.ros2.org/
Apache License 2.0
215 stars 194 forks source link

[launch] Refactoring ExecuteProcess into Execute and Executable #272

Open roger-strain opened 4 years ago

roger-strain commented 4 years ago

Not sure if this is the right naming or format; if there are issues, please let me know. Already had a couple of bits of feedback from @ivanpauno that are not yet addressed, but wanted to hopefully give this higher visibility so we might be able to start implementation of the concept if things look reasonable.

Design to address ros2/launch#114

ivanpauno commented 4 years ago

Not sure if this is the right naming or format; if there are issues, please let me know

After a comment in a team meeting, this may be too launch specific, and it would be better to move it to the doc folder there https://github.com/ros2/launch/tree/master/launch/doc/source. launch is a ROS agnostic package, so it may be good to split the documentation between launch and [launch_ros] (https://github.com/ros2/launch_ros).

We can finish the reviewing process in this PR to avoid splitting the discussion, and split the document between launch and launch_ros when everything is ready (including an implementation).

ivanpauno commented 4 years ago

Adding @wjwwood and @hidmic for feedback.

claireyywang commented 4 years ago

@roger-strain friendly ping

roger-strain commented 4 years ago

@roger-strain friendly ping

Don't worry, this was actually next on my list. Things have been a little unsettled lately. Hope to have some updates pushed this week.

roger-strain commented 4 years ago

@ivanpauno @hidmic I've gone through and tried to revisit this with the comments you left in mind. I've tried to split the node definitions out so they are further isolated from the actual execution Action itself. This ended up leaving me with a couple of independent parents, one on the Executable side and one on the Node side; I've tried to join them with an ExecutableNode class, which itself inherits from Executable, but has a Node as a property. It's the best I could come up with having stared at this for too long, so if there's a cleaner solution, I'd be all ears.

I believe the various bits of functionality can work in this proposed architecture, but it's definitely another step or two removed from the current implementation, so I'd definitely appreciate some more experienced eyes to make sure I haven't overlooked something showstopping.

One thing I noticed as I was doing this is that as it stands, I can't put a LifecycleNode in as part of a composable node; however, that also doesn't appear to be possible in the current system, unless I have a fundamental misunderstanding of part of the launch system. I don't discount that possibility, so please do enlighten me if I've missed something big.

hidmic commented 4 years ago

I think we're getting closer @roger-strain. First of all, thank you for pushing.

I spent some time thinking on what you wrote, in comments and in the document itself. We have different concepts here that need to be represented and that today are somewhat mixed up (often just for the sake of simplicity and convenience):

And then there's lifecycle nodes. You're right those are tricky and that lifecycle composable nodes are not currently supported. Personally, I think the difficulties stem from the fact we are thinking in terms of lifecycle nodes instead of nodes that have a lifecycle. IMO, having a lifecycle should be a node trait (at least in launch_ros). Then, a launch_ros.descriptions.Node or any of its subclasses could take a launch_ros.traits.HasLifecycle to complete the picture.

Of course, we can come up with a bunch of handy combinations to both avoid breaking the current API and keeping the typical use cases simple. WDYT?

roger-strain commented 4 years ago

At first blush, I think I see where you're going, and like the general concepts. I personally would shy away from having both a launch.descriptions.Executable and a launch_ros.descriptions.Executable, because I would be a little concerned about confusion because of the extreme similarity between the names, but that might just be me being a little over-conservative about that sort of thing.

One question I have on this is, do we expect there to be any launch_ros.description.Executables other than Nodes? I think one reason I had steered toward the ExecutableNode approach was because I didn't see it ever being anything else.

I definitely like the idea of moving the concept of ComposableNodeContainer to being a subclass of launch_ros.description.Node instead. That could easily take the list of launch_ros.description.ComposableNodes as part of its argument list.

I'll have to ponder this a little more fully before I can start putting together an update. I like to make sure I've really got all the pieces sorted in my head first. But I like where this is going, and really appreciate your help in moving it forward.

hidmic commented 4 years ago

One question I have on this is, do we expect there to be any launch_ros.description.Executables other than Nodes? I think one reason I had steered toward the ExecutableNode approach was because I didn't see it ever being anything else.

The thing is, an executable is not a node. An executable has nodes. This is a fundamental shift from ROS 1, where there was a 1-to-1 mapping between nodes and processes. Simple executables often have only one node, but they could contain dozens. That's why I think that both concepts have to be decoupled. For sure, that doesn't mean that we can't have a launch_ros.actions.Node-like wrapper to keep the common one-node-per-process use case as straightforward as possible.

I would be a little concerned about confusion because of the extreme similarity between the names, but that might just be me being a little over-conservative about that sort of thing.

I do see the potential source for confusion. I'm not great at naming things :sweat_smile:

I'll have to ponder this a little more fully before I can start putting together an update. I like to make sure I've really got all the pieces sorted in my head first.

Thank you for pushing !

hidmic commented 4 years ago

A standard container for composition is nothing but a well known launch_ros.descriptions.Node.

...and actually, this isn't entirely true. Any node could implement the dynamic composition API. It's a stretch, but this could be yet another trait of a node.

roger-strain commented 4 years ago

Apologies, I've been redirected onto a different effort for the past several weeks, but I'm trying to spin back up on this. Now that I'm getting into the details of trying to break this down as we've discussed, there are a couple of sticky points I would love some insight on so I can properly address them now, rather than getting perplexed when trying to implement.

Simple executables often have only one node, but they could contain dozens.

This is a straightforward concept, but there's at least one part of the current implementation that bugs me. In the current launch_ros.actions.Node, a variety of parameters can be passed as command line arguments during execution: node_name (as __node:=), node_namespace (as __ns:=), parameters (as __params:=), and remapping (as {}:={}). In the current iteration of the refactor, those fields are all captured as part of the proposed launch_ros.descriptions.Node object (which is not itself a launch.descriptions.Executable). Additionally, there is an arguments parameter which is additionally passed when executing the node (in the current design).

Once we start delving into the concept of having a single executable which is host to multiple nodes (not using the composable interface you mentioned) I wonder how those should now apply. Conceivably you could specify alternate names, namespaces, parameters, remaps, etc. for the various nodes which are part of an executable. I haven't dug down into the other side of this to know if there are ways around it, so I'm hoping if there's some technique for properly passing those through and getting them to the right internal place, you can point me in the right direction. If something like specifying alternate node names for multiple nodes in a process isn't supported, then how should we approach things? I could possibly see doing something like either keeping an ExecutableNode concept somewhere, with it (and it alone) being able to pass some of those command line arguments; whereas by using the base class with multiple node support, you explicitly give up the ability to specify those parameters. But then, the definitions are contained at the ros_launch.descriptions.Node level, which means it would be possible to define them, even if they aren't actually usable. I've kind of twisted myself into knots on this one, so I really hope you can help untangle.

For the Composable node design, you mention that theoretically any node might implement the composition API. Would it be better to have a subclass of ros_launch.descriptions.Node along the lines of ros_launch.descriptions.ComposingNode, which simply adds the nodes parameter (a list of ros_launch.descriptions.ComposableNodes) and sends the appropriate call after launching the main node, or should that functionality just be built into ros_launch.descriptions.Node itself, and if you choose to include a list of nodes, then you are implicitly asserting that the node supports the composition API?

For the Lifecycle side of things I've got a hopefully simple question; Python isn't my native dialect, so your mention of "traits" is intriguing to me. I've found a couple of things which I believe might be what you're referring to, but can you point me at a concrete example so I know for sure what concepts I should be exploiting here?

hidmic commented 4 years ago

Once we start delving into the concept of having a single executable which is host to multiple nodes (not using the composable interface you mentioned) I wonder how those should now apply. Conceivably you could specify alternate names, namespaces, parameters, remaps, etc. for the various nodes which are part of an executable.

That's a good point, and it is supported. Take a look at the Capabilities section of the ROS2 command line arguments design document. In short, you can target a specific node by providing its name as a prefix when specifying remaps and parameters. This is already given when using parameters file. When changing its name, the name it was given in code has to be provided. Thus, in the worst case scenario, the user has to provide the original node name along with launch_ros.descriptions.Node description in launch.

Would it be better to have a subclass of ros_launch.descriptions.Node [...] or should that functionality just be built into ros_launch.descriptions.Node itself [...]?

IMHO inheritance is fine for composable node descriptions, but not for the container. And I'd rather not inject that functionality into the basic node description.

For the Lifecycle side of things [...] your mention of "traits" is intriguing to me.

It's not a specific Python feature, but a concept. We know that a lifecycle node has a specific behavior and interface. Same for a node that can compose other nodes. So instead of encoding all that in a Python type object, we do so in a separate "trait" object that descriptions can take.

For instance, instead of:

 launch_ros.descriptions.ComposableNodesContainerWithLifecycle(
    node_name='my_node',
    nodes=[
         launch_ros.descriptions.ComposableNode(
              plugin_name='some::Node',
         ),
         # ...
    ],
    # ...
 )

which is to say that it IS a lifecycle, composable nodes' container node, we go for:

launch_ros.descriptions.Node(
    node_name='my_node',
    traits=[
        launch_ros.traits.HasLifecycle(),
        launch_ros.traits.ComposesNodes(
           nodes=[
               launch_ros.descriptions.ComposableNode(
                  plugin_name='some::Node',
                  traits=[
                      # and so on...
                   ]
               ),
               # ...
           ]
        )
    ],
)

which is to say that it IS a node THAT has a lifecycle and composes other nodes.

In a way, it's like a type system based on runtime data.

This is, however, my personal take on how we could tackle this problem in a scalable way. I'm sure @wjwwood and @ivanpauno have ideas too.

roger-strain commented 4 years ago

@hidmic Took a while for me to think this through and get somewhere I'm happy with, but I think this might be getting closer to what you're envisioning. I think there are several places where we'll want to maintain wrappers that simplify some of the structure for typical use, but I think this manages to get things split into reasonable places. There are quite a few changes in the ros_launch sector, so probably best to come at it as a fresh whole. See what you think; I'm looking forward to getting this into shape.

roger-strain commented 4 years ago

@hidmic New version pushed up to address feedback. Still one point I'm not sure about w.r.t. whether package should be required for ComposableNodes. I also was thinking about changing ComposableNode from being a subclass of Node to instead being a trait, but I kind of feel like the overhead of syntax isn't really worth it there. Would like your opinion as well.

hidmic commented 4 years ago

I also was thinking about changing ComposableNode from being a subclass of Node to instead being a trait, but I kind of feel like the overhead of syntax isn't really worth it there.

I agree. I think the ability to be composed is significant enough (in terms of usability and implementation) to warrant a new derived type.

roger-strain commented 3 years ago

@hidmic I've updated the PR based on the latest feedback. I'd been holding off to see if @ivanpauno or @wjwwood had anything else to add so I didn't clutter up the commit history too much.

How close do you think we are to getting to a point of being able to start implementation? I've held off because I'd hate to start doing work that doesn't line up with the overall project goals, but we do have some other things (most notably adding support for multi-machine launch files) that this will impact, so I'd like to give our other guys some certainty that this is the path we're going to follow. I feel like it's pretty close myself, but will be happier with a few more head-nods.

hidmic commented 3 years ago

@hidmic I've updated the PR based on the latest feedback. I'd been holding off to see if @ivanpauno or @wjwwood had anything else to add so I didn't clutter up the commit history too much.

Thanks for your patience and work.

How close do you think we are to getting to a point of being able to start implementation? I've held off because I'd hate to start doing work that doesn't line up with the overall project goals, but we do have some other things (most notably adding support for multi-machine launch files) that this will impact, so I'd like to give our other guys some certainty that this is the path we're going to follow. I feel like it's pretty close myself, but will be happier with a few more head-nods.

I'll proof read this at some point this week, but I think it's pretty good as-is. Let's see what others say.

ros-discourse commented 3 years ago

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

https://discourse.ros.org/t/ros-2-tsc-meeting-minutes-2020-07-16/15468/1

roger-strain commented 3 years ago

@hidmic I think we've probably gotten about as far as we will with this, so I'd like to see what the next steps should be. This is a larger issue than I've tackled in a project like this, so I'm not sure what the normal approach is. I'm thinking that the changes within launch are probably a little smaller, and should have appropriate backward-compatible interfaces, so maybe focus on that part first as an individual PR, then work on the launch_ros portions afterward? Are PRs like this typically living items so there can be frequent reviews, or are they held until the functionality is essentially complete so it can be reviewed as a whole?

Also, if @wjwwood has a chance for any final feedback, it would be greatly appreciated.

hidmic commented 3 years ago

I'm thinking that the changes within launch are probably a little smaller, and should have appropriate backward-compatible interfaces, so maybe focus on that part first as an individual PR, then work on the launch_ros portions afterward?

Building up support for this in launch first sounds reasonable to me. But I'd encourage you to split the work across multiple PRs. New classes and concepts can coexist with existing ones, even if and until the latter become a shell for the first. Small, incremental contributions are also easier to review, which means you'll get better feedback and faster than if you submit a single massive PR.

I'd also recommend you clearly state what the PR series is going to look like for reviewers' context.

Are PRs like this typically living items so there can be frequent reviews, or are they held until the functionality is essentially complete so it can be reviewed as a whole?

In general, you don't want to hold back any significant amount of work until it's "done" (whatever the definition of done is). Architectural changes require early review and discussion, and this design document has already built some consensus. Implementation wise, complying with contribution guidelines and getting early feedback from OSRF and/or the community will overall result in less re-work.

So, if your PR is medium sized or you come across some oversight in this document, submitting a draft is perhaps a good idea. But if you keep PRs small, you may as well just open them.

roger-strain commented 3 years ago

@ivanpauno I've been working up a plan for how the PRs might look for this, and would appreciate a once over. Also have a few questions towards the end about things I think need to be dealt with, but that we haven't discussed yet. (Apologies that this is long, just wanting to make sure I have things covered.)

Conflicting classes:

Planned PR sequence:

Questions:

ivanpauno commented 3 years ago

The overall plan sounds good to me, I have some minimal comments:

All original parameters/properties still exist, though one is renamed (extra_arguments -> arguments)

You could still accept the old argument name, with a deprecation warning.

Add backward compatibility

Maybe, we can create a "feature" branch in launch and launch_ros, and don't merge it on master until a coherent subset of things is merged.

For example:

In that way, we ensure we're only merging to master classes that can already be used. The risk of having conflicts is low, as each time a "refactor" step is done the feature branch is merged into master.

Add tests to launch_ros/launch_testing_ros as possible.

Instead of adding testing in a separate step, each PR should test the class/feature is being added.

Extensiveness of unit testing? Haven't dived in yet, not sure if tests should be launching processes, nodes, etc. to verify behavior.

Tests should launch process/nodes/etc to verify. The one where is most important to test well that launching a something is working is ExecuteLocal. With things like RosExecutable, unit testing that the result of the description is correct without launching it is fine (though ideally, it's good to actually try launching something).

Introduce backward compatibility as features become complete? Guessing not, because there might be some ill-defined behavior if only some classes expect the new structure.

I'm not sure if I understand the question. I think I've just suggested to do the opposite.

Support for XML/YAML launch files? Haven't discussed this at all yet, so not sure how to approach that, and whether it should be baked in during the PRs or added late.

Don't break the existing XML/YAML support :joy:. For the moment, you won't need to extend it.

roger-strain commented 3 years ago

I think we're actually pretty close to the same page here.

In the list I put together, the top level items basically represented a conceptual PR. So the testing would be part of the same PR as the class(es) being implemented. I mostly wanted to make sure I was representing adding those tests during the process, not as one block at the end.

The order you put together actually exposes one of my concerns regarding backward compatibility. I don't want to, for instance, introduce a backward compatible Node until we also have LifecycleNode ready to go, for instance. Since the current LifecycleNode inherits from the current Node, I get slightly nervous thinking about whether it will still behave properly once the internals of the class are replaced. It should be backward compatible from the user/API perspective, but might (likely won't) be from the perspective of an inheriting class.

And I will gladly stay hands-off from the XML/YAML for now. :)

ivanpauno commented 3 years ago

The order you put together actually exposes one of my concerns regarding backward compatibility. I don't want to, for instance, introduce a backward compatible Node until we also have LifecycleNode ready to go, for instance. Since the current LifecycleNode inherits from the current Node, I get slightly nervous thinking about whether it will still behave properly once the internals of the class are replaced. It should be backward compatible from the user/API perspective, but might (likely won't) be from the perspective of an inheriting class.

It shouldn't be a problem. In that case for example, I think LifecycleNode is only using the parent class constructor, execute, the node_name property, one private method. If it's using something else, it shouldn't be too hard to fix it.

My suggestion is because I prefer introducing the "brand new" way, without introducing duplicated code. That make things easier to maintain. Your code will also be exercised with all existing tests for Node, which is also a good idea.

Either way, the suggested order can be modified through the process if we find a problem. The overall plan LGTM!

hidmic commented 3 years ago

Maybe, we can create a "feature" branch in launch and launch_ros, and don't merge it on master until a coherent subset of things is merged.

Indeed. This is probably the best path forward. We can create the branch once you submit the first PR of each subset.

sjahr commented 3 years ago

What is the current progress on this proposed refactoring. I would like to use lifecycle nodes in a component but as far as I can see that is still not possible right now (or did I miss something?) Thanks in advance for your answer!

roger-strain commented 3 years ago

What is the current progress on this proposed refactoring. I would like to use lifecycle nodes in a component but as far as I can see that is still not possible right now (or did I miss something?) Thanks in advance for your answer!

A lot of the work is done at this point, although not the lifecycle portion just yet. We're trying to sort out a project issue behind the scenes to get the relevant PRs freed up and finalized. So still in the works, but not there just yet.

sjahr commented 3 years ago

What is the current progress on this proposed refactoring. I would like to use lifecycle nodes in a component but as far as I can see that is still not possible right now (or did I miss something?) Thanks in advance for your answer!

A lot of the work is done at this point, although not the lifecycle portion just yet. We're trying to sort out a project issue behind the scenes to get the relevant PRs freed up and finalized. So still in the works, but not there just yet.

Awesome, thanks for your quick response and work so far. Let me know if you need help with reviewing or testing the PR(s)!