Open tupiznak opened 3 years ago
cc @hidmic @sloretz
Yeah, you are right, this is not the best way to embed one executor
into another.
But as you said the ideal option is not possible since it requires changes inside the rclpy
library. Therefore I suggest a compromise. Thanks to such a hack, it became possible to use asyncio
inside the node code (for example, the use of await asyncio.sleep(1)
in the line 89
).
Globally it is a blocking code, but only one thread is blocked when it is receiving a command from a client. Everething else inside this is asynchronous.
In my current job I have to write asynchronous code. But due to the impossibility of using asyncio
inside the node, I had to reinvent the wheel and rewrite things like asyncio.sleep
, asyncio.gather
and so on. On top of that you can use pytest-asyncio
, but that's another story.
Sorry, I'm still not convinced of the value of the example. I will let the maintainers of the package decide if they want to merge the example or close.
But as you said the ideal option is not possible since it requires changes inside the rclpy library.
Thinking about this a bit more, there are ways to integrate the rclpy executor with the asyncio executor without blocking and without modifying rclpy.
e.g. you can run a coroutine from the ros callback with https://docs.python.org/3/library/asyncio-task.html#asyncio.run_coroutine_threadsafe and you will need to be both running the asycio loop and the ros loop (aka spinning) in different threads. That approach is more unlikely to have issues, as it doesn't block the rclpy executor.
I tried many different options and that one is the best I could think of.
Moving asyncio loop
and ros executor
to different threads is not convenient because this approach doesn't allow to use something like await asyncio.sleep()
inside a node callback. It will be necessary to handle all asyncio
features outside of ros
code.
If you could show another example that would solve that issues and have the same functionality, it would be great.
Long term, I agree with @ivanpauno. To have RMW events being waited on a background thread and dispatched to an asyncio
loop is probably the simplest approach. Incoming messages from subscriptions would be serviced in the loop, and not through a callback invoked from the background thread.
Short term, I'm not against local asyncio
loops to bridge asynchronous code, so long as its limitations are understood (e.g. it's blocking, it's short lived, it's not safe for two loops in different threads to interact). To merge this patch and make that a recommendation is a different story.
Moving asyncio loop and ros executor to different threads is not convenient because this approach doesn't allow to use something like await asyncio.sleep() inside a node callback
You can do that using https://docs.python.org/3/library/asyncio-task.html#asyncio.run_coroutine_threadsafe.
something like await asyncio.sleep() inside a node callback. It will be necessary to handle all asyncio features outside of ros code.
You can already do that in rclpy though https://github.com/ros2/rclpy/blob/95fc7495efded6463a0be47ce6fbed4d9ce26746/rclpy/test/test_executor.py#L166. IIUC we run that in a non-blocking way correctly, @sloretz probably knows better.
The only problem is that not all asyncio coroutines can be called, because there's no asycnio event loop.
You can already do that in rclpy though https://github.com/ros2/rclpy/blob/95fc7495efded6463a0be47ce6fbed4d9ce26746/rclpy/test/test_executor.py#L166.
await asyncio.sleep(0)
equivalent generator https://github.com/python/cpython/blob/5024dc1c6e08247693aea6ad6e225ec5dcaf0721/Lib/asyncio/tasks.py#L586. I have seen this code.
Added the ability to use the asyncio library inside the callback. It can be convenient.