Skip to content
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

Closed
1st1 opened this issue Jun 4, 2015 · 39 comments
Closed

consider implementing __await__ on concurrent.futures.Future #68571

1st1 opened this issue Jun 4, 2015 · 39 comments
Assignees
Labels
stdlib Python modules in the Lib dir topic-asyncio type-feature A feature request or enhancement

Comments

@1st1
Copy link
Member

1st1 commented Jun 4, 2015

BPO 24383
Nosy @gvanrossum, @scoder, @vstinner, @agronholm, @1st1, @ajdavis
Files
  • concurrent.patch
  • concurrent.patch
  • concurrent.patch
  • concurrent_alt.patch
  • concurrent.patch
  • asyncio_await.patch
  • concurrent.patch
  • concurrent.patch
  • 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:

    assignee = 'https://github.com/1st1'
    closed_at = <Date 2016-12-16.21:37:12.452>
    created_at = <Date 2015-06-04.15:38:14.031>
    labels = ['type-feature', 'library', 'expert-asyncio']
    title = 'consider implementing __await__ on concurrent.futures.Future'
    updated_at = <Date 2016-12-16.21:37:12.451>
    user = 'https://github.com/1st1'

    bugs.python.org fields:

    activity = <Date 2016-12-16.21:37:12.451>
    actor = 'yselivanov'
    assignee = 'yselivanov'
    closed = True
    closed_date = <Date 2016-12-16.21:37:12.452>
    closer = 'yselivanov'
    components = ['Library (Lib)', 'asyncio']
    creation = <Date 2015-06-04.15:38:14.031>
    creator = 'yselivanov'
    dependencies = []
    files = ['39620', '39621', '39622', '39623', '39624', '40090', '40103', '40106']
    hgrepos = []
    issue_num = 24383
    keywords = ['patch']
    message_count = 39.0
    messages = ['244832', '244835', '244837', '244838', '244839', '244895', '247270', '247271', '247786', '247788', '247804', '247806', '247807', '247812', '247813', '247817', '247820', '247867', '247868', '247869', '247871', '247874', '247877', '247879', '247882', '247886', '247891', '247892', '247923', '247925', '247928', '247931', '247935', '248200', '248204', '248209', '248210', '248932', '283443']
    nosy_count = 7.0
    nosy_names = ['gvanrossum', 'scoder', 'vstinner', 'alex.gronholm', 'Yury.Selivanov', 'yselivanov', 'emptysquare']
    pr_nums = []
    priority = 'normal'
    resolution = 'wont fix'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue24383'
    versions = ['Python 3.6']

    @1st1 1st1 self-assigned this Jun 4, 2015
    @1st1 1st1 added stdlib Python modules in the Lib dir topic-asyncio type-feature A feature request or enhancement labels Jun 4, 2015
    @1st1
    Copy link
    Member Author

    1st1 commented Jun 4, 2015

    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.

    @1st1
    Copy link
    Member Author

    1st1 commented Jun 4, 2015

    Added a unittest for cancellation

    @1st1
    Copy link
    Member Author

    1st1 commented Jun 4, 2015

    Alternative patch with monkeypatching instead of Future subclassing.

    @gvanrossum
    Copy link
    Member

    Sorry, I don't like that either. See my review.

    @gvanrossum
    Copy link
    Member

    Thinking about this more I think we should pass on this for now.

    @scoder
    Copy link
    Contributor

    scoder commented Jun 6, 2015

    (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.

    @agronholm
    Copy link
    Mannequin

    agronholm mannequin commented Jul 24, 2015

    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?

    @YurySelivanov
    Copy link
    Mannequin

    YurySelivanov mannequin commented Jul 24, 2015

    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.

    @agronholm
    Copy link
    Mannequin

    agronholm mannequin commented Jul 31, 2015

    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.

    @1st1
    Copy link
    Member Author

    1st1 commented Jul 31, 2015

    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).

    @vstinner
    Copy link
    Member

    vstinner commented Aug 1, 2015

    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?

    @agronholm
    Copy link
    Mannequin

    agronholm mannequin commented Aug 1, 2015

    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?

    @vstinner
    Copy link
    Member

    vstinner commented Aug 1, 2015

    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.

    @1st1
    Copy link
    Member Author

    1st1 commented Aug 1, 2015

    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.

    @scoder
    Copy link
    Contributor

    scoder commented Aug 1, 2015

    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.

    @agronholm
    Copy link
    Mannequin

    agronholm mannequin commented Aug 1, 2015

    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.

    @1st1
    Copy link
    Member Author

    1st1 commented Aug 1, 2015

    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.

    @agronholm
    Copy link
    Mannequin

    agronholm mannequin commented Aug 2, 2015

    Yury, your tests complete even without any patches on cpython default (@74fc1af57c72). How is that possible? I verified that they are run.

    @agronholm
    Copy link
    Mannequin

    agronholm mannequin commented Aug 2, 2015

    Nevermind, I was running the wrong Python version.

    @agronholm
    Copy link
    Mannequin

    agronholm mannequin commented Aug 2, 2015

    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.

    @1st1
    Copy link
    Member Author

    1st1 commented Aug 2, 2015

    Alex, the updated patch is attached.

    Guido, do you like the new approach? Can I commit this in 3.6?

    @agronholm
    Copy link
    Mannequin

    agronholm mannequin commented Aug 2, 2015

    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.

    @scoder
    Copy link
    Contributor

    scoder commented Aug 2, 2015

    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.

    @agronholm
    Copy link
    Mannequin

    agronholm mannequin commented Aug 2, 2015

    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.

    @gvanrossum
    Copy link
    Member

    [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.

    @1st1
    Copy link
    Member Author

    1st1 commented Aug 2, 2015

    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.

    @agronholm
    Copy link
    Mannequin

    agronholm mannequin commented Aug 2, 2015

    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.

    @scoder
    Copy link
    Contributor

    scoder commented Aug 2, 2015

    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.

    @agronholm
    Copy link
    Mannequin

    agronholm mannequin commented Aug 3, 2015

    +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.

    @agronholm
    Copy link
    Mannequin

    agronholm mannequin commented Aug 3, 2015

    Created bpo-24785 for the documentation of wrap_future().

    @gvanrossum
    Copy link
    Member

    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.

    @agronholm
    Copy link
    Mannequin

    agronholm mannequin commented Aug 3, 2015

    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.

    @gvanrossum
    Copy link
    Member

    Sorry, there's no space in this issue for an answer.

    @agronholm
    Copy link
    Mannequin

    agronholm mannequin commented Aug 7, 2015

    Where do we stand with this then? Should I start a thread on python-dev to get the ball rolling?

    @1st1
    Copy link
    Member Author

    1st1 commented Aug 7, 2015

    Alex, sure, go ahead. Although I think python-ideas would be a better choice.

    @gvanrossum
    Copy link
    Member

    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.

    @agronholm
    Copy link
    Mannequin

    agronholm mannequin commented Aug 7, 2015

    I've already made my case on python-ideas, so let's talk it over there.

    @agronholm
    Copy link
    Mannequin

    agronholm mannequin commented Aug 21, 2015

    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.

    @1st1
    Copy link
    Member Author

    1st1 commented Dec 16, 2016

    I think we should close this one. Making concurrent.futures awaitable will likely only complicate things for end-users.

    @1st1 1st1 closed this as completed Dec 16, 2016
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    stdlib Python modules in the Lib dir topic-asyncio type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants