Re: [python-tulip] inspect.iscoroutine only works for "native" coros

[Yury Selivanov]

Hi Justin,

> On Nov 10, 2016, at 6:59 PM, Justin Mayfield  wrote:
> 
> Thank you for your thoughtful and informative reply Yury.
> 
> I'm mostly convinced that doc updates (to which you have already made 
> several) informing users of potential peril is the safest thing to do.  So I 
> won't harp on the issue going passed this reply.
> 
> With that being said, I am still a bit stuck on this belief that 
> CO_ITERABLE_COROUTINE and as you now pointed out collections.abc.Coroutine 
> types should be given the same treatment as native coros where the question 
> being asked is, "Can this be used like a coroutine?”

Yes, and that’s what why we have ‘inspect.isawaitable’ and 
‘asyncio.iscoroutine’ functions.

We still can’t change ‘inspect.iscoroutine' to return ‘True’ for 
CO_ITERABLE_COROUTINE and collection.abc.Coroutine instances.  Reason 1: that 
would break the contract for many programs that are our there right now.  The 
following code will break (and I know such code is out there because I’ve 
written some similar code myself):

  if inspect.iscoroutine(obj):
  if obj.cr_running:
  do_something(obj.cr_await)

Reason 2: is* functions in the inspect module return ‘True’ only for built-in 
types.  We don’t use collections.abc.Generator for inspect.isgenerator(), and 
we don’t want to change that.  Changing only inspect.iscoroutine to use 
collections.abc doesn’t make a lot of sense.


>  From a high level perspective a user won't tend to care if the underlying 
> implementation of a coro is generator based when asking questions about coro 
> protocol.  They just want to know things like if they need to call `await 
> foo()` or `foo()`.  Put another way, it is an implementation detail that some 
> coros are generator based or otherwise (see Cython collections.abc.Coroutine 
> subtypes).
> 
> It's my belief that by increasing the breadth of a test like 
> inspect.iscoroutinefunction() you don't undermine the fact that some of those 
> functions are also generator functions, you merely complement it.  One can 
> still ask if a coro is a generator if needed and by answering yes to an 
> object being a coroutine you aren't (or shouldn't be) altering its generator 
> protocol.  To me that would be the deciding factor for any calls that might 
> be extending their breadth in this regard.  For example, I ask myself, what 
> harm comes from changing the check in `_PyGen_Finalize` to look for all types 
> of coros, or just calling a more open minded implementation of 
> `inspect.iscoroutine`, as a determinant for that super awesome "coroutine __ 
> was never awaited" message?  As far as I can tell it only increases 
> consistency and determinism by reducing variance.

Maybe we could update _PyGen_Finalize to issue warnings for 
CO_ITERABLE_COROUTINE.  We’ll have to make sure that the change makes sense for 
frameworks like curio, which uses generators decorated with types.coroutine to 
implement some core machinery.  And the thing is, it’s too late to make it 
happen in Python 3.6.  

Updating finalization (and inspect.iscoroutine fwiw) in 3.7 won’t affect much, 
because when 3.7 is released I hope almost everybody will switch to async/await 
(and forget about generator-based coroutines).

> 
> Again, thank you for taking the time to walk through this with me.  I'm a 
> huge asyncio fan and love building things with it.  Now it's time to get back 
> to doing just that…

NP!  Please continue working with asyncio and asking questions!

Yury

Re: [python-tulip] inspect.iscoroutine only works for "native" coros

[Justin Mayfield]

Thank you for your thoughtful and informative reply Yury.

I'm mostly convinced that doc updates (to which you have already made
several) informing users of potential peril is the safest thing to do.  So
I won't harp on the issue going passed this reply.

With that being said, I am still a bit stuck on this belief
that CO_ITERABLE_COROUTINE and as you now pointed out
collections.abc.Coroutine types should be given the same treatment as
native coros where the question being asked is, "Can this be used like a
coroutine?"  From a high level perspective a user won't tend to care if the
underlying implementation of a coro is generator based when asking
questions about coro protocol.  They just want to know things like if they
need to call `await foo()` or `foo()`.  Put another way, it is an
implementation detail that some coros are generator based or otherwise (see
Cython collections.abc.Coroutine subtypes).

It's my belief that by increasing the breadth of a test like
inspect.iscoroutinefunction() you don't undermine the fact that some of
those functions are also generator functions, you merely complement it.
One can still ask if a coro is a generator if needed and by answering yes
to an object being a coroutine you aren't (or shouldn't be) altering its
generator protocol.  To me that would be the deciding factor for any calls
that might be extending their breadth in this regard.  For example, I ask
myself, what harm comes from changing the check in `_PyGen_Finalize` to
look for all types of coros, or just calling a more open minded
implementation of `inspect.iscoroutine`, as a determinant for that super
awesome "coroutine __ was never awaited" message?  As far as I can tell it
only increases consistency and determinism by reducing variance.

Again, thank you for taking the time to walk through this with me.  I'm a
huge asyncio fan and love building things with it.  Now it's time to get
back to doing just that...

Cheers,
Justin


On Tue, Nov 8, 2016 at 10:37 AM Yury Selivanov  wrote:

> Justin,
>
> First, thanks a lot for a detailed review of PEP 492/inspect/asyncio docs.
>
> > On Nov 8, 2016, at 4:39 AM, Justin Mayfield  wrote:
> >
> > Reading into these flags more, I wonder if modifications to the
> inspect.iscoroutine* functions as well as to genobject.c's finalizer are
> justified.  Specifically to check for both CO_ITERABLE_COROUTINE and
> CO_COROUTINE in any place where a coroutine is expected.  Note that this is
> an inversion of my last email's statements about setting both flags in
> types.coroutine.  Here is some discovery to help in the discussion...
> >
> > The "New Coroutine Declaration Syntax" section of PEP 482 states,
> "Internally, two new code object flags were introduced: CO_COROUTINE is
> used to mark native coroutines (defined with new syntax).
> CO_ITERABLE_COROUTINE is used to make generator-based coroutines compatible
> with native coroutines (set by types.coroutine() function)." Along with,
> "When a coroutine is garbage collected, a RuntimeWarning is raised if it
> was never awaited on (see also Debugging Features ).”
>
> This is a bug in the PEP, I’ve pushed an update: https://git.io/vXuEQ
>
> >
> > The last comment does not delineate CO_ITERABLE_COROUTINE from
> CO_COROUTINE, which makes me think that the genobject.c finalizer code
> should check for un-awaited coroutines of both types.   There are a few
> other places affected by this including the repr output of objects as seen
> by before and after repr() calls to asyncio.sleep from my previous
> experiment in setting both COROUTINE flags...
> >
> > #stock  behavior..
> >>>> repr(asyncio.sleep(1))
> >''
> >
> > # hacked behavior setting both CO_ITERABLE_COROUTINE and CO_COROUTINE...
> >>>> asyncio.sleep(1)
> >__main__:1: RuntimeWarning: coroutine 'sleep' was never awaited
> >
> >
> > I posit that this last behavior is desirable but should be accomplished
> by finding all places that check for CO_COROUTINE and have them check for
> CO_ITERABLE_COROUTINE too.
>
> I don’t think we should do that.  CO_COROUTINE denotes that the code
> object will return a new distinct type - PyCoro_Type (as defined in
> genobject.h).
>
> CO_ITERABLE_COROUTINE can’t exist on its own, it’s a modifier for
> CO_GENERATOR. Code objects with CO_ITERABLE_COROUTINE will still return
> *generators*, with only one extra feature: they can be awaited.  We decided
> that we don’t want to change generators semantics too much, after all we
> have CoroWrapper & debug mode in asyncio to catch unawaited coroutines.  We
> even use CoroWrapper for native coroutines, because it provides you with a
> more detailed stack trace.
>
> >
> > Lastly, the docstring from inspect.iscoroutinefunction says "Return true
> if the object is a coroutine function.  Coroutine functions are defined
> with "async def" syntax, or generators decorated with `types.coroutine`."
>  What I find problematic here is that types.coroutine 

Re: [python-tulip] inspect.iscoroutine only works for "native" coros

[Justin Mayfield]

Reading into these flags more, I wonder if modifications to the
inspect.iscoroutine* functions as well as to genobject.c's finalizer are
justified.  Specifically to check for both CO_ITERABLE_COROUTINE
and CO_COROUTINE in any place where a coroutine is expected.  Note that
this is an inversion of my last email's statements about setting both flags
in types.coroutine.  Here is some discovery to help in the discussion...

The "New Coroutine Declaration Syntax" section of PEP 482 states, *"Internally,
two new code object flags were introduced: CO_COROUTINE is used to mark
native coroutines (defined with new syntax). CO_ITERABLE_COROUTINE is used
to make generator-based coroutines compatible with native coroutines (set
by types.coroutine() function)."* Along with, *"When a coroutine is garbage
collected, a RuntimeWarning is raised if it was never awaited on (see also
Debugging Features )."*

The last comment does not delineate CO_ITERABLE_COROUTINE
from CO_COROUTINE, which makes me think that the genobject.c finalizer code
should check for un-awaited coroutines of both types.   There are a few
other places affected by this including the repr output of objects as seen
by before and after repr() calls to asyncio.sleep from my previous
experiment in setting both COROUTINE flags...

#stock  behavior..
   >>> repr(asyncio.sleep(1))
   ''

# hacked behavior setting both CO_ITERABLE_COROUTINE and CO_COROUTINE...
   >>> asyncio.sleep(1)
   __main__:1: RuntimeWarning: coroutine 'sleep' was never awaited
   

I posit that this last behavior is desirable but should be accomplished by
finding all places that check for CO_COROUTINE and have them check for
CO_ITERABLE_COROUTINE too.

Lastly, the docstring from inspect.iscoroutinefunction says *"Return true
if the object is a coroutine function.  Coroutine functions are defined
with "async def" syntax, or generators decorated with `types.coroutine`."
  *What I find problematic here is that types.coroutine never produces a
function that is compatible with inspect.iscoroutinefunction.

My suggestion is that we use a mask of something like CO_COROUTINE_ANY
defined as (CO_ITERABLE_COROUTINE & CO_COROUTINE), which can be used
anywhere coroutine tests need to be performed.  I can think of only a few
places where you would need to know if it is native or not and for those
cases I imagine an internal check of the co_flags would suffice.  If not,
perhaps a new function for inspect could cover this, such as
inspect.isnativecoroutinefunction().

Justin



On Tue, Nov 8, 2016 at 12:01 AM Justin Mayfield  wrote:

Comments inline..

>From types.coroutine (which is also called by asyncio.coroutine)...
211 def coroutine(func):
212 """Convert regular generator function to a coroutine."""
213
214 if not callable(func):
215 raise TypeError('types.coroutine() expects a callable')
216
217 if (func.__class__ is FunctionType and
218 getattr(func, '__code__', None).__class__ is CodeType):
219
220 co_flags = func.__code__.co_flags
221
222 # Check if 'func' is a coroutine function.
223 # (0x180 == CO_COROUTINE | CO_ITERABLE_COROUTINE)
224 if co_flags & 0x180:
225 return func
226
227 # Check if 'func' is a generator function.
228 # (0x20 == CO_GENERATOR)
229 if co_flags & 0x20:
230 # TODO: Implement this in C.
231 co = func.__code__
232 func.__code__ = CodeType(
233 co.co_argcount, co.co_kwonlyargcount, co.co_nlocals,
234 co.co_stacksize,
*235 co.co_flags | 0x100,  # 0x100 == CO_ITERABLE_COROUTINE*
236 co.co_code,
237 co.co_consts, co.co_names, co.co_varnames,
co.co_filename,
238 co.co_name, co.co_firstlineno, co.co_lnotab,
co.co_freevars,
239 co.co_cellvars)
240 return func

Line 235: Setting CO_ITERABLE_COROUTINE but not CO_ITERABLE makes
inspect.awaitable(corofunc()) pass but prevents
inspect.iscoroutinefunction(forofunc) from passing.  Setting the flags to
(CO_ITERABLE_COROUTINE & CO_ITERABLE) in this same context actually
produces passing behavior for inspect.iscoroutinefuncion.

In addition, it triggers some other diagnostic code from genobject so you
get some handy messages like, "RuntimeWarning: coroutine 'sleep' was never
awaited".

See (from genobject.c):
  62 /* If `gen` is a coroutine, and if it was never awaited on,
  63issue a RuntimeWarning. */
  64 if (gen->gi_code != NULL &&
  65 ((PyCodeObject *)gen->gi_code)->co_flags & CO_COROUTINE &&
  66 gen->gi_frame->f_lasti == -1) {
  67 if (!error_value) {
  68 PyErr_WarnFormat(PyExc_RuntimeWarning, 1,
  69  "coroutine '%.50S' was never awaited",
  70  gen->gi_qualname);
  71 }
  72 }

My primary question here is about the intended difference
between 

Re: [python-tulip] inspect.iscoroutine only works for "native" coros

[Justin Mayfield]

Comments inline..

>From types.coroutine (which is also called by asyncio.coroutine)...
211 def coroutine(func):
212 """Convert regular generator function to a coroutine."""
213
214 if not callable(func):
215 raise TypeError('types.coroutine() expects a callable')
216
217 if (func.__class__ is FunctionType and
218 getattr(func, '__code__', None).__class__ is CodeType):
219
220 co_flags = func.__code__.co_flags
221
222 # Check if 'func' is a coroutine function.
223 # (0x180 == CO_COROUTINE | CO_ITERABLE_COROUTINE)
224 if co_flags & 0x180:
225 return func
226
227 # Check if 'func' is a generator function.
228 # (0x20 == CO_GENERATOR)
229 if co_flags & 0x20:
230 # TODO: Implement this in C.
231 co = func.__code__
232 func.__code__ = CodeType(
233 co.co_argcount, co.co_kwonlyargcount, co.co_nlocals,
234 co.co_stacksize,
*235 co.co_flags | 0x100,  # 0x100 == CO_ITERABLE_COROUTINE*
236 co.co_code,
237 co.co_consts, co.co_names, co.co_varnames,
co.co_filename,
238 co.co_name, co.co_firstlineno, co.co_lnotab,
co.co_freevars,
239 co.co_cellvars)
240 return func

