classification
Title: consider implementing __await__ on concurrent.futures.Future
Type: enhancement Stage: resolved
Components: asyncio, Library (Lib) Versions: Python 3.6
process
Status: closed Resolution: wont fix
Dependencies: Superseder:
Assigned To: yselivanov Nosy List: Yury.Selivanov, alex.gronholm, emptysquare, gvanrossum, scoder, vstinner, yselivanov
Priority: normal Keywords: patch

Created on 2015-06-04 15:38 by yselivanov, last changed 2016-12-16 21:37 by yselivanov. This issue is now closed.

Files
File name Uploaded Description Edit
concurrent.patch yselivanov, 2015-06-04 15:42 review
concurrent.patch yselivanov, 2015-06-04 16:30 review
concurrent.patch yselivanov, 2015-06-04 17:09 review
concurrent_alt.patch yselivanov, 2015-06-04 19:00 review
concurrent.patch yselivanov, 2015-06-04 19:16 review
asyncio_await.patch alex.gronholm, 2015-07-31 23:16 review
concurrent.patch yselivanov, 2015-08-02 14:14 review
concurrent.patch alex.gronholm, 2015-08-02 19:30 review
Messages (39)
msg244832 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2015-06-04 15:42
> Maybe it's possible to give an interpretation to awaiting on a threaded
> Future? __await__ could return a new asyncio Future, and add a completion
> callback to the original Future that makes the asyncio Future ready and
> transfers the result/exception. This would have to use
> loop.call_soon_threadsafe() to transfer control from the exector thread to
> the thread where the loop is running.

It didn't occur to me that we can implement __await__ on concurrent.Future to integrate it *with* asyncio.  I guess it makes sense (although we don't have this kind of integration in 3.4 with 'yield from')

> The only thing I don't know is whether it's possible for __await__ to
> return a Future.

It is -- we just have to return "iter(asyncio.Future())"

Please see the attached patch -- it needs some more tuning, but it demonstrates that the integration is possible.
msg244835 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2015-06-04 16:30
Added a unittest for cancellation
msg244837 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2015-06-04 19:00
Alternative patch with monkeypatching instead of Future subclassing.
msg244838 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2015-06-04 19:04
Sorry, I don't like that either. See my review.
msg244839 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2015-06-04 19:47
Thinking about this more I think we should pass on this for now.
msg244895 - (view) Author: Stefan Behnel (scoder) * Date: 2015-06-06 09:33
(Copying my review comment here so that it doesn't get lost as it's more of a general design thing than a specific review comment.)

Having Future know and deal with asyncio seems wrong to me. asyncio should be able to deal *somehow* with a Future that is being awaited, but a Future shouldn't require asyncio in order to be awaited.
msg247270 - (view) Author: Alex Grönholm (alex.gronholm) * Date: 2015-07-24 11:45
Yes, Yury's approach is wrong here -- Futures should not know about asyncio, but asyncio should be able to handle Futures natively. This seems like the obvious solution to me. Any counterarguments?
msg247271 - (view) Author: Yury Selivanov (Yury.Selivanov) * Date: 2015-07-24 11:54
> Any counterarguments?

There are no counterarguments. There is no obvious way to support concurrent.futures transparently, though:

   await conc_fut

requires conc_fut to implement __await__.

So we either have to implement __await__ for concurrent futures and provide some kind of registry for frameworks, or we can implement a wrapper function:

    await asyncio_compat(conc_fut)

Anyways, concrete ideas and API suggestions are welcome.
msg247786 - (view) Author: Alex Grönholm (alex.gronholm) * Date: 2015-07-31 23:16
Sorry to keep you waiting. This sample code runs fine with the attached patch: https://gist.github.com/agronholm/43c71be0028bb866753a

In short, I implemented support for concurrent.futures in the asyncio Task class instead of making concurrent.futures aware of asyncio. The __await__ implementation in concurrent.futures closely mirrors that of asyncio's Future.
msg247788 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2015-07-31 23:26
> In short, I implemented support for concurrent.futures in the asyncio Task class instead of making concurrent.futures aware of asyncio. The __await__ implementation in concurrent.futures closely mirrors that of asyncio's Future.

I like your approach and the proposed patch. I think we should commit this in 3.6 (the final patch should include docs & tests).
msg247804 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-08-01 10:10
I'm not sure that it's a good idea to start adding special cases in Task
class which is already very complex.

Instead we may add a way to register custom awaitable objects?
msg247806 - (view) Author: Alex Grönholm (alex.gronholm) * Date: 2015-08-01 10:15
I think concurrent.futures.Future warrants adding support for in Task. It predates asyncio and is generic enough. Can you elaborate on what other types you would want to support as awaitables in Task?
msg247807 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-08-01 12:24
There are other implementations of asyncio than the one in CPython.
Pulsar and Tornado define their own Task class. The greenio project
also has a special task object. aioeventlet and aiogevent may also
benefit from a way to register new "awaitable" things.

I'm not opposed to support concurrent.futures.Future object. I don't
like the idea of starting to add a special case for one module. Others
may want to do the same for their library.
msg247812 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2015-08-01 14:33
> There are other implementations of asyncio than the one in CPython.
> Pulsar and Tornado define their own Task class. The greenio project

I'm not sure if it's possible (or even makes any sense) to integrate 
tasks from other frameworks into asyncio.

greenio simply inherits its task class from asyncio.Task, and will
automatically support concurrent.futures if we support them in
asyncio.

> I'm not opposed to support concurrent.futures.Future object. I don't
> like the idea of starting to add a special case for one module. Others
> may want to do the same for their library.

concurrent.futures is in a unique position -- it's already supported and
integrated in asyncio.  We have executors and they are even used for
DNS lookups.  Supporting concurrent.futures in asyncio.Task seems 
natural to me.

I like Alex's approach -- his idea of '__await__' for concurrent.Future
is very generic, so any other framework can integrate it.  Creating
any kind of registry in asyncio.Task seems a bit unnecessary to me at 
this stage.
msg247813 - (view) Author: Stefan Behnel (scoder) * Date: 2015-08-01 15:00
Funny. I just thought about this yesterday and came up with pretty much the
same idea. +1 for the patch.

I don't think there are more classes to support here. Quite the contrary,
other tools should try to integrate via concurrent.futures.Future instead.
msg247817 - (view) Author: Alex Grönholm (alex.gronholm) * Date: 2015-08-01 19:04
I agree with Stefan and Yury. As for the tests, Yury seemed to have those in his patches -- I'll take a look and see if they're directly applicable.

For Python 3.5 and earlier, there is a workaround to awaiting for concurrent Futures, but it requires the use of the undocumented wrap_future() function. It is the fact that it's undocumented that bothers me.
msg247820 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2015-08-01 19:18
> I agree with Stefan and Yury. As for the tests, Yury seemed to have those in his patches -- I'll take a look and see if they're directly applicable.

Good idea, the tests are in a fine shape.

> For Python 3.5 and earlier, there is a workaround to awaiting for concurrent Futures, but it requires the use of the undocumented wrap_future() function. It is the fact that it's undocumented that bothers me.

I didn't even know about it :(  Please create an issue.
msg247867 - (view) Author: Alex Grönholm (alex.gronholm) * Date: 2015-08-02 13:28
Yury, your tests complete even without any patches on cpython default (@74fc1af57c72). How is that possible? I verified that they are run.
msg247868 - (view) Author: Alex Grönholm (alex.gronholm) * Date: 2015-08-02 13:29
Nevermind, I was running the wrong Python version.
msg247869 - (view) Author: Alex Grönholm (alex.gronholm) * Date: 2015-08-02 13:55
I'm having trouble compiling the latest default (@859a45ca1e86). Getting linker errors. I've updated the patch with Yury's tests in it. Would someone mind running them? Apparently they do pass on 3.5b3 at least.
msg247871 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2015-08-02 14:14
Alex, the updated patch is attached.

Guido, do you like the new approach? Can I commit this in 3.6?
msg247874 - (view) Author: Alex Grönholm (alex.gronholm) * Date: 2015-08-02 14:29
Ah hehe, I forgot to actually attach the patch. Thanks Yury.

The asyncio docs need to explicitly mention that concurrent.futures.Futures can be directly awaited from coroutines. Aside from that, I think we're set -- just need Guido to chime in on this.
msg247877 - (view) Author: Stefan Behnel (scoder) * Date: 2015-08-02 15:07
It would be nice to have this applied to 3.5 even. I'm aware that this is a
new feature that shouldn't go in any more at this point, but if it could
get added in 3.5.1 at least, I think it would fill a rather clear and
annoying gap when it comes to tool integration.

If someone explained async/await as a new syntax feature for async
programming to me, and then mentioned that you can't await a Future, I'd be
rather surprised, to put it mildly.
msg247879 - (view) Author: Alex Grönholm (alex.gronholm) * Date: 2015-08-02 15:56
You're right Stefan -- I too was appalled that this was not possible in 3.5 to begin with. It feels completely natural to be able to await for concurrent Futures. But as this is considered a feature, it'll probably have to wait until 3.6. Otherwise you'll end up your app requiring a specific micro-release which is a big no-no in the Python world, as far as features go at least.

