Why does this need to be part of the Node action API? Doesn't it only need to be on a LifecycleNode action?

It is mostly so that it doesn't crash if someone uses it. is_lifecycle_node is set to false for this node type. The launch_ros.Node doesn't support the autostart usage. Its ignored.

I could remove it if you preferred. I just didn't want people to get stuck with these kinds of traces when the other 2 node types support it:

Traceback (most recent call last):
  File "/root/jazzy_ws/./src/launch_ros/launch_ros/examples/lifecycle_autostart_pub_sub_launch.py", line 55, in <module>
    main()
  File "/root/jazzy_ws/./src/launch_ros/launch_ros/examples/lifecycle_autostart_pub_sub_launch.py", line 33, in main
    talker_node = launch_ros.actions.Node(
                  ^^^^^^^^^^^^^^^^^^^^^^^^
  File "/root/jazzy_ws/./src/launch_ros/launch_ros/examples/../launch_ros/actions/node.py", line 221, in __init__
    super().__init__(cmd=cmd, **kwargs)
  File "/opt/ros/rolling/lib/python3.12/site-packages/launch/actions/execute_process.py", line 240, in __init__
    super().__init__(process_description=executable, **kwargs)
  File "/opt/ros/rolling/lib/python3.12/site-packages/launch/actions/execute_local.py", line 188, in __init__
    super().__init__(**kwargs)
TypeError: Action.__init__() got an unexpected keyword argument 'autostart'

I mean, it makes sense to me that a standard Node doesn't expect an autostart keyword. I would prefer it not to arguments it never uses.

I think it would be ok to check for the argument and raise a better error if you think it would help too.

Same comment as for Node action.

Composable nodes can be also lifecycle nodes -- see Nav2 :-)

This is actually the main application for which I'm opening this PR. Its very difficult to work with lifecycle-components in launch_ros. There's no technical way (before this) to transition up a component that's a lifecycle node within launch. The other events that are used by the launch_ros.LifecycleNode reject updates from other node types.

I guess I find it weird still that you'd pass an argument that may or may not be ignored. Wouldn't using the type system be good here, and having a composable lifecycle node type class to disambiguate it?

If we have a use case for a generic type that can be either a lifecycle node or something else, then maybe this is fine, but I believe you always know if a node is going to be lifecycle or not, otherwise you wouldn't pass autostart. Also, if you think the node is lifecycle, and so you pass autostart, but it actually isn't, wouldn't you want to catch that? Seems like a bug waiting to be missed.

Why does this need to be a SomeSubstitutionsType? Not saying it's wrong, just curious what use case you have in mind?

And further, I'm not sure this works because this would essentially be a type name as a string, but before this was a value not a type, and we passed that instance of the object in to the event as event.action. So maybe I do think this isn't right.

I needed this to not be specifically a LifecycleNode due to circular dependencies using this within LifecycleNode itself for the transitioning. So, to make sure that it is still a lifecycle-compliant node, I used the Optional type and checked that it was a proper instance of a lifecycle-compliant node using is_lifecycle_node (since components, lifecycle, and node in launch_ros can all be used to launch a rclcpp::LifecycleNode)

Im not sure this works

I can't comment confidently on the reasoning you have for it not working, but I can say from testing with the examples I provided that it does indeed work. I provided the testing samples here for that reason (and to augment the existing test samples of similar nature)

but before this was a value not a type

Un-confidently: Isn't LifecycleNode a type and Optional[SomeSubstitutionsType] a type? Is there something else you'd suggest I try here instead? I just used Optional[SomeSubstitutionsType] based on what I saw elsewhere in the codebase for similar purposes, but I won't claim that this is right -- only that it appears to work and looks consistent to my naive eye with the rest of the codebase. My objective was to remove the circular dependencies, I don't know what non-LifecycleNode I should put here instead if not this if you think its wrong.

- target_lifecycle_node: LifecycleNode = None,
+ target_lifecycle_node: Optional[SomeSubstitutionsType] = None,

Isn't LifecycleNode a type and Optional[SomeSubstitutionsType] a type?

I mean, yes, but that's like saying replacing rclcpp::Node with std::string in c++ is ok because they're both types...

If it's just an issue with circular dependencies then for the Type annotations you can just string quote the type, e.g.:

Or I think you can do something like this at the top of the file:

from typing import TYPE_CHECKING
if TYPE_CHECKING:
    from ... import LifecycleNode

See: https://stackoverflow.com/a/39757388

btw, didn't mean to come off snarky with the c++ comparison, but that is the sort of what's happening with the SomeSubstitutionsType, since that type essentially gets resolved into a string.

Might want to clarify here whether or not the name needs to be fully qualified or not.

This is copy+pasted from the lifecycle node abstracted into a manager to share with composition node. I don't really know one way or another, I'm not the original author. I could try to find out, but I'd prefer if this got grandfathered in as 'this was already here'

Well you may have copy-pasted it, but it's still important for you and the users to know what needs to be passed here. If you're only forwarding it to the other constructor, then maybe you can simply reference it from here rather than copy-pasting the explanation.

Again, if the user emits their own ChangeState pointing to their LifecycleNode action, then this will not match it right?

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:

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:

Here's an example that won't likely work:

That is expecting the actions objects to match...