asyncio.as_completed documentation misleading

[bharel]

BPO	40405
Nosy	@gvanrossum, @mitsuhiko, @asvetlov, @hynek, @1st1, @bharel, @miss-islington, @tirkarthi, @aeros
PRs	bpo-40405: Fix asyncio.as_completed docs #19753 [3.9] bpo-40405: Fix asyncio.as_completed docs (GH-19753) #20337 [3.8] bpo-40405: Fix asyncio.as_completed docs (GH-19753) #20338

^{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 = None
closed_at = <Date 2020-05-23.23:25:56.625>
created_at = <Date 2020-04-27.11:14:50.589>
labels = ['3.8', 'docs', '3.7', '3.9', 'expert-asyncio']
title = 'asyncio.as_completed documentation misleading'
updated_at = <Date 2020-05-23.23:25:56.624>
user = 'https://github.com/bharel'

bugs.python.org fields:

activity = <Date 2020-05-23.23:25:56.624>
actor = 'aeros'
assignee = 'docs@python'
closed = True
closed_date = <Date 2020-05-23.23:25:56.625>
closer = 'aeros'
components = ['Documentation', 'asyncio']
creation = <Date 2020-04-27.11:14:50.589>
creator = 'bar.harel'
dependencies = []
files = []
hgrepos = []
issue_num = 40405
keywords = ['patch']
message_count = 16.0
messages = ['367410', '367430', '367436', '367437', '367438', '367485', '367621', '367622', '367625', '367626', '367630', '367733', '369757', '369758', '369759', '369760']
nosy_count = 10.0
nosy_names = ['gvanrossum', 'aronacher', 'asvetlov', 'docs@python', 'hynek', 'yselivanov', 'bar.harel', 'miss-islington', 'xtreak', 'aeros']
pr_nums = ['19753', '20337', '20338']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue40405'
versions = ['Python 3.7', 'Python 3.8', 'Python 3.9']

[bharel]

Continuing with bpo-27589, looks like as_completed documentation is still misleading. According to the docs, it "Return(s) an iterator of Future objects. Each Future object returned represents the earliest result from the set of the remaining awaitables."

There's only one problem: The only thing it definitely doesn't do, is return an iterator of future objects. To be honest with you, I'm not entirely sure how to phrase it.

For reference, I fell for this:

mapping = {fut: index for fut, index in enumerate(futures)}
for fut in as_completed(mapping):
  mapping[fut]  # KeyError

[gvanrossum]

I declare this not a bug.

The docs do not promise that the Futures being returned are the *same* Futures that were passed in. They are not. They are (or at least may be) new Futures that represent the same event. Since Futures, when used as dict keys, use identity as equality, those new Futures will not be present as keys in the mapping of Futures passed in by the OP.

[gvanrossum]

Reopening so as to give the OP one more chance to state their case. They wrote:

"""
You've immediately closed the issue so I couldn't even reply to it,
Unfortunately, it doesn't return a Future object at all, so technically you're wrong, together with the docs of course, which was the bug I reported...
"""

If it's not a Future then what? You're not showing that in your report.

[bharel]

It's a coroutine. Basically the same coroutine yielded over and over, returning the first future's result each time.
Like I said, I'm not entirely sure how to phrase it.
Maybe "Returns an iterator of awaitables"?

[gvanrossum]

Oh, you're right. The docstring correctly says this. :-(

Do you have the power to submit a PR? I think it should just say that the return values are coroutines (which is what it does). @yury what do you think?

[1st1]

@yury what do you think?

Yeah, the documentation needs to be fixed.

Maybe "Returns an iterator of awaitables"?

I'd suggest to change to: "Return an iterator of coroutines. Each coroutine allows to wait for the earliest next result from the set of the remaining awaitables."

[aeros]

Yury Selivanov wrote:

I'd suggest to change to: "Return an iterator of coroutines. Each coroutine allows to wait for the earliest next result from the set of the remaining awaitables."

The first sentence looks good to me, but I'm not certain about the second sentence, particularly "allows to wait". Instead, I'm thinking something like this might read better:

"Each coroutine represents the earliest next result from the set of the remaining awaitables, based upon the order of completion."

Thoughts?

[aeros]

based upon the order of completion

I forgot to explain this in the above message. I think something that mentions "order of completion" should be included. Although it's implied in the name as_completed, to me it seems worthwhile to be fully explicit with what exactly is meant by "earliest".

[aeros]

Alternatively, I think "can wait for" would also read better than "allows to wait for", if that is preferred over my above suggestion.

[gvanrossum]

Please work this out in the PR between the two of you, Kyle and Bar.

[bharel]

@kyle, looks good to me. Maybe this will work better? Together with the current example which shows the "earliest_result" variable of course.

"Each coroutine returns the next result from the set of the remaining awaitables, based upon the order of completion."

Represents -> Returns (less obscure, and coroutines are awaited on to get the result)
Earliest -> removed, "order of completion" looks great.

[aeros]

@bar as requested by Guido, let's continue the discussion in the PR: #19753 (review).

[aeros]

New changeset 13206b5 by Bar Harel in branch 'master':
bpo-40405: Fix asyncio.as_completed docs (GH-19753)
13206b5

[miss-islington]

New changeset 9181e2e by Miss Islington (bot) in branch '3.9':
bpo-40405: Fix asyncio.as_completed docs (GH-19753)
9181e2e

[miss-islington]

New changeset 2fecb48 by Miss Islington (bot) in branch '3.8':
bpo-40405: Fix asyncio.as_completed docs (GH-19753)
2fecb48

[aeros]

Thanks for bring this to our attention and working on this Bar Harel, and thanks to Yury for the suggestions.