-
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
New launch system #74
Conversation
I added an example in f353805 for anyone who wants to start reviewing. |
45e63bc added the tests I that I had time to clean up for this pull request. The more detailed actions still need proper automated tests, but are otherwise pretty well covered in demos I have to pull request later. I'll add expanding the test coverage to the list of new issues that need to be made as follow up work. |
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.
the
architecture.rst
document
Looks very good to me. 👍👍
Only one minor comment to clarify required arguments and a nitpick.
Does the current code come with any callables which can be passed to the OnProcessIO
handler to e.g. just print to stdout
? Since the example defines its own local function for that.
the use of type checking with Python's
typing
module andmypy
I haven't used mypy
before but the approach looks fine to me on a first look.
the example
Why is the example file called launch_counter_good_bad_ugly.py
? It looks pretty clean and intuitive to read or do you consider any part of it bad / ugly style?
- :class:`launch.actions.DeclareLaunchDescriptionArgument` | ||
|
||
- This action will declare a launch description argument, which can have a name, default value, and documentation. | ||
- The argument will be exposed via a command line option for a root launch description, or as action configurations to the include launch description action for the included launch description. |
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.
Can an argument be marked as "required"? So if it doesn't have a default value but isn't provided the invocation would fail.
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.
My plan was to make argument without default values required, I think this is how it works in roslaunch
: https://wiki.ros.org/roslaunch/XML/arg#Usecase_pattern
launch/doc/source/architecture.rst
Outdated
Substitutions | ||
------------- | ||
|
||
A substitution is a something that cannot, or should not, be evaluated until it's time to execute the launch description that they are used in. |
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 substitution is a something that cannot ...
The second "a" seems to be wrong?
Not right now, the reason is that I plan to integrate the "default" one from
Sorry, that's because there are three counters, one that behaves, one that ignores sigint (bad), and one that ignores sigint and sigterm (ugly). I can shorten the name. It was late when I made that file :) |
Output will likely be moved to an “OutputHandler” type class in the future.
Access to argv. Access to shutdown state (from LaunchService via LaunchContext). Ability to asynchronously add completion events. Add concept of globals to go with locals.
Note, I will create issues for the things listed at the end of the description of this issue after merging. |
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 done a partial review, mostly of the documentation. I can do some more on this on Thursday.
launch/doc/source/architecture.rst
Outdated
======================== | ||
|
||
`launch` is designed to provide core features like describing actions (e.g. executing a process or including another launch description), generating events, introspecting launch descriptions, and executing launch descriptions. | ||
While at the same time, it provides extension points so that the set of things on which these core features can operate on, or integrate with, can be expanded with additional packages. |
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.
Can probably drop the While
in the above sentence, as it is a bit redundant.
launch/doc/source/architecture.rst
Outdated
|
||
When visited, entities may yield additional entities to be visited, and this pattern is used from the "root" of the launch, where a special entity called :class:`launch.LaunchDescription` is provided to start the launch process. | ||
|
||
The :class:`launch.LaunchDescription` class encapsulates the intent of the user as a list of discrete :class:`launch.Action`'s (which are also derived from :class:`launch.LaunchDescriptionEntity`. |
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.
Close parens at the end here.
launch/doc/source/architecture.rst
Outdated
These substitutions are things which can be evaluated during launch and can be used to do various things like: get a launch configuration, get an environment variable, or evaluate arbitrary Python expressions. | ||
|
||
Launch descriptions, and the actions contained therein, can either be introspected directly or launched by a :class:`launch.LaunchService`. | ||
A launch service, is a long running activity which handles the event loop and dispatches actions. |
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.
No comma needed in this sentence.
^^^^^^^^^^^^^ | ||
|
||
`launch` provides the foundational actions on which other more sophisticated actions may be built. | ||
This is a non-exhaustive list of actions that `launch` may provide: |
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.
Thinking about this as a user, why is this a may provide
? It seems like this is a (non-exhaustive) list of things that launch
does provide, or am I misunderstanding?
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 also asked myself this question.
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.
Simply because I didn't want to have to come back here and update the list every time I added a trivial action to the list in launch
. These are the important ones (as I see it). This is just supposed to be an overview, not the complete API docs.
launch/doc/source/architecture.rst
Outdated
|
||
- :class:`launch.actions.GroupAction` | ||
|
||
- This action will yield other actions, but can associated with conditionals (allowing you to use the conditional on the group action rather than on each sub-action individually) and can optionally scope the launch configurations. |
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.
but can associated
-> but can be associated
launch/doc/source/architecture.rst
Outdated
|
||
- :class:`launch.actions.RegisterEventHandler` | ||
|
||
- This action will register a :class:`launch.EventHandler` class, which takes user defined lambda to handle some event. |
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.
which takes user defined lambda
-> which takes a user defined lambda
@@ -0,0 +1,79 @@ | |||
#!/usr/bin/env python3 | |||
|
|||
# Copyright 2015 Open Source Robotics Foundation, Inc. |
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.
2018, probably :).
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 one is based on the counter.py
in the test folder for the old launch, so I think 2015 is ok here.
@@ -0,0 +1,132 @@ | |||
#!/usr/bin/env python3 | |||
|
|||
# Copyright 2015 Open Source Robotics Foundation, Inc. |
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.
2018
# TODO(wjwwood) check that it is not only callable, but also a callable that matches | ||
# the correct signature for a handler in this case | ||
self.__on_exit = on_exit | ||
self.__actions_on_exit: List[LaunchDescriptionEntity] = [] |
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.
yes, that looks better. I added 89d39de to be more consistent with how the OnShutdown handler is described, in the case a handler is passed in.
""" | ||
super().__init__(**kwargs) | ||
self.__text = text | ||
self.__from_stdin = fd == 0 |
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 sys.stdin.fileno()
appropriate in place of just 0
here and e.g. https://github.com/ros2/launch/pull/74/files/165e928a4021689017683033d75bf16bbb43a3ad#diff-76180495de0daffea911889f8030444fR34? I know it really just has to match between the two, so it might not make sense.
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 don't think that's appropriate in this case, since the files uses for the stdout/stderr/stdin communication with the process are most often not sys.stdin.fileno()
. I thought about using an enum or a string, but decided to just use an int. I could change the mapping mechanism if you want, but using the fileno from sys doesn't seem like the right thing to do.
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.
OK 👍
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.
OK as is I mean.
launch/launch/launch_service.py
Outdated
_logger.warn(base_msg) | ||
self._shutdown(reason='ctrl-c (SIGINT)', due_to_sigint=True) | ||
sigint_received = True | ||
else: |
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 the opposite to what we've done in the previous launch
: https://github.com/ros2/launch/pull/58/files
I think you've mentioned before ROS 1 does a similar thing, escalating based on the number of interrupts received, is that right?
the reason it's relevant is because I press it twice by habit sometimes (which is what prompted that PR)
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.
The signal handling of the service needs more work, I also don't handle SIGTERM, which I probably should do in order to avoid zombie processes.
I'll spend some time on this right now, but at some point I'll need to just start making issues for things, otherwise this pr will never get merged in time.
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.
To me it's fine if this is one of the things that gets ticketed (it will be easier to review as an incremental improvement anyway). My intent is to point out what I see while reviewing (because otherwise we might forget), then we can make the judgement on them individually.
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.
For sure, didn't mean to sound like I was complaining, just saying I might fix it up some now and ticket other stuff for later, even if it ends up in the release as a follow and smaller change.
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 pushed some enhancements in 010edcb, one of which is that repeated ctrl-c does not exit early.
if default is None: | ||
self.__default = default | ||
else: | ||
# convert any items in default that are not a Substitution or str to a str |
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 want to note that it might trip users up that the default gets converted to a string underneath. It might be better to just enforce it's passed in as a string (if it's not a substitution) rather than doing it ourselves, so that a user that passes in a dictionary or something doesn't expect to be able to retrieve a dictionary later.
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 a trade-off, as it's convenient to pass literals or the result of expressions to the SetLaunchConfiguration
action directly. I can change 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.
I dunno, it's pretty convenient. I think I'd prefer to leave it.
If you want, I'll change it anyways, but what if I just improved the documentation a bit to be clear that the default must be a string in the end, so non-text/non-substitution things are converted to text implicitly?
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.
yeah I already clarified a bit in 760fe46#diff-55c61397a0c771113a1e78e3143e31fdR77. I can see the convenience, so let's leave it; maybe one day we will store dicts as dicts even.
cwd: Optional[SomeSubstitutionsType] = None, | ||
env: Optional[Dict[SomeSubstitutionsType, SomeSubstitutionsType]] = None, | ||
shell: bool = False, | ||
sigterm_timeout: SomeSubstitutionsType = LaunchConfiguration('sigterm_timeout', default=5), |
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 imagine you are setting the default via a LaunchConfiguration in order to retrieve its value when it's actually used (as opposed to just passing in a float). But is it actually possible that someone could modify it before then, externally? FWIU it gets evaluated here and we don't modify it internally, so maybe we can just pass it as a float?
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.
answering myself: it is modifiable externally in the same way the launch-prefix
configuration is.
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 imagine you are setting the default via a LaunchConfiguration in order to retrieve its value when it's actually used (as opposed to just passing in a float).
Yes.
But is it actually possible that someone could modify it before then, externally?
Yes, consider:
ld = LaunchDescription([
SetLaunchConfiguration('sigterm_timeout', 42.0),
ExecuteProcess(cmd=['ls']),
ExecuteProcess(
cmd=['ls'],
sigterm_timeout=EnvironmentVariable('CUSTOM_SIGTERM_TIMEOUT')),
ExecuteProcess(cmd=['ls', '-la']),
# ExecuteProcess(cmd=['ls', '-la'], sigterm_timeout=1.0),
])
The first and third execute processes would use the timeout set in the launch configuration, but the second would not because it was explicitly passed and the fourth (commented out) would be a nice to have interface where sigterm_timeout
could also take a non-text/non-substitution numeric, but it's not necessary.
FWIU it gets evaluated here and we don't modify it internally, so maybe we can just pass it as a float?
Not exactly, the substitutions that make up sigterm_timeout
are passed to the TimerAction
there, but they are not substituted until the TimerAction
is visited, and the substitutions should not be evaluated until then I think. We couldn't expand the substitution there even if we wanted to, because when called via describe, it would not have a LaunchContext
which is required to perform 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.
yep, it doesn't get evaluated until the TimerAction is visited/executed: https://github.com/ros2/launch/pull/74/files#diff-a40b801740c81b57ba0a59a56cc0d36fR79.
I documented this behaviour in f3417d4
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.
Looks alright to me!
Thanks for the review @dhood! Here's some CI (with |
OK, looks good, I just have some small comments. I'm happy to open a followup PR if you agree with them. It's fine that there's no this message should instead say the function needs to be called from the main thread, right? Did it used to get called on module import? the use of "exception" in this text makes it sound like their handler won't be called if it raises a keyboard interrupt, where it actually will still be called. |
There's no else because I believe
That's right, it started out as something I was doing on import and decided to change it to a function.
Well there's not way of knowing if their handler will raise |
Please re-read: I am referring to custom signal handlers.
Just removing the first part of the sentence is enough to clarify. again, I'm happy to do it, but there wasn't an open PR. now I'll build on 77 |
Sorry, I thought you meant original handler since that's the line you linked to (117 is I'd check it in the |
But you're right, your text makes total sense. I didn't read it closely enough. |
yeah that was what I mentioned. I will do that; wasn't trying to create more work for you, just to get approval of the direction/intent :) |
This is the first pull request in a series, which adds the first pass at the reference implementation described in ros2/design#163.
I'm still working on this pull request, but some highlights to review in the mean time:
architecture.rst
document: https://github.com/ros2/launch/pull/74/files#diff-e25d31144ab0a51cf4d41fa107f6c9c3typing
module andmypy
mypy
, and I'd like feedback on thatExecuteProcess
is currently modeled afterroslaunch
, but could be refactored to be likelaunch
was, or something else entirelyWhat's known to be missing from this pr (and forth coming as soon as I can redo the commits) are tests and some example usage.
In a follow up pull request, I will add the
launch_ros
package and some demos using it, as well as theros2launch
package.Things to open issues about after merging:
ensure_argument_type()
, see: New launch system #74 (comment)