-
Notifications
You must be signed in to change notification settings - Fork 79
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
Autostarting lifecycle nodes and example launch file demo #430
base: rolling
Are you sure you want to change the base?
Conversation
Signed-off-by: Steve Macenski <[email protected]>
Signed-off-by: Steve Macenski <[email protected]>
Signed-off-by: Steve Macenski <[email protected]>
Signed-off-by: Steve Macenski <[email protected]>
Signed-off-by: Steve Macenski <[email protected]>
Signed-off-by: Steve Macenski <[email protected]>
Signed-off-by: Steve Macenski <[email protected]>
Signed-off-by: Steve Macenski <[email protected]>
@mjcarroll can I get a review on this now that we're back from the holidays? :-) |
That would be amazing to have this part of the standard launch! |
@mjcarroll can I request a review? ❤️ |
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 few questions, but looking pretty good. Thanks for the contribution!
|
||
def _on_change_state_event(self, context: launch.LaunchContext) -> None: | ||
typed_event = cast(ChangeState, context.locals.event) | ||
if not typed_event.lifecycle_node_matcher(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.
Again, if the user emits their own ChangeState
pointing to their LifecycleNode
action, then this will not match it right?
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 copy+pasted from the lifecycle node abstracted into a manager to share with composition node. I'd prefer not to make functional changes in this PR if that's OK with you. This has been the state of things previously and just reorganized to be shared among multiple node types
But w.r.t. this comment: No, actually! The subscription keeps them aligned:
launch_ros/launch_ros/launch_ros/utilities/lifecycle_event_manager.py
Lines 127 to 131 in 84c44de
self.__rclpy_subscription = node.create_subscription( | |
lifecycle_msgs.msg.TransitionEvent, | |
'{}/transition_event'.format(self.__node_name), | |
functools.partial(self._on_transition_event, context), | |
10) |
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 copy+pasted from the lifecycle node abstracted into a manager to share with composition node. I'd prefer not to make functional changes in this PR if that's OK with you. This has been the state of things previously and just reorganized to be shared among multiple node types
I get where you're coming from, but in this case your copy-paste is into a different class with a different type where you also use self
, so you're essentially changing the behavior even though it's a copy-paste because of that.
I don't see how the subscriptions keep them aligned? The typed_event.lifecycle_node_matcher
is user defined and may not look at the subscription at all. Plus the matcher explicitly expects a LifecycleNode
here not the type of self in your copy-pasted code:
def lifecycle_node_matcher(self) -> Callable[['LifecycleNode'], bool]: |
Here's an example that won't likely work:
lifecycle_node_matcher=launch.events.matches_action(talker_node), |
That is expecting the actions objects to match...
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 override the __eq__
operator so that the lifecycle node object is used for those comparisons :-)
def __eq__(self, other):
return self.__lifecycle_node == other
After the changes in the other comments, I tried all of the launch examples again and they all work (including setting autostart to false, true, and removed):
Node via pub_sub_launch.py
:
- No autostart, gives error if one is given
Lifecycle node via lifecycle_pub_sub_launch.py
with external triggers:
- The registered event handlers and event emiting works properly, nodes come up as expected
- I think this was your concern, this does work.
Lifecycle nodes via lifecycle_autostart_pub_sub_launch.py
with internal autostart:
- Autostart = true, autostarts and pub/sub is logged. Autostart logging is seen.
- Autostart=false, system comes up, no pub/sub logging. Autostart logging is not seen.
- Autostart not set, system comes up, no pub/sub logging. Autostart logging is not seen.
Lifecycle component nodes via lifecycle_component_autostart_pub_sub_launch.py
with internal autostart:
- Autostart=true, Autostart logging is seen.
- Autostart=false, Autostart logging is not seen.
- Autostart not set, Autostart logging is not seen.
Note for the last one lifecycle_component_autostart_pub_sub_launch.py
, the components themselves are not lifecycle enabled, so the logging is used to validate that the autostart keyword is parsed and will setup the transition. They end up publishing / subscribing regardless of autostart since they're not actually lifecycle nodes at all. Unfortunately the lifecycle listener / talker demos are not components for me to use, so I have to use the components that we have. I did test and do debug logging to make sure this is correct and I'm confident that it 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 override the eq operator so that the lifecycle node object is used for those comparisons :-)
That's kind of my point, which is that the type annotations are indicating that you should expect something different. It just so happens that the matchers we use only do ==
, but a user sees it takes a LifecycleNode
and may think they can call anything available on that class in their matcher, but that's not the case because you're passing a different class and in one case making it appear to be correct. It would be better to just pass the correct class, or have the new class actually inherit from LifecycleNode
, or actually change the API of the callback for matchers so the false expectation is avoided.
My preference is for one of the first two options honestly. Trying to pass off this new class as if it were a LifecycleNode
seems a bit too clever to me. It's gonna be surprising and confusing to someone at some point, in my opinion.
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 did this to avoid making other changes to matchers. I actually did this originally but backed that out because the launch.events
matcher [1] compares the nodes rather than any specific attribute. The existing lifecycle pub/sub launch example uses that matcher and would break the behavior if ==
didn't match. Since launch.events
is pretty core, I didn't want to introduce any change that would break that behavior, even though the launch_ros.events
versions work perfectly for us [2]. The launch_ros version honestly seem better to me by being less fragile based on attributes rather than comparing full objects. However anyone using the launch.events
version rather than the launch_ros.events
version would see a break, so I implemented the __eq__
to make them both continue to work as expected.
[1] https://github.com/ros2/launch/blob/rolling/launch/launch/events/matchers.py#L22-L24
[2] https://github.com/ros2/launch_ros/blob/rolling/launch_ros/launch_ros/events/matchers/matches_node_name.py#L25-L29
It would be better to just pass the correct class, or have the new class actually inherit from LifecycleNode ... My preference is for one of the first two options honestly.
I'm always open for critique - I don't really understand the suggestion though, can you clarify your vision for: "pass the right class"?
I odn't think the new class is good since it would make users have to use a LifecycleAutostartingNode
object just to obtain the autostart
field, which is unnatural and seems confusing. Plus, it would require me to duplicate code. I'm on board with the semantic difference between ComposableLifecycleNode
and ComposableNode
, but having a second LifecycleNode
for a feature that is intrinsically linked to lifecycle nodes is strange.
Trying to pass off this new class as if it were a LifecycleNode seems a bit too clever to me. It's gonna be surprising and confusing to someone at some point, in my opinion.
Pending on understanding your point about "pass the right class", I think what is currently here is a better solution. Its a little clever, but not really much so, I don't think. Especially if documented (which I can improve) and keeps all matchers working
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 really understand the suggestion though, can you clarify your vision for: "pass the right class"?
You already have a new class, LifecycleEventManager
, which you're passing on this line as self
. My point was that you should either pass self.__lifecycle_node
here or have LifecycleEventManager
inherit from LifecycleNode
since that is the type that the matcher is expecting. That way you don't need to override the ==
behavior and the matcher is getting what it expects and future changes, that might do more than ==
, will continue to 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.
Naively, this is a concrete change that would satisfy me here:
if not typed_event.lifecycle_node_matcher(self): | |
if not typed_event.lifecycle_node_matcher(self.__lifecycle_node): |
Though that might not work for some other reason, in which case you might have to do the inheritance.
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.
Well, I guess not, because if there's something in the LifecycleEventManager
class that you need in the matcher, you could just do lifecycle_node.__lifecycle_event_manager
(which btw there should probably be an accessor for the event manager from the lifecycle node).
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 already have a new class, LifecycleEventManager, which you're passing on this line as self. My point was that you should either pass self.__lifecycle_node here
This is another obvious simplicity from heritage decisions where this started by only passing in the lifecycle node's name, not the object itself. Now that we have the object, that is exactly the right thing to do for the case of _on_change_state_event
. I'll do that in a commit since there's absolutely no good reason not to do that.
However, that doesn't address the issue of the launch.event
matcher which uses the ==
operator - which can be defined externally by a user. For example in the lifecycle_pub_sub_launch.py
demo example. I don't actually think (?) that deriving from the lifecycle node class would solve that issue either, since the LifecycleEventManager
would then be of type LifecycleNode
, but not the same instance of it.
Edit: Actually, I just found a way around that. I can just pass in the lifecycle node into the state transition too StateTransition(action=self.__lifecycle_node, msg=msg)
.
This is done 😎
Personally, I think the launch.event
matcher is extremely fragile. I feel like having the actions be required to have a name
field which could be autogenerated as a hash if not provided would be better.
which btw there should probably be an accessor for the event manager from the lifecycle node
ACKed and done
Signed-off-by: Steve Macenski <[email protected]>
@wjwwood I addressed all of your comments and made a fix by your review requests |
I sent a few responses, resolved a few others. Sorry it took so long to reply, I'll try to be quicker on the next iteration. Thanks again! |
Signed-off-by: Steve Macenski <[email protected]>
Signed-off-by: Steve Macenski <[email protected]>
Signed-off-by: Steve Macenski <[email protected]>
Signed-off-by: Steve Macenski <[email protected]>
I appreciate it, I know this isn't the most active project in the ROS core ecosystem, so I appreciate the attention here :-) I think I've completed all of the tasks that need to be addressed |
Sorry, one more iteration on the matchers. |
See updated comment :-) |
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 really close
I continued our conversation on the other thread, and pointed out a potential issue with how autostart is being typed and documented. Other than that, it's mostly cleanup requests after your most recent set of changes.
@@ -43,6 +39,7 @@ def __init__( | |||
*, | |||
name: SomeSubstitutionsType, | |||
namespace: SomeSubstitutionsType, | |||
autostart: Optional[bool] = False, |
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 a docstring entry about this parameter here would be helpful. It's not clear to me why it's optional and what happens when it's 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.
Done for docstring, I remove None optionality
|
||
# If autostart is enabled, transition to the 'active' state. | ||
autostart_actions = None | ||
if self.node_autostart: |
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.
Do you need to have a condition here for when autostart is None
? Currently this condition is implicitly like if self.node_autostart is not None and self.node_autostart is True
, which is maybe what you want, but also then why can it ever be 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.
I remove None optionality
@property | ||
def node_autostart(self): | ||
"""Getter for autostart.""" | ||
return False | ||
|
||
@property | ||
def is_lifecycle_node(self): | ||
return False | ||
|
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 this should be added to Node
either. It's a bad smell for lifecycle to be mentioned in the Node class at all. If we wanted to add another kind of node in the future we don't want to have to change the node class each time. Can we find a way to not need these two functions on node? This seems like a job for the type system.
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.
Done 👍
@@ -17,6 +17,7 @@ | |||
from typing import List | |||
from typing import Optional | |||
|
|||
import launch |
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 this is needed anymore.
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
@@ -29,6 +30,7 @@ | |||
from launch_ros.parameters_type import SomeParameters | |||
from launch_ros.remap_rule_type import RemapRules | |||
from launch_ros.remap_rule_type import SomeRemapRules | |||
from launch_ros.utilities import LifecycleEventManager |
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 this is needed anymore.
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
@property | ||
def node_autostart(self): | ||
"""Getter for autostart.""" | ||
return False | ||
|
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 would be great to avoid this addition too.
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.
Done 👍
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.
Sorry, after re-reviewing the new class you recently added I found some more things that should be looked into.
package: SomeSubstitutionsType, | ||
plugin: SomeSubstitutionsType, | ||
name: Optional[SomeSubstitutionsType] = None, | ||
namespace: Optional[SomeSubstitutionsType] = None, | ||
parameters: Optional[SomeParameters] = None, | ||
autostart: Optional[bool] = False, | ||
remappings: Optional[SomeRemapRules] = None, | ||
extra_arguments: Optional[SomeParameters] = None, | ||
condition: Optional[Condition] = 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.
You can just forward these arguments to the ComposableNode class, only adding autostart
, instead of redeclaring them here. This is also more robust to future changes as new arguments to ComposableNode
will automatically be accepted here too.
You can use kwargs
to do this. Take a look at how LifecycleNode
is done. It duplicates a few parameters that it needs to use, which is fine, but pass the rest along entirely in kwargs
.
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, done!
@property | ||
def package(self) -> List[Substitution]: | ||
"""Get node package name as a sequence of substitutions to be performed.""" | ||
return self._ComposableNode__package |
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.
We should try to use accessors here. Rather than reach around the public/private name mangling.
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.
Totally agree 👍 Done
|
||
self.__autostart = autostart | ||
self.__lifecycle_event_manager = None | ||
self.__node_name = self._ComposableNode__node_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.
Same here, use an accessor. Make one if there's not one.
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.
Totally agree 👍 Done
# If autostart is enabled, transition to the 'active' state. | ||
if node_description.node_autostart: | ||
complete_node_name = request.node_namespace + request.node_name | ||
if not complete_node_name.startswith('/'): | ||
complete_node_name = '/' + complete_node_name | ||
self.__logger.info( | ||
'Autostart enabled for requested lifecycle node {}'.format(complete_node_name)) | ||
node_description.init_lifecycle_event_manager(node_description, context) | ||
autostart_actions.append( | ||
LifecycleTransition( | ||
lifecycle_node_names=[complete_node_name], | ||
transition_ids=[lifecycle_msgs.msg.Transition.TRANSITION_CONFIGURE] | ||
)) | ||
autostart_actions.append( | ||
LifecycleTransition( | ||
lifecycle_node_names=[complete_node_name], | ||
transition_ids=[lifecycle_msgs.msg.Transition.TRANSITION_ACTIVATE] | ||
), | ||
) | ||
|
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 won't block my review on this, but I wanted to point out that adding this lifecycle specific logic here isn't ideal. Instead, it would be great to have something like on_execute(context) -> List[Action]
on ComposableNode
, calling that for each node_description
and accumulating the actions returned to be later returned at the end of this function as you're doing with autostart_actions
but generically.
Then the on_execute
in ComposableNode
could be empty for now, and all of this highlighted logic could be moved into LifecycleComposableNode
's override of on_execute()
. That way this class never has to understand anything about lifecycle nodes.
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 disagree it would nicely bin the lifecycle logic in discrete places, but the composable node / lifecycle composable nodes are both descriptions not actions. I think it would be against the design intent of the object to have execution-related methods like that.
So, I think this can be left as-is?
complete_node_name = '/' + complete_node_name | ||
self.__logger.info( | ||
'Autostart enabled for requested lifecycle node {}'.format(complete_node_name)) | ||
node_description.init_lifecycle_event_manager(node_description, 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.
This is related to my comment about an on_execute
"virtual" function on the composable node classes, but it's weird to be passing itself into this function. I think this could just be:
node_description.init_lifecycle_event_manager(node_description, context) | |
node_description.init_lifecycle_event_manager(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.
I agree, this is an artifact of moving the logic from lifecycle node to the composition node.
Completed
def init_lifecycle_event_manager(self, node, context: launch.LaunchContext) -> None: | ||
# LifecycleEventManager needs a pre-substitution node name | ||
self.__node_name = perform_substitutions(context, node.node_name) | ||
self.__lifecycle_event_manager = LifecycleEventManager(node) | ||
self.__lifecycle_event_manager.setup_lifecycle_manager(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.
Following up on my previous comment, this could just be:
def init_lifecycle_event_manager(self, node, context: launch.LaunchContext) -> None: | |
# LifecycleEventManager needs a pre-substitution node name | |
self.__node_name = perform_substitutions(context, node.node_name) | |
self.__lifecycle_event_manager = LifecycleEventManager(node) | |
self.__lifecycle_event_manager.setup_lifecycle_manager(context) | |
def init_lifecycle_event_manager(self, context: launch.LaunchContext) -> None: | |
# LifecycleEventManager needs a pre-substitution node name | |
self.__lifecycle_event_manager = LifecycleEventManager(self) | |
self.__lifecycle_event_manager.setup_lifecycle_manager(context) |
Also, the node_name
line seems completely unnecessary? Either that or the line in the constructor of this class where you do self.__node_name = self._ComposableNode__node_name
isn't 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.
Done - actually did this change from the previous comment before I got down to this one 😆
Also, the node_name line seems completely unnecessary? Either that or the line in the constructor of this class where you do self.__node_name = self._ComposableNode__node_name isn't needed?
It is actually necessary. The node name at the start is a substitution and we need toperform the substitution at this point to get the actual value to pass into the lifecycle event manager.
Signed-off-by: Steve Macenski <[email protected]>
Signed-off-by: Steve Macenski <[email protected]>
Signed-off-by: Steve Macenski <[email protected]>
@wjwwood all of your comments have been addressed, I believe fully now. I see I must have missed a DCO sign off on my commits, but I really don't want to mess with the git commit history bc I know that'll cause you more trouble re-reviewing not knowing what exactly I just changed. As the CEO of my own company, I trust that I can just say I give myself permissions to do this. I can also fix DCO after you review and approve if you like. I just don't want to make your life hard. |
Signed-off-by: Steve Macenski <[email protected]>
This resolves #418 which implements autostarting lifecycle nodes. This is complete and ready for a review
You'll notice a couple of key changes worth explaining:
There is a new
is_lifecycle_node
attribute of the node and lifecycle nodes which is needed to remove a circular dependency on the LifecycleNode within theLifecycleTransition
class which only is type checking. I replace this type check with checking if a non-None object (1) has the attribute at all and (2) has a lifecycle attribute with appropriate logging between the difference of a non-lifecycle node being attempted vs a non-node. This has been tested as well to function using the demos.You will also notice that I refactored lifecycle node to have a separate util
LifecycleEventManager
, who handles the event emitting, handling, and topic listening for lifecycle nodes. This way, this can be used in lifecycle-components as well! No changes were made here except removing theself.__current_state
variable which was completely unused. TheLifecycleEventManager
is only created if the node requests autostart at Launch time and otherwise has no overhead nor exposes the interfaces/event handler/emitter if its a non-lifecycle node.The component nodes don't add a
/
before the node names with the namespaces. This messes with the node event matcher. I add in a leading/
so that theaction.node_name == node_name
which has the/
forcably applied in the node event matcherI tested
LifecycleNode
,ComposableNodeContainer
,LoadCompoableNodes
for all cases:autostart=True
autostart=False
, andautostart
field not supplied. You can also run any experiments you like using the 2 extra launch files I provide in the examples directory which shows all the features at work/