GeezerVison is showing github.com — Awaitable ABC incompatible with functools.singledispatch · Issue #68588 · python/cpython
Awaitable ABC incompatible with functools.singledispatch #68588Description
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'] Activitybdarnell commented on Jun 7, 2015 MannequinAuthor More actions 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 commented on Jun 7, 2015 I think it's actually good feedback and we should fix this during the beta. 1st1 commented on 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 commented on 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 commented on Jun 8, 2015 1st1 commented on 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 commented on 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 commented on Jun 9, 2015 Cleaned up the patch a little bit. 1st1 commented on Jun 9, 2015 One more patch fixing minor bug in types.coroutine + a unittest for that. The patch should be ready for reviews. scoder commented on Jun 9, 2015 1st1 commented on Jun 9, 2015 vadmium commented on Jul 2, 2015 The last change to /Doc/conf.py seems to have screwed up my docs build. Was that an accident?
$ make -C Doc/ htmlsphinx-build -b html -d build/doctrees -D latex_paper_size= . build/html
Running Sphinx v1.2.3
loading pickled environment... done Theme error:
no theme named 'classic' found (missing theme.conf?)
make: *** [build] Error 1
[Exit 2] python-dev commented on Jul 2, 2015 1st1 commented on Jul 2, 2015 Thanks, Martin, it was indeed something that shouldn't been committed. 1st1 commented on 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 commented on Jul 3, 2015
- 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).
Absolutely. That was the main theme behind the whole type split.
- inspect.isgenerator() can be used along with inspect.isawaitable() to detect generator-based coroutines (generators with CO_ITERABLE_COROUTINE flag).
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 commented on Jul 3, 2015
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.
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 commented on 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:
>>> class Seq:
... def __getitem__(self, idx):
... raise IndexError
...
>>> iter(Seq())
<iterator object at 0x7fa6ac596748>
>>> import collections
>>> isinstance(Seq(), collections.Iterable)
False 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 commented on 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":
I think we should add "inspect.isawaitable()", and have it pass for *anything* that can be used in an await expression (whether that's by implementing the Awaitable protocol, or due to special casing at the interpreter level).
python-dev commented on Jul 3, 2015 1st1 commented on Jul 3, 2015
I think we should add "inspect.isawaitable()", and have it pass for *anything* that can be used in an await expression (whether that's by implementing the Awaitable protocol, or due to special casing at the interpreter level).
I agree. I've committed the change.
Larry, please make sure that the latest patch lands in 3.5beta3. It's really important. bdarnell commented on Jul 3, 2015 MannequinAuthor More actions 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 commented on 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 :)
MetadataMetadataDevelopmentNo branches or pull requestsIssue actions
|