New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
consider implementing __await__ on concurrent.futures.Future #68571
Comments
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')
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. |
Added a unittest for cancellation |
Alternative patch with monkeypatching instead of Future subclassing. |
Sorry, I don't like that either. See my review. |
Thinking about this more I think we should pass on this for now. |
(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. |
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? |
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:
Anyways, concrete ideas and API suggestions are welcome. |
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. |
I like your approach and the proposed patch. I think we should commit this in 3.6 (the final patch should include docs & tests). |
I'm not sure that it's a good idea to start adding special cases in Task Instead we may add a way to register custom awaitable objects? |
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? |
There are other implementations of asyncio than the one in CPython. I'm not opposed to support concurrent.futures.Future object. I don't |
I'm not sure if it's possible (or even makes any sense) to integrate greenio simply inherits its task class from asyncio.Task, and will
concurrent.futures is in a unique position -- it's already supported and I like Alex's approach -- his idea of '__await__' for concurrent.Future |
Funny. I just thought about this yesterday and came up with pretty much the I don't think there are more classes to support here. Quite the contrary, |
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. |
Good idea, the tests are in a fine shape.
I didn't even know about it :( Please create an issue. |
Yury, your tests complete even without any patches on cpython default (@74fc1af57c72). How is that possible? I verified that they are run. |
Nevermind, I was running the wrong Python version. |
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. |
Alex, the updated patch is attached. Guido, do you like the new approach? Can I commit this in 3.6? |
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. |
It would be nice to have this applied to 3.5 even. I'm aware that this is a If someone explained async/await as a new syntax feature for async |
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. |
[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. |
I agree. I'm -1 on pushing this to 3.5.0 or to 3.5.1. |
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. |
There are a couple of use cases in the context of asyncio, but definitely
concurrent.futures.Future is not just any Future, it's the one that is in |
+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. |
Created bpo-24785 for the documentation of wrap_future(). |
Every individual use case can be dealt with easily by adding simple helper |
Why do you want to keep threads separate from asyncio? What's the downside here? |
Sorry, there's no space in this issue for an answer. |
Where do we stand with this then? Should I start a thread on python-dev to get the ball rolling? |
Alex, sure, go ahead. Although I think python-ideas would be a better choice. |
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. |
I've already made my case on python-ideas, so let's talk it over there. |
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. |
I think we should close this one. Making concurrent.futures awaitable will likely only complicate things for end-users. |
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:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: