Repository navigation
Awaitable ABC incompatible with functools.singledispatch #68588
Description
| BPO | 24400 |
|---|---|
| Nosy | @gvanrossum, @ncoghlan, @scoder, @vstinner, @larryhastings, @bitdancer, @asvetlov, @bdarnell, @vadmium, @1st1 |
| Files |
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:
assignee = 'https://github.com/1st1'
closed_at = <Date 2015-07-03.17:13:37.061>
created_at = <Date 2015-06-07.05:06:53.709>
labels = ['interpreter-core', 'type-feature', 'release-blocker', 'expert-asyncio']
title = 'Awaitable ABC incompatible with functools.singledispatch'
updated_at = <Date 2015-07-03.23:07:24.592>
user = 'https://github.com/bdarnell'bugs.python.org fields:
activity = <Date 2015-07-03.23:07:24.592>
actor = 'ncoghlan'
assignee = 'yselivanov'
closed = True
closed_date = <Date 2015-07-03.17:13:37.061>
closer = 'yselivanov'
components = ['Interpreter Core', 'asyncio']
creation = <Date 2015-06-07.05:06:53.709>
creator = 'Ben.Darnell'
dependencies = []
files = ['39664', '39665', '39666', '39669', '39675', '39720', '39722', '39723', '39729', '39730', '39740', '39745', '39751', '39764']
hgrepos = ['312']
issue_num = 24400
keywords = ['patch']
message_count = 74.0
messages = ['244942', '244961', '245005', '245013', '245014', '245073', '245074', '245076', '245077', '245083', '245097', '245098', '245100', '245101', '245102', '245103', '245105', '245108', '245109', '245113', '245114', '245125', '245129', '245130', '245136', '245179', '245210', '245300', '245304', '245307', '245414', '245440', '245475', '245490', '245507', '245529', '245549', '245552', '245553', '245557', '245570', '245579', '245585', '245615', '245636', '245637', '245742', '245746', '245749', '245757', '245762', '245776', '245962', '246001', '246002', '246004', '246008', '246009', '246010', '246013', '246039', '246040', '246050', '246051', '246052', '246137', '246140', '246141', '246150', '246152', '246191', '246192', '246229', '246230']
nosy_count = 12.0
nosy_names = ['gvanrossum', 'ncoghlan', 'scoder', 'vstinner', 'larry', 'r.david.murray', 'asvetlov', 'Yury.Selivanov', 'python-dev', 'Ben.Darnell', 'martin.panter', 'yselivanov']
pr_nums = []
priority = 'release blocker'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue24400'
versions = ['Python 3.5', 'Python 3.6']
Activity
bdarnell commentedon Jun 7, 2015
The new collections.abc.Awaitable ABC relies on __instancecheck__, which makes it incompatible with functools.singledispatch (singledispatch works based on args[0].__class__; any instance-level information is discarded). This surprised me because the first thing I tried to do with Awaitable was add it to my singledispatch-based coroutine compatibility layer.
Ideally coroutine would be an actual subclass of generator, instead of a generator with an extra bit set on the instance that changes how it answers isinstance() checks. That would be a big change, though, so it might be better to just document that Awaitable is kind of unusual (if we weren't already in the beta period I might argue that the ABCs should be removed and we should just use the functions in the inspect module instead).
gvanrossum commentedon Jun 7, 2015
I think it's actually good feedback and we should fix this during the beta.
1st1 commentedon Jun 8, 2015
I think that the only proper way to solve this is to make coroutines a separate type. I've actually prototyped that before: 1st1@a3f1059
scoder commentedon Jun 8, 2015
I agree that the "extra bit" is a quirk. Ideally, Coroutine and Generator would share a common base class that has "send", "throw" and "close". Additionally, a Coroutine would be an Awaitable whereas a Generator is an Iterator. The internal implementation of generators and coroutines would (or at least, could) then follow by splitting them into separate types.
The tricky bridge between the two is the types.coroutine() decorator, which is intended to say "this Generator is a Coroutine", i.e. it turns a Generator into an Awaitable Generator. As the current implementation shows, however, this can also be done without the internal flag. All this needs is a wrapper, and that could be implemented in C to make it fast and even special cased to further speed it up.
In a way, the current implementation focusses on runtime optimisation, not on a clean design.
1st1 commentedon Jun 8, 2015
I'll have a patch soon.
1st1 commentedon Jun 9, 2015
Please see the attached patch.
Couple of notes:
It adds a new type: 'coroutine' (PyCoro_Type). The implementation is still heavily based on generators, there are no changes there.
Unfortunately, a new opcode needs to be added - GET_YIELD_FROM_ITER. The idea is that YIELD_FROM from generators decorated with @coroutine must accept
'async def' coroutines. But 'for' loops (iter(), etc) must throw an error on them. On the bright side, because of this new opcode, the ceval code became much easier to follow.
This change removes the need to use __instancecheck__ for 'async def' coroutines. BUT, it's still there for generators decorated with @coroutine. Stefan suggests to wrap generators in a thin wrapper with __await__ method, but I don't like this idea -- it will inevitably make existing asyncio code slower in 3.5; I'm -0.5 on this idea.
While this patch addresses initial request from Ben only partially (generator-based coroutines still require __instancecheck__), I believe that it's still a good idea to separate 'async def' coroutines & generators in 3.5. I'm fully confident that we shouldn't have any issues with that.
scoder commentedon Jun 9, 2015
Looks good, simplifies the code visibly and makes the implementation quite similar to the one I wrote for Cython.
I like the fact that it separates generators from coroutines at an isinstance() level. The fact that "async def" functions currently have the same type as yield-based generators smells too much like an implementation detail to keep it visible at the language level.
1st1 commentedon Jun 9, 2015
Cleaned up the patch a little bit.
1st1 commentedon Jun 9, 2015
One more patch fixing minor bug in types.coroutine + a unittest for that. The patch should be ready for reviews.
scoder commentedon Jun 9, 2015
I added some review comments. The main thing is that the coroutine type should be awaitable.
For reference, here's my current implementation for Cython. It's a bit more involved as it needs to support Python 2.6+. I may also add some special casing for CPython's own coroutine type when compiling in Py3.5 if this change makes it in.
https://github.com/cython/cython/blob/bb0dec2fab91cbde443e6756c3dc29ee009caba7/Cython/Utility/Coroutine.c
1st1 commentedon Jun 9, 2015
Please find attached a new patch.
Stefan, while working on the patch, I (re-)discovered that __await__ for coroutines should return an iterator that also implements '.send', '.throw', and '.close', to comply with PEP-380 yield from implementation: https://www.python.org/dev/peps/pep-0380/#proposal
Please try to compile this python file: https://gist.github.com/1st1/4ee1d072309068dd2798
55 remaining items
vadmium commentedon Jul 2, 2015
The last change to /Doc/conf.py seems to have screwed up my docs build. Was that an accident?
Theme error:
no theme named 'classic' found (missing theme.conf?)
make: *** [build] Error 1
[Exit 2]
python-dev commentedon Jul 2, 2015
New changeset 68996acdec6f by Yury Selivanov in branch '3.5':
docs/conf: Undo changes in b2a3baa1c2b0; issue bpo-24400
https://hg.python.org/cpython/rev/68996acdec6f
1st1 commentedon Jul 2, 2015
Thanks, Martin, it was indeed something that shouldn't been committed.
1st1 commentedon Jul 3, 2015
Guido, Ben, Stefan, Nick,
I want to re-open this issue. I'm still not satisfied with the current state of things, mainly with the __instancecheck__ hack for Awaitable & Coroutine ABCs (as Ben initially suggested).
I think we should remove the __instancecheck__ from the ABCs, and resurrect inspect.isawaitable() function:
abc.Coroutine and abc.Awaitable will guarantee that objects that implement them have '__await__' and it's safe to access it (that means that they will fail for generator-based coroutines).
inspect.isgenerator() can be used along with inspect.isawaitable() to detect generator-based coroutines (generators with CO_ITERABLE_COROUTINE flag).
scoder commentedon Jul 3, 2015
Absolutely. That was the main theme behind the whole type split.
I still don't like the idea of having an inspect.isawaitable() that only
checks for an ABC. If you want an ABC test, say it in your code. If you
want to test for an "__await__" attribute, say it. A helper function
inspect.isawaitable() suggests that there is some (builtin) type to inspect
here, not a protocol. ABCs are about abstract protocols, inspect is about
concrete types.
If you want to cover the "iterable coroutine" case, why not add an inspect
helper function for that? That's clearly a concrete type (even more
concrete than a concrete type) that can be inspected.
1st1 commentedon Jul 3, 2015
Because I view "iterable coroutines" as a temporary, transitional thing.
'inspect.isawaitable()' might be useful in the future even when
we remove CO_ITERABLE_COROUTINE.
ncoghlan commentedon Jul 3, 2015
"might be useful in the future" is an API design red flag that suggests to me we may not know what "good" looks like here yet. At the same time, we need something Tornado et al can use to accept the "can be used with await expressions, but doesn't implement the Awaitable protocol" types.
However, I think we can actually evaluate the consequences of two alternative designs, and decide based on the implications:
a) Add "inspect.isawaitable(obj)", tell people to use that over checking just the ABC, since an interpreter may allow more types than just Awaitable instances.
b) Add an "inspect._islegacyawaitable(obj)" API, and tell people to use "isinstance(obj, Awaitable) or inspect._islegacyawaitable(obj)", rather than just checking the ABC.
I think it makes sense to document that there are types acceptable to the "await" expression that don't implement the Awaitable protocol, just as there are types acceptable to iter() that don't implement the Iterable protocol:
In 3.6, we can add an API like "operator.getfuture()" to expose the implementation of the "retrieve a future from an awaitable" part of the await expression to Python so folks can switch to a "try it and see if it works" approach, rather than having to check with the inspect module.
ncoghlan commentedon Jul 3, 2015
Sorry, I forgot to state my main conclusion in comparing the consequences of adding "inspect.isawaitable" vs adding an API specifically for checking "isn't Awaitable, but can be used in an await expression":
python-dev commentedon Jul 3, 2015
New changeset cb1aafc9ad7e by Yury Selivanov in branch '3.5':
Issue bpo-24400: Resurrect inspect.isawaitable()
https://hg.python.org/cpython/rev/cb1aafc9ad7e
New changeset a14f6a70d013 by Yury Selivanov in branch 'default':
Merge 3.5 (Issue bpo-24400)
https://hg.python.org/cpython/rev/a14f6a70d013
1st1 commentedon Jul 3, 2015
I agree. I've committed the change.
Larry, please make sure that the latest patch lands in 3.5beta3. It's really important.
bdarnell commentedon Jul 3, 2015
I don't think operator.getfuture() is possible because there are multiple ways of turning an awaitable into a Future. asyncio has one way; tornado has another.
ncoghlan commentedon Jul 3, 2015
My hypothetical "operator.getfuture()" would be a functional spelling of
"what an await expression does to retrieve a future from an awaitable
object".
Whether that's actually useful is an open question, hence deferring the
idea to 3.6 at the earliest :)