gh-77714: Implement as_completed as an asynchronous generator#10251
gh-77714: Implement as_completed as an asynchronous generator#10251mivade wants to merge 5 commits intopython:mainfrom
Conversation
Lib/asyncio/tasks.py
Outdated
| exceptions!) of the original Futures (or coroutines), in the order | ||
| in which and as soon as they complete. | ||
| This differs from PEP 3148; results are yielded from asynchronous | ||
| iteration rather than futures. |
There was a problem hiding this comment.
The new implementation should also return futures, just like the one from concurrent.futures does. The difference is that with async iteration we can return the original futures, which are guaranteed to have completed.
Changing yield future.result() to yield future in __aiter__ makes it correct, and compatible with PEP 3148.
There was a problem hiding this comment.
The existing implementation of as_completed already yields results. I also think yielding results rather than futures is more natural with async for.
There was a problem hiding this comment.
The existing implementation yields futures which you must await.
The idea is to yield futures which are guaranteed to have completed, as they complete. This is how concurrent.futures.as_completed works, but the benefit goes beyond consistency with concurrent.futures. It allows code to be robust in the face of exceptions raised by futures, and to associate side data with the futures and retrieve it from as_completed. For example, when yielding futures, this usage example immediately becomes usable with asyncio.as_completed, just changing for to async for.
The sync version doesn't support that usage because it gives you a different future than any of those passed to as_completed. Among other things, that's what prompted the BPO issue.
As far as I can tell, your implementation can easily support that, just by removing .result() at the yield point.
There was a problem hiding this comment.
You're right, I misread what was going on before. Thanks for the explanation!
|
It has indeed been quite some time! I should be able to take a look at resolving conflicts later this week if there is not a competing PR that would be ready to merge. |
gvanrossum
changed the title
|
@kumaraditya303 Can you review this? I just don't have the time, and you expressed interest in the GH issue before. |
|
What needs to happen for this to get merged? |
|
I’ve asked @serhiy-storchaka for help. |
| # This is *not* a @coroutine! It is just an iterator (yielding Futures). | ||
| def as_completed(fs, *, timeout=None): | ||
| """Return an iterator whose values are coroutines. | ||
| class as_completed(object): |
There was a problem hiding this comment.
| class as_completed(object): | |
| class as_completed: |
Inheriting from object isn't necessary
|
Thank you for your PR, but #22491 is more complete. It returns an iterator (not simply an iterable) and contains tests. So we focused on it. |
|
Glad to see there's movement on addressing this issue! |
12 participants
This implements
asyncio.as_completedwithasync forsupport while keeping the old iteration method for backwards compatibility.This obviously needs new tests. I was also hoping for some additional feedback regarding the implementation.
https://bugs.python.org/issue33533