Awaitable ABC incompatible with functools.singledispatch

@gvanrossum

BPO	24400
Nosy	@gvanrossum, @ncoghlan, @scoder, @vstinner, @larryhastings, @bitdancer, @asvetlov, @bdarnell, @vadmium, @1st1
Files	corotype.patch corotype.patch corotype.patch corotype.patch corotype.patch corotype.patch corotype.patch: bug fixes & more tests for types.coroutine corotype.patch: Fix compiler warnings; micro optimizations corotype.patch: added documentation corotype.patch: update docstrings and error messages corotype.patch: cr_* slots for coroutine; changes to set_coro_wrapper; new funcs in inspect corotype.patch: fix typo; update whatsnew corotype.patch: docs updates; iscoroutine() no longer uses abc; other fixes per Nick's and Martin's review. corotype.patch: Fix few nits; enable ducktyping for inspect.getcoroutinelocals

^{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']

Nick, Guido, I think we should commit this. While working on this I'm getting more and more confident that it's the right thing to do. I like that coroutines implement __await__ in the latest patch -- PEP-492 just now "clicks" with PEP-380.

A quick scan of which files have been modified suggests the new opcode
still needs docs (although I think the PEP-492 docs in general are still
pending, in which case, this could just be rolled into that)

I'll review the full patch later today (too big to review on my phone)

Nick Coghlan added the comment:

A quick scan of which files have been modified suggests the new opcode
still needs docs (although I think the PEP-492 docs in general are still
pending, in which case, this could just be rolled into that)

Sure. If the patch looks good I'll update it with the docs!

(although I think the PEP-492 docs in general are still pending, in which case, this could just be rolled into that)

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)

While this patch addresses initial request from Ben only partially
(generator-based coroutines still require __instancecheck__),

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?

All this checking for coroutine-ness feels very strange to me. It's anti-duck-typing: [..]

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).

Now, not only must it return a generator with the special coroutine flag, the callable itself must be of the right type [..]

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__".

Attempting to divide generators into awaitable and non-awaitable subsets is a complex solution to a problem that I'm not convinced is real.

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.

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?

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>
wrote:

Yury Selivanov added the comment:

> All this checking for coroutine-ness feels very strange to me. It's
anti-duck-typing: [..]

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).

Anti-duck-typing isn't quite the right word. What I meant was that Python
3.4 had generators that were used as coroutines. When we introduced
__await__ in 3.5, we could have added __await__ as a method for all
generators, since all generators have the necessary send/throw/close
methods. But we didn't, and now we require an explicit marker that a given
object was intended for a particular use instead of inferring that
intention from the set of methods available.

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).

The check for coroutine-ness is not just in asyncio; it happens in the core
interpreter. Tornado has to adapt to this check in order to interoperate
with 'async def' coroutines.

> Now, not only must it return a generator with the special coroutine
flag, the callable itself must be of the right type [..]

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__".

The type of the callable matters for the types.coroutine decorator. In
order to get a coroutine object instead of a generator object, I must apply
types.coroutine to the actual underlying generator, and not any wrapper.

> Attempting to divide generators into awaitable and non-awaitable subsets
is a complex solution to a problem that I'm not convinced is real.

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.

There are three eras of coroutines to consider: 'async def' in 3.5+, 'yield
from' in 3.3-3.4, and 'yield' in 2.5+. I'm trying to provide a path from
'yield' to 'async def'; this compatibility layer will remain relevant as
long as people are migrating from 2.7.

> 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?

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.

I understand that yield-based coroutines can be confusing, but is this
particular kind of confusion bad enough that if someone writes 'await
fibonacci_generator()' we have to raise "TypeError: object generator can't
be used in 'await' expression" instead of "RuntimeError: Task got bad
yield: 1"? (that error message from asyncio could definitely be improved,
but the fact that no one has improved it suggests that the problem isn't
that severe)

My goal here is to make it possible for Tornado applications running on 3.5
to use 'async def' and 'await' even though Tornado itself must remain
compatible with 2.7 (as do many of the libraries that make up the Tornado
ecosystem; I don't want libraries to have to update for compatibility with
this feature). This is doable (I think) but non-trivial in the current
design; making all generators awaitable would make it easier.

On June 9, 2015 at 11:11:11 PM, Ben Darnell (report@bugs.python.org) wrote:
[..]

The type of the callable matters for the types.coroutine decorator. In
order to get a coroutine object instead of a generator object, I must apply
types.coroutine to the actual underlying generator, and not any wrapper.

