feat: use pyo3-asyncio to get a fresh tokio runtime #2
Conversation
|
Expand on description and add how we tested it. We can take this info from our internal PR that implemented this feature. |
PengLiVectra
force-pushed
the
use-pyo3-asynocio
branch
from
bd00c59
to
92a3cac
Compare
python/src/lib.rs
Outdated
| @@ -4,6 +4,8 @@ mod error; | |||
| mod filesystem; | |||
| mod schema; | |||
| mod utils; | |||
| extern crate pyo3; | |||
There was a problem hiding this comment.
we can probably remove this.
|
Should we have a GitHub issue in github/delta-io/delta-rs/issues for this feature request? Also how might this relate to github/delta-io/delta-rs/1315? |
PengLiVectra
force-pushed
the
use-pyo3-asynocio
branch
from
92a3cac
to
6254fd7
Compare
| @@ -840,22 +846,22 @@ impl RawDeltaTable { | |||
| pub fn get_py_storage_backend(&self) -> PyResult<filesystem::DeltaFileSystemHandler> { | |||
| Ok(filesystem::DeltaFileSystemHandler { | |||
| inner: self._table.object_store(), | |||
| rt: Arc::new(rt()?), | |||
| rt: Arc::new(rt_pyo3()?), | |||
There was a problem hiding this comment.
DeltaFileSystemHandler (https://github.com/delta-io/delta-rs/blob/main/python/src/filesystem.rs#L58) uses the runtime in utils.rs (https://github.com/delta-io/delta-rs/blob/main/python/src/utils.rs#L10-L13), which is not pyo3-asyncio. So I keep the original rt but change name to rt_pyo3 to avoid errors.
Is there a better way to handle this?
There was a problem hiding this comment.
Interesting, so utils.rs and lib.rs both have a runtime that they create? I don't actually know what is the right approach here without more knowledge of runtimes and tokio. Should we get help or learn ourselves?
There was a problem hiding this comment.
https://sourcecode.vectra.io/projects/DP/repos/delta-rs/pull-requests/26/overview I think this provides the answer. This change seems worthy of pushing upstream, and would require a separate PR.
There was a problem hiding this comment.
That answers the question of why we're changing lib.rs. However, my question is related specifically to our having two runtime functions: should we also be using py03-asyncio's tokio-runtime in utils.rs?
There was a problem hiding this comment.
Let's get this out and have the community decide. Frankly @tsh56 and I have higher-priority issues to resolve.
I think it's related to delta-io#1315. |
yeah and especially, felt like we did implement alternative to delta-io#1361 |
|
do we know why some tests are failing? |
|
PengLiVectra
force-pushed
the
use-pyo3-asynocio
branch
2 times, most recently
from
556c778
to
46d4f7b
Compare
PengLiVectra
force-pushed
the
use-pyo3-asynocio
branch
from
4ec7485
to
f71515c
Compare
PengLiVectra
force-pushed
the
use-pyo3-asynocio
branch
from
f71515c
to
ccfccc7
Compare
Description
This PR greatly reduces network connections and dns request volume by the delta-rs library when using Python bindings. The approach here is to utilize pyo3-asyncio's tokio-runtime feature as the source of the Runtime. This yields the same runtime across function calls which preserves connections in the connection pool. The previous code created a new runtime per python function call, which established all new socket connections and issued new DNS requests.
Related Issue(s)
Partly delta-io#1315
Testing:
Ran a script that called hundreds of delta operations, and watched tcpdump. Only saw one dns request.
Documentation