[launch] Refactoring ExecuteProcess into Execute and Executable

[roger-strain]

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]

The idea seems good.
My feedback here is similar to the one I previously provided in ros2/launch#114 (comment).

I think that we need to address if an alternate launch context is needed or not before going ahead with an implementation.

[ivanpauno]

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]

Adding @wjwwood and @hidmic for feedback.

[hidmic]

Left a few comments. I'm sure @wjwwood has thoughts too (and he may not necessarily agree with me 😃).

[hidmic]

@roger-strain nit: here and everywhere else, capitalize each description sentence.

[roger-strain]

A lot of these were copied and pasted from the current class docs, so I was just maintaining how they appeared there. Easy enough change, however; addressing in upcoming revision.

[hidmic]

@roger-strain I think we're mixing things up again here e.g. a process is yet another execution detail that may or may not apply to a node. The fact that some of its arguments may be dependent on the actual execution environment sets this one apart from the rest. launch.descriptions.Executable or launch.descriptions.Program maybe? We can later compose things to serve common use cases.

[hidmic]

@roger-strain again I think we're mixing things up. I'd imagine e.g. Execute and SSHExecute actions only that can take Executable descriptions, delegating all description-specific details to the descriptions themselves.

[hidmic]

Hmm, there can be many Nodes in a single process and even be dynamically composable. I'd imagine a ROSAwareExecutable that can bear Nodes.

[claireyywang]

@roger-strain friendly ping

[roger-strain]

@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]

@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]

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):

• The act of executing a thing. That you've correctly separated into launch.actions.ExecuteLocalProcess and launch.actions.ExecuteSshProcess actions. It could be further generalized into a single launch.actions.Execute action that takes execution proxies or protocols, but it's fine as it stands.
• The thing that is being executed. For that you have launch.description.Executable and launch_ros.description.ExecutableNode and launch_ros.description.ComposableNodeContainer. I think we can simplify this down to launch.descriptions.Executable and launch_ros.descriptions.Executable. Nodes are not executables, executables bear nodes.
• The nodes that run within the thing. A launch_ros.description.Executable, to be found in a ROS package, would take launch_ros.descriptions.Node instances (a ROS aware executable always has at least one built-in node in it, or it wouldn't be ROS aware). A standard container for composition is nothing but a well known launch_ros.descriptions.Node. Composable nodes are also just launch_ros.descriptions.Node instances with a plugin name. So a couple launch_ros.descriptions.Node subclasses would suffice for composition support.

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]

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]

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 😅

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]

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]

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]

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]

@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.

[hidmic]

Sorry for the delay @roger-strain. It's looking really good !

[hidmic]

@roger-strain nit:

Suggested change

	As initially implemented, some aspects of this are tightly coupled to both the implementation chosen and the assumption that processes will execute only in the local environment of the launch process itself.
	As initially implemented, some aspects of this class are tightly coupled to both the implementation chosen and the assumption that processes will execute only in the local environment of the launch process itself.

[hidmic]

@roger-strain nit:

Suggested change

	The following changes are designed to provide a more decoupled architecture that will ease support of launching processes locally, remotely, or potentially in containerized environments.
	The following changes are designed to provide a more decoupled architecture that will ease support of launching processes locally, remotely, and potentially in containerized environments.

Containerization isn't mutually exclusive with execution in a remote host :)

[hidmic]

@roger-strain hmm, this is a good point. To some extent, I'd expect Python standard library to abstract us away from that. I think we should try to keep OS idiosyncrasies an implementation detail, preferably in an upstream package :)

[roger-strain]

Agree with you here; mostly it's an explanation of why some things that might normally be assumed to be part of an executable might need to be restricted to a more specific context. Assuming we get to the point of implementing non-local execution (which is where we want to go next) some of this design may have be reconsidered/adjusted as we find out which things really are and are not common. But that's a discussion for the future, I think. If there's anything that immediately seems like it's in the wrong spot, please let me know. :)

[hidmic]

@roger-strain why do executing actions have to deal with anything but an Executable (or derived classes)?

[roger-strain]

Good catch, removing reference to Node here.

[hidmic]

@roger-strain why can't the IsExecutable trait be subsumed into this Executable subclass, taking package name and executable name as arguments (besides node descriptions)? The current Node equivalent would then be:

ExecuteLocal(
   ROSExecutable(
       package_name='my_package',
       executable_name='my_node_executable',
       nodes=[
           Node(name='my_node')
       ]
   )
)

[roger-strain]

I like this. I've made that change (though the parameter names may need to be tweaked). See how it looks.

[hidmic]

I think we can drop the note entirely. cmd is not a valid argument for RosExecutable's constructor. No need to ask for it to not be used. If it comes along, TypeError: invalid arguments should be raised.

[hidmic]

@roger-strain hmm, I'd rather say ROS executables and node plugins live in packages, not nodes. So I'm not sure if it makes sense for node descriptions to refer to a package.

[roger-strain]

I've adjusted this now as well. package has moved to the RosExecutable object, and been re-introduced for ComposableNode. I also want the one on ComposableNode to be optional, so it can be picked up from the executable context if possible. If that's not reasonable, we can remove that concept. Would be nice to not have to respecify the package for each composed node. As I'm writing that, it feels wrong, so I think it might have to come back out. Let me know what you think.

[hidmic]

I also want the one on ComposableNode to be optional, so it can be picked up from the executable context if possible. If that's not reasonable, we can remove that concept. Would be nice to not have to respecify the package for each composed node.

That's interesting sugar. I like it.

[roger-strain]

@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]

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.

[hidmic]

This makes sense to me. I'll proof read it next week. @ivanpauno @wjwwood you guys should take a look too if you can.

[hidmic]

@roger-strain I like the feature, but when loading a composable node into an already running executable, you may not necessarily have access to package names. So we should probably mention that this deduction may fail.

[roger-strain]

I'm adding a note to that effect, but as I think about it I worry that we may not have that information more often than not. Might have to determine whether this is feasible at implementation time.

[hidmic]

@roger-strain nit:

Suggested change

	|node_executable|Name of the executable to find|
	| executable_name |Name of the executable to find|

?

[roger-strain]

Reasonable. Was just copying/pasting parameter names from existing things, I think.

[hidmic]

I think we can drop the note entirely. cmd is not a valid argument for RosExecutable's constructor. No need to ask for it to not be used. If it comes along, TypeError: invalid arguments should be raised.

[hidmic]

@roger-strain consider:

Suggested change

	Additional parameters that may be passed, which are handled by `launch.actions.ExecuteProcess`: `cmd`, `name`, `cwd`, `env`, `additional_env`.
	Additional parameters that may be passed, which are handled by `launch.actions.Executable`: `name`, `cwd`, `env`, `additional_env`.

[hidmic]

@roger-strain hmm, one thought (nothing to be changed): if connection_description were to become an interface to a given execution environment, a single launch.actions.Execute() could later deal with all forms of execution generically. May be overkill though.

[roger-strain]

@hidmic Wanted to follow up on this, as I was having some thoughts on that topic and wanted to run an idea by you before I start writing it up in the formal document. What would you think about going ahead with launch.actions.Execute(), which takes two things: the launch.descriptions.Executable to actually run, and optionally a launch.descriptions.ExecutionEnvironment or subclass thereof, which describes how it should be executed. If you pass nothing, local execution is assumed, and a default launch.descriptions.LocalExecutionEnvironment object is created under the hood.

You could inspect the object to get a reference to that default object later, if necessary. If you wanted to set any of the parameters currently assigned to launch.actions.ExecuteLocal, you would instead create your own launch.descriptions.LocalExecutionEnvironment with the appropriate bits.

(All of the following is forward-thinking and not part of the scope of this refactor):
This then would allow you to also define things like launch.descriptions.SshExecutionEnvironment, which would open an SSH session using the various parameters passed to it, or launch.descriptions.PsExecutionEnvironment if you have something with a PowerShell flavor. Where this could really shine is if you wanted to do something fancy, like have a single SSH session which could run multiple processes. That could look like a SharedSshExecutionEnvironment that you pass to all the relevant launch.actions.Execute calls, and then at the end a SharedSshExecutionEnvironment.ExecuteAll() method would let you launch one SSH session and run all the things that had been queued up inside it.

I think I'd need to mull a little more about where certain things end up, but it seemed like an interesting approach, and I wanted to capture my thoughts and start the discussion before I forgot.

[hidmic]

At first sight, all of that makes sense to me. If I may, I'd also suggest a Group-like action to easily put multiple actions in the same execution environment (akin to roslaunch's <machine/> tag).

[roger-strain]

@hidmic After poking around at this for a bit, I'm not sure we're really gaining anything with that aspect of the refactor. Basically everything that is defined in the currently proposed launch.actions.ExecuteLocal would have to be migrated into the new launch.descriptions.LocalExecutionEnvironment, and several methods which are currently overrides of launch.actions.Action would have to be redefined and reimplemented on a base launch.descriptions.ExecutionEnvironment. The new launch.actions.Execute class would basically be a nearly-empty shell that reroutes its own methods into calling the same-named methods on the launch.descriptions.ExecutionEnvironment class. It kind of feels like just adding an extra layer with little to no real benefit.

If it turns out that there would be benefit later, such as when trying to add multi-machine support, we could address it at that point.

Curious as to your thoughts. Assuming we're okay just staying with the design as it stands, do we need any further feedback from other contributors, or is it time to start implementation?

[roger-strain]

@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.

[ivanpauno]

I have to read this more thoroughly, the idea sounds good to me.
I've left a couple minimal comments.

[hidmic]

@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]

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

[ivanpauno]

I'm not sure of why the apply_context method is needed.
Besides that, I really like the proposal.

[ivanpauno]

I'm still unsure if the concept of overriding apply_context is needed.
I don't see much benefit on it

[roger-strain]

@ivanpauno The intent of allowing overrides of apply_context is so an executable in a more specific domain can perform domain-specific substitutions. Basically, at this level, we're in a context that has no knowledge of ROS or any ROS-specific concepts. We can do some level of substitution here, but can't take everything into consideration. To support those ROS-specific concepts, we have the launch_ros.descriptions.RosExecutable class. It would override apply_context so that it could pass substitution information down to the launch_ros.descriptions.Node objects, and possibly perform other actions necessary for preparing the ROS executable to run.

My fear is that if we don't do things this way, then we either have to make the launch namespace aware of ROS-specific concepts like nodes, or we alternately need to come up with some other more generic framework to ensure all aspects of the definition have an opportunity to do pre-launch setup (primarily substitutions). If there's another approach I'm missing that doesn't require the override, I'd be very happy to discuss it and look for a different take.

[ivanpauno]

The intent of allowing overrides of apply_context is so an executable in a more specific domain can perform domain-specific substitutions

Can you give an example of a domain specific substitution?
I'm trying to understand the use case, but I'm failing to see how this will help.

so that it could pass substitution information down to the launch_ros.descriptions.Node objects, and possibly perform other actions necessary for preparing the ROS executable to run.

Can you give an example of what "other actions" can be.

---

You can still go ahead with a proposed implementation with this comment unsolved, it's not a big deal.
But I want to understand how this detail is supposed to help.

[roger-strain]

@ivanpauno It might be best to show you the current functionality I'm trying to find a place for. In the current setup, launch_ros.actions.Node is a subclass of launch.actions.ExecuteProcess. ExecuteProcess defines the execute method, which is overridden by Node. Part of what Node does with the override is applying the various substitutions by calling _perform_substitutions(LaunchContext). Some of those substitutions are ROS-specific, some may not be. (Honestly, I'm going to have to be very careful in that area to make sure I split things out appropriately.) Those substitutions still need to be possible after the refactor.

The purpose of this call is to allow a subclass (in this case, launch_ros.descriptions.RosExecutable) to inject any additional substitution logic. For instance, the proposed launch.descriptions.Executable has absolutely no reason to know anything at all about a launch_ros.descriptions.Node's Name property, but it still needs to have an opportunity to perform substitutions. I'll admit to not being 100% on all the functionality going on down there, but my feeling is that I need some path to make sure everything can be covered. This was my best effort at ensuring that path exists when it's needed.

[ivanpauno]

Oh, I see what you mean now.

You need a way to "normalize" all launch.descriptions.Executable derived classes, in a way that launch.actions.ExecuteLocal or other actions can actually execute them.

The proposal makes sense to me!!

[roger-strain]

@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]

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]

@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:

• launch_ros.descriptions.ComposableNode
  • Original class has no inner functionality and simply provides a collection of fields.
  • Original constructor parameters/properties: package, node_plugin, node_name, node_namespace, parameters, remappings, extra_arguments
  • New class constructor parameters/properties:
    • Direct: package, node_plugin
    • Inherited (launch_ros.descriptions.Node): node_name, node_namespace, parameters, remappings, arguments, traits
  • All original parameters/properties still exist, though one is renamed (extra_arguments -> arguments)
  • Additional parameter+property will be added to provide backward-compatibility, but will map to same data as new arguments