But if this is considered a bugfix, it could still go into 3.5.0... *wink wink*. That's what I'm hoping for of course.
msg247882 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2015-08-02 16:48
[Repeat from the review]

I still don't think this should be done for 3.5.0, it's definitely a feature, and there are no more betas planned (rc1 is next weekend).

We may be able to do this for 3.5.1 (since PEP 492 was accepted provisionally, see https://mail.python.org/pipermail/python-dev/2015-May/139844.html) although it's still a pretty big new feature.

We should also have more discussion (on python-dev, IMO, not just in the issue) about whether we want this at all -- I find it questionable to mix await and threads, as I have said several times in the discussion about Nick Coghlan's proposal to introduce helper functions with a similar function.

The argument "but they're both Futures" seems pretty bogus to me.
msg247886 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2015-08-02 18:07
> We may be able to do this for 3.5.1 (since PEP 492 was accepted provisionally, see https://mail.python.org/pipermail/python-dev/2015-May/139844.html) although it's still a pretty big new feature.

I agree.  I'm -1 on pushing this to 3.5.0 or to 3.5.1.
msg247891 - (view) Author: Alex Grönholm (alex.gronholm) * Date: 2015-08-02 19:30
Updated patch per review comments.

I also corrected the order of the lines in the new block. If _must_cancel is True, it would have tried to call cancel() on _fut_waiter before it was set. Now the code matches that of the original block.

The docs don't seem to explicitly say anywhere that they only accept asyncio's Futures and not concurrent Futures, so I'm unsure if any changes are needed there at all.
msg247892 - (view) Author: Stefan Behnel (scoder) * Date: 2015-08-02 19:38
> I find it questionable to mix await and threads, as I have said several
> times in the discussion about Nick Coghlan's proposal to introduce
> helper functions with a similar function.

There are a couple of use cases in the context of asyncio, but definitely
also a major one: blocking database drivers. Normally, applications keep a
pool of connections open to a database and use a bounded size for it in
order to limit the resource usage on server side. Starting a thread pool of
the same size in the application provides a trivial way to make use of
these connections concurrently and efficiently with a blocking database
driver and/or ORM (e.g. SQLAlchemy). "concurrent.futures" makes this really
easy.

> The argument "but they're both Futures" seems pretty bogus to me.

concurrent.futures.Future is not just any Future, it's the one that is in
the standard library, i.e. pretty much the only one that every tool could
agree on (whether or not it knows about or uses asyncio). The fact that
it's thread-safe doesn't really matter to me.
msg247923 - (view) Author: Alex Grönholm (alex.gronholm) * Date: 2015-08-03 11:36
+1. It was specifically SQLAlchemy (but not limited to it -- there are many other blocking APIs) that made me look for a way to easily use threads with native coroutines.

The best workaround I've come up with:

from asyncio import wrap_future

async def foo():
    await wrap_future(executor.submit(...))

But as I mentioned before, wrap_future() is nowhere to be found in the asyncio docs.
msg247925 - (view) Author: Alex Grönholm (alex.gronholm) * Date: 2015-08-03 11:45
Created issue 24785 for the documentation of wrap_future().
msg247928 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2015-08-03 15:03
Every individual use case can be dealt with easily by adding simple helper
functions. I really want to keep async and threads separate. And it's no
coincidence that concurrent.futures is threadsafe; that's part of its spec.
msg247931 - (view) Author: Alex Grönholm (alex.gronholm) * Date: 2015-08-03 16:11
Why do you want to keep threads separate from asyncio? What's the downside here?
The use of helper functions is justifiable when integrating a third party framework or similar with asyncio, but standard library components should just integrate well with each other. Requiring boilerplate wrappers for such use seems unreasonable to me.
msg247935 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2015-08-03 16:28
Sorry, there's no space in this issue for an answer.
msg248200 - (view) Author: Alex Grönholm (alex.gronholm) * Date: 2015-08-07 15:45
Where do we stand with this then? Should I start a thread on python-dev to get the ball rolling?
msg248204 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2015-08-07 16:17
Alex, sure, go ahead.  Although I think python-ideas would be a better choice.
msg248209 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2015-08-07 17:01
You could also withdraw. The more I think about it the more I dislike it. I just don't think we should do *anything* that encourages confusion between threads and tasks. They are fundamentally different concepts and should remain so.
msg248210 - (view) Author: Alex Grönholm (alex.gronholm) * Date: 2015-08-07 17:04
I've already made my case on python-ideas, so let's talk it over there.
msg248932 - (view) Author: Alex Grönholm (alex.gronholm) * Date: 2015-08-21 00:43
I've implemented my background-calling code in my framework via run_in_executor() now, so this has become a non-issue for me. I have no more interest in this patch.
msg283443 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2016-12-16 21:37
I think we should close this one.  Making concurrent.futures awaitable will likely only complicate things for end-users.
History
Date User Action Args
2016-12-16 21:37:12yselivanovsetstatus: open -> closed
resolution: wont fix
messages: + msg283443

stage: resolved
2015-08-21 00:43:42alex.gronholmsetmessages: + msg248932
2015-08-17 20:01:01emptysquaresetnosy: + emptysquare
2015-08-07 17:04:27alex.gronholmsetmessages: + msg248210
2015-08-07 17:01:41gvanrossumsetmessages: + msg248209
2015-08-07 16:17:45yselivanovsetmessages: + msg248204
2015-08-07 15:45:05alex.gronholmsetmessages: + msg248200
2015-08-03 16:28:09gvanrossumsetmessages: + msg247935
2015-08-03 16:11:56alex.gronholmsetmessages: + msg247931
2015-08-03 15:03:11gvanrossumsetmessages: + msg247928
2015-08-03 11:45:37alex.gronholmsetmessages: + msg247925
2015-08-03 11:36:02alex.gronholmsetmessages: + msg247923
2015-08-02 19:38:59scodersetmessages: + msg247892
2015-08-02 19:30:22alex.gronholmsetfiles: + concurrent.patch

messages: + msg247891
2015-08-02 18:07:16yselivanovsetmessages: + msg247886
2015-08-02 16:48:10gvanrossumsetmessages: + msg247882
2015-08-02 15:56:48alex.gronholmsetmessages: + msg247879
2015-08-02 15:07:44scodersetmessages: + msg247877
2015-08-02 14:29:12alex.gronholmsetmessages: + msg247874
2015-08-02 14:14:52yselivanovsetfiles: + concurrent.patch

messages: + msg247871
2015-08-02 13:55:37alex.gronholmsetmessages: + msg247869
2015-08-02 13:29:54alex.gronholmsetmessages: + msg247868
2015-08-02 13:28:59alex.gronholmsetmessages: + msg247867
2015-08-01 19:18:10yselivanovsetmessages: + msg247820
2015-08-01 19:04:38alex.gronholmsetmessages: + msg247817
2015-08-01 15:00:26scodersetmessages: + msg247813
2015-08-01 14:33:33yselivanovsetmessages: + msg247812
2015-08-01 12:24:29vstinnersetmessages: + msg247807
2015-08-01 10:15:25alex.gronholmsetmessages: + msg247806
2015-08-01 10:10:24vstinnersetmessages: + msg247804
2015-07-31 23:26:10yselivanovsetmessages: + msg247788
2015-07-31 23:16:24alex.gronholmsetfiles: + asyncio_await.patch

messages: + msg247786
2015-07-24 11:54:48Yury.Selivanovsetnosy: + Yury.Selivanov
messages: + msg247271
2015-07-24 11:45:07alex.gronholmsetnosy: + alex.gronholm
messages: + msg247270
2015-06-06 09:33:34scodersetmessages: + msg244895
2015-06-04 22:30:48yselivanovsetversions: - Python 3.5
2015-06-04 19:47:32gvanrossumsetmessages: + msg244839
2015-06-04 19:16:12yselivanovsetfiles: + concurrent.patch
2015-06-04 19:04:17gvanrossumsetmessages: + msg244838
2015-06-04 19:00:14yselivanovsetfiles: + concurrent_alt.patch

messages: + msg244837
2015-06-04 17:09:57yselivanovsetfiles: + concurrent.patch
2015-06-04 16:30:21yselivanovsetfiles: + concurrent.patch

messages: + msg244835
2015-06-04 15:45:06yselivanovsetmessages: - msg244834
2015-06-04 15:44:49yselivanovsetmessages: + msg244834
2015-06-04 15:44:18gvanrossumsetmessages: - msg244833
2015-06-04 15:43:48gvanrossumsetmessages: + msg244833
2015-06-04 15:42:58yselivanovsetfiles: + concurrent.patch
keywords: + patch
messages: + msg244832
2015-06-04 15:38:14yselivanovcreate