-
-
Notifications
You must be signed in to change notification settings - Fork 30.9k
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
Implemenation of the PEP 492 - Coroutines with async and await syntax #68205
Comments
Here's the first patch (git diff master..await). Should be easier to review and work from there. |
Attaching a patch generated with mercurial |
Attaching a revised patch (all Victor's comments but asyncio changes) |
Could we have type slots for the new special methods? Otherwise, implementing the protocol in C would be fairly inefficient, especially for async iteration. I'm asking because Cython's generator type is not Python's generator type, but implementing the rest of the proposed protocol doesn't seem to be all that difficult. |
I don't think it's necessary to have slots for __aiter__, __anext__, __aenter__ and __aexit__. Async iteration will never be as fast as regular iteration, and there is plenty overhead in it. And we don't need slots for async context managers as we don't have slots for regular ones. What might be a good idea is to add a slot for __await__ method -- tp_await. This will allow to implement Futures in C efficiently. I'd rename 'tp_reserved' for this purpose. Victor, your thoughts? |
In fact I will likely add tp_await in the next PEP iteration. I need it to implement another feature. |
You seem to be assuming that the outer loop is the asyncio I/O loop. That might not be the case. It might be a thread-pool executor, it might be an in-memory task switcher, or it might just be something passing on items from a list. At least "__anext__" is worth being fast, IMHO. Also note that one advantage of slots is that the user can cache their value to keep calling the same C function multiple times. That is not the case for a Python method, which can easily be replaced. Some iterators do that with their __next__ method, and it's perfectly valid. Having to look up the Python method on each iteration and calling through it sounds like unnecessary overhead. |
I think we can continue this discussion *after* the PEP's been accepted. |
I'll upload the most recent patch soon. |
Third patch attached. Victor, it would be great if you can review it! |
Another iteration:
|
We also need a Coroutine ABC. Both the "GeneratorType" and "CO_COROUTINE" checks are too restrictive. Also see bpo-24018, which this one should in fact depend on. |
Review sent - very nice work on this Yury. Highlights:
|
Thanks a lot, Nick! Highlights:
Do you think that tp_as_async is a better name? (I explained my point of view in code review comments) Also, do we need slots for __aenter__ and __aexit__? We don't have slots for regular context manager protocol, fwiw.
I will. We definitely need it.
I agree that CO_COROUTINE is something that we should use for 'async def' functions (instead of CO_NATIVE_COROUTINE). However, CO_ITERABLE_COROUTINE sounds a bit odd to me, as generator-based coroutines (at least in asyncio) aren't supposed to be iterated over. How about CO_GENBASED_COROUTINE flag?
Big +1. Your names are much better. |
On Sun, May 10, 2015 at 7:21 PM, Yury Selivanov <report@bugs.python.org> wrote:
Maybe CO_ASYNC_COROUTINE and CO_OLDSTYLE_COROUTINE?
|
New patch is attached. Nick, I think that all of your feedback should be addressed in this patch. Major changes:
|
Nick, Guido, |
Latest version looks good to me (aside from a quibble about whether StopAsyncIteration should inherit from BaseException instead of Exception - see my review for details). Based on Guido's explanation in the review, I also suggested adding the following example method to the PEP as part of the rationale for StopAsyncIteration: def __anext__(self):
try:
data = await self._get_data()
except EOFError:
raise StopAsyncIteration
return data The trick is that when __anext__ is itself a coroutine, we really do have 3 exit paths:
|
Guido convinced me that having StopAsyncIteration inherit from Exception was the right approach, as it means errors are more likely to be of the "we caught it when we shouldn't have" variety, rather than the harder to debug "an exception escaped when it shouldn't have" variety. This isn't like SystemExit, KeyboardInterrupt or GeneratorExit where they're specifically designed to reliably terminate a thread of execution. That means I can offer an unreserved +1 on the current patch (#6) for beta 1 :) |
New changeset 957478e95b26 by Yury Selivanov in branch '3.4': New changeset 44c1db190525 by Yury Selivanov in branch 'default': |
New changeset eeeb666a5365 by Yury Selivanov in branch 'default': |
Guido, Nick, Victor, |
Thank you Yury! You are a coding machine. On Mon, May 11, 2015 at 8:06 PM, Yury Selivanov <report@bugs.python.org>
|
New changeset 3a3cc2b9a1b2 by Yury Selivanov in branch 'default': |
New changeset 0dc3b61f1dfa by Yury Selivanov in branch 'default': |
New changeset ee7d2c9c70ab by Yury Selivanov in branch '3.4': New changeset 874edaa34b54 by Yury Selivanov in branch 'default': |
New changeset 843fe7e831a8 by Yury Selivanov in branch '3.5': New changeset 87509d71653b by Yury Selivanov in branch 'default': |
I added a couple of review comments to patch 6, but since no-one has responded so far, I guess they simply haven't been noticed. So I'll just repeat them here. getawaitablefunc / aiternextfunc / getaiterfunc Is there a reason why these need to have their specific C type name instead of just reusing unaryfunc, or at least the existing iternextfunc / getiterfunc? They are unprefixed global names in the C namespace and I think we should be careful when adding more of those. Awaitable.register(Coroutine) I think this is incorrect. A Coroutine is not Awaitable unless it also implements "__await__". How else should it be awaited? I propose to use this wrapping code as a fallback for types.coroutine() in the case that a Generator (ABC) is passed instead of a generator (yield): class types_coroutine(object):
def __init__(self, gen):
self._gen = gen
class as_coroutine(object):
def __init__(self, gen):
self._gen = gen
self.send = gen.send
self.throw = gen.throw
self.close = gen.close
def __await__(self):
return self._gen
def __call__(self, *args, **kwargs):
return self.as_coroutine(self._gen(*args, **kwargs)) Note that the resulting Awaitable Coroutine type is not an Iterable. This differs from a (yield) coroutine, but it matches the Coroutine and Awaitable protocols, and the intention to separate both in order to avoid mistakes on user side. Additionally, regarding the tests: def test_func_2(self):
async def foo():
raise StopIteration
run_async(foo()) Why is this actually necessary? I'm aware that it's also mentioned in the PEP, but is there an actual reason why a coroutine should behave the same as a generator here? Is it just an implementation detail for legacy reasons because generators and coroutines happen to share the same type implementation? (I don't think they need to, BTW.)
async def foo():
return 'spam'
I find it surprising that this works at all. Yield-from iterates, and a coroutine is not supposed to be iterable, only awaitable (at least, that's what all error messages tell me when I try it). So why should "yield from" work on them? What if foo() was not an Iterable but a Coroutine? Should "yield from" then call "__await__" on it internally? I would find that *really* surprising, but given the above, I think it would be necessary to achieve consistency. |
It *is* correct, see PEP-492. Awaitable is either a coroutine *or* an object with an __await__ method. Generally, being an awaitable means that the object can be used in "await" expression.
Just implement tp_await/await for coroutine-like objects coming from C-API or Cython. In general, iteration protocol is still the foundation for Future-like objects, so there is nothing wrong with this. Generator ABC isn't supposed to be used with "await" expression.
Coroutines are implemented on top of generators. Until we clearly separate them (in 3.6?) I don't think we should allow coroutines to bubble up StopIteration.
This is a special backwards-compatibility thing. In general, generators cannot yield-from coroutines (unless they are decorated with @types.coroutine). |
Another question: is it ok if Cython implements and uses the "tp_as_async" slot in all Py3.x versions (3.2+)? It shouldn't hurt, but it would speed up async/await in Cython at least under Py3.x. Only Py2.6/7 would then have to resort to calling "__await__()" etc. at the Python level. One drawback is that Py<3.5 currently (needlessly) checks that "tp_reserved" and "tp_richcompare" are both implemented, but that can be worked around by also implementing the latter... |
"coroutine", yes. But "Coroutine"? Shouldn't the Coroutine ABC then require
Sure, that's how it's done. (Specifically, Coroutine is not an I was just wondering how Cython should compile Python code that makes use
That's not really reflected in the ABCs, is it? |
This is an interesting idea. Practically, when you register something as a Coroutine, you expect it to I’m curious what Guido and Nick think about this. I think that we can
Can't your Coroutine object return itself from its __await__, and implement
I think we can update types.coroutine to continue using CO_ITERABLE_COROUTINE
Awaitable has its __await__ defined as a generator... |
Thanks for highlighting these Stefan - you guessed correctly that I'd missed them on the review. For your first question, I agree getawaitablefunc / aiternextfunc / getaiterfunc should all be dropped and replaced with the existing "unaryfunc". For your second question, I agree it makes more sense for Coroutine to inherit from Awaitable than it does to have it registered with it. For the other three, I don't have a strong opinion, except that we should make sure that whatever we do on the CPython side works by default for Cython. |
I have no problem with that. But why do we have iternextfunc & getiterfunc (no "a" prefix)? |
Given that tp_call is just "ternaryfunc", my guess would be "because when the iterator protocol was added, someone went with function-pointer-type-per-slot rather than function-pointer-type-per-call-signature". We *are* a little inconsistent here (e.g. "reprfunc" could also just use the "unaryfunc" signature), but Stefan's right that that isn't a good reason to *add* to the inconsistency. |
That was my first try, sure, and it mostly worked. It has a drawback, All of these little details make this trick appear like a potential source On a related note, my testing made me stumble over this code in if coro.__class__ is types.GeneratorType:
self._coro = coro
else:
self._coro = iter(coro) # Use the iterator just in case. This seems wrong regardless of how you look at it. And it definitely fails |
BTW, given that "iter(iterator)" works and returns the iterator, should we also allow "await x.__await__()" to work? I guess that would be tricky to achieve given that __await__() is only required to return any kind of arbitrary Iterator, and Iterators cannot be awaited due to deliberate restrictions. But it might be nice to have for wrapping purposes. |
That only answers the half-serious first part of my question. ;) This code only works if foo() returns an Iterable, including a (yield) @types.coroutine
def bar():
return (yield from foo()) It does not work for arbitrary Coroutines as they are not iterable, but it |
New changeset 09327f653ec5 by Yury Selivanov in branch '3.4': New changeset adf72cffceb7 by Yury Selivanov in branch '3.5': New changeset 9c0a00247021 by Yury Selivanov in branch 'default': |
New changeset dfa0288c91fd by Yury Selivanov in branch '3.5': New changeset 99dcca3466d3 by Yury Selivanov in branch 'default': |
Stefan, I've already committed fixes for:
I've also opened a couple of new issues (with patches for a review):
I'll reply to some of your messages below:
I think it's totally OK, given that you can workaround the drawback you mentioned.
I think if "next(cython_coro)" does not fail is acceptable. It's not ideal, but |
Thanks Yury, I'll give it a try ASAP. |
Stefan, Because of https://mail.python.org/pipermail/python-committers/2015-May/003410.html I've decided to commit 24315 and 24316 today. Please try to check that everything works before new beta 2. |
Tried it, works for me. Thanks! |
This is really good news! Thanks! |
New changeset 0708aabefb55 by Yury Selivanov in branch '3.4': New changeset 1dc232783012 by Yury Selivanov in branch '3.5': New changeset 2e7c45560c38 by Yury Selivanov in branch 'default': |
New changeset a0a699b828e7 by Yury Selivanov in branch '3.5': New changeset 89521ac669f0 by Yury Selivanov in branch 'default': |
New changeset 1e9e0664ee9b by Yury Selivanov in branch '3.5': New changeset 6fcb64097b1c by Yury Selivanov in branch 'default': |
Was __await__() deliberately left out of concurrent.futures.Future or was that an oversight? Or am I misunderstanding something? |
I don't think concurrent.Future is supposed to be used with asyncio (in 'yield from' or 'await' expressions). |
Hmm, but IMHO a) the new syntax isn't just for asyncio and b) awaiting a Future seems like a *very* reasonable thing to do. I think opening a new ticket for this is a good idea. |
Stefan, I honestly have bo idea what concurrent.Future.__await__ would do. There is no loop for concurrent module. If you have a patch with tests in mind, please open a separate issue (targeting 3.6). |
Maybe it's possible to give an interpretation to awaiting on a threaded The only thing I don't know is whether it's possible for __await__ to |
Guido, Stefen, please see bpo-24383. |
See bpo-24400 regarding a split of yield-based generators and async-def coroutines at a type level. |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: