Awaitable ABC incompatible with functools.singledispatch #68588
Comments
|
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). |
|
I think it's actually good feedback and we should fix this during the beta. |
|
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 |
|
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. |
|
I'll have a patch soon. |
|
Please see the attached patch. Couple of notes:
|
|
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. |
|
Cleaned up the patch a little bit. |
|
One more patch fixing minor bug in types.coroutine + a unittest for that. The patch should be ready for reviews. |
|
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. |
|
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 |
|
A quick scan of which files have been modified suggests the new opcode I'll review the full patch later today (too big to review on my phone) |
Sure. If the patch looks good I'll update it with the docs! |
Actually most of PEP-492 docs are merged already (including new opcodes) via bpo-24180. They can be definitely improved though (I'll try to reserve some time just for that closer to the rc) |
A partial solution doesn't mean much to me: as long as the __instancecheck__ is sometimes necessary, I'll have to use inspect.iscoroutine all the time instead of using singledispatch with Awaitable. If anything, this just magnifies the risk of mysterious failures as things will work with async def but not yield from. I think I'd rather not have the ABC than have one with this kind of quirk. All this checking for coroutine-ness feels very strange to me. It's anti-duck-typing: all generators have all the methods necessary to satisfy the coroutine interface, but you can't use them as coroutines without some magical indication that that's what you meant. Prior to 3.5, a coroutine was just a callable returning a generator. Now, not only must it return a generator with the special coroutine flag, the callable itself must be of the right type, which causes problems when the underlying generator function is wrapped in complex ways (https://github.com/tornadoweb/tornado/blob/2971e857104f8d02fa9107a0e13f50170eb4f30d/tornado/testing.py#L476). Attempting to divide generators into awaitable and non-awaitable subsets is a complex solution to a problem that I'm not convinced is real. Was there a problem in practice with Python 3.4's asyncio in which people used "yield from" in a coroutine with generators that were intended to be iterators instead? |
Why is it "anti-duck-typing"? Awaitable is an object that implements __await__. With this patch coroutines are a separate type with __await__ (although, ceval doesn't use it to make things faster). In asyncio we check for "coroutine-ness" only to raise errors if someone passes a wrong object, or to make @asyncio.coroutine work (which will go away eventually).
Not sure what you mean here. It doesn't matter what callable is. It only matters if it returns a native coroutine, a generator-based coroutine, or an object with "__await__".
Well, 'await' expression is a new operator, and it makes total sense to limit its usage only to awaitables. Awaiting on a generator that yields a fibonacci sequence just doesn't make any sense, and *is* error prone. I think it would be a major mistake to allow this just to make things a little bit more convenient during the transition period. This particular patch does not divide generators in awaitables and non-awaitables, it introduces a new type for 'async def' coroutines. All confusion with old generator-based coroutines and @coroutine decorator is here only because we try to be backwards compatible; the compatibility layer can be removed in 3.7 or 3.8.
Yes, a lot of people were confused where they have coroutines and where they have generators, and this was even mentioned in the Rationale section of the PEP. |
|
On Tue, Jun 9, 2015 at 10:12 PM, Yury Selivanov <report@bugs.python.org>
Anti-duck-typing isn't quite the right word. What I meant was that Python
The check for coroutine-ness is not just in asyncio; it happens in the core
The type of the callable matters for the types.coroutine decorator. In
There are three eras of coroutines to consider: 'async def' in 3.5+, 'yield
I understand that yield-based coroutines can be confusing, but is this My goal here is to make it possible for Tornado applications running on 3.5 |
|
On June 9, 2015 at 11:11:11 PM, Ben Darnell (report@bugs.python.org) wrote:
I get it now, thanks for explaining. FWIW, I’ve recently updated types.coroutine: Line 164 in eae2bd7 Instead of unwrapping the callable, it wraps its result with a special
I understand. My line of thoughts here is: we are introducing a new Please take a look at the recently refactored types.coroutine |
|
GeneratorWrapper helps, but it fails when applied to non-generator functions that return a value (while both tornado.gen.coroutine and asyncio.coroutine take pains to support such usage). The "raise TypeError" should be removed; just return the result without wrapping if it's not a generator. GeneratorWrapper also runs afoul of some places where I do explicit type checking instead of duck typing (isinstance(x, types.GeneratorType)). Using the Generator ABC doesn't work because the generator methods are set as attributes on __init__ instead of defined on the class. The methods should be defined on the class, even if they're overwritten with instance attributes later for speed. (related: inspect.iscoroutine is defined in terms of collections.abc.Coroutine. Should inspect.isgenerator be redefined to use the new collections.abc.Generator?) |
I think this is reasonable. I'll try it tomorrow to see if there are any corner cases, but I doubt there are any.
Sure, this can be fixed too. Could you please update types.coroutine locally and verify that it will work for Tornado?
Since abc.Generator is a new thing I guess we still can do that. Do you have a good use case for it? We need a good one to convince Larry to include this post-beta. |
|
A couple of high level observations:
|
|
With the two changes I described things appear to be working, although I've only done light testing so far. For inspect.isgenerator(), my use case is here: https://github.com/tornadoweb/tornado/blob/2971e857104f8d02fa9107a0e13f50170eb4f30d/tornado/gen.py#L222 I currently do isinstance(x, types.GeneratorType), which will fail if x is actually a GeneratorWrapper. I can change this to use collections.abc.Generator when it exists, but then I'd have to have some conditional logic to switch between collections.abc and types depending on what version I'm running on. It would be nice if that were encapsulated in inspect.isgenerator(). More generally, the inconsistency between isgenerator() and iscoroutine() is kind of odd. I would expect that either all inspect functions or none of them would use a suitable ABC if one exists. |
Glad to hear that! I've attached a new patch fixing types.coroutine per your request.
iscoroutine() is just a newer API than isgenerator(). I'll create a new issue for this soon. |
I originally planned to make the next Cython release patch the Generator
+1, code that needs exactly a generator, e.g. for "gi_running" and friends,
Yes, it's odd. Either way would work ("types is for types only" vs. "types I think the mere fact that there is a higher level function than |
|
FYI I am on vacation and don't have the bandwidth to look into this, so I |
|
New changeset 9aee273bf8b7 by Yury Selivanov in branch '3.5': New changeset fa097a336079 by Yury Selivanov in branch 'default': |
Done: https://pypi.python.org/pypi/backports_abc Feedback welcome. Stefan |
I'd really remove it. It's not referring to an actual type, so it doesn't fit the purpose of the inspect module. The fact that the only place where "collections.abc" is used in this module is in isawaitable() should be a clear indication that it's misplaced. |
+1. I added 'isawaitable()' before we decided to add ABCs, and now it is redundant (and we don't have isiterable, ishashable etc) Ben, I saw that you're using isawaitable() in tornado, but it looks like you can easily switch the code to use Awaitable ABC, right? |
|
Yes, I can switch use the ABC instead, and I agree that it doesn't make sense to have the inspect method if it's going to be equivalent to the ABC. I'm happy with the outcome here but AFAIK the original issue still stands: the Awaitable ABC is unusual in that |
|
New changeset e20c197f19d6 by Yury Selivanov in branch '3.5': New changeset 800bf6a0e0d5 by Yury Selivanov in branch 'default': |
|
Look like you forgot to adjust test_inspect for the removal. eg: http://buildbot.python.org/all/builders/AMD64%20Windows8.1%20Non-Debug%203.x/builds/54 |
|
New changeset 0b7c313851ca by Yury Selivanov in branch '3.5': New changeset 8c85291e86bf by Yury Selivanov in branch 'default': |
|
|
Opened bpo-24541 related to this latest change. The test and documentation are still inconsistent, even if the test passes. |
|
New changeset a9d38701536d by Yury Selivanov in branch '3.5': |
|
New changeset b2a3baa1c2b0 by Yury Selivanov in branch '3.5': New changeset 4bf1d332fe73 by Yury Selivanov in branch 'default': |
|
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... doneTheme error: |
|
New changeset 68996acdec6f by Yury Selivanov in branch '3.5': |
|
Thanks, Martin, it was indeed something that shouldn't been committed. |
|
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:
|
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 If you want to cover the "iterable coroutine" case, why not add an inspect |
Because I view "iterable coroutines" as a temporary, transitional thing. |
|
"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)
FalseIn 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. |
|
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": |
|
New changeset cb1aafc9ad7e by Yury Selivanov in branch '3.5': New changeset a14f6a70d013 by Yury Selivanov in branch 'default': |
I agree. I've committed the change. Larry, please make sure that the latest patch lands in 3.5beta3. It's really important. |
|
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. |
|
My hypothetical "operator.getfuture()" would be a functional spelling of Whether that's actually useful is an open question, hence deferring the |
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: