[launch] consider refactoring ExecuteProcess into Execute and Executable

[wjwwood]

I believe it might be wise to separate the description of a process to be executed and its state while running from the Action which executes it.

Currently all of this information is encapsulated in the ExecuteProcess action, and my proposal would be to replace that with an action called Execute that takes an Executable (not an action or launch description entity, just a new kind of class). The Executable would contain any information needed by Execute to actually execute the executable described or introspect it. There could be sub classes of both Execute and Executable. For instance, a sub class of Executable might be Node or LifecycleNode, and a sub class of Execute might be ExecuteRemotely (execute on a different machine).

The reasons for doing this include:

• using Executables (or subclasses thereof) with other actions
• cleaner abstractions
  • i.e. the ExecuteProcess class is really big now and it would be nice to break into smaller parts
• more readable
  • e.g. Execute(Executable(...)) or Execute(Node(...)) rather than ExecuteProcess(...) and Node(...)

The first reason is the most impactful, because it allows for a few use cases that aren't currently easy to implement:

• Collect or generate Executable instances and later decide whether to pass to Execute, ExecuteRemotely, or ExecuteInDocker (just ideas)
• Pass ComponentNode (possibly a subclass of Executable which represents a node that can be loaded dynamically) to either Execute() or ExecuteNodesInSharedProcess() (name needs work)
  • where the latter would take several ComponentNode instances and run them all in a single process

I still have some design questions in mind, like:

• should Node inherit from Executable
• should Executable contain process related things like pid and produce events for stdout and process exit, etc...
  • or should there be a Process that also inherits from Executable
  • because Node might not be associated with a process, or rather a process might not be associated with a single Node
• what is the contract between Executable and Execute?
  • which creates event handlers and stores information?
  • what exactly are the responsibilities of Execute?

[ivanpauno]

I thought a little bit about this, and about how it would look in the new frontends (examples only in XML here).

I would have the following classes:

• A Node class, that describes a node (name, namespace, remappings, parameters).
  • A ComposableNode class.
  • A LifecycleNode class.
• An Executable class (or Process class), that describes an executable (with process information).
  • A SingleNodeExecutable class that describes an executable that runs only one node. It may also inherit from Node, or take a Node in its constructor (is an implementation detail).
  • A MultiNodeExecutable, which inherits from node and takes a list of Node in its constructor.
• An abstract BaseExecute action.
  • An Execute action.
    • An ExecuteContainer action. It should take both a Node, that is the container, and a list of ComposableNode as arguments.
  • We could have ExecuteRemotely, ExecuteInDocker, etc.

For XML (or other frontend), I think we should check the parent entity and behave differently according to that.
e.g.:

<node ATTRIBUTES_FOR_EXECUTION_EXECUTABLE_AND_NODE_DESCRIPTION/>
<execute_remotely ATTRIBUTES_FOR EXECUTION>
  <node ATTRIBUTES_FOR_EXECUTABLE_AND_NODE_DESCRIPTION/>
  <multi_node ATTRIBUTES_FOR_EXECUTABLE>
    <node ATTRIBUTES_FOR_DESCRIBING_A_NODE/>
    <node ATTRIBUTES_FOR_DESCRIBING_A_NODE/>
  </multi_node>
</execute_remotely>

So in each case the tag may accept different attributes, but it would be less verbose.

[roger-strain]

I've been working with the guys who are putting together ideas for Multi-machine launching (ros2/design#255), and wanted to follow up on some of the feedback there. I saw the mention of this issue, and I'd like to see if we can help on getting some of this work done, and figure out how to use this concept for our remote execution needs. Also want to make sure we don't start running with ideas that have been obsoleted by other changes. There are good notes above from both @wjwwood and @ivanpauno; does everything here still feel current?

Also, there's discussion in the FP about some changes to how things would be called, for instance:

e.g. Execute(Executable(...)) or Execute(Node(...)) rather than ExecuteProcess(...) and Node(...)

Are there any concerns about breaking changes and backward compatibility in this area?

[ivanpauno]

Are there any concerns about breaking changes and backward compatibility in this area?

Yes. It should be done in a backwards compatible way. i.e.: The ExecuteProcess class, etc, should still exist, making use of the new objects to avoid duplicated code, and print a deprecated warning when an user uses it.

I think that everything previously commented here still stands.
I think that a good start would be to propose an API, and share it for review.
In general, my idea is to separate descriptions from "Execute" actions. We've already done that in some cases (like here), but we haven't applied that consistently.

If you need any extra input for the design of the API, please let me know and I will try to help.

[roger-strain]

@ivanpauno Finally had some time to dig into this and learn quite a bit more about how things are structured. I'm not sure if this is in quite the format you were wanting, but we've put together some thoughts about how to approach this. I've created a document in a fork of the design repo, but I'm not sure the document itself needs to be pulled there. Just needed a place to park some thoughts.

Take a look to see if things are on the right track; if you'd like me to move this somewhere else, I certainly can.

Also, (and this is probably a little premature) assuming we eventually get to the point of implementing the refactor, can you point me to the appropriate unit tests so I can make sure to update them/add new ones as appropriate?

ExecuteProcess Refactor discussion

[ivanpauno]

@roger-strain Thanks for working on this!

I had a quick look, here some comments:

• Probably, launch_ros.descriptions.ComposableNode can't inherit from launch_ros.descriptions.Node, as it doesn't have for example a node_executable argument. Maybe both can inherit from a common base class, that doesn't specify either plugin name nor executable name. Maybe, we don't need a base class at all.
• About the apply_context idea. I think that probably people would want to have both kind of substitutions, e.g.: for some things check an environment variable locally, from some others check an environment variable in the remote computer. I would rather have another set of substitutions for the ones that have to be executed in the remote computer, e.g.: launch.substitutions.remote.EnvironmentVariableSubstitution. We could add some magic to infer the connection_description if the Substitution is composed in a ExecuteSshProcess action, to avoid duplication (I have a mental idea of how to do that).

I think that before continuing the discussion, it would be good to open a PR in ros2/design.
It will have more visibility there, and it's also better for receiving feedback (making comments right after the place it applies is nicer).

[roger-strain]

@ivanpauno Is the document naming/formatting okay for a PR as is? I can go set that up now if it looks like it's in good enough format for that discussion.

[ros-discourse]

This issue 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