Execute process refactor #215

roger-strain · 2021-01-25T21:52:06Z

This is an initial commit toward the process described in ros2/design#272.

Part of implementation of refactor for ros2/launch#114, and depends on ros2/launch#454; unit tests will fail without those changes.

roger-strain · 2021-01-25T21:57:16Z

This has taken longer to get in than I would like, but such is the way of things sometimes. A few questions/notes as I was getting this together:

In the refactoring, I had carried arguments through to the Node level, but I wonder now if they might better belong to the Executable; they seem to be more related to that concept, whereas parameters tie neatly to Node.
We discussed adding support for multiple Nodes in a single Executable, but fully implementing that still seems a bit tricky, at least as part of this PR. I'm starting to think we might be better served by getting this one done, with only having one Node truly supported, then come back and add in multiple-node support later. (As an example, we had discussed how to pass down parameters to individual nodes when multiple exist, but then I stumbled when trying to figure out how to support renaming multiple nodes in a single command line.)
Some of the composable node unit tests didn't seem to be working, but I'm not sure it was due to these changes. Are they all in good shape? I'll keep digging into them if so.
There probably are plenty of other discussion points, but I'll wait for an initial review before diving too far into other things.

launch_ros/launch_ros/descriptions/node.py

hidmic · 2021-01-27T14:45:31Z

launch_ros/launch_ros/descriptions/node.py

+        name, value = param.evaluate(context)
+        return f'{name}:={yaml.dump(value)}'
+
+    def prepare(self, context: LaunchContext, executable: Executable) -> None:


@roger-strain seems like traits are ignored?

Possibly so; those are going to come into play more strongly with the next couple of PRs/commits related to this as we get into composable/lifecycle concepts. At the moment, they were just placeholders with no functionality. I'll see about adding in the appropriate calls. As you noted elsewhere, at the moment they call for an Action, whereas here I have Executable instead. I'll need to sanity check what's really necessary, and may have to add more plumbing depending on that result.

Yes! A review of the call chain would be healthy.

hidmic · 2021-01-27T14:52:20Z

launch_ros/launch_ros/descriptions/node_trait.py

+        """
+        pass
+
+    def prepare(self, node, context: LaunchContext, action: Action):


@roger-strain nit:

Suggested change

def prepare(self, node, context: LaunchContext, action: Action):

def prepare(self, node: Node, context: LaunchContext, action: Action):

plus necessary imports (dealing with any circularity)

Also, what action? And where are you going to grab that from?

launch_ros/launch_ros/actions/node.py

hidmic · 2021-01-27T15:20:42Z

In the refactoring, I had carried arguments through to the Node level, but I wonder now if they might better belong to the Executable; they seem to be more related to that concept, whereas parameters tie neatly to Node.

I'd say arguments belong to the Executable (and RosExecutable).

but then I stumbled when trying to figure out how to support renaming multiple nodes in a single command line

See https://index.ros.org/doc/ros2/Tutorials/Node-arguments/#passing-remapping-arguments-to-specific-nodes.

BTW check your DCO!

hidmic · 2021-01-27T15:39:19Z

Overall, I think this is going in the right direction. I'd revisit the architecture, make sure that entity setup via descriptions and/or traits is clean (no crust like redundant arguments, no issues like unnecessary coupling). I'll try to do a deeper review later this week to both feature branches.

roger-strain · 2021-02-09T17:47:59Z

@hidmic And here's the corresponding changes on this side. I've tried to simplify and clean up some of the argument lists. I was trying to remove some of the extra layers of calls going on, but had to reintroduce one to make the unit tests happy; they wanted certain parts of the substitutions to happen, but I couldn't let the full preparation go through or it would have needed a lot more mock structure in the tests than I thought was really warranted.

If this looks reasonable, I can go ahead and look at the Composable/Lifecycle stuff next. If there are other things you want to see here first, though, please let me know. The more specific the better. :)

roger-strain · 2021-02-25T15:11:10Z

@hidmic Just wanted to check in and see if there's anything else I can do to help on this at the moment.

hidmic

@roger-strain my apologies for not circling back sooner. Just did another pass. Check your failing PR job!

hidmic · 2021-03-09T21:59:50Z

launch_ros/launch_ros/descriptions/node.py

+                execute_process_logger.warning(
+                    'there are now at least {} nodes with the name {} created within this '
+                    'launch context'.format(node_name_count, self.node_name)
+                )


@roger-strain nit: missing newline

hidmic · 2021-03-09T22:04:45Z

launch_ros/launch_ros/actions/node.py


-        self.__substitutions_performed = False
+    def prepare(self, context: LaunchContext):
+        self.__node_desc.prepare(context, self.__ros_exec, self)


@roger-strain why preparing the node description here? Won't ExecuteLocal.prepare() invoke RosExecutable.prepare(), which in turn will invoke NodeDescription.prepare()? That'd make sense to me.

On reflection, I believe you're correct. I think this was probably added earlier in the development cycle, before the RosExecutable portion came in.

hidmic · 2021-03-09T22:05:21Z

launch_ros/launch_ros/descriptions/node.py

+        **kwargs
+    ) -> None:
+        """
+        Construct an Node description.


@roger-strain nit:

Suggested change

Construct an Node description.

Construct a Node description.

hidmic · 2021-03-09T22:06:15Z

launch_ros/launch_ros/descriptions/node.py

+        Construct an Node description.
+
+        Many arguments are passed eventually to
+        :class:`launch.actions.ExecuteProcess`, so see the documentation of


@roger-strain did you mean to launch.actions.ExecuteLocal? At any rate, it shouldn't specify an executor. Perhaps documentation needs a revisit?

Definitely was a copy/paste leftover from something. I've removed the clause entirely. I think the rest of it is okay, but if there are other deficiencies please let me know.

hidmic · 2021-03-09T22:07:38Z

test_launch_ros/test/test_launch_ros/actions/test_lifecycle_node.py

@@ -51,5 +51,5 @@ def test_node_name():
        namespace='my_ns',
    )
    lc = LaunchContext()
-    node_object._perform_substitutions(lc)
+    node_object._Node__node_desc._perform_substitutions(lc, [])


@roger-strain is it necessary to access a "private" attribute like this? Isn't the functionality under test observable via public API? Same everywhere else below.

It actually wasn't (easily) available without making some assumptions about data types that I didn't want to make. I've added a new getter on the actions.Node for ros_exec, which will allow us to easily get access to the ROS-style executable, rather than the more generic Executable description, which didn't provide the things we needed.

hidmic · 2021-03-09T22:10:50Z

launch_ros/launch_ros/descriptions/node.py

+        if self.__node_name is not None:
+            ros_specific_arguments['name'] = '__node:={}'.format(self.__expanded_node_name)
+        if self.__expanded_node_namespace != '':
+            ros_specific_arguments['ns'] = '__ns:={}'.format(self.__expanded_node_namespace)


@roger-strain meta: this will probably need a revisit. A blanket __node:= remapping will apply to all nodes in the same executable. Probably not what a user would intend in this case. Same for __ns:=.

This is probably a carry-over from the earlier design which assumed one Node per executable process. I'm looking at the docs here (https://docs.ros.org/en/foxy/Guides/Node-arguments.html) and it looks like I need to prefix things with the "old" name of the node. That's something that I don't believe we've captured in the descriptions.Node structure to this point. I'm adding a new parameter to capture original_node_name; if it's not set, there's nothing we can do but let the global apply. (This will actually work out okay, though, because I think it maintains current functionality for backward-compatibility.)

I'm now having another thought, though, which is related to this. For "normal" (read: Not node name/namespace) parameters, the docs show the ability to prefix individual parameters with the (new) node name. I'm guessing this is something we'll need to bake in at this layer as well? How does that come into play if we're passed or creating parameter dictionary files? Not sure how far down this rabbit hole is the responsibility of this layer, vs telling the developer they need to specify things with the appropriate prefix. Would love your thoughts on which parts to tackle internally.

