Enable implicit ROS startup by launch_ros actions. #128

hidmic · 2020-03-10T15:57:01Z

Addresses #120. This pull request drops the concept of a default launch description in lieu of a so called ROS adapter accessible from and managed by a launch context. Actions that make use of it can trigger implicit and idempotent instantiation of such adapter, though a user may do so explicitly.

jacobperron

Thanks for working on this :)

Feedback below.

ros2launch/ros2launch/api/api.py

launch_ros/launch_ros/ros_adapters.py

jacobperron · 2020-03-10T17:01:15Z

launch_ros/launch_ros/actions/explicit_ros_startup.py

+import rclpy
+
+
+class ExplicitROSStartup(launch.actions.OpaqueFunction):


I'm guessing that the purpose of this action is to allow users to initialize the ROS adapter. But, I don't think we should encourage users to use the launch ROS context (or node) directly. It is more of an implementation detail of launch_ros. It could also lead to misuse (e.g. I can imagine a user passing the node to their own executor).

I think it makes more sense for users to create their own nodes and manage their own contexts if they need them, and it doesn't involve too much overhead.

Maybe someone else can weigh in here.

👍, keep the launch_ros context and node hidden, let the user create their own if they want to use ROS for something.

I can see how this could lead to misuse, yeah. But on the other hand, we'd be disallowing any form of ROS context configuration, which was previously possible via ROSSpecificLaunchStartup. So I'd be personally inclined towards simply documenting this properly.

One last thought. It may prove useful to allow users to create nodes and have launch servicing them and handling their lifetime.

One last thought. It may prove useful to allow users to create nodes and have launch servicing them and handling their lifetime.

We could provide that, but I think that's a different use case than this class. This class is useful (imo) for other actions to build on. If the we wanted to share something with the user and manage the lifetime for them, then we need a way to know when they are done with it and a way to give them access to the node and context we created. Though there is a lot of overlap, it seems like a different use case to me.

Having the ability to configure the context seems compelling to me, but I wonder if there are other ways to allow that without exposing this to the user.

Following @wjwwood rationale, if users won't be making use of this class for their own purposes then there's no need for the context to be configurable. See f5fd17b.

ivanpauno

@hidmic why is ExplicitROSStartup needed?
IMO, I would just provide implicit startup, though I don't mind strongly.

I've left some other comments.

ivanpauno · 2020-03-10T16:02:35Z

launch_ros/launch_ros/actions/explicit_ros_startup.py

+import rclpy
+
+
+class ExplicitROSStartup(launch.actions.OpaqueFunction):


why inheriting from OpaqueFunction? why just not inheriting from Action and overriding the execute method?

Mimic'd ROSSpecificLaunchStartup structure. Don't mind either way.

Action dropped in f5fd17b.

ivanpauno · 2020-03-10T18:39:29Z

launch_ros/launch_ros/actions/lifecycle_node.py

        # Create a subscription to monitor the state changes of the subprocess.