I get it now, thanks for explaining.

FWIW, I’ve recently updated types.coroutine:

cpython/Lib/types.py

Line 164 in eae2bd7

def coroutine(func):

Instead of unwrapping the callable, it wraps its result with a special
object with __await__. For instance, this enables regular functions
(not generator functions) that are decorated with @types.coruoutine and
return a generator, or a generator-like object compiled with Cython
to work with ‘await’. Maybe this is something you can use in Tornado?
There is also bpo-24325 — to implement most of types.coroutine in C
to make it faster.

I understand that yield-based coroutines can be confusing, but is this
particular kind of confusion bad enough that if someone writes 'await
fibonacci_generator()' we have to raise "TypeError: object generator can't
be used in 'await' expression" instead of "RuntimeError: Task got bad
yield: 1"? (that error message from asyncio could definitely be improved,
but the fact that no one has improved it suggests that the problem isn't
that severe)

My goal here is to make it possible for Tornado applications running on 3.5
to use 'async def' and 'await' even though Tornado itself must remain
compatible with 2.7 (as do many of the libraries that make up the Tornado
ecosystem; I don't want libraries to have to update for compatibility with
this feature). This is doable (I think) but non-trivial in the current
design; making all generators awaitable would make it easier.

I understand. My line of thoughts here is: we are introducing a new
operator. However we define it now, will be set in stone after 3.5 is
released. We should try to solve possible backwards compatibility
issues without making its semantics less strict just to make things
easier.

Please take a look at the recently refactored types.coroutine
(there is a link above).

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?)

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.

I think this is reasonable. I'll try it tomorrow to see if there are any corner cases, but I doubt there are any.

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.

Sure, this can be fixed too.

Could you please update types.coroutine locally and verify that it will work for Tornado?

(related: inspect.iscoroutine is defined in terms of collections.abc.Coroutine. Should inspect.isgenerator be redefined to use the new collections.abc.Generator?)

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:

As Yury notes, more clearly separating coroutines and generators is the
intent of the PEP. The computer could clearly cope with them being the same
class, but humans tended to get confused. We were still blurring that line
too much in the initial implementation, this patch aims to fix that by
*really* introducing a separate class for native coroutines.
Missing integrations in other modules that are likely to result in
misbehaviour in user code (such as the inspect module not accounting for
the new ABCs) are good candidates for filing as behaviour bugs - finding
that kind of gap in PEP implementations is one of the benefits of the beta
period.
Integrating nicely with both asyncio and Tornado is a good pragmatic
design goal to avoid making too many asyncio specific assumptions

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.

With the two changes I described things appear to be working, although I've only done light testing so far.

Glad to hear that! I've attached a new patch fixing types.coroutine per your request.

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.

iscoroutine() is just a newer API than isgenerator(). I'll create a new issue for this soon.

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.

I originally planned to make the next Cython release patch the Generator
and Coroutine ABCs into collections.abc, but I now think it would be worth
uploading an "abc_backports" package to PyPI instead that does that and on
which asyncio, tornado and other packages could simply depend with a
try-import.

It would be nice if that were
encapsulated in inspect.isgenerator().

+1, code that needs exactly a generator, e.g. for "gi_running" and friends,
can still test for isinstance(obj, types.GeneratorType) in a completely
backwards compatible way, which is more explicit anyway.

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.

Yes, it's odd. Either way would work ("types is for types only" vs. "types
includes protocols"), but having both disagree seems wrong.

I think the mere fact that there is a higher level function than
isinstance() suggests that it should really be more high level and not just
doing exactly the same for all times.

FYI I am on vacation and don't have the bandwidth to look into this, so I
hope you will all work together to find a solution without my help.

Low level review sent.

Regarding the new opcode, it appears the main thing it provides is early detection of yielding from a coroutine in a normal generator, which is never going to work properly (a for loop or other iterative construct can't drive a coroutine as it expects to be driven), but would be incredibly painful to debug if you did it accidentally.

For me, that counts as a good enough reason to add a new opcode during the beta period (just as adding the new types is necessary to fix various problems with the original not-actually-a-different-type implementation).

Regarding the idea of doing a typedef for the new coro type at the C level: looking further at the way the new type integrates with the eval loop, it's essential that they actually retain the exact same memory layout if we don't want to rewrite a whole lot of code.

I still like the idea of adding the typedef so we can at least make the distinction in cases where the C level handling *isn't* shared between the two types, it would just need a great big disclaimer saying that the memory layout of the coroutine struct can't be altered independently of the generator layout.

I agree that a typedef is a good idea. It doesn't cost anything but gives
us more freedom for future changes.

Also worth noting: *adding* state to coroutines that isn't present on generators would be fine, they just need to *start* with a layout that exactly matches PyGenObject, since we'll sometimes process them as one.

We don't need that now, but separating the types provides the future option.

One problem with 2015-06-10’s patch (x86-64 Linux):

>>> async def f():
...     pass
... 
>>> c = f()
>>> w = c.__await__()
>>> w
<coroutine_wrapper object at 0x7f1013130b88>
>>> dir(w)
TypeError: object does not provide __dir__
>>> type(w)
Segmentation fault (core dumped)
[Exit 139]

Strangely, if I call w.__dir__() first, everything seems to start behaving properly.

Nick, Stefan, Martin,

Please find an updated patch attached. All issues that you guys found (thanks a lot for the reviews btw!) should be resolved.

Another iteration of the patch is attached. Nick, I think it's ready for your review.

In my last set of review comments, I suggested changing the Python level attributes for coroutine objects to cr_frame, cr_code, and cr_running.

That reminded me that now that coroutines are their own type, we should also give them their own state introspection API, matching the API for generators: https://docs.python.org/3/library/inspect.html#current-state-of-a-generator

New patch is attached.

Updates:

Coroutine type now has 'cr_' slots *both in Python *and* in C.
set_coroutine_wrapper now works *only* on coroutines created by 'async def' functions (generators wrapped with types.coroutine won't be intercepted).
RuntimeWarning that coroutine wasn't ever awaited will be raised *only* for 'async def' coroutines.
Coroutine objects no longer have 'CO_GENERATOR' flag.

Actually, I'm not sure that we should use 'cr_' prefix instead of 'gi_' for coroutines.

Coroutines reusing generators machinery is a two-fold thing: on the one hand it makes the implementation simpler; on the other -- __await__ must return an *iterator*. If you want to push values into __await__, it must return a *generator*. Essentially, as Guido said in one of his emails, we should see PEP-492 as a refinement of 'yield from' and existing generator-based coroutines. I love the idea of separating types for coroutines and generators, but I'm not so sure about 'cr_*' prefix.

Nick, Guido, what do you think about this?

My view is that the "gi_*" prefixed attributes are the ducktyped representation of whether an object is a generator-iterator or a coroutine. With the change, it means "hasattr(obj, 'cr_running')" is to "hasattr(obj, 'gi_running') as "isinstance(obj, collections.abc.Coroutine)" is to "isinstance(obj, collections.abc.Generator)".

The main lesson I personally learned from our experience with PEP-342 is that generator-as-iterator and generator-as-coroutine are fundamentally different concepts, and that having them *look* similar on the surface turned out to be inherently confusing.

I see PEP-492 as the natural consequence of that perspective: from an eval loop perspective, native coroutines and generators-as-coroutines remain essentially interchangeable, but on the surface, they'll now look *completely* different. That allows folks to learn them as two distinct concepts (generator-iterator and coroutine), and defer (perhaps indefinitely) the realisation that they share a common underlying foundation.

Originally we were happy to let that distinction be purely syntactic, and still let the underlying implementation details leak through in the object model. This issue helped highlight that that approach was likely to prove to be a recipe for hard to debug problems as we tried to figure out where we were dealing with generator-iterator objects, native coroutine objects and generator-as-coroutine objects.

Based on that last comment, I realised there's something else @types.coroutine should now do: delegate both the existing gi_* attributes *and* the new cr_* attributes to the underlying generator.

That should allow all of the poking around in code object flags to be encapsulated entirely in the types module, as the 3 kinds of object would subsequently be distinguishable by their attributes:

gi_running: generator-iterator or generator-as-coroutine
cr_running: native coroutine or generator-as-coroutine
both: generator-as-coroutine

OK, let's go with cr_* prefix for coroutine type's slots.

Nick, there is one more thing I'd be glad to receive your input on. Currently, inspect.iscoroutine(o) uses isinstance(o, abc.Coroutine) to do the check. Ben and Stefan reasonably ask to refactor isgenerator() to use abc.Generator.

However, after giving this some thought, I think that inspect.iscoroutine() should actually use isinstance(types.CoroutineType). Lots of existing code assumes, that if isgenerator() returns True, then the object has gi_code etc, so the function returns True when the passed object is a pure Python generator. Same logic should work for iscoroutine(), right?

isawaitable(), however, should continue using abc.Awaitable, since it only checks for __await__ presence on the type (or should we just drop it?)

I believe the key issue of concern for Cython is being able to emulate the native CPython types in a way that things like asyncio "just work", rather than specifically needing to have inspect report them as native generators and coroutines.

As such, having inspect.isgenerator() and inspect.iscoroutine() be consistent in checking specifically for the *native* types makes sense to me, but it should be made clear that they're stricter checks than "isinstance(obj, collections.abc.Generator)" and "isinstance(obj, collections.abc.Coroutine)".

Nick, Martin, here's an updated patch with all your comments addressed.

Latest version looks good to me - I'd suggest we merge this, and file any remaining problems as separate issues.

I added three more comments to the review. The rest looks good to me, too.

An updated patch is attached. I'll commit it tomorrow morning.

New changeset 7a0a1a4ac639 by Yury Selivanov in branch '3.5':
Issue bpo-24400: Introduce a distinct type for 'async def' coroutines.
https://hg.python.org/cpython/rev/7a0a1a4ac639

New changeset 44253ce374fc by Yury Selivanov in branch 'default':
Issue bpo-24400: Merge 3.5
https://hg.python.org/cpython/rev/44253ce374fc

Nick, Martin, Stefan, thanks to all of you for the code review!

New changeset f4b702672beb by Yury Selivanov in branch '3.4':
asyncio: Merge changes from issue bpo-24400.
https://hg.python.org/cpython/rev/f4b702672beb

New changeset 3a78be4bcbde by Yury Selivanov in branch '3.4':
Issue bpo-24400: Fix CoroWrapper for 'async def' coroutines
https://hg.python.org/cpython/rev/3a78be4bcbde

New changeset 338597d2e93b by Yury Selivanov in branch '3.5':
Issue bpo-24400: Fix CoroWrapper for 'async def' coroutines
https://hg.python.org/cpython/rev/338597d2e93b

New changeset 2e4d604c449d by Yury Selivanov in branch 'default':
Issue bpo-24400: Fix CoroWrapper for 'async def' coroutines
https://hg.python.org/cpython/rev/2e4d604c449d

New changeset 8f4e738cb07f by Yury Selivanov in branch '3.5':
Issue bpo-24495, bpo-24400: Test asyncio.Task.repr in debug mode
https://hg.python.org/cpython/rev/8f4e738cb07f

New changeset eb6fb8e2f995 by Yury Selivanov in branch '3.5':
Issue bpo-24325, bpo-24400: Add more unittests for types.coroutine; tweak wrapper implementation.
https://hg.python.org/cpython/rev/eb6fb8e2f995

New changeset 7a2a79362bbe by Yury Selivanov in branch 'default':
Merge 3.5 (Issue bpo-24325, bpo-24400)
https://hg.python.org/cpython/rev/7a2a79362bbe

New changeset 9aee273bf8b7 by Yury Selivanov in branch '3.5':
Issue bpo-24400, bpo-24325: More tests for types._GeneratorWrapper
https://hg.python.org/cpython/rev/9aee273bf8b7

New changeset fa097a336079 by Yury Selivanov in branch 'default':
Merge 3.5 (issue bpo-24325 & bpo-24400)
https://hg.python.org/cpython/rev/fa097a336079

I originally planned to make the next Cython release patch the Generator
and Coroutine ABCs into collections.abc, but I now think it would be worth
uploading an "abc_backports" package to PyPI instead that does that and on
which asyncio, tornado and other packages could simply depend with a
try-import.

Done:

https://pypi.python.org/pypi/backports_abc

Feedback welcome.

Stefan

isawaitable(), however, should continue using abc.Awaitable, since it only checks for __await__ presence on the type (or should we just drop it?)

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.

> isawaitable(), however, should continue using abc.Awaitable, since it only checks for __await__ presence on the type (or should we just drop it?)

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 isinstance(x, Awaitable) may produce different results than issubclass(x.__class__, Awaitable). I'd like to see more explicit documentation of the facts that A) functools.singledispatch is not always equivalent to isinstance() and B) Awaitable is one case (the only one in the stdlib?) where this distinction occurs.

New changeset e20c197f19d6 by Yury Selivanov in branch '3.5':
Issue bpo-24400: Remove inspect.isawaitable().
https://hg.python.org/cpython/rev/e20c197f19d6

New changeset 800bf6a0e0d5 by Yury Selivanov in branch 'default':
Merge 3.5 (Issue bpo-24400)
https://hg.python.org/cpython/rev/800bf6a0e0d5

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':
Issue bpo-24400: Fix failing unittest
https://hg.python.org/cpython/rev/0b7c313851ca

New changeset 8c85291e86bf by Yury Selivanov in branch 'default':
Merge 3.5 (Issue bpo-24400)
https://hg.python.org/cpython/rev/8c85291e86bf

Look like you forgot to adjust test_inspect for the removal. eg:
My bad. Thanks, David!

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':
Issue bpo-24400: Add one more unittest for CoroutineType.__await__
https://hg.python.org/cpython/rev/a9d38701536d

New changeset b2a3baa1c2b0 by Yury Selivanov in branch '3.5':
Issue bpo-24400: Mention that __instancecheck__ is used in abc.Awaitable and Coroutine
https://hg.python.org/cpython/rev/b2a3baa1c2b0

New changeset 4bf1d332fe73 by Yury Selivanov in branch 'default':
Merge 3.5 (issue bpo-24400)
https://hg.python.org/cpython/rev/4bf1d332fe73

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]

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

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:

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).

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.

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.

