GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-77714: Implement as_completed as an asynchronous generator by mivade · Pull Request #10251 · python/cpython

Conversation

@mivade

@mivade mivade commented Oct 31, 2018 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

This implements asyncio.as_completed with async for support 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

  • Issue: Provide an async iterator version of as_completed #77714

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@mivade
Implement as_completed as an asynchronous iterator
1e5d05c
@mivade
mivade requested review from 1st1 and asvetlov as code owners October 31, 2018 02:17
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Oct 31, 2018
@bedevere-bot bedevere-bot added the awaiting review label Oct 31, 2018
@mivade
Use timeout handling from previous implementation
67a46d6
hniksic
hniksic reviewed Oct 31, 2018
View reviewed changes
Comment thread Lib/asyncio/tasks.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

hniksic
hniksic reviewed Oct 31, 2018
View reviewed changes
Comment thread 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.

@hniksic hniksic Oct 31, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@mivade mivade Oct 31, 2018

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The existing implementation of as_completed already yields results. I also think yielding results rather than futures is more natural with async for.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@hniksic hniksic Oct 31, 2018

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@mivade mivade Nov 1, 2018

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, I misread what was going on before. Thanks for the explanation!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
mivade added 2 commits October 31, 2018 08:46
@mivade
Fix import
52b10da
@mivade
Yield a future with async for
ab8e762
@JustinTArthur JustinTArthur mentioned this pull request Oct 2, 2020
gh-77714: Provide an async iterator version of as_completed #22491
Merged
@ezio-melotti ezio-melotti removed the CLA signed label Jul 13, 2022
@hniksic hniksic mannequin mentioned this pull request Apr 10, 2022
Provide an async iterator version of as_completed #77714
Closed
@iritkatriel iritkatriel added the topic-asyncio label Dec 6, 2022
willingc
willingc reviewed Apr 26, 2023
View reviewed changes

@willingc willingc left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @mivade, Thanks for the PR. Since this PR has been open for a while, would you prefer to close the PR or resolve the CI errors? Thanks.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@willingc willingc added awaiting changes and removed awaiting review labels Apr 26, 2023
@mivade

mivade commented May 1, 2023

Copy link
Copy Markdown
Contributor Author

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.

arhadthedev, willingc, and bvd0 reacted with thumbs up emoji
All reactions
  • 👍 3 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mivade
Merge branch 'main' into async-as-completed
03b0561
@mivade
mivade requested review from gvanrossum and kumaraditya303 as code owners May 7, 2023 15:47
@gvanrossum gvanrossum changed the title bpo-33533: Implement as_completed as an asynchronous generator gh-77714: Implement as_completed as an asynchronous generator May 7, 2023
@bedevere-bot bedevere-bot added awaiting review and removed awaiting changes labels May 7, 2023
@gvanrossum

gvanrossum commented May 7, 2023

Copy link
Copy Markdown
Member

@kumaraditya303 Can you review this? I just don't have the time, and you expressed interest in the GH issue before.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@kumaraditya303 kumaraditya303 added the 3.13 only security fixes label May 10, 2023
@kumaraditya303
kumaraditya303 removed request for 1st1 and asvetlov May 10, 2023 09:53
@bvd0

bvd0 commented Dec 16, 2023

Copy link
Copy Markdown

What needs to happen for this to get merged?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@gvanrossum

gvanrossum commented Dec 16, 2023

Copy link
Copy Markdown
Member

I’ve asked @serhiy-storchaka for help.

bvd0 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@serhiy-storchaka
serhiy-storchaka self-requested a review December 16, 2023 17:35
Gobot1234
Gobot1234 reviewed Dec 16, 2023
View reviewed changes
Comment thread Lib/asyncio/tasks.py
# 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):

@Gobot1234 Gobot1234 Dec 16, 2023

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
class as_completed(object):
class as_completed:

Inheriting from object isn't necessary

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bvd0 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@serhiy-storchaka

serhiy-storchaka commented Dec 24, 2023

Copy link
Copy Markdown
Member

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.

mivade reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@serhiy-storchaka serhiy-storchaka closed this Dec 24, 2023
@mivade

mivade commented Dec 24, 2023

Copy link
Copy Markdown
Contributor Author

Glad to see there's movement on addressing this issue!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@willingc willingc willingc left review comments
@gvanrossum gvanrossum Awaiting requested review from gvanrossum
@kumaraditya303 kumaraditya303 Awaiting requested review from kumaraditya303 kumaraditya303 is a code owner
@serhiy-storchaka serhiy-storchaka Awaiting requested review from serhiy-storchaka
+2 more reviewers
@hniksic hniksic hniksic left review comments
@Gobot1234 Gobot1234 Gobot1234 left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

3.13 only security fixes awaiting review topic-asyncio

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

12 participants

@mivade @gvanrossum @bvd0 @serhiy-storchaka @hniksic @willingc @Gobot1234 @iritkatriel @the-knights-who-say-ni @ezio-melotti @bedevere-bot @kumaraditya303
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.