fix(python): release GIL on most operations #2512
Conversation
adriangb
requested review from
wjones127,
fvaleye,
roeap and
ion-elgreco
as code owners
|
ACTION NEEDED delta-rs follows the Conventional Commits specification for release automation. The PR title and description are used as the merge commit message. Please update your PR title and description to match the specification. |
adriangb
changed the title
|
Never mind, it was just stuck. |
adriangb
changed the title
There was a problem hiding this comment.
Although I am not familiar with the delta-rs codebase, @adriangb suggested I might want to take a quick look at this given the pain with blocking on IO and holding the GIL at the same time.
For what it's worth, I agree that it's a problem and this PR is the current solution. It does sadden me a bit that it's so verbose to have to do this, though.
Upstream in PyO3 we're looking at a #[pyo3(allow_threads)] attribute on functions which will do this for you behind the scenes - PyO3/pyo3#3610
There's also the question of more radical changes like automatically releasing the GIL when none of the input arguments need the GIL but there's a performance cost to releasing the GIL so I think it needs to be user choice.
Finally, I see that you use futures here so maybe there's a good argument that more investment in PyO3's async story would solve this in a different way by allowing you to make these functions async and allow a Python reader to await them. There are definitely complexities in mixing Rust and Python async runtimes so that's not an easy solution either.
|
@davidhewitt @adriangb was quite busy this week, so hadn't gotten time to look at until now : ) Yeah it's fine to release the GIL where possible, there was never really a need for it before, so I guess that's why it wasn't brought up outside of that other PR which wasn't finished. @davidhewitt I think a derive allow_threads is definitely useful if the whole function can release the GIL. I am not sure, what pyo3 async would provide us though |
|
Thanks! @adriangb |
Fixes #2269