added taskify(), accepting Futures and coroutines

from discussion in https://groups.google.com/forum/#!topic/python-tulip/KRh7E_WL_hM

[Nikolay Kim, 2013-05-13 18:23:55]

You should assign code review to someone with "Publish+Mail Comments", 'm'
shortcut. assign to GvR or me.

1. you should not modify existing code to use taskify()
2. need tests specifically for taskify()
3. i think taskify() should always return Task object.
4. In general, i dont like taskify() idea, but thats up to Guido to accept
patch.

[agrif, 2013-05-13 19:09:44]

On 2013/05/13 18:23:55, Nikolay Kim wrote:
> You should assign code review to someone with "Publish+Mail Comments", 'm'
> shortcut. assign to GvR or me.

Done. I really do hope this is the last thing I messed up on, workflow-wise.

> 1. you should not modify existing code to use taskify()

I was under the impression that taskify() was to be used wherever Task() is used
now, in rather the same way that Tulip uses 'yield from' even in cases where
'yield' would work (if not for the special guard against it). If it's not used
almost everywhere, then it doesn't solve the original issue I had.

> 2. need tests specifically for taskify()

I'll start adding them, but exactly what tests need adding depends on whether
the existing tests will also use taskify(), which depends on (1.)

> 3. i think taskify() should always return Task object.

I'm honestly uncomfortable about the name, but I haven't been able to come up
with something obviously better. Maybe 'futurify' or 'make_future', to make it
clearer it returns a future? I think it definitely should return a Future,
though, because wrapping a Future in a Task would muddy the original purpose of
Task: to turn coroutines into Futures.

> 4. In general, i dont like taskify() idea, but thats up to Guido to accept
patch.

I'm happy enough just bringing the issue up for discussion. It's fine if I turn
out to be the only one who thinks it's important.

[Nikolay Kim, 2013-05-13 19:18:38]

On May 13, 2013, at 12:09 PM, aargri@gmail.com wrote:

> On 2013/05/13 18:23:55, Nikolay Kim wrote:
>> You should assign code review to someone with "Publish+Mail Comments",
> 'm'
>> shortcut. assign to GvR or me.
> 
> Done. I really do hope this is the last thing I messed up on,
> workflow-wise.
> 
>> 1. you should not modify existing code to use taskify()
> 
> I was under the impression that taskify() was to be used wherever Task()
> is used now, in rather the same way that Tulip uses 'yield from' even in
> cases where 'yield' would work (if not for the special guard against
> it). If it's not used almost everywhere, then it doesn't solve the
> original issue I had.

i see taskify() more as helper function. there is no reason to use it 
if you know explicitly that you have coroutine or future.
and definitely you should not change base_events.py
I am against using taskify() in tulip's code, but thats up to Guido.

> 
>> 2. need tests specifically for taskify()
> 
> I'll start adding them, but exactly what tests need adding depends on
> whether the existing tests will also use taskify(), which depends on
> (1.)

no, thats is not right. you have to add unit tests specifically for taskify()

> 
>> 3. i think taskify() should always return Task object.
> 
> I'm honestly uncomfortable about the name, but I haven't been able to
> come up with something obviously better. Maybe 'futurify' or
> 'make_future', to make it clearer it returns a future? I think it
> definitely should return a Future, though, because wrapping a Future in
> a Task would muddy the original purpose of Task: to turn coroutines into
> Futures.

Task is concurrency primitive. You don't need future to use coroutines.

[agrif, 2013-05-13 19:35:16]

On 2013/05/13 19:18:38, Nikolay Kim wrote:
> i see taskify() more as helper function. there is no reason to use it 
> if you know explicitly that you have coroutine or future.
> and definitely you should not change base_events.py

The only change I made in base_events.py is in a place where you do not
explicitly know if you have a coroutine or a future. I probably couldn't find a
*better* example of where taskify() should be used.

