-
Notifications
You must be signed in to change notification settings - Fork 139
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adding Executable description class #454
Conversation
@ivanpauno @hidmic Getting started on the implementation of this. Wanted to just put a quick first cut of one class up first to make sure I get the process right. Looking forward to feedback and suggestions, especially with respect to testing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've left some minor comments, looks good to me.
from launch.utilities import perform_substitutions | ||
|
||
_global_process_counter_lock = threading.Lock() | ||
_global_process_counter = 0 # in Python3, this number is unbounded (no rollover) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is going to be a problem until ExecuteProcess
doesn't internally use this new description, as there is going to be two global counters (and thus, names may collide).
This does not need to be fixed now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've changed the name to a local-specific name regardless. There's no need for those to conflict.
process_event_args = self.__process_event_args | ||
if process_event_args is None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: this can be done in a single line
process_event_args = self.__process_event_args | |
if process_event_args is None: | |
if self.__process_event_args is None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this check even needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't seem like it. Also, as personal nit, I'd much rather have __expand_substitutions
return something to apply_context
instead of the former mutating state directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the identified check. __expand_substitutions
is still modifying the object, but it's doing is by adding newly-defined values instead of changing the existing ones. Not sure if it's really different enough for this concern, so if we want to continue tweaking it, please let me know. If we really would rather have a stateful object to pass around instead of changing this one, I think I can see the outlines of doing that, but I'm not sure if we're gaining much (other than complexity). Open to suggestions/thoughts/comments, of course.
|
||
def __init__( | ||
self, *, | ||
cmd: Iterable[SomeSubstitutionsType], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bit unrelated: maybe we should take advantage of this new description to fix:
- Change
cmd
parameter inExecuteProcess
#263 - Passing a string to ExecuteProcess arguments behaves unintuitively #394
?
Using a single string instead than a list of strings plays better with substitutions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've taken a stab at addressing this. Also added a few unit test cases to evaluate how it's handling the different cases described in #263 itself. I'd very much like to add more tests to make sure we're getting the expected results, so if you have any additional suggestions, I'd love to hear them.
with _global_process_counter_lock: | ||
global _global_process_counter | ||
_global_process_counter += 1 | ||
self.__name = '{}-{}'.format(name, _global_process_counter) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: use f-strings
I will create a branch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall it's looking good @roger-strain. I don't have much more to say on top of what @ivanpauno already pointed out.
process_event_args = self.__process_event_args | ||
if process_event_args is None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't seem like it. Also, as personal nit, I'd much rather have __expand_substitutions
return something to apply_context
instead of the former mutating state directly.
@property | ||
def name(self): | ||
"""Getter for name.""" | ||
return self.__name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In an unrelated (or maybe not quite unrelated) note, launch
and launch_ros
actions are inconsistent w.r.t how action properties interact with the action lifecycle:
launch.actions.ExecuteProcess.name
gets you a normalized substitution, always.launch_ros.actions.Node.node_name
raises if you try to get it before executing the action.
launch_ros.parameter_description.Parameter.name
, though arguably not an action, introduces yet another variation and gets you a normalized substitution before evaluating the parameter and the actual name after evaluating it.
@ivanpauno @wjwwood perhaps we should settle this here and stick to it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hidmic Would it be worth introducing more explicitly named parameters instead, to clarify which thing you're requesting? Maybe name
always returns the value as it was originally specified, without applying substitutions, and applied_name
returns the one with substitutions applied to it, raising an exception if substitutions haven't been performed yet?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hidmic Would it be worth introducing more explicitly named parameters instead, to clarify which thing you're requesting? Maybe name always returns the value as it was originally specified, without applying substitutions, and applied_name returns the one with substitutions applied to it, raising an exception if substitutions haven't been performed yet?
I think that's ok.
bikeshed: I'm not convinced about naming, maybe final_name
sounds better than applied_name
. I don't mind much.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've taken a stab at this in and updated. In reviewing how the objects were used, I don't think this was the right place to be creating the packed dictionary anyway, so I've removed that; it'll probably come back in ExecuteLocal
. For all the parameters which can have substitutions applied, there's now a "final_
" version which can be accessed. Right now, that isn't throwing an error if the substitutions haven't been applied, but I can make it do so if that's the standard approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's OK for now. It's not up to you to solve these discrepancies anyways.
7693958
to
8b9f068
Compare
@ivanpauno I've taken a first stab at trying to refactor the main execute_process class into the new execute_local. I spent quite a while trying to figure out how to write some unit tests to actually verify it's doing what it should, but in all honesty, this part of the system is still pretty unfamiliar to me, and I wasn't able to come up with any good samples to follow. The code itself is by and large copied straight from the original class, so I'm very happy to try to clean things up and improve them from this point, so any direction there would be appreciated, as well as suggestions on how to put some useful tests together. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me.
The only missing thing for merging is refactoring ExecuteProcess
to reuse ExecuteLocal
and Executable
. If all previous tests pass, I think this would be ready to merge.
It would also be great to have some direct testing of ExecuteLocal
(and more extended testing of Executable
)
cmd = shlex.split(cmd) | ||
self.__final_cmd = cmd |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you cannot handle both the "old style cmd" and the "new style cmd" in the same argument.
If you want, you can ignore this change and keep the old cmd
style, that can be changed later.
e.g.:
['cmd', TextSubstitution(text='my arg with spaces')]
was used like: ['cmd', 'my arg with spaces']
with the new logic, it should be used like: ['cmd', 'my', 'arg', 'with', 'spaces']
Changing the semantics of cmd
from a "iterable of substitutions" to a single substitution is a breaking change.
With the new semantics, the user must quote "my arg with spaces" if a single argument is desired.
So, it we want to keep backwards compatibility, we need two arguments in the constructor:
cmd: SomeSubstitutionsType,
cmd_old: Iterable[SomeSubstitutionsType]
Users would use cmd
directly, and we would used cmd_old
through ExecuteProcess
to keep backwards compatibility (and of course, some logic is needed in the constructor to ensure only one was passed).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in the interest of keeping this PR simpler, I'll err on the side of just retaining the old style for cmd
. I'd like to be able to help pick up the other issue as well, but it's probably better to let that be a separate task down the line.
launch/test/test_executable.py
Outdated
def test_cmd_multiple_arguments_in_string(): | ||
exe = Executable(cmd=['ls', '-opt1', '-opt2', '-opt3']) | ||
exe.apply_context(LaunchContext()) | ||
assert all([a == b for a, b in zip(exe.final_cmd, ['ls', '-opt1', '-opt2', '-opt3'])]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you could also have tests for final_name
, final_cwd
and final_env
.
Note: Linters aren't passing, check http://build.ros2.org/job/Rpr__launch__ubuntu_focal_amd64/82/. |
That's the plan for the next step, but I wanted to make sure things looked sane to this point first.
I would absolutely love to get more testing in place, but I'm kind of lost on how to best accomplish that. If there are some samples I could work from that show testing execution of things, I would really love to dig into that so I can apply it to this. Actually spinning up external processes as part of unit testing seems a little dicey, especially considering the variety of platforms the system should work on. If you've got any guidance, it'd be a huge help. |
@ivanpauno Pushed part of this a couple of weeks ago, and then commented on the wrong PR. Sorry about that. I think the unit tests are happy now; I'm still not 100% confident, but I'm not sure I ever will be. When you have a chance, give it a look over and see what you think. I'm going to take this out of draft status at this point. |
self.__stderr_buffer = io.StringIO() | ||
|
||
self.__executed = False | ||
self.__executable = Executable(cmd=cmd, prefix=prefix, name=name, cwd=cwd, env=env, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@roger-strain nit: it'd seem to me that self.__executable
is redundant with self.process_description
# Delivered to the U.S. Government with Unlimited Rights, as defined in DFARS | ||
# Part 252.227-7013 or 7014 (Feb 2014). | ||
# | ||
# This notice must appear in all copies of this file and its derivatives. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tfoote would you give this preamble a look? I'm not well versed in such licensing qualifications.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tfoote friendly ping
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@roger-strain Does this notice have to be on these files? It is much easier for us if this is just not there; then it looks the same as all of the rest of the licensing in our code and isn't a special snowflake. If it absolutely has to be there for some reason, I'm going to have to check with some others about whether this is allowed in this codebase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@clalancette I've asked back up my chain to revisit this. I will say that we tried to come up with something that would meet the requirements we were given, but also would be compatible with existing licensing here. That being said, we're looking again. Is there a specific part of it that's especially troublesome, or is the whole thing the issue? Might help to focus the discussions if I can target things appropriately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's really the whole thing. The basic problem is that we are not lawyers, and so that anything that deviates from the standard license text is a problem.
Please let me know what you decide. Once you've come to your decision, I'll pass it along to those more in the know on this side. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to follow up, we're still in active discussions about how to sort this out. Appreciate your patience.
def test_cmd_multiple_arguments_in_string(): | ||
exe = Executable(cmd=['ls', '-opt1', '-opt2', '-opt3']) | ||
exe.apply_context(LaunchContext()) | ||
assert all([a == b for a, b in zip(exe.final_cmd, ['ls', '-opt1', '-opt2', '-opt3'])]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@roger-strain tests for other arguments besides cmd
and/or using substitutions would be most welcome.
@property | ||
def name(self): | ||
"""Getter for name.""" | ||
return self.__name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's OK for now. It's not up to you to solve these discrepancies anyways.
def prepare(self, context: LaunchContext): | ||
"""Prepare the action for execution.""" | ||
self.__process_description.apply_context(context) | ||
self.__expand_substitutions(context) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@roger-strain meta, safe to ignore: I'm not a huge fan of methods with side effects like this. Purity is more expensive but less surprising.
@roger-strain I think we'll have a better grasp of the refactor once a somewhat larger portion of it lands. Are you looking forward to continue your contributions in the near future? |
@hidmic Yes, I'm hoping to get the next section of this next week. This is the only PR that will be targeting |
@roger-strain friendly ping |
Sorry, holidays caught me off guard and I'm now in the process of getting my laptop rebuilt. I believe I have the code finished for the next update on this and the first corresponding PR over in launch_ros, but I need to get more unit tests in place before I'm ready to officially push them up. Targeting later this week, assuming nothing else blows up on me in the process. |
bc27324
to
265ed45
Compare
@hidmic Finally finished a big chunk of work over in ros2/launch_ros#215, and updated this one to go alongside it. Please take a look and see how you think it's looking. Still need to go in and do the bits for Composable and Lifecycle nodes, and I'm sure there are other bits that I've missed, but hopefully this is pretty close to what you're looking for otherwise. |
@hidmic Some more updates over here; little bit of cleanup, and also part of the refactor discussed in ros2/launch_ros#215 regarding the proper place for arguments; they now are handled at the |
a408d92
to
7f21a4f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@roger-strain my since apologies for not coming back to this earlier. Too many things to do.
Nothing specifically added by this patch calls my attention in a negative way. Its PR job doesn't seem happy though. There's plenty of failing tests. Mind to take a look?
a601e00
to
dc7f80b
Compare
972082b
to
bc185af
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the bump @mlanting. Overall LGTM pending green CI
"""Describes an executable (usually a single process) which may be run by the launch system.""" | ||
|
||
def __init__( | ||
self, *, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mlanting this still applies.
:param cmd: A list where the first item is the executable and the rest are | ||
arguments to the executable, each item may be a string or a list of strings | ||
and Substitutions to be resolved at runtime | ||
:param: prefix a set of commands/arguments to preceed the cmd, used for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mlanting nit:
:param: prefix a set of commands/arguments to preceed the cmd, used for | |
:param prefix: a set of commands/arguments to preceed the cmd, used for |
: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. | ||
:param: arguments list of extra arguments for the executable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mlanting nit:
:param: arguments list of extra arguments for the executable | |
:param arguments: list of extra arguments for the executable |
@@ -45,15 +45,15 @@ class OnProcessExit(BaseEventHandler): | |||
def __init__( | |||
self, | |||
*, | |||
target_action: 'ExecuteProcess' = None, | |||
target_action: 'ExecuteLocal' = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This still applies. OK to defer though.
@mlanting the sole test failure is unrelated. Mind to address the nits? Then we can (finally) merge. |
@hidmic I made the suggested changes regarding the colon placement. The indentation in the launch/launch/descriptions/executable.py file seems correct to me, according to the PEP8 guidelines (https://www.python.org/dev/peps/pep-0008/#indentation) - since that is a hanging indent for function params, it should be indented an extra level. Given that, the lines in on_process_exit should be indented further as well, but I wanted to check which way you'd prefer before I changed it one way or the other. I'll defer the abstract Execute action to when I add in the remote launching classes. |
@mlanting you're right about PEP8 indentation, but I will point out that Linters are happy, but I'm somewhat inclined towards being consistent with the rest of the codebase.
@mlanting Agreed. BTW this needs a rebase! |
Part of implementation of refactor for ros2#114 Distro A; OPSEC #2893 Signed-off-by: Roger Strain <[email protected]>
Modified handling of cmd parameter as described in ros2#263 Distro A; OPSEC #2893 Signed-off-by: Roger Strain <[email protected]>
Distro A; OPSEC #4584 Signed-off-by: Roger Strain <[email protected]>
Distro A; OPSEC #4584 Signed-off-by: Roger Strain <[email protected]>
Distro A; OPSEC #4584 Signed-off-by: Roger Strain <[email protected]>
Distro A; OPSEC #4584 Signed-off-by: Roger Strain <[email protected]>
Distro A; OPSEC #4584 Signed-off-by: Roger Strain <[email protected]>
Distro A; OPSEC #4584 Signed-off-by: Roger Strain <[email protected]>
Distro A; OPSEC #4584 Signed-off-by: Roger Strain <[email protected]>
Distro A; OPSEC #4584 Signed-off-by: Roger Strain <[email protected]>
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]>
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]>
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]>
4effcb7
to
aa4d847
Compare
@hidmic I've fixed the indentation and done the rebase. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, I think this is good to go. Thank you for your patience @mlanting!
On a related note, when do you expect moving forward with the rest? Humble is still quite a few months away but it'd be great to land the full refactor @roger-strain intended to land before then.
I'm going to start moving forward with it immediately. Once this is merged, I'll start working on getting the launch_ros changes rebased so we can get those added, and then start work on the rest. |
_global_process_counter_lock = threading.Lock() | ||
_global_process_counter = 0 # in Python3, this number is unbounded (no rollover) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: these are unused after this PR, and could be deleted
_global_process_counter_lock = threading.Lock() | ||
_global_process_counter = 0 # in Python3, this number is unbounded (no rollover) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these globals are not being used anywhere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, I overlooked those. Thanks for pointing it out @ivanpauno.
@mlanting mind to fix it?
: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. | ||
:param arguments: list of extra arguments for the executable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this issue is quite old, but what's the purpose of this argument? Why could they not be part of the cmd
argument?
This is an initial commit toward the process described in ros2/design#272. It should not be merged to master yet, and probably needs a feature branch instead.
Haven't added unit testing yet, but also not as familiar as I'd like with the testing setup, so guidance toward getting appropriate tests would be greatly appreciated.
Part of implementation of refactor for #114
Distro A; OPSEC #2893
Signed-off-by: Roger Strain [email protected]