-
Notifications
You must be signed in to change notification settings - Fork 131
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
Python Node API #54
Python Node API #54
Conversation
The `Send` Trait is required by pyo3 for the python binding
Adding `next` and `send_output` requires an async threadpool as the communication Layer defined by the Middleware Layer returns an async Future stream. I solve this issue by adding a tokio runtime on a separater thread that is connected with two channels. One for sending data and one for receiving data. Those channel are then exposed synchronously to Python. This should not be cause for concern as channel are really fast. Looking at Zenoh Python client, they are heavily using `pyo3-asyncio` implementation of futures to pass Rust futures into Python. This can be a solution as well, but, from previous experiment, I'm concerned about performance on such solution. I have experienced that putting futures from Rust into the `asyncio` queue to be slow. I'm concerned also by mixing `async` and `sync` code in Python, as it might be blocking. This might requires 2 threadpool in Python. This might seem as heavy overhead for some operations.
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 good! I think the channel-based approach should work well enough for now, so I agree that trying to use pyo3-asyncio
is probably not worth the effort.
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 good to me! Two minor things, then this is ready for merging from my side.
Since the merge of PyO3/maturin#605 , we can add features flag when maturin build the python project, removing the need to always enabling `extension-module` flag when building. The `extension-module` flag creates crash when building and testing outside of maturin. Previous fix required to disable default feature flag when building and testing which is not ideal.
Description of the API
The Python API will consist of two principal methods:
next
andsend_output
. Those methods will be connected to atokio
runtime through channels.Problem
The Python Node API requires an async threadpool as the communication Layer defined by the Middleware Layer returns an async Future stream.
Solution
I solve this issue by adding a
tokio
runtime on a separate thread that is connected with two channels. One for sending data and one for receiving data.Those channels are then exposed synchronously to Python. This should not be cause for concern as channels are really fast.
Alternatives
Looking at Zenoh Python client, they are heavily using
pyo3-asyncio
implementation of futures to pass Rust futures into Python.This can be a solution as well, but, from previous experiments, I'm concerned about the performance of this solution. I have experienced that putting futures from Rust into the
asyncio
queue to be slow.I'm concerned also about mixing
async
andsync
code in Python, as it might be blocking. This might requires 2 threadpools in Python. This might seem a heavy overhead for some operations.