Also, my original issue is basically that you shouldn't have to explicitly know
whether you have a coroutine or a future. Futures have an __iter__ that lets you
use them with 'yield from', specifically so that people don't have to remember
whether to use 'yield' or 'yield from', and (at least in my opinion) so that
both futures and coroutines look the same when you wait on them in a coroutine;
however, they don't look the same when you need to register something to be
called when they finish. taskify() was meant to solve that.

> I am against using taskify() in tulip's code, but thats up to Guido.
> 
> > 
> >> 2. need tests specifically for taskify()
> > 
> > I'll start adding them, but exactly what tests need adding depends on
> > whether the existing tests will also use taskify(), which depends on
> > (1.)
> 
> no, thats is not right. you have to add unit tests specifically for taskify()

Oh, right. For some reason I thought I'd have to add tests to check the behavior
of Task itself but that's not right at all, those are already there.

> > 
> >> 3. i think taskify() should always return Task object.
> > 
> > I'm honestly uncomfortable about the name, but I haven't been able to
> > come up with something obviously better. Maybe 'futurify' or
> > 'make_future', to make it clearer it returns a future? I think it
> > definitely should return a Future, though, because wrapping a Future in
> > a Task would muddy the original purpose of Task: to turn coroutines into
> > Futures.
> 
> Task is concurrency primitive. You don't need future to use coroutines.
> 

I know, but that is the purpose of Task, right? To give you a Future
corresponding to a coroutine? Unless I'm massively misunderstanding something.

[Nikolay Kim, 2013-05-13 22:39:12]

On May 13, 2013, at 12:35 PM, aargri@gmail.com wrote:

> On 2013/05/13 19:18:38, Nikolay Kim wrote:
>> i see taskify() more as helper function. there is no reason to use it
>> if you know explicitly that you have coroutine or future.
>> and definitely you should not change base_events.py
> 
> The only change I made in base_events.py is in a place where you do not
> explicitly know if you have a coroutine or a future. I probably couldn't
> find a *better* example of where taskify() should be used.
> 
> Also, my original issue is basically that you shouldn't have to
> explicitly know whether you have a coroutine or a future. Futures have
> an __iter__ that lets you use them with 'yield from', specifically so
> that people don't have to remember whether to use 'yield' or 'yield
> from', and (at least in my opinion) so that both futures and coroutines
> look the same when you wait on them in a coroutine; however, they don't
> look the same when you need to register something to be called when they
> finish. taskify() was meant to solve that.

there is no difference between future and coroutine in tulip. you can't 'yield'
future and
you can't yield coroutine as well. if you deal with registering callback then
you probably 
should stay at callback api level and don't mix coroutines with callbacks.
i'd like to see real use case where taskify() makes code easier. 


> 
>> I am against using taskify() in tulip's code, but thats up to Guido.
> 
>> >
>> >> 2. need tests specifically for taskify()
>> >
>> > I'll start adding them, but exactly what tests need adding depends
> on
>> > whether the existing tests will also use taskify(), which depends on
>> > (1.)
> 
>> no, thats is not right. you have to add unit tests specifically for
> taskify()
> 
> Oh, right. For some reason I thought I'd have to add tests to check the
> behavior of Task itself but that's not right at all, those are already
> there.
> 
>> >
>> >> 3. i think taskify() should always return Task object.
>> >
>> > I'm honestly uncomfortable about the name, but I haven't been able
> to
>> > come up with something obviously better. Maybe 'futurify' or
>> > 'make_future', to make it clearer it returns a future? I think it
>> > definitely should return a Future, though, because wrapping a Future
> in
>> > a Task would muddy the original purpose of Task: to turn coroutines
> into
>> > Futures.
> 
>> Task is concurrency primitive. You don't need future to use
> coroutines.
> 
> 
> I know, but that is the purpose of Task, right? To give you a Future
> corresponding to a coroutine? Unless I'm massively misunderstanding
> something.

main purpose of Task is to run coroutine concurrently (out of current flow).
Usually, we just create task and make it run. If you need result you just wait
on it with yield from.
again, i don't understand reasons for mixing callbacks and coroutines in
application code.

in any case you should split you patch into two parts.
1. just taskify implementation + tests
2. everything else.
that will make reviewing process faster.