Planned PR sequence:

• Launch descriptions:
  • launch.descriptions.Executable
    • Implement class with constructors, properties, and overridable apply_context method.
    • Implement any substitutions possible within this context.
  • Add tests to launch/launch_testing as possible. (May be limited; no execution support by this point.)
• Launch actions:
  • launch.actions.ExecuteLocal
    • Implement class with constructors, properties, methods, and events.
    • Reuse code from launch.actions.ExecuteProcess.
  • Add tests to launch/launch_testing as possible.
• Launch_ros base descriptions:
  • launch_ros.descriptions.Node
    • Implement class with constructors, properties, and overridable prepare method.
    • Implement any substitutions possible within this context.
  • launch_ros.traits.NodeTrait
    • Define abstract class with overridable prepare method.
  • Add tests to launch_ros/launch_testing_ros as possible. (May be limited; no ROS execution support by this point.)
• Launch_ros executable description:
  • launch_ros.descriptions.RosExecutable
    • Implement class with constructors, properties, and overridden apply_context method.
    • Call prepare() on all nodes, determine executable if necessary
  • Add tests to launch_ros/launch_testing_ros as possible.
• Composable Node support descriptions:
  • launch_ros.descriptions.ComposableNode
    • Implement subclass with constructors and properties.
    • Verify backward-compatibility with existing class.
  • launch_ros.traits.ComposesNodes
    • Implement subclass with constructors, properties, and overridden prepare method.
    • Add functionality to launch composed nodes when container node is started.
  • Add tests to launch_ros/launch_testing_ros as possible.
• Lifecycle Node support descriptions:
  • launch_ros.tratis.HasLifecycle
    • Implement subclass with overridden prepare method.
    • Add functionality to handle and emit lifecycle events
  • Add tests to launch_ros/launch_testing_ros as possible.
• Add backward compatibility
  • launch
    • launch.actions.ExecuteProcess -> launch.descriptions.Executable + launch.actions.ExecuteLocal
  • launch_ros
    • launch_ros.actions.Node -> launch_ros.descriptions.Node + launch.descriptions.RosExecutable + launch.actions.ExecuteLocal
    • launch_ros.descriptions.ComposableNode -> already backward compatible
    • launch_ros.actions.ComposableNodeContainer -> launch_ros.actions.Node + launch_ros.traits.ComposesNodes
    • launch_ros.actions.LifecycleNode -> launch_ros.actions.Node + launch_ros.traits.HasLifecycle

Questions:

• Extensiveness of unit testing? Haven't dived in yet, not sure if tests should be launching processes, nodes, etc. to verify behavior.
• 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.
• 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.

[ivanpauno]

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:

• launch
  • feature branch:
    • Add launch.descriptions.Executable
    • Add launch.actions.ExecuteLocal
    • Refactor launch.actions.ExecuteProcess -> launch.descriptions.Executable + launch.actions.ExecuteLocal
  • merge feature branch in master
• launch_ros
  • feature branch:
    • Add launch_ros.descriptions.Node
    • Add launch_ros.traits.NodeTrait
    • Add launch_ros.descriptions.RosExecutable
    • Refactor launch_ros.actions.Node -> launch_ros.descriptions.Node + launch.descriptions.RosExecutable + launch.actions.ExecuteLocal
  • merge feature branch in master
  • feature branch 2:
    • Add launch_ros.descriptions.ComposableNode
    • Add launch_ros.traits.ComposesNodes
    • Refactor launch_ros.actions.ComposableNodeContainer -> launch_ros.actions.Node + launch_ros.traits.ComposesNodes
  • merge feature branch 2 in master
  • feature branch 3:
    • Add launch_ros.tratis.HasLifecycle
    • Refactor launch_ros.actions.LifecycleNode -> launch_ros.actions.Node + launch_ros.traits.HasLifecycle
  • merge feature branch 3 in master

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 😂. For the moment, you won't need to extend it.

[roger-strain]

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]

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]

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.

[hidmic]

@roger-strain please, could you remind me why we have apply_context and prepare (as opposed to just prepare)? If it's for (semantic) consistency's sake, I can see several variations of prepare already.

[roger-strain]

@hidmic I don't know if there's a specific reason for both. It might have evolved from building from both ends, if you will. If you'd rather simplify that down to just one or the other, I can definitely look at doing that.

[hidmic]

The latter sounds good.

[sjahr]

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]

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]

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)!