Line 235: Setting CO_ITERABLE_COROUTINE but not CO_ITERABLE makes
inspect.awaitable(corofunc()) pass but prevents
inspect.iscoroutinefunction(forofunc) from passing.  Setting the flags to
(CO_ITERABLE_COROUTINE & CO_ITERABLE) in this same context actually
produces passing behavior for inspect.iscoroutinefuncion.

In addition, it triggers some other diagnostic code from genobject so you
get some handy messages like, "RuntimeWarning: coroutine 'sleep' was never
awaited".

See (from genobject.c):
  62 /* If `gen` is a coroutine, and if it was never awaited on,
  63issue a RuntimeWarning. */
  64 if (gen->gi_code != NULL &&
  65 ((PyCodeObject *)gen->gi_code)->co_flags & CO_COROUTINE &&
  66 gen->gi_frame->f_lasti == -1) {
  67 if (!error_value) {
  68 PyErr_WarnFormat(PyExc_RuntimeWarning, 1,
  69  "coroutine '%.50S' was never awaited",
  70  gen->gi_qualname);
  71 }
  72 }

My primary question here is about the intended difference
between CO_ITERABLE_COROUTINE and CO_ITERABLE;  How perverse is a change
that naively modifies line 235 of types.coroutine to set both flags?  In my
limited testing it produced better behavior than stock, but I honestly
can't figure out the intended differences between those flags.

On Mon, Nov 7, 2016 at 10:58 PM Yury Selivanov 
wrote:

> On 2016-11-08 12:47 AM, Justin Mayfield wrote:
>
> > That's quite surprising to me too, BTW.  I was focused on generator vs
> > `async def ...` coros and hadn't even tried the legacy decorator.
> >
> > Would it be possible for asyncio code to be altered in a way that
> produces
> > awaitables which conform with inspect's protocol?   Ie. through code
> flags
> > or some other flag that inspect looks for?  I assume a similar stance
> could
> > then be taken by other async libraries.
> There is no protocol here. The inspect module is simply tailored for
> introspection of builtin types. If something mimics a builtin type (a
> coroutine, a generator, or a simple function) inspect utils won't
> recognize it.
>
> > If it's just not in the cards may I humbly suggest the docs for the
> inspect
> > module include some language to help users avoid some of these
> > idiosyncrasies?
>
> Yes, I think we can improve docs: better clarify what is a
> coroutine/coroutine function, reference asyncio functions, add an
> example.  Feel free to submit a patch!
>
> Yury
>
>

Re: [python-tulip] inspect.iscoroutine only works for "native" coros

[Yury Selivanov]

On 2016-11-08 12:47 AM, Justin Mayfield wrote:


That's quite surprising to me too, BTW.  I was focused on generator vs
`async def ...` coros and hadn't even tried the legacy decorator.

Would it be possible for asyncio code to be altered in a way that produces
awaitables which conform with inspect's protocol?   Ie. through code flags
or some other flag that inspect looks for?  I assume a similar stance could
then be taken by other async libraries.
There is no protocol here. The inspect module is simply tailored for 
introspection of builtin types. If something mimics a builtin type (a 
coroutine, a generator, or a simple function) inspect utils won't 
recognize it.



If it's just not in the cards may I humbly suggest the docs for the inspect
module include some language to help users avoid some of these
idiosyncrasies?


Yes, I think we can improve docs: better clarify what is a 
coroutine/coroutine function, reference asyncio functions, add an 
example.  Feel free to submit a patch!


Yury

Re: [python-tulip] inspect.iscoroutine only works for "native" coros

[Justin Mayfield]

That's quite surprising to me too, BTW.  I was focused on generator vs
`async def ...` coros and hadn't even tried the legacy decorator.

Would it be possible for asyncio code to be altered in a way that produces
awaitables which conform with inspect's protocol?   Ie. through code flags
or some other flag that inspect looks for?  I assume a similar stance could
then be taken by other async libraries.

If it's just not in the cards may I humbly suggest the docs for the inspect
module include some language to help users avoid some of these
idiosyncrasies?

Justin

On Mon, Nov 7, 2016 at 10:20 PM Yury Selivanov  wrote:

> Introspection functions from the inspect module are designed for native
> types. Introspection functions from asyncio are designed for what asyncio
> treats as coroutines.  Consider this example:
>
> @asyncio.coroutine
> def coro():
> print(‘hello’)
>
> ‘asyncio.iscoroutinefunction' will return ‘True' for ‘coro’, because it
> knows how specifically asyncio wraps non-generator functions.  To make
> ‘inspect.iscoroutinefunction’ recognize ‘coro’ as a coroutine function we
> would need to make ‘inspect’ aware of asyncio.  But asyncio is just one
> user of async/await and generator-based coroutines.  Twisted and Tornado
> have their own coroutine decorators, we can’t make inspect to recognize
> coroutines from them all.
>
> Yury
>
>
>
> > On Nov 8, 2016, at 12:03 AM, Justin Mayfield  wrote:
> >
> > I know this is documented in PEP 492 but I'm struggling to understand
> why the inspect module has several test functions (iscoroutine,
> iscoroutinefunction, isawaitable) that only work for native coros.
> Comparatively the same functions in the asyncio module work for both native
> and generator based coros.
> >
> > It was an interesting exercise for me to figure this out the hard way so
> it's safe to say there is peril in this distinction for at least one
> developer.  Furthermore, the docs for both sets of functions don't elude to
> these subtle but important distinctions.
> >
> > Are there good reasons for this distinction going forward?
> >
> > Here are some surprising (to me) tests...
> >
> > Python 3.5.2 (default, Oct 14 2016, 12:54:53)
> > [GCC 6.2.1 20160916 (Red Hat 6.2.1-2)] on linux
> > Type "help", "copyright", "credits" or "license" for more information.
> > >>> import inspect
> > >>> import asyncio
> > >>> inspect.iscoroutinefunction(asyncio.sleep)
> > False
> > >>> asyncio.iscoroutinefunction(asyncio.sleep)
> > True
> > >>> inspect.iscoroutine(asyncio.sleep(1))
> > False
> > >>> asyncio.iscoroutine(asyncio.sleep(1))
> > True
> > >>> inspect.isawaitable(asyncio.sleep(1))
> > 256
> >
> >
> > The last test is particularly interesting;  At least it's truthy?
> >
>
>

Re: [python-tulip] inspect.iscoroutine only works for "native" coros

[Yury Selivanov]

Introspection functions from the inspect module are designed for native types. 
Introspection functions from asyncio are designed for what asyncio treats as 
coroutines.  Consider this example:

@asyncio.coroutine
def coro():
print(‘hello’)

‘asyncio.iscoroutinefunction' will return ‘True' for ‘coro’, because it knows 
how specifically asyncio wraps non-generator functions.  To make 
‘inspect.iscoroutinefunction’ recognize ‘coro’ as a coroutine function we would 
need to make ‘inspect’ aware of asyncio.  But asyncio is just one user of 
async/await and generator-based coroutines.  Twisted and Tornado have their own 
coroutine decorators, we can’t make inspect to recognize coroutines from them 
all.

Yury



> On Nov 8, 2016, at 12:03 AM, Justin Mayfield  wrote:
> 
> I know this is documented in PEP 492 but I'm struggling to understand why the 
> inspect module has several test functions (iscoroutine, iscoroutinefunction, 
> isawaitable) that only work for native coros.  Comparatively the same 
> functions in the asyncio module work for both native and generator based 
> coros.
> 
> It was an interesting exercise for me to figure this out the hard way so it's 
> safe to say there is peril in this distinction for at least one developer.  
> Furthermore, the docs for both sets of functions don't elude to these subtle 
> but important distinctions.
> 
> Are there good reasons for this distinction going forward?
> 
> Here are some surprising (to me) tests...
> 
> Python 3.5.2 (default, Oct 14 2016, 12:54:53) 
> [GCC 6.2.1 20160916 (Red Hat 6.2.1-2)] on linux
> Type "help", "copyright", "credits" or "license" for more information.
> >>> import inspect
> >>> import asyncio
> >>> inspect.iscoroutinefunction(asyncio.sleep)
> False
> >>> asyncio.iscoroutinefunction(asyncio.sleep)
> True
> >>> inspect.iscoroutine(asyncio.sleep(1))
> False
> >>> asyncio.iscoroutine(asyncio.sleep(1))
> True
> >>> inspect.isawaitable(asyncio.sleep(1))
> 256
> 
> 
> The last test is particularly interesting;  At least it's truthy?
>