[agrif, 2013-05-14 18:26:36]

I've updated this issue to include just the implementation and tests. Should I
create a new issue for the rest, or wait until this part is over?

On 2013/05/13 22:39:12, Nikolay Kim wrote:
> there is no difference between future and coroutine in tulip. you can't
'yield'
> future and
> you can't yield coroutine as well. if you deal with registering callback then
> you probably 
> should stay at callback api level and don't mix coroutines with callbacks.
> i'd like to see real use case where taskify() makes code easier. 

There are a couple places in tulip where we don't know whether we'll get a
future or a coroutine. Off the top of my head, wait() and as_completed() in
tasks.py, and run_until_complete() in base_events.py.

Really, though, it comes down to this: Tulip tries really hard to make
coroutines and futures look the same to a programmer in coroutine-land. You wait
for them to complete in the same way, using 'yield from'. However, there's no
general way to say "run this, but let me work on other things in the meantime".
With a coroutine you call Task(), but with a future you do nothing. taskify() is
meant to get rid of this difference by completely replacing the public Task()
constructor.

(Incidentally, replacing the public Task() with a wrapper function could also
solve the issue noted in the docstring for run_until_complete() in
base_events.py. A wrapper function could cache the Tasks created for each
coroutine to prevent creating duplicates.)

Sorry for badgering you so much about this; I just feel like I'm not
communicating the issue I see well enough.

[GvR, 2013-05-16 01:04:57]

Whoops, somehow I missed this discussion. I'm adding myself now and will try to
review the code and the discussion.

[GvR, 2013-05-16 01:28:20]

I have a bunch of nits for the code.

I wonder if a good name for this function would be async().  It's short, and it
emphasizes that it can run asynchronously, and it doesn't contain the words
task, future or the ending "ify".

Looking at the first version of the patch, I agree it's wrong to change all the
tests to call taskify() instead of Task().  There is no reason to reduce the
visibility of the Task class or the @task decorator.  However the changes to
replace _wrap_coroutines() with taskify() make sense.  However I'd keep the
_wrap_coroutines() name, just replacing its body with a loop calling taskify(). 
Similar with the change to run_until_complete().

Agrif, are you willing to do another version of the patch?

https://codereview.appspot.com/9245047/diff/8001/tests/tasks_test.py
File tests/tasks_test.py (right):

https://codereview.appspot.com/9245047/diff/8001/tests/tasks_test.py#newcode90
tests/tasks_test.py:90: def test_taskify_future(self):
Even though Task is a subclass of Future, I'd still like to see an explicit test
that tries taskify with a Task.

https://codereview.appspot.com/9245047/diff/8001/tulip/tasks.py
File tulip/tasks.py (right):

https://codereview.appspot.com/9245047/diff/8001/tulip/tasks.py#newcode74
tulip/tasks.py:74: if loop != None or timeout != None:
It should be okay if loop is the same as coro_or_future._loop.

Similar for timeout; that requires Future to store the timeout as self._timeout.

https://codereview.appspot.com/9245047/diff/8001/tulip/tasks.py#newcode75
tulip/tasks.py:75: raise ValueError("Cannot use loop or timeout arguments with a
Future")
Style nits: please break this line to be < 80 chars, and use '' instead of "".

https://codereview.appspot.com/9245047/diff/8001/tulip/tasks.py#newcode80
tulip/tasks.py:80: assert False, 'A Future or coroutine is required'
This should raise TypeError.

[agrif, 2013-05-16 20:55:46]

On 2013/05/16 01:28:20, GvR wrote:
> I wonder if a good name for this function would be async().  It's short, and
it
> emphasizes that it can run asynchronously, and it doesn't contain the words
> task, future or the ending "ify".

Oooh, yes. I've done that rename in this patch set; I hope that doesn't add too
much noise to the diffs. This is the first name that finally felt *right*.

The rest of the changes should be as requested, except for the change in
base_events_test.py: run_until_complete() used to use an `assert False` when it
received an incorrect type (like my implementation of taskify() in patch set 2).
I didn't know whether to leave that assert in, or to fix the test to look for
the TypeError from async() instead. I went with TypeError in the end.

Is there anywhere else in tulip where async() should be used? In my original
post to the mailing list, I thought of using something like async() as the most
common way of creating Task objects. Maybe a better way of putting it now is
"the most common way to signal that a computation can continue asynchronously".
If that's still the goal, there are probably at least a few more places where
Task() should be replaced.

I mentioned it earlier, but using a function like async() as the standard way of
creating Task objects could solve the problem of accidentally creating duplicate
Tasks for a single coroutine object (mentioned in the docstring for
run_until_complete()). As an example, async() could cache the Task as an
attribute on the coroutine object, using weak references.

[Nikolay Kim, 2013-05-16 22:37:28]

On 2013/05/16 20:55:46, agrif wrote:
> On 2013/05/16 01:28:20, GvR wrote:
> > I wonder if a good name for this function would be async().  It's short, and
> it
> > emphasizes that it can run asynchronously, and it doesn't contain the words
> > task, future or the ending "ify".
> 
> Oooh, yes. I've done that rename in this patch set; I hope that doesn't add
too
> much noise to the diffs. This is the first name that finally felt *right*.
> 
> The rest of the changes should be as requested, except for the change in
> base_events_test.py: run_until_complete() used to use an `assert False` when
it
> received an incorrect type (like my implementation of taskify() in patch set
2).
> I didn't know whether to leave that assert in, or to fix the test to look for
> the TypeError from async() instead. I went with TypeError in the end.
> 
> Is there anywhere else in tulip where async() should be used? In my original
> post to the mailing list, I thought of using something like async() as the
most
> common way of creating Task objects. Maybe a better way of putting it now is
> "the most common way to signal that a computation can continue
asynchronously".
> If that's still the goal, there are probably at least a few more places where
> Task() should be replaced.
> 
> I mentioned it earlier, but using a function like async() as the standard way
of
> creating Task objects could solve the problem of accidentally creating
duplicate
> Tasks for a single coroutine object (mentioned in the docstring for
> run_until_complete()). As an example, async() could cache the Task as an
> attribute on the coroutine object, using weak references.

right now async() adds ambiguity. it should always return Task or fail.
tulip.Task() is very explicit what it does. with async() i need to examine
context of call. i think it is not good for public api.

[gvrpython, 2013-05-16 22:55:31]

On Thu, May 16, 2013 at 3:37 PM,  <fafhrd91@gmail.com> wrote:
> right now async() adds ambiguity. it should always return Task or fail.
> tulip.Task() is very explicit what it does. with async() i need to
> examine context of call. i think it is not good for public api.

Maybe that's a matter of documentation? async() always returns a
*Future*. I would have recommended moving the function to futures.py
except that it references Task() if the argument is a coroutine.

I wonder if it might make sense to merge async() and wrap_future()?
That also turns some other asynchronous thing (in that case a
thread-based Future) into a Tulip Future, and an earlier version of it
(the EventLoop method) even accepted a Tulip Future and returned it
unchanged. And there may be other things later that we might want to
convert to Futures.

OTOH I could easily be convinced that this is a case of
hypergeneralization and I won't push it.

> https://codereview.appspot.com/9245047/

-- 
--Guido van Rossum (python.org/~guido)

[GvR, 2013-05-16 23:01:11]

agrif, have you submitted a PSF contributor agreement?
http://www.python.org/psf/contrib/contrib-form/

https://codereview.appspot.com/9245047/diff/21001/tests/base_events_test.py
File tests/base_events_test.py (right):

https://codereview.appspot.com/9245047/diff/21001/tests/base_events_test.py#n...
tests/base_events_test.py:265: def test_run_until_complete_type(self):
Instead of 'type' use 'type_error' or plain 'error'.

[Nikolay Kim, 2013-05-17 16:29:19]

On May 16, 2013, at 3:55 PM, Guido van Rossum <guido@python.org> wrote:

> On Thu, May 16, 2013 at 3:37 PM,  <fafhrd91@gmail.com> wrote:
>> right now async() adds ambiguity. it should always return Task or fail.
>> tulip.Task() is very explicit what it does. with async() i need to
>> examine context of call. i think it is not good for public api.
> 
> Maybe that's a matter of documentation? async() always returns a
> *Future*.

thats where ambiguity is. if i get task then probably all i need just run
coroutine concurrently,
if i get future then i just wait on it. and thats two absolutely different
actions.

> I would have recommended moving the function to futures.py
> except that it references Task() if the argument is a coroutine.
> 
> I wonder if it might make sense to merge async() and wrap_future()?
> That also turns some other asynchronous thing (in that case a
> thread-based Future) into a Tulip Future, and an earlier version of it
> (the EventLoop method) even accepted a Tulip Future and returned it
> unchanged. And there may be other things later that we might want to
> convert to Futures.
> 
> OTOH I could easily be convinced that this is a case of
> hypergeneralization and I won't push it.

i'm not really see use case for async(). if it is for coroutine programing then
we
can use "yield from" instead. if it is for callback programming then this is
probably
too high level method.

[agrif, 2013-05-19 19:25:38]

On 2013/05/16 23:01:11, GvR wrote:
> agrif, have you submitted a PSF contributor agreement?
> http://www.python.org/psf/contrib/contrib-form/

Done. I've updated the patch set as well.

On 2013/05/17 16:29:19, Nikolay Kim wrote:
> On May 16, 2013, at 3:55 PM, Guido van Rossum <mailto:guido@python.org> wrote:
> > Maybe that's a matter of documentation? async() always returns a
> > *Future*.
> 
> thats where ambiguity is. if i get task then probably all i need just run
> coroutine concurrently,
> if i get future then i just wait on it. and thats two absolutely different
> actions.

(#1) yield from foo
     means "if it hasn't been started yet, start doing foo, and wait for it to
finish".

(#2) async(foo)
     means "if it hasn't been started yet, start doing foo, and return
immediately".

(I know it doesn't really mean much to "start" a future, but futures are almost
always associated with some work that has already started somewhere else. Here
I'm thinking of both coroutines and futures as handles for work that needs
doing.)

These are both core ways of interacting with asynchronous jobs. Coroutines and
futures are treated like the same thing in #1, which makes sense, because they
both represent work with an eventual result. But before async() there was no
unified way to do #2. It was confusing (to me at least) to treat them like the
same thing in #1 but not in #2.

[GvR, 2013-05-21 15:34:22]

LGTM.  Agrif, do you want commit privileges so you can check this in yourself? 
Or should I just commit it for you?

[Nikolay Kim, 2013-05-21 16:28:36]

On 2013/05/21 15:34:22, GvR wrote:
> LGTM.  Agrif, do you want commit privileges so you can check this in yourself?

> Or should I just commit it for you?

i still think this will add more confusion than value. Lets not make it public
at least for now.

[gvrpython, 2013-05-21 16:46:33]

Nikolay, what is causing the confusion in your opinion? There are
already two places in the Tulip implementation where this
functionality is useful. I suspect that users writing certain types of
Tulip code will design their own APIs that would benefit from this,
and if it wasn't public they would just reinvent the same code pattern
themselves. The primary use case would be code that needs to adapt
coroutines to a callback-based framework, and want to add completion
callbacks to tasks given to them. They should not have to care about
whether they are being passed a coroutine, a Future, or a Task, and
async() is perfect for this.

--Guido

On Tue, May 21, 2013 at 9:28 AM,  <fafhrd91@gmail.com> wrote:
> On 2013/05/21 15:34:22, GvR wrote:
>>
>> LGTM.  Agrif, do you want commit privileges so you can check this in
>
> yourself?
>>
>> Or should I just commit it for you?
>
>
> i still think this will add more confusion than value. Lets not make it
> public at least for now.
>
> https://codereview.appspot.com/9245047/



-- 
--Guido van Rossum (python.org/~guido)

[Nikolay Kim, 2013-05-21 17:01:14]

My confusion was related to mixing callbacks and yield from. Also i was confused
with discussion about Task() replacement with async().
but if it is only for callbacks then it makes sense. 


On May 21, 2013, at 9:46 AM, Guido van Rossum <guido@python.org> wrote:

> Nikolay, what is causing the confusion in your opinion? There are
> already two places in the Tulip implementation where this
> functionality is useful. I suspect that users writing certain types of
> Tulip code will design their own APIs that would benefit from this,
> and if it wasn't public they would just reinvent the same code pattern
> themselves. The primary use case would be code that needs to adapt
> coroutines to a callback-based framework, and want to add completion
> callbacks to tasks given to them. They should not have to care about
> whether they are being passed a coroutine, a Future, or a Task, and
> async() is perfect for this.
> 
> --Guido
> 
> On Tue, May 21, 2013 at 9:28 AM,  <fafhrd91@gmail.com> wrote:
>> On 2013/05/21 15:34:22, GvR wrote:
>>> 
>>> LGTM.  Agrif, do you want commit privileges so you can check this in
>> 
>> yourself?
>>> 
>>> Or should I just commit it for you?
>> 
>> 
>> i still think this will add more confusion than value. Lets not make it
>> public at least for now.
>> 
>> https://codereview.appspot.com/9245047/
> 
> 
> 
> -- 
> --Guido van Rossum (python.org/~guido)

[gvrpython, 2013-05-21 17:03:38]

Ok, with that cleared up, LGTM again, and I repeat my question to
Aaron: do you want commit privileges to check this in yourself, or
should I do it?

On Tue, May 21, 2013 at 10:01 AM, Nikolay Kim <fafhrd91@gmail.com> wrote:
> My confusion was related to mixing callbacks and yield from. Also i was
confused with discussion about Task() replacement with async().
> but if it is only for callbacks then it makes sense.
>
>
> On May 21, 2013, at 9:46 AM, Guido van Rossum <guido@python.org> wrote:
>
>> Nikolay, what is causing the confusion in your opinion? There are
>> already two places in the Tulip implementation where this
>> functionality is useful. I suspect that users writing certain types of
>> Tulip code will design their own APIs that would benefit from this,
>> and if it wasn't public they would just reinvent the same code pattern
>> themselves. The primary use case would be code that needs to adapt
>> coroutines to a callback-based framework, and want to add completion
>> callbacks to tasks given to them. They should not have to care about
>> whether they are being passed a coroutine, a Future, or a Task, and
>> async() is perfect for this.
>>
>> --Guido
>>
>> On Tue, May 21, 2013 at 9:28 AM,  <fafhrd91@gmail.com> wrote:
>>> On 2013/05/21 15:34:22, GvR wrote:
>>>>
>>>> LGTM.  Agrif, do you want commit privileges so you can check this in
>>>
>>> yourself?
>>>>
>>>> Or should I just commit it for you?
>>>
>>>
>>> i still think this will add more confusion than value. Lets not make it
>>> public at least for now.
>>>
>>> https://codereview.appspot.com/9245047/
>>
>>
>>
>> --
>> --Guido van Rossum (python.org/~guido)
>



-- 
--Guido van Rossum (python.org/~guido)

[agrif, 2013-05-21 17:34:51]

On 2013/05/21 17:03:38, gvrpython wrote:
> Ok, with that cleared up, LGTM again, and I repeat my question to
> Aaron: do you want commit privileges to check this in yourself, or
> should I do it?

You should probably do it. I've barely used mercurial, and I'd hate to make a
silly mistake on the repo.

[gvrpython, 2013-05-21 18:03:02]

On Tue, May 21, 2013 at 10:34 AM,  <aargri@gmail.com> wrote:
> On 2013/05/21 17:03:38, gvrpython wrote:
>>
>> Ok, with that cleared up, LGTM again, and I repeat my question to
>> Aaron: do you want commit privileges to check this in yourself, or
>> should I do it?

> You should probably do it. I've barely used mercurial, and I'd hate to
> make a silly mistake on the repo.

Ok, you can leave making silly mistakes on the repo to me then. :-)

> https://codereview.appspot.com/9245047/

-- 
--Guido van Rossum (python.org/~guido)