feat: add #[pyo3(allow_threads)] to release the GIL in (async) functions #3610

wyfo · 2023-11-30T02:59:27Z

Relates to #1632

Draft based on #3609

codspeed-hq · 2023-11-30T03:57:53Z

CodSpeed Performance Report

Merging #3610 will not alter performance

_{Comparing wyfo:allow_threads (f51c087) with main (7263fa9)}

Summary

✅ 67 untouched benchmarks

wyfo · 2023-12-07T08:02:19Z

@davidhewitt Here is the next one! I'm more explicit now 😄

src/coroutine.rs

davidhewitt

Excellent!

src/coroutine.rs

adamreichold · 2023-12-09T08:48:05Z

@davidhewitt I have a few minutes now and the merge queue failed. If we could hold off on merging I could have a look as well?

adamreichold · 2023-12-09T08:50:50Z

tests/ui/invalid_pyfunction_signatures.stderr

@@ -16,7 +16,7 @@ error: expected argument from function definition `y` but got argument `x`
 13 | #[pyo3(signature = (x))]
   |                     ^

-error: expected one of: `name`, `pass_module`, `signature`, `text_signature`, `crate`
+error: expected one of: `allow_threads`, `name`, `pass_module`, `signature`, `text_signature`, `crate`


What I would really want to have here is a test that checks that I cannot get py: Python<'_> as my argument when using allow_threads. This should not be possible to do as the GIL is released, right?

(Ideally, it would also have a nice error message explaining why these two contradict each other.)

You would have a compilation error, because Python is not Ungil, but you're right, I will add that case in the UI tests.

As expected, there is a compilation error, but the output will not make @davidhewitt happy 😅

You can add an error message for Python, but not really for things like &PyAny

Uff. Could you split the test into two, one for the GIL token and for the GIL bound reference?

For the GIL token: Since we are injecting this with our proc macro machinery, we should be able to do something about the error message for this one, shouldn't we?

You can add an error message for Python, but not really for things like &PyAny

Indeed, but I think we should do what we can. Sorry for putting more work on your table.

Not an issue, it will just wait a few hours but I will do the modification 😃

(Just for coverage, we should also duplicate the compiler error test for plain and async functions.)

I've added a check for Python<'_> field for both allow_threads and async functions. The error raised by Bound<'_, PyAny> for these cases is still ugly, but as said earlier, I don't think it's possible to add a pretty error message, and this is already the case for Python::allow_threads tests in ui/not_send.rs and ui/not_send2.rs.

davidhewitt · 2023-12-09T08:52:14Z

Sure thing, I'll leave this for you to review and merge 👍

src/coroutine.rs

adamreichold · 2023-12-09T08:59:53Z

src/coroutine.rs

+        self: Pin<&mut Self>,
+        py: Python<'_>,
+        waker: &Waker,
+        allow_threads: bool,


This might be me going off into the weeds, but we turning a compile time information into a runtime flag here and generating code to handle both cases even only one will ever be executed.

Hence, I wonder if we should use a const generic const ALLOW_THREADS: bool on Coroutine to ensure that only the required code is generated?

In fact, that's what I have in my POC. I'd made the whole Coroutine generic and boxed it in the #[pyclass].
But I decide to go for simplicity here. I can change that if you think the performance impact will be significant.

I think simplicity is certainly good for the first iteration and this could definitely be a follow-up PR.

That said, I was wondering if we could hide this behind the boxed CoroutineFuture trait to avoid generics all the way up, e.g. a marker wrapper like

struct AllowThreads<F>(F); impl<F, T, E> CoroutineFuture for AllowThreads<F> where F: Future<Output = Result<T, E>> + Send, T: IntoPy<PyObject> + Send, E: Into<PyErr> + Send;

but I am not sure how well this can be tucked away to avoid complicating the instantiation too much.

Done.

EDIT: I've kept allow_thread as a function parameter instead of generic parameter in the constructor, because it will be more user-friendly when the constructor will be made public.

wyfo · 2024-04-06T19:53:23Z

I don't know what I can do for the nightly pipeline error 🤷‍♂️

error: non-local `impl` definition, they should be avoided as they go against expectation

mejrs · 2024-04-06T22:02:39Z

I don't know what I can do for the nightly pipeline error 🤷‍♂️
error: non-local `impl` definition, they should be avoided as they go against expectation

Feel free to allow the lint for now. You'll have to chase down where our code triggers this and apply #[allow(non_local_definitions)] (and a TODO) there.

It's a new nightly lint, it's not something you did. We have done work earlier to adapt our code to this lint, but this part is new. You can fix it if you want but the last time it was a substantial amount of work so it should be fixed in its own PR.

wyfo · 2024-04-07T01:36:52Z

You'll have to chase down where our code triggers this

That's the issue, it's not my code which is triggering the lint. My changes have nothing to do with the following error

error: non-local `impl` definition, they should be avoided as they go against expectation
   --> tests/test_proto_methods.rs:643:1
    |
643 | #[pymethods]
    | ^^^^^^^^^^^^
    |
    = help: move this `impl` block outside the of the current function `trampoline` and up 3 bodies

It's an issue of generated code (parts I haven't touched). My explaination is that a bug has been very recently introduced in the nightly build.
By the way #4052 is failing too for the same reasons, while it's just a typo correction. There is definitely an issue with the nightly CI.

mejrs · 2024-04-07T16:07:48Z

You can also wait for someone to send a PR (probably me tomorrow) and rebase after it is merged.

wyfo · 2024-04-18T19:22:40Z

#4033 broke some code here, I will update the PR.
Could I ask then for a new review? @adamreichold? I would like to unblock the async PRs to close the async chapter for the next release.

Icxolu

Generally LGTM, but this is definitely a new area for me, so someone with more experience here should have a look as well.

We might additionally want a test to verify that for (async) allow_threads methods only &self/&mut self receivers are allowed and Bound/PyRef/PyRefMut are rejected.

Icxolu · 2024-05-08T21:29:46Z

src/gil.rs

+        fn without_gil(_arg1: PyObject, _arg2: PyObject) {
+            GIL_COUNT.with(|c| assert_eq!(c.get(), 0));
+        }
+        Python::with_gil(|gil| {


We generally call the Python token py. I think we should stick with that for consistency here as well.

Icxolu · 2024-05-08T21:36:34Z

src/coroutine.rs

@@ -88,10 +125,10 @@ impl Coroutine {
        } else {
            self.waker = Some(Arc::new(AsyncioWaker::new()));


Maybe we can split this here and do something like

let waker = self .waker .get_or_insert_with(|| Arc::new(AsyncioWaker::new()));

after the (possible) reset, to avoid the unwrap()s below?

Icxolu · 2024-05-08T21:48:14Z

tests/test_compile_error.rs

@@ -54,6 +54,6 @@ fn test_compile_errors() {
    t.compile_fail("tests/ui/invalid_pymodule_two_pymodule_init.rs");
    #[cfg(feature = "experimental-async")]
    #[cfg(any(not(Py_LIMITED_API), Py_3_10))] // to avoid PyFunctionArgument for &str


Given you've changed the argument types to i32, I think this comment/feature gate is not accurate anymore.

davidhewitt · 2024-05-17T11:49:45Z

pyo3-macros-backend/src/method.rs

+        {
+            ensure_spanned!(
+                self.asyncness.is_none(),
+                py_arg.ty.span() => "GIL token cannot be passed to async function"


👍 it looks like this already doesn't work so this makes the error message nicer. One suggestion given the GIL is potentially an outdated idea:

Suggested change

py_arg.ty.span() => "GIL token cannot be passed to async function"

py_arg.ty.span() => "`Python<'_>` token cannot be passed to async functions"

davidhewitt · 2024-05-17T11:54:53Z

pyo3-macros-backend/src/method.rs

+            );
+            ensure_spanned!(
+                self.allow_threads.is_none(),
+                py_arg.ty.span() => "GIL cannot be held in function annotated with `allow_threads`"


Similarly here I think we want to just name the type:

Suggested change

py_arg.ty.span() => "GIL cannot be held in function annotated with `allow_threads`"

py_arg.ty.span() => "`Python<'_>` cannot be passed to a function annotated with `allow_threads`"

davidhewitt · 2024-05-17T16:19:55Z

pyo3-macros-backend/src/method.rs

+                quote! {{
+                    #self_arg_decl
+                    #(let #arg_names = #args;)*
+                    py.allow_threads(|| function(#self_arg_name #(#arg_names),*))


There's a question in my head here about after #3646 which of the allow_threads variants we will use here (cc @adamreichold). I think it would be possible to create a scoped TLS issue by setting up a TLS scope and then calling into Python. Similar thinking applies to the async case below. I guess we have to stick to the principle that the soundness is important and use the "safe but slow" form for this attribute. If users want to go unsafe they can release the GIL themselves. This feels the right way to do it, I think.

This feels the right way to do it, I think.

Yes, I think so too. Especially since we have already accumulated a significant amount of high risk changes for 0.22 and we can always optimize things in following releases.

davidhewitt · 2024-05-19T13:49:32Z

@wyfo I may try to rebase and merge this tomorrow at the PyCon sprints. Any objections?

itamarst · 2024-10-31T13:59:35Z

If anyone has time to work on this, it could make a PR I need to write have rather smaller diffs 😁

Icxolu · 2024-10-31T21:49:26Z

@davidhewitt If we're good with including this in 0.23 I could go rebase this and fixup the last final remarks from above.

davidhewitt · 2024-10-31T22:52:18Z

I would love to have this, but I am worried about interactions with #3646. I think merging this before that PR is potentially a problem (might have breakage), and I am worried that PR is necessary but also makes async extremely complicated (because that PR basically disables all TLS with allow_threads, but the tokio runtime is frequently accessed by TLS, for example). I haven't had a chance to sit down and reason through all of this myself, however I would be happy to hand over for someone else to evaluate the design space and understand how bad the interaction between #3646 and async is.

itamarst · 2024-11-01T01:01:45Z

(My personal use case is non-async functions, so I'd be happy with this feature to land with initially just supporting normal functions. But I do see why this would be much more of a useful thing for async functions.)

mejrs · 2024-11-01T01:12:20Z

The interaction between async runtimes, threadlocals and #3646 remains whether or not this PR is merged. Is your concern that we'll have more fallout if we merge this and then that?

wyfo · 2024-11-01T19:12:34Z

@davidhewitt Why does the PR disable all TLS?

mejrs · 2024-11-02T12:27:18Z

@davidhewitt Why does the PR disable all TLS?

Merging that PR means that allow_threads runs its closures on a threadpool, so it's executed on a different thread. It'd be surprising to people who are accessing threadlocals from it.

davidhewitt · 2024-11-02T13:02:39Z

The interaction between async runtimes, threadlocals and #3646 remains whether or not this PR is merged. Is your concern that we'll have more fallout if we merge this and then that?

Yes, I still feel like we should aim to do something like #3646 but the more I look at it the more extreme the fallout seems to be. Merging this seems to encourage use of allow_threads by making it really cheap to slap as an attribute on basically everything. Which I do think is probably a good thing, but if #3646 then goes and breaks all those, I'm worried about getting the order of these additions right.

itamarst · 2024-11-02T13:36:02Z

Merging that PR means that allow_threads runs its closures on a threadpool, so it's executed on a different thread. It'd be surprising to people who are accessing threadlocals from it.

As a user I would find the use of a thread pool very surprising and it would make #[pyo3(allow_threads)] into something I probably can't use. As context I'm looking at using this in Polars Python APIs, that all need to release the GIL before calling into Polars' internal thread pool (otherwise you get deadlocks). #[pyo3(allow_threads)] resulting in dispatching to a completely different thread pool first would I suspect be undesirable to Polars core devs.

(From my personal perspective the thread pool option seems very intrusive and in my own projects I would likely default to the unsound-if-you-put-Python-objects-into-TLS version that runs in current thread.)

More practically for this PR: there could be two different attributes, one for each proposed allow_threads strategy?

Icxolu · 2024-11-02T13:57:03Z

More practically for this PR: there could be two different attributes, one for each proposed allow_threads strategy?

I don't think we can add an attribute for any unsafe strategy. These rely on safety invariants that we can't validate within our macro code. I think it is important that this is spelled out in the code and not hidden within an attribute.

wyfo · 2024-11-02T20:31:47Z

Could https://doc.rust-lang.org/reference/unsafe-keyword.html#unsafe-attributes-unsafeattr be a thing?

davidhewitt · 2024-11-02T20:34:03Z

Yes, though it's still in RFC and we might want to see how that works out: rust-lang/rfcs#3715

wyfo marked this pull request as draft November 30, 2023 02:59

wyfo force-pushed the allow_threads branch 3 times, most recently from 32c2050 to 77b24fa Compare November 30, 2023 03:11

wyfo force-pushed the allow_threads branch from 77b24fa to 285bb15 Compare November 30, 2023 15:39

wyfo mentioned this pull request Nov 30, 2023

feat: add coroutine::await_in_coroutine to await awaitables in coroutine context #3611

Draft

wyfo force-pushed the allow_threads branch 7 times, most recently from 905f1eb to 3bc7d67 Compare December 7, 2023 07:55

wyfo marked this pull request as ready for review December 7, 2023 07:57

wyfo force-pushed the allow_threads branch from 3bc7d67 to 639a1aa Compare December 7, 2023 08:21

davidhewitt reviewed Dec 7, 2023

View reviewed changes

src/coroutine.rs Outdated Show resolved Hide resolved

wyfo force-pushed the allow_threads branch from 639a1aa to 657991a Compare December 7, 2023 18:24

davidhewitt approved these changes Dec 9, 2023

View reviewed changes

src/coroutine.rs Outdated Show resolved Hide resolved

davidhewitt added this pull request to the merge queue Dec 9, 2023

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 9, 2023

adamreichold reviewed Dec 9, 2023

View reviewed changes

src/coroutine.rs Outdated Show resolved Hide resolved

adamreichold reviewed Dec 9, 2023

View reviewed changes

src/coroutine.rs Outdated Show resolved Hide resolved

wyfo force-pushed the allow_threads branch from 657991a to 62849ae Compare December 9, 2023 08:57

adamreichold reviewed Dec 9, 2023

View reviewed changes

wyfo force-pushed the allow_threads branch from 62849ae to d3dfa6b Compare December 9, 2023 09:10

wyfo force-pushed the allow_threads branch from eab7a6b to 199d4f5 Compare April 7, 2024 09:22

wyfo force-pushed the allow_threads branch 2 times, most recently from 4f6990a to 1fe17cc Compare April 7, 2024 19:48

wyfo force-pushed the allow_threads branch 2 times, most recently from 40c0b9f to 08948df Compare April 25, 2024 09:19

feat: add #[pyo3(allow_threads)] to release the GIL in (async) functions

f51c087

wyfo force-pushed the allow_threads branch from 08948df to f51c087 Compare May 7, 2024 08:26

Icxolu reviewed May 8, 2024

View reviewed changes

davidhewitt reviewed May 17, 2024

View reviewed changes

davidhewitt mentioned this pull request May 17, 2024

fix(python): release GIL on most operations delta-io/delta-rs#2512

Merged

davidhewitt mentioned this pull request Jun 16, 2024

0.22 release #4038

Closed

5 tasks

davidhewitt mentioned this pull request Aug 25, 2024

feat: expose coroutine constructor #3613

Draft

		@@ -88,10 +125,10 @@ impl Coroutine {
		} else {
		self.waker = Some(Arc::new(AsyncioWaker::new()));

	py_arg.ty.span() => "GIL token cannot be passed to async function"
	py_arg.ty.span() => "`Python<'_>` token cannot be passed to async functions"

	py_arg.ty.span() => "GIL cannot be held in function annotated with `allow_threads`"
	py_arg.ty.span() => "`Python<'_>` cannot be passed to a function annotated with `allow_threads`"

feat: add #[pyo3(allow_threads)] to release the GIL in (async) functions #3610

Are you sure you want to change the base?

feat: add #[pyo3(allow_threads)] to release the GIL in (async) functions #3610

Conversation

wyfo commented Nov 30, 2023

codspeed-hq bot commented Nov 30, 2023 • edited Loading

Merging #3610 will not alter performance

Summary

wyfo commented Dec 7, 2023

davidhewitt left a comment

Choose a reason for hiding this comment

adamreichold commented Dec 9, 2023

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

wyfo Dec 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidhewitt commented Dec 9, 2023

Choose a reason for hiding this comment

wyfo Dec 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wyfo Apr 6, 2024 • edited Loading

Choose a reason for hiding this comment

wyfo commented Apr 6, 2024

mejrs commented Apr 6, 2024 • edited Loading

wyfo commented Apr 7, 2024

mejrs commented Apr 7, 2024

wyfo commented Apr 18, 2024

Icxolu 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidhewitt commented May 19, 2024

itamarst commented Oct 31, 2024

Icxolu commented Oct 31, 2024

davidhewitt commented Oct 31, 2024

itamarst commented Nov 1, 2024

mejrs commented Nov 1, 2024

wyfo commented Nov 1, 2024

mejrs commented Nov 2, 2024

davidhewitt commented Nov 2, 2024

itamarst commented Nov 2, 2024

Icxolu commented Nov 2, 2024

wyfo commented Nov 2, 2024

davidhewitt commented Nov 2, 2024

codspeed-hq bot commented Nov 30, 2023 •

edited

Loading

wyfo Dec 9, 2023 •

edited

Loading

wyfo Dec 9, 2023 •

edited

Loading

wyfo Apr 6, 2024 •

edited

Loading

mejrs commented Apr 6, 2024 •

edited

Loading