Allow optional scoping for including a launchfile

[SuperJappie08]

Feature request

IncludeLaunchDescription is not scoped if I understand correctly. (Based on #313 #689 and the design docs)

Feature description

An extra argument for IncludeLaunchDescription action, which could enable scoping.
This would prevent the leakage of arguments.

Implementation considerations

This is already possible by including into a group with forwarding set to false.
Default could be false to not break the current behavior.

EDIT: I made a proof of concept package https://github.com/SuperJappie08/launch_scoped_include/, which implements this on a new action called IncludeScopedLaunchDescription

EDIT2: I can make a pull request for this if the feature is desired

[wjwwood]

@SuperJappie08 thanks for the contribution, it's definitely something that others have run into and found surprising.

I really like the idea of a new Action, like ScopedIncludeLaunchDescription or similar, and I would be in favor of including it in this repository if you want.

Also, I really appreciate you making it into a stand-alone package as a first step. It definitely helps demonstrate the idea and value, and it makes it easier for folks to try it or use it, even in older releases of ROS 2.

[alsora]

Hi @SuperJappie08 any update on this?

[SuperJappie08]

Hi @SuperJappie08 any update on this?

Hi,

I was initially waiting on a response from a maintainer before spending a lot of time on this because I have a tendency to spend a lot of time on the wrong things.
However, some family stuff has come up recently, which required a shuffle in my priorities, so I have not had the time (yet) to start this.

I think I would be able to start this week.

[Timple]

it's definitely something that others have run into and found surprising.

Certainly!

So why not make this the default? Breaking changes are allowed between ROS releases.

I know breaking backward compatibility should not be considered lightly. But it's a trade-off between:

• new developers running into this surprising feature
• existing developers actually using this feature

The first group might actually be larger than the second.

[SuperJappie08]

it's definitely something that others have run into and found surprising.

Certainly!

So why not make this the default? Breaking changes are allowed between ROS releases.

I know breaking backward compatibility should not be considered lightly. But it's a trade-off between:

* new developers running into this surprising feature

* existing developers actually using this feature

The first group might actually be larger than the second.

Having it as a separate action, could mean the default include action could change.
But I think your point is valid, and it possibly could make sense to rename the old IncludeLaunchDescription to IncludeUnscopedLaunchDescription so the new scoped version could be the easier/shorted(/ more likely to be adopted as the default) name (IncludeLaunchDescription).

Normally, I would also cheer for backwards compatibility, but since the unscoped version feels counterintuitive to many people, it might make sense to fix it.

EDIT: Clarification

[SuperJappie08]

I found a fatal flaw after doing more testing on this feature with an implementation similar to my initial attempt in the package.

Currently, the 'scoped' is essentially a normal include wrapped in a scoped non-forwarding group, which only keeps the provided launch arguments.
This gives essentially the expected behavior from this feature, however, since only the launch argument to the nested files are kept in systems depending on non-user set launch configurations, such as the PushRosNameSpace and SetRemap actions from launch_ros.

My initial reaction was to attempt to include launch configurations which were not defined explicitly by the child file.
However, this requires a lot of rework to support multi-nesting situations.

Since not being able to do things, like name spacing a launch file, would not resolve the current non-scoping confusion behavior, I am unsure on how to continue (for now).

Does anyone have suggestions for a direction to move forward? (I will try to think of reasonable solutions as well)

EDIT: My current suggestion would be to pass all launch configurations except those explicitly defined in (recursively) included files. However, this does not seem a clear and understandable solution to users of the scoped include action

[SuperJappie08]

Another option could be to register overscoping launch configurations in the context, which will always be included.
This could be implemented as a launch configuration, which contains the keys of the unscopable launch configurations.

[SteveMacenski]

I'd love to have something like this in the package (and by default!) :-)

[SuperJappie08]

@alsora, an initial working implementation is complete (#841).

[ijnek]

This gives essentially the expected behavior from this feature, however, since only the launch argument to the nested files are kept in systems depending on non-user set launch configurations, such as the PushRosNameSpace and SetRemap actions from launch_ros.

So, I think the concerns are these launch configurations:

• ros_remaps
• ros_namespace
• global_params

I think a nice solution would be to have no launch configurations forwarded as done in this PR, and have a IncludeLaunchDescription (or something named a bit different to avoid confusion - like RosIncludeLaunchDescription), that inherits the ScopedIncludeLaunchDescription here, but auto-forwards the launch configurations listed above.

This can be thought of a similar concept to the launch_ros' Node class extending launch's ExecuteProcess.

[SuperJappie08]

@ijnek Thank you for the input.
That is an option I hadn't considered yet.

However, my concern is that a ROS specific RosIncludeLaunchDescription is less flexible, since in the current attempted implementation, other launch extensions (think a launch_system_nodes or something like a (non-existing) launch_gz) could just hook/register their own exclusions.
This would make combing extensions trivial, whereas with the alternative, this would get more complex.

However, it is a balancing act, since these exclusions should not be overused and are only practical/make sense in specific situations.

I'm unsure what the best way forward would be in this case, where it's a choice between flexibility vs. simplicity.

[ijnek]

Sorry — I completely missed your comment (#841 (comment)) mentioning that you had already worked around this issue using global launch configurations.

Personally, I find it odd that we use launch configurations to pass ros_remaps, ros_namespace, and global_params to included launch files. While looking into launch, it seems we could instead store remapping and namespacing info in the local stack rather than using launch configurations.

launch/launch/launch/launch_context.py

Lines 63 to 64 in 820e5a2

	self.__locals_stack: List[Dict[Text, Any]] = []
	self.__locals: Dict[Text, Any] = {}

I’ve drafted a PR to explore this idea: ros2/launch_ros#470.

This approach would avoid the need to track a global list of which launch configurations are meant to propagate. Let me know what you think.