I'll start by saying that some of these rough edges when dealing with remapping predate your work, and that I'm not pushing to get them all addressed here.

I don't think launch should be mutating parameter files to propagate a remapping and such. That's on the underlying executable's plate at best. IMHO launch should simply provide a way to express the intent to remap.

For executables that bear many nodes, the current command line interface allows both node-specific and blanket configurations. To support the latter, we can simply let the ROSExecutable action take extra command line arguments. To support the former, Node descriptions have to bear the original name, that's right. We can allow a single Node description to not specify it for convenience, in which case I think the ROSExecutable action should enforce only one such description is given.

I think I ended up trying to make this more complicated than it needed to be. If we're setting aside the dictionary files, then in all cases we know what the applied node name should be by the time we're setting up command line parameters, so I can just add that as the normal way of doing it.

~~Here's a question on parameters, though. Is there such a thing as a node-specific (non-remapping) parameter? If so, I don't see it in the sample documentation I've been referring to.~~

Ignore the previous part. I confused myself. I think we're good.

hidmic · 2021-03-09T22:11:30Z

launch_ros/launch_ros/descriptions/node_trait.py

+        pass
+
+    def prepare(self, node: 'Node', context: LaunchContext, action: Action):
+        """Perform any actions necessary to prepare the node for execution."""


@roger-strain missing pass after docstring?

roger-strain · 2021-03-22T21:42:57Z

Check your failing PR job!

About to push up a new version that includes several of the changes discussed above. Something about my local unit test setup isn't capturing things the same as the build server does, so I may end up having to do a few passes to get that sorted, apologies for that.

