msg244832 - (view) |
Author: Yury Selivanov (yselivanov) * |
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) * |
Date: 2015-06-04 16:30 |
Added a unittest for cancellation
|
msg244837 - (view) |
Author: Yury Selivanov (yselivanov) * |
Date: 2015-06-04 19:00 |
Alternative patch with monkeypatching instead of Future subclassing.
|
msg244838 - (view) |
Author: Guido van Rossum (gvanrossum) * |
Date: 2015-06-04 19:04 |
Sorry, I don't like that either. See my review.
|
msg244839 - (view) |
Author: Guido van Rossum (gvanrossum) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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.
|
|
Date |
User |
Action |
Args |
2022-04-11 14:58:17 | admin | set | github: 68571 |
2016-12-16 21:37:12 | yselivanov | set | status: open -> closed resolution: wont fix messages:
+ msg283443
stage: resolved |
2015-08-21 00:43:42 | alex.gronholm | set | messages:
+ msg248932 |
2015-08-17 20:01:01 | emptysquare | set | nosy:
+ emptysquare
|
2015-08-07 17:04:27 | alex.gronholm | set | messages:
+ msg248210 |
2015-08-07 17:01:41 | gvanrossum | set | messages:
+ msg248209 |
2015-08-07 16:17:45 | yselivanov | set | messages:
+ msg248204 |
2015-08-07 15:45:05 | alex.gronholm | set | messages:
+ msg248200 |
2015-08-03 16:28:09 | gvanrossum | set | messages:
+ msg247935 |
2015-08-03 16:11:56 | alex.gronholm | set | messages:
+ msg247931 |
2015-08-03 15:03:11 | gvanrossum | set | messages:
+ msg247928 |
2015-08-03 11:45:37 | alex.gronholm | set | messages:
+ msg247925 |
2015-08-03 11:36:02 | alex.gronholm | set | messages:
+ msg247923 |
2015-08-02 19:38:59 | scoder | set | messages:
+ msg247892 |
2015-08-02 19:30:22 | alex.gronholm | set | files:
+ concurrent.patch
messages:
+ msg247891 |
2015-08-02 18:07:16 | yselivanov | set | messages:
+ msg247886 |
2015-08-02 16:48:10 | gvanrossum | set | messages:
+ msg247882 |
2015-08-02 15:56:48 | alex.gronholm | set | messages:
+ msg247879 |
2015-08-02 15:07:44 | scoder | set | messages:
+ msg247877 |
2015-08-02 14:29:12 | alex.gronholm | set | messages:
+ msg247874 |
2015-08-02 14:14:52 | yselivanov | set | files:
+ concurrent.patch
messages:
+ msg247871 |
2015-08-02 13:55:37 | alex.gronholm | set | messages:
+ msg247869 |
2015-08-02 13:29:54 | alex.gronholm | set | messages:
+ msg247868 |
2015-08-02 13:28:59 | alex.gronholm | set | messages:
+ msg247867 |
2015-08-01 19:18:10 | yselivanov | set | messages:
+ msg247820 |
2015-08-01 19:04:38 | alex.gronholm | set | messages:
+ msg247817 |
2015-08-01 15:00:26 | scoder | set | messages:
+ msg247813 |
2015-08-01 14:33:33 | yselivanov | set | messages:
+ msg247812 |
2015-08-01 12:24:29 | vstinner | set | messages:
+ msg247807 |
2015-08-01 10:15:25 | alex.gronholm | set | messages:
+ msg247806 |
2015-08-01 10:10:24 | vstinner | set | messages:
+ msg247804 |
2015-07-31 23:26:10 | yselivanov | set | messages:
+ msg247788 |
2015-07-31 23:16:24 | alex.gronholm | set | files:
+ asyncio_await.patch
messages:
+ msg247786 |
2015-07-24 11:54:48 | Yury.Selivanov | set | nosy:
+ Yury.Selivanov messages:
+ msg247271
|
2015-07-24 11:45:07 | alex.gronholm | set | nosy:
+ alex.gronholm messages:
+ msg247270
|
2015-06-06 09:33:34 | scoder | set | messages:
+ msg244895 |
2015-06-04 22:30:48 | yselivanov | set | versions:
- Python 3.5 |
2015-06-04 19:47:32 | gvanrossum | set | messages:
+ msg244839 |
2015-06-04 19:16:12 | yselivanov | set | files:
+ concurrent.patch |
2015-06-04 19:04:17 | gvanrossum | set | messages:
+ msg244838 |
2015-06-04 19:00:14 | yselivanov | set | files:
+ concurrent_alt.patch
messages:
+ msg244837 |
2015-06-04 17:09:57 | yselivanov | set | files:
+ concurrent.patch |
2015-06-04 16:30:21 | yselivanov | set | files:
+ concurrent.patch
messages:
+ msg244835 |
2015-06-04 15:45:06 | yselivanov | set | messages:
- msg244834 |
2015-06-04 15:44:49 | yselivanov | set | messages:
+ msg244834 |
2015-06-04 15:44:18 | gvanrossum | set | messages:
- msg244833 |
2015-06-04 15:43:48 | gvanrossum | set | messages:
+ msg244833 |
2015-06-04 15:42:58 | yselivanov | set | files:
+ concurrent.patch keywords:
+ patch messages:
+ msg244832
|
2015-06-04 15:38:14 | yselivanov | create | |