Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: support
async fn
in macros with coroutine implementation #3540feat: support
async fn
in macros with coroutine implementation #3540Changes from all commits
627841f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I can't help but wonder if somehow deep down in the Rust future, if it awaits a Python awaitable, whether we want to yield whatever the Python awaitable that blocked it here, rather than make a new
asyncio.Future
. That seems like a possible avenue to support all Python runtimes automatically.However, it needs some way to lift whatever that object was up to here. Maybe that's done through the waker, or maybe that's done through our own mini thread-local "runtime"?
Of course, there's also the possibility that this Rust future never awaits on anything from Python, and if that's the case, doing this
asyncio
dance seems correct.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.
Actually, I've an idea to make this possible, and in a quite elegant way, but it would require a nightly feature.
This would indeed be a way to support arbitrary awaitables. You would still need to target a particular runtime for arbitrary Rust futures (but you could also have a runtime-agnostic coroutine which would only support Python awaitables). However, it would not be possible to do a
select!
between two Python awaitables, (because you can't yield two objects), so you would have topanic!
in that case, and I'm not sure about the ergonomy of this.An additional side effect would be cancellation, because if you delegate to the Python awaitable, you should also delegate cancellation, and
CoroutineCancel
should not register the exception in that case.I really like this idea though! I may implement a POC in the next days, maybe in pyo3-async to begin with.
EDIT: Actually, you can't
select!
with only one Python awaitable, because you can't use its future to wake up the other branch, as it would prevent it to get its result.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.
It would be interesting to explore, and if we find that this design makes the most sense but requires nightly, we can put it behind our
nightly
feature and use it as evidence for upstream to explore for stabilisation!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.
This will make our handling of iterators even more of a mess, won't it? So this is basically what we want to do with all iterators but it would apply only to coroutines? Maybe we should align all of this if we go ahead?
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.
(Sorry if this not clear, but this is mainly directed at @davidhewitt, not so much at @wyfo.)
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.
I honestly think
__next__
should allow to returnPyResult<PyObject>
in addition toPyResult<IterNextOutput<PyObject, PyObject>>
orPyResult<Option<PyObject>>
.It would remove the need for this small utilitary function.
But I did want to touch too much parts of PyO3 in this first draft.
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.
We have a long discussion in #3190 about reworking
__next__
and__anext__
. Really just needs someone to write up a bunch of test cases / examples so we can commit to a design. I keep meaning to do it, but am sort of trying to avoid breaking too much at the same time as #3382