-        self.__rclpy_subscription = context.locals.launch_ros_node.create_subscription(
+        self.__rclpy_subscription = node.create_subscription(


What if ExplicitROSStartup wasn't executed before?

Then get_ros_node will ensure node comes into existence.

wjwwood · 2020-03-10T20:53:26Z

launch_ros/launch_ros/ros_adapters.py

+        context.register_event_handler(launch.event_handlers.OnShutdown(
+            on_shutdown=lambda *args, **kwargs: ros_adapter.shutdown()
+        ))
+    return context.locals['ros_adapter']


Should we consider making this thread-safe? I'm just imaging some action creating a thread and trying to access this from the thread and another action accessing it from the asyncio run loop in a different thread, at the same time.

Hmm, I had the impression launch wasn't thread-safe i.e. that using the same launch context across asyncio loops in separate threads wasn't supported. We can lock it otherwise.

Parts of it are, if we think a function may be called outside of the asyncio loop (like from a user thread) then it should be thread-safe. I'm not talking about calling the run loop for asyncio in multiple threads concurrently, that is definitely not supported.

I see. The thing is that something like:

I'm just imaging some action creating a thread and trying to access this from the thread and another action accessing it from the asyncio run loop in a different thread, at the same time.

won't work properly because launch.Context itself isn't thread-safe e.g. one cannot emit an event or affect context globals or locals in a thread-safe manner. So I'd rather push for that in a follow-up PR i.e. make launch.Context thread-safe and then make this function thread-safe. WDYT?

The context is not thread-safe, but things that use it are, e.g.: https://github.com/ros2/launch/blob/0d040023306843d71b6316b160fab7b219f41892/launch/launch/launch_service.py#L124-L144

I'd say I agree with you if the user or other actions were accessing the context directly, but they're not, they are calling this function which happens to use the context as an implementation detail.

I think it should still be thread-safe to be honest.

Why not just a lock in this function? Either a static (global) one or a one owned by this class?

Because of the reasons exposed above. Also, won't that result in a false sense of safety? There's very little inside launch that is actually thread-safe. And action authors can still use multiple threads by grabbing the node in their execute() method, so we're not blocking a use case.

Sorry I'll try to address the two points directly.

(a) two threads using two different launch.LaunchContext instances may contend with each other for no reason

Is that actually a problem? The scope and time bounds on this function are straightforward, so I don't think there's a deadlock problem. Are you concerned about performance due to lock contention?

(b) launch.LaunchContext global dict itself isn't thread-safe (which may be solved with such a lock, yes, but if and only if no other API that could potentially run concurrently attempts to do the same).

Of course if someone trys to access this fields in the context directly it may be an issue, but I thought the idea was to always access this field of the context (context.locals.ros_adapter) via this method right?

Do you mean issues when someone asynchronously calls this function while someone else concurrently accesses a different field in the context?

I'm happy to drop this if you don't think it's a concern, my concern is definitely academic for the moment.

Are you concerned about performance due to lock contention?

A bit of that, a bit of OCD :)

Do you mean issues when someone asynchronously calls this function while someone else concurrently accesses a different field in the context?

Precisely. In particular, launch.LaunchContext.extend_locals() and launch.LaunchContext.extend_globals() do not seem safe to use concurrently as per this Python 3.8 FAQ (see disclaimer at the bottom, due to __del__ being called as a side effect of dict.update). I give you that the chance of actually hitting a problem is (likely very) low though.

If you think there's no risk of this backfiring in the future, I'll add that lock.

We can leave it out. It would be nice to mention it explicitly in the docstring, as I feel this class is more likely to be used concurrently (like in the callback of a subscription or something) than other context functions.

Done in 63ff30f.

launch_ros/launch_ros/ros_adapters.py

jacobperron

LGTM

hidmic · 2020-03-12T14:24:14Z

CI up to test_launch_ros and test_communication to put this to test:

Linux (unrelated test failures for WString publishers using CycloneDDS)
Linux-aarch64
MacOS (unrelated test failures for WString publishers using CycloneDDS)
Windows (unrelated test failures for WString publishers using CycloneDDS)

ivanpauno · 2020-03-12T15:57:45Z

launch_testing_ros/launch_testing_ros/test_runner.py

@@ -19,19 +19,4 @@


 class LaunchTestRunner(launch_testing.test_runner.LaunchTestRunner):


We should consider deprecating launch_testing_ros

I thought about it, but I chose not to do that now both to keep this PR small and to not take that leap and remove a package we may need again afterwards.

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

hidmic · 2020-03-12T18:51:23Z

Had to rebase to solve conflicts, sorry folks!

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

hidmic · 2020-03-17T13:40:24Z

Alright, going in! Thank you all for the reviews!

launch_testing_ros/launch_testing_ros/test_runner.py

hidmic requested review from wjwwood, jacobperron and ivanpauno March 10, 2020 15:57

jacobperron reviewed Mar 10, 2020

View reviewed changes

ivanpauno requested changes Mar 10, 2020

View reviewed changes

wjwwood reviewed Mar 10, 2020

View reviewed changes

hidmic force-pushed the hidmic/implicit-startup branch from 577d77b to f5fd17b Compare March 11, 2020 17:24

jacobperron approved these changes Mar 11, 2020

View reviewed changes

hidmic requested a review from ivanpauno March 12, 2020 14:21

ivanpauno approved these changes Mar 12, 2020

View reviewed changes

hidmic added 5 commits March 12, 2020 15:49

Enable implicit ROS startup by launch_ros actions.

b68ed17

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

Drop all launch_ros.get_default_launch_description() uses.

8f20a0b

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

Please linters

207b16d

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

Address peer review comments.

f2f3cbb

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

Fix failing test_launch_ros tests.

5476e3d

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

hidmic force-pushed the hidmic/implicit-startup branch from 9c6f840 to 5476e3d Compare March 12, 2020 18:51

Document get_ros_node() & get_ros_adapter() thread-safety.

63ff30f

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

hidmic requested a review from wjwwood March 16, 2020 13:34

wjwwood approved these changes Mar 16, 2020

View reviewed changes

hidmic merged commit 8cc6df2 into master Mar 17, 2020

delete-merged-branch bot deleted the hidmic/implicit-startup branch March 17, 2020 13:40

dirk-thomas reviewed Mar 18, 2020

View reviewed changes

launch_testing_ros/launch_testing_ros/test_runner.py Show resolved Hide resolved

hidmic mentioned this pull request Mar 18, 2020

Remove unused 'launch' import. #133

Merged

This was referenced Mar 18, 2020

stop using constructors deprecated in pytest 5.4 ros2/launch#391

Merged

Avoid using ros_testing, and use launch_testing instead ros2/ros2cli#464

Closed

rotu mentioned this pull request Mar 19, 2020

test_cli.py misses launch_ros for rmw_cyclonedds_cpp ros2/ros2cli#456

Closed

ivanpauno mentioned this pull request Mar 24, 2020

Revisit injection of launch_ros description #120

Closed

sloretz mentioned this pull request Mar 27, 2020

Update tests to expect no launch_ros node ros2/ros2cli#474

Merged

mvoellmy mentioned this pull request Aug 20, 2020

Make launch file foxy compatible. ros-drivers/nmea_navsat_driver#110

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable implicit ROS startup by launch_ros actions. #128

Enable implicit ROS startup by launch_ros actions. #128

hidmic commented Mar 10, 2020 •

edited

Loading

jacobperron left a comment

jacobperron Mar 10, 2020

wjwwood Mar 10, 2020

hidmic Mar 10, 2020

wjwwood Mar 10, 2020

wjwwood Mar 10, 2020

hidmic Mar 11, 2020 •

edited

Loading

ivanpauno left a comment

ivanpauno Mar 10, 2020

hidmic Mar 10, 2020

hidmic Mar 11, 2020 •

edited

Loading

ivanpauno Mar 10, 2020

hidmic Mar 10, 2020

wjwwood Mar 10, 2020

hidmic Mar 10, 2020

wjwwood Mar 10, 2020

hidmic Mar 11, 2020

wjwwood Mar 11, 2020

hidmic Mar 12, 2020 •

edited

Loading

wjwwood Mar 12, 2020

hidmic Mar 13, 2020

wjwwood Mar 13, 2020

hidmic Mar 16, 2020

jacobperron left a comment

hidmic commented Mar 12, 2020 •

edited

Loading

ivanpauno Mar 12, 2020

hidmic Mar 12, 2020

hidmic commented Mar 12, 2020

hidmic commented Mar 17, 2020

		import rclpy


		class ExplicitROSStartup(launch.actions.OpaqueFunction):

		@@ -19,19 +19,4 @@


		class LaunchTestRunner(launch_testing.test_runner.LaunchTestRunner):

Enable implicit ROS startup by launch_ros actions. #128

Enable implicit ROS startup by launch_ros actions. #128

Conversation

hidmic commented Mar 10, 2020 • edited Loading

jacobperron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hidmic Mar 11, 2020 • edited Loading

Choose a reason for hiding this comment

ivanpauno left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hidmic Mar 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hidmic Mar 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacobperron left a comment

Choose a reason for hiding this comment

hidmic commented Mar 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hidmic commented Mar 12, 2020

hidmic commented Mar 17, 2020

hidmic commented Mar 10, 2020 •

edited

Loading

hidmic Mar 11, 2020 •

edited

Loading

hidmic Mar 11, 2020 •

edited

Loading

hidmic Mar 12, 2020 •

edited

Loading

hidmic commented Mar 12, 2020 •

edited

Loading