(If you have any tips on setting up a good environment to make sure the tests are working locally, I'd love to hear it.)

roger-strain · 2021-03-24T13:12:46Z

@hidmic

Check your failing PR job!

Oh, just realized some of the failures here may be because this PR depends on the PR over in launch, which isn't merged yet. Is there any way to configure the build/test here to use that branch instead of master? Or do we just need to wait until that one goes through before we can get a better view of how this one is doing build/test-wise?

roger-strain · 2021-04-02T12:51:01Z

@hidmic Hey, just wanted to check back in. I know we're still pending the launch PR before we can really confirm build/test behavior on this one. I've pushed some other updates here in the meantime to address other comments. Is there anything else you'd like to see done here in the meantime, or should we just hold this until the other one gets sorted?

I've been considering moving forward with the Composable and Lifecycle portions of the refactor, but didn't want to get too much pending at once.

mlanting · 2021-11-22T19:14:46Z

@hidmic I've merged our changes back on top of master but Jenkins is running into errors because it doesn't seem to have the changes that were merged into the launch repository. Is there something I need to do to tell it to update?

Also, the ros2launch_security repo will require a couple changes to work with the refactored code. I can submit a PR, but I thought this might warrant a bit of discussion first and want to make sure the discussion happens here instead of being split across two PR's.

The ros2launch_security plugin was based on an extension to launch_ros called NodeActionExtension. However, this refactor deprecates the Node action in favor of simply passing a RosExecutable description to a standard ExecuteLocal action. The RosExecutable description contains one or more Node descriptions. I've moved NodeActionExtension into the new Node description class and changed the function signature of prepare_for_execute to take a RosExecutable and a Node description instead of a Node action.

I noticed that the NodeActionExtension interface looks very similar to the NodeTrait description we included in anticipation of adding support for things like composable nodes and lifecycle nodes that might need additional setup steps before being fully launched. Would it be worth wrapping the two concepts (node traits and node action extensions) into one? Security as a node trait makes some sense, but I'm not so sure about composable/lifecycle nodes as plugins/extensions. Still, the interfaces are similar enough (almost identical) that I figure it's worth mentioning.

clalancette · 2021-11-29T14:25:12Z

@hidmic I've merged our changes back on top of master but Jenkins is running into errors because it doesn't seem to have the changes that were merged into the launch repository. Is there something I need to do to tell it to update?

No, we need to do a new release of launch in order to get those changes out. I'll go ahead and do that.

hidmic · 2021-12-06T19:18:29Z

Would it be worth wrapping the two concepts (node traits and node action extensions) into one?

Ideally, consolidating would be best. Those extensions are plugin-based and implicit though. Not sure what the best path forward is here. Perhaps some traits can also be plugin-based and implicit? I'm open to ideas. FYI @gbiggs.

hidmic · 2021-12-06T19:19:03Z

@ros-pull-request-builder retest this please.

hidmic · 2021-12-23T14:40:57Z

@mlanting there's still a bunch of test failures in that Rpr job.

mlanting · 2022-01-06T19:00:45Z

@hidmic Yeah, I still had a few I was working through but wanted to get some fixes pushed up before I left for vacation. I've fixed a few more of them. **The remaining failures that I'm seeing are all failing when the testing infrastructure tries to call tests in the talker_listener_launch_test.py file in launch_testing_ros.

It's giving type errors along the lines of TypeError: test_fuzzy_data() missing 3 required positional arguments: 'launch_service', 'listener', and 'proc_output' I wonder if you or anyone else might have some insights as to what the issue could be here? I'll keep looking into it.

**Actually, i seem to be getting a few intermittent failures on some of these still, so i'll keep looking into them.

mlanting · 2022-01-20T20:26:54Z

I created a pull request (ros2/launch#587) with a fix for a test function in the launch repo to address the remaining fix.

For an intermittent timer failure I was seeing, I simply increased the tolerance a bit. The tolerance was originally 0.1 seconds. The largest difference I saw in the few times I occurred was 0.37 seconds, so I just increased the tolerance to 0.5.

While running the tests repeatedly trying to get the timer to fail, I also saw the error described here on a couple occasions: #280. I did not make any attempts to address it, but I do want to note that the issue can apparently happen on Ubuntu and is not just limited to Windows.

mlanting · 2022-01-26T16:53:48Z

@hidmic friendly ping

mlanting · 2022-02-11T16:51:08Z

@hidmic friendly ping

hidmic

@mlantting did another pass. I'm sorry for the delay. I've been swamped.

hidmic · 2022-02-17T13:14:03Z

launch_ros/launch_ros/descriptions/ros_executable.py

+# OPSEC #4584.
+#
+# Delivered to the U.S. Government with Unlimited Rights, as defined in DFARS
+# Part 252.227-7013 or 7014 (Feb 2014).


@mlanting there was a discussion about this already in ros2/launch#454. I'd be best if we can follow suit (by removing it).

You're correct, I left this in by mistake. I'll remove it.

hidmic · 2022-02-17T13:22:36Z

launch_ros/launch_ros/actions/node.py

-        self.__substitutions_performed = False
-
-        self.__logger = launch.logging.get_logger(__name__)
+        self.__node_desc = NodeDescription(node_name=name, node_namespace=namespace,


@mlanting nit: why self.__node_desc when we can self.__ros_exec.nodes[0]?

This class only exists for backward compatibility with the old node action which really only supported a single node and this was just an artifact of the way we were thinking about the class when it was written. I've left it as is, but I don't have strong feelings either way and can change it if you'd prefer the other way.

hidmic · 2022-02-17T13:22:57Z

launch_ros/launch_ros/actions/node.py

+        self.__node_desc = NodeDescription(node_name=name, node_namespace=namespace,
+                                           parameters=parameters, remappings=remappings)
+        ros_exec_kwargs = {'additional_env': additional_env}
+        self.__ros_exec = RosExecutable(package=package, executable=executable,


@mlanting nit^2: consider

Suggested change

self.__ros_exec = RosExecutable(package=package, executable=executable,

self.__executable = RosExecutable(package=package, executable=executable,

The rest is implied IMHO.

I'll make the change.

hidmic · 2022-02-17T13:23:42Z

launch_ros/launch_ros/actions/node.py

@@ -197,47 +123,20 @@ def __init__(
            passed to the node as ROS remapping rules
        :param: ros_arguments list of ROS arguments for the node
        :param: arguments list of extra arguments for the node
+        :param additional_env: Dictionary of environment variables to be added. If env was
+            None, they are added to the current environment. If not, env is updated with
+            additional_env.


@mlanting hmm, what about env?

@mlanting nit: consider extracting RosExecutable specific keyword arguments from kwargs instead of duplicating argument documentation (as in {key: value for key, value in kwargs.items() if key in ROS_EXECUTABLE_KWARGS} or similar).

I like the idea of pulling out the keywords args and passing them via a pared-down kwargs variable. I used the inspect module to get the available keywords from the Executable class to filter out the appropriate pairs.

hidmic · 2022-02-17T14:15:06Z

launch_ros/launch_ros/descriptions/ros_executable.py

+            nodes: Iterable[Node] = None,
+            ros_arguments: Optional[Iterable[SomeSubstitutionsType]] = None,
+            arguments: Optional[Iterable[SomeSubstitutionsType]] = None,
+            **kwargs


@mlanting meta: I wonder if it would make sense to allow for "global" parameters and remapping rules here (i.e. without a node name prefix to scope them).

@mlanting even more so, there was (and probably still is) some confusion around parameter file formats and how do each apply to launch_ros actions, mainly owing to the fact that the file format expected by rcl accounts for multiple nodes in the same process whereas the file format expected by rclcpp_components is bound to a single node.

We could potentially sort that out. RosExecutable would accept the former while Node descriptions would accept the latter.

Is there more detailed discussion on this issue you can point me to?

Do you want to try to do anything with regard to this issue in this PR or are you just pointing it out as something to keep in mind for future changes?

hidmic · 2022-02-17T14:21:49Z

launch_ros/launch_ros/descriptions/node.py

+        node_name: Optional[SomeSubstitutionsType] = None,
+        node_namespace: Optional[SomeSubstitutionsType] = None,
+        original_node_name: Optional[SomeSubstitutionsType] = None,


@mlanting nit: consider

Suggested change

node_name: Optional[SomeSubstitutionsType] = None,

node_namespace: Optional[SomeSubstitutionsType] = None,

original_node_name: Optional[SomeSubstitutionsType] = None,

name: Optional[SomeSubstitutionsType] = None,

namespace: Optional[SomeSubstitutionsType] = None,

original_name: Optional[SomeSubstitutionsType] = None,

as node is implied.

FWIW, we deprecated (and then removed) the arguments with the "node_" prefix in the existing Node action.

I've removed the node_ prefixes

hidmic · 2022-02-17T14:26:15Z

launch_ros/launch_ros/descriptions/node.py

+        :param: node_name the name of the node
+        :param: node_namespace the ROS namespace for this Node
+        :param: original_node_name the name of the node before remapping; if not specified,
+            remappings/parameters (including node name/namespace changes) may be applied
+            to all nodes which share a command line executable with this one


@mlanting hmm, this is a bit clunky. Thinking out loud, perhaps node_name and node_namespace shouldn't be prescriptive but descriptive. That is, to be mandatory and expected to match what the actual node already defines, leaving name and namespace remapping to be done separately e.g.:

Node( name = 'actual_name', namespace = 'actual_namespace' remappings = Node.naming( name = 'my_new_name', namespace = 'my_new_namespace' ) + [('/chit', '/chat')], )

CC @wjwwood @jacobperron for feedback.

Either way it seems a bit brittle to me if we expect the name in launch to match the name in the code. You have to remember to update both places; and worse if the launch file is not maintained by the node author (e.g. in some end-users package).

However, I don't see another way around this problem besides introducing some far-fetched mechanism to query the coded node name from launch. I like @hidmic's suggestion since I think it will result in less hidden surprises for users (i.e. it avoids accidentally renaming a bunch of nodes in a process to the same name). But I'm also worried that being strict about providing the correct node name will be frustrating for users, who currently don't have to do this.

I'm torn.

hidmic · 2022-02-17T14:44:28Z

launch_ros/launch_ros/descriptions/node.py

+from ..remap_rule_type import SomeRemapRules
+
+
+class NodeActionExtension:


Would it be worth wrapping the two concepts (node traits and node action extensions) into one? Security as a node trait makes some sense, but I'm not so sure about composable/lifecycle nodes as plugins/extensions.

@mlanting circling back to this. These extensions were added specifically to support osrf/ros2launch_security. Looking at those extensions, it'd seem like hoisting this to the RosExecutable action level would make the most sense (since --enclave is a process-wide ROS argument).

hidmic · 2022-02-17T14:48:21Z

test_launch_ros/test/test_launch_ros/actions/test_node.py

@@ -184,7 +186,8 @@ def test_launch_node_with_parameter_descriptions(self):
        )
        self._assert_launch_no_errors([node_action])

-        expanded_parameter_arguments = node_action._Node__expanded_parameter_arguments
+        for node in node_action.ros_exec.nodes:
+            expanded_parameter_arguments = node.expanded_parameter_arguments


@mlanting hmm, should these be extending the expanded_parameter_arguments list instead of overwriting? Same below.

Yeah, that would probably be more appropriate. I'll make the change.

clalancette · 2022-07-13T17:51:44Z

This still didn't work. I think that this is caching problem on the buildfarm side, due to some recent changes we made. This should be cleared up tonight, I believe.

That said, while we like to have the Rpr jobs green, it isn't totally blocking for this. What is blocking is getting this approved in review and also running the https://ci.ros2.org jobs for all platforms.

mlanting · 2022-07-13T18:06:14Z

Alright, thank you for your help. Is there anything I need to do to trigger the CI jobs?

@jacobperron @hidmic Are you content to move forward with this?

clalancette · 2022-07-14T13:24:32Z

@ros-pull-request-builder retest this please

roger-strain · 2022-10-07T18:49:26Z

@clalancette @hidmic I think this PR has fallen through the cracks, and I wanted to check in with you guys on whether it's something we should continue to put energy into. This was intended to be a stepping stone towards some other design goals, but I'm honestly concerned at this point that other developments may have impacted what we were trying to do here. I'd appreciate your candid thoughts on how to best move forward at this point.

vknisley-swri · 2023-11-06T17:25:16Z

This PR is a couple years old, but I've rebased and resolved some warnings. Is this functionality that we would still like to include in launch_ros?

Distro A; OPSEC #4584 Signed-off-by: Roger Strain <[email protected]>

Distro A, OPSEC #4584. You may have additional rights; please see https://rosmilitary.org/faq/?category=ros-2-license Signed-off-by: matthew.lanting <[email protected]>

context in descriptions/node.py. Distro A, OPSEC #4584. You may have additional rights; please see https://rosmilitary.org/faq/?category=ros-2-license Signed-off-by: matthew.lanting <[email protected]>

Distro A, OPSEC #4584. You may have additional rights; please see https://rosmilitary.org/faq/?category=ros-2-license Signed-off-by: matthew.lanting <[email protected]>

parameters between classes. Distro A, OPSEC #4584. You may have additional rights; please see https://rosmilitary.org/faq/?category=ros-2-license Signed-off-by: matthew.lanting <[email protected]>

Distro A, OPSEC #4584. You may have additional rights; please see https://rosmilitary.org/faq/?category=ros-2-license Signed-off-by: matthew.lanting <[email protected]>

…esolve ros2/ros2#1254 by clalancette. Signed-off-by: veronica.knisley <[email protected]>

Signed-off-by: veronica.knisley <[email protected]>

vknisley-swri · 2023-12-15T22:48:19Z

@clalancette @hidmic I just wanted to check whether this PR is still something we would like to include in launch_ros. It was originally opened a few years ago, but I've rebased it to include the latest rolling changes (from approximately 3 weeks ago). Thanks!

roger-strain mentioned this pull request Jan 25, 2021

Adding Executable description class ros2/launch#454

Merged

hidmic reviewed Jan 27, 2021

View reviewed changes

roger-strain force-pushed the execute-process-refactor branch from 56a5946 to e8b2dcb Compare February 9, 2021 17:42

clalancette assigned hidmic Feb 25, 2021

roger-strain force-pushed the execute-process-refactor branch from 0742de2 to c648bbc Compare March 3, 2021 15:25

hidmic reviewed Mar 9, 2021

View reviewed changes

roger-strain force-pushed the execute-process-refactor branch from b431784 to a68f1d8 Compare March 26, 2021 20:36

mlanting force-pushed the execute-process-refactor branch from a68f1d8 to 861b2f9 Compare November 8, 2021 20:06

mlanting force-pushed the execute-process-refactor branch from 1cff138 to 8ba498f Compare November 29, 2021 17:08

hidmic mentioned this pull request Dec 8, 2021

consider porting RewrittenYaml from Nav2 to allow param overrides to YAML from launch #258

Open

mlanting mentioned this pull request Jan 20, 2022

Added case for instances of ExecuteLocal in resolveProcess function ros2/launch#587

Merged

hidmic reviewed Feb 17, 2022

View reviewed changes

mlanting force-pushed the execute-process-refactor branch from 774923e to fcfaf43 Compare April 11, 2022 19:30

vknisley-swri force-pushed the execute-process-refactor branch from f5a3528 to e2ee8da Compare November 6, 2023 17:11

vknisley-swri requested review from adityapande-1995 and methylDragon as code owners November 6, 2023 17:11

vknisley-swri force-pushed the execute-process-refactor branch from e2ee8da to cac239f Compare November 6, 2023 17:19

Roger Strain and others added 19 commits December 15, 2023 15:42

Refactor Node action into description classes

6689fe0

Distro A; OPSEC #4584 Signed-off-by: Roger Strain <[email protected]>

Refactor arguments from nodes, address feedback

e5ef999

Distro A; OPSEC #4584 Signed-off-by: Roger Strain <[email protected]>

Fix missing node name parameter

83c4529

Distro A; OPSEC #4584 Signed-off-by: Roger Strain <[email protected]>

Improve node name handling, unit test fixes, more

87948a8

Distro A; OPSEC #4584 Signed-off-by: Roger Strain <[email protected]>

Add node prefix to command line remaps

2f4937d

Distro A; OPSEC #4584 Signed-off-by: Roger Strain <[email protected]>

Move NodeActionExtension to descriptions/Node.py

b38ea73

Distro A, OPSEC #4584. You may have additional rights; please see https://rosmilitary.org/faq/?category=ros-2-license Signed-off-by: matthew.lanting <[email protected]>

Use node_description and ros_executable instead of node_action.

4be19e5

Distro A, OPSEC #4584. You may have additional rights; please see https://rosmilitary.org/faq/?category=ros-2-license Signed-off-by: matthew.lanting <[email protected]>

Updated function signature and accompanying documentation.

9ad74b5

Distro A, OPSEC #4584. You may have additional rights; please see https://rosmilitary.org/faq/?category=ros-2-license Signed-off-by: matthew.lanting <[email protected]>

WIP Fixes for failing tests.

f1ae020

Distro A, OPSEC #4584. You may have additional rights; please see https://rosmilitary.org/faq/?category=ros-2-license Signed-off-by: matthew.lanting <[email protected]>

Change "ros_params" string to "global_params" when getting params from

5f4ac9f

context in descriptions/node.py. Distro A, OPSEC #4584. You may have additional rights; please see https://rosmilitary.org/faq/?category=ros-2-license Signed-off-by: matthew.lanting <[email protected]>

More unit test fixes.

1bb350e

Distro A, OPSEC #4584. You may have additional rights; please see https://rosmilitary.org/faq/?category=ros-2-license Signed-off-by: matthew.lanting <[email protected]>

More unit test fixes.

29788c4

Distro A, OPSEC #4584. You may have additional rights; please see https://rosmilitary.org/faq/?category=ros-2-license Signed-off-by: matthew.lanting <[email protected]>

Increase tolerance for timer tests from 0.1 seconds to 0.5 seconds

43f1912

Distro A, OPSEC #4584. You may have additional rights; please see https://rosmilitary.org/faq/?category=ros-2-license Signed-off-by: matthew.lanting <[email protected]>

removed license language to match aggreed upon format

b9531dd

Distro A, OPSEC #4584. You may have additional rights; please see https://rosmilitary.org/faq/?category=ros-2-license Signed-off-by: matthew.lanting <[email protected]>

Move Executable specific keyword args to kwargs instead of duplicating

d5b83d5

parameters between classes. Distro A, OPSEC #4584. You may have additional rights; please see https://rosmilitary.org/faq/?category=ros-2-license Signed-off-by: matthew.lanting <[email protected]>

Remove 'node_' prefix from members and arguments.

4b45bc5

Distro A, OPSEC #4584. You may have additional rights; please see https://rosmilitary.org/faq/?category=ros-2-license Signed-off-by: matthew.lanting <[email protected]>

extend instead of assign expanded_parameter_arguments in tests.

846295c

Distro A, OPSEC #4584. You may have additional rights; please see https://rosmilitary.org/faq/?category=ros-2-license Signed-off-by: matthew.lanting <[email protected]>

Resolve SelectableGroups deprecation warning, using approach used to r…

32409af

…esolve ros2/ros2#1254 by clalancette. Signed-off-by: veronica.knisley <[email protected]>

Resolve arguments typing issue

6e95e60

Signed-off-by: veronica.knisley <[email protected]>

vknisley-swri force-pushed the execute-process-refactor branch from cac239f to 6e95e60 Compare December 15, 2023 22:43

	def prepare(self, node, context: LaunchContext, action: Action):
	def prepare(self, node: Node, context: LaunchContext, action: Action):

	self.__ros_exec = RosExecutable(package=package, executable=executable,
	self.__executable = RosExecutable(package=package, executable=executable,

		from ..remap_rule_type import SomeRemapRules


		class NodeActionExtension:

Execute process refactor #215

Are you sure you want to change the base?

Execute process refactor #215

Conversation

roger-strain commented Jan 25, 2021

roger-strain commented Jan 25, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hidmic commented Jan 27, 2021

hidmic commented Jan 27, 2021

roger-strain commented Feb 9, 2021

roger-strain commented Feb 25, 2021

hidmic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roger-strain Mar 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roger-strain commented Mar 22, 2021

roger-strain commented Mar 24, 2021

roger-strain commented Apr 2, 2021

mlanting commented Nov 22, 2021

clalancette commented Nov 29, 2021

hidmic commented Dec 6, 2021

hidmic commented Dec 6, 2021

hidmic commented Dec 23, 2021 • edited Loading

mlanting commented Jan 6, 2022 • edited Loading

mlanting commented Jan 20, 2022

mlanting commented Jan 26, 2022

mlanting commented Feb 11, 2022

hidmic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clalancette commented Jul 13, 2022

mlanting commented Jul 13, 2022

clalancette commented Jul 14, 2022

roger-strain commented Oct 7, 2022

vknisley-swri commented Nov 6, 2023

vknisley-swri commented Dec 15, 2023

roger-strain Mar 26, 2021 •

edited

Loading

hidmic commented Dec 23, 2021 •

edited

Loading

mlanting commented Jan 6, 2022 •

edited

Loading