"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.

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).

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

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.

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
"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 :)

Navigation Menu

Repository navigation

Awaitable ABC incompatible with functools.singledispatch #68588

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Awaitable ABC incompatible with functools.singledispatch #68588

Description

Activity

1st1 commented on Jun 9, 2015

ncoghlan commented on Jun 9, 2015

YurySelivanov commented on Jun 9, 2015

YurySelivanov commented on Jun 9, 2015

bdarnell commented on Jun 10, 2015

1st1 commented on Jun 10, 2015

bdarnell commented on Jun 10, 2015

YurySelivanov commented on Jun 10, 2015

bdarnell commented on Jun 10, 2015

1st1 commented on Jun 10, 2015

ncoghlan commented on Jun 10, 2015

bdarnell commented on Jun 10, 2015

1st1 commented on Jun 10, 2015

scoder commented on Jun 10, 2015

gvanrossum commented on Jun 11, 2015

ncoghlan commented on Jun 12, 2015

ncoghlan commented on Jun 13, 2015

scoder commented on Jun 13, 2015

ncoghlan commented on Jun 13, 2015

vadmium commented on Jun 16, 2015

1st1 commented on Jun 17, 2015

1st1 commented on Jun 18, 2015

ncoghlan commented on Jun 19, 2015

1st1 commented on Jun 19, 2015

1st1 commented on Jun 19, 2015

ncoghlan commented on Jun 20, 2015

ncoghlan commented on Jun 20, 2015

1st1 commented on Jun 20, 2015

ncoghlan commented on Jun 20, 2015

1st1 commented on Jun 20, 2015

ncoghlan commented on Jun 21, 2015

scoder commented on Jun 21, 2015

1st1 commented on Jun 22, 2015

python-dev commented on Jun 22, 2015

1st1 commented on Jun 22, 2015

python-dev commented on Jun 24, 2015

python-dev commented on Jun 24, 2015

python-dev commented on Jun 24, 2015

python-dev commented on Jun 24, 2015

python-dev commented on Jun 24, 2015

scoder commented on Jun 24, 2015

scoder commented on Jun 29, 2015

1st1 commented on Jun 30, 2015

bdarnell commented on Jun 30, 2015

python-dev commented on Jun 30, 2015

bitdancer commented on Jul 1, 2015

python-dev commented on Jul 1, 2015

1st1 commented on Jul 1, 2015

vadmium commented on Jul 1, 2015

python-dev commented on Jul 1, 2015

python-dev commented on Jul 1, 2015

vadmium commented on Jul 2, 2015

python-dev commented on Jul 2, 2015

1st1 commented on Jul 2, 2015

1st1 commented on Jul 3, 2015

scoder commented on Jul 3, 2015

1st1 commented on Jul 3, 2015

ncoghlan commented on Jul 3, 2015

ncoghlan commented on Jul 3, 2015

python-dev commented on Jul 3, 2015

1st1 commented on Jul 3, 2015

bdarnell commented on Jul 3, 2015

ncoghlan commented on Jul 3, 2015

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions