10000 gh-95601: restore support for awaitable objects that are not futures in `asyncio.wait` by graingert · Pull Request #95708 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content

gh-95601: restore support for awaitable objects that are not futures in asyncio.wait #95708

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.

Sign up for GitHub

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

Closed
wants to merge 25 commits into from
Closed

gh-95601: restore support for awaitable objects that are not futures in asyncio.wait #95708

Changes from 1 commit
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
88580c9
gh-95601: forbid non-futures in asyncio.wait
graingert Aug 5, 2022
1363c9b
📜🤖 Added by blurb_it.
blurb-it[bot] Aug 5, 2022
543c4f2
Merge branch 'main' into forbid-non-futures-in-asyncio-wait
graingert Aug 5, 2022
6661052
clarify awaitable-not-future
graingert Aug 5, 2022
5b740df
update asyncio.wait deprecation notice
graingert Aug 5, 2022
5b974c5
Update Doc/library/asyncio-task.rst
graingert Aug 5, 2022
c4a7237
Merge branch 'main' into forbid-non-futures-in-asyncio-wait
graingert Aug 5, 2022
d4dac27
Merge branch 'main' into forbid-non-futures-in-asyncio-wait
graingert Aug 9, 2022
cb00279
Apply suggestions from code review
graingert Aug 9, 2022
38fb812
Update Misc/NEWS.d/next/Library/2022-08-05-11-16-33.gh-issue-95601.hJ…
graingert Aug 9, 2022
5383116
Update Lib/test/test_asyncio/test_tasks.py
graingert Aug 9, 2022
b6ea455
upgrade WaitTests to an IsolatedAsyncioTestCase
graingert Aug 9, 2022
980a3b9
Update Lib/test/test_asyncio/test_tasks.py
graingert Aug 9, 2022
d8354db
Update Lib/test/test_asyncio/test_tasks.py
graingert Aug 9, 2022
5f1d061
Update Lib/test/test_asyncio/test_tasks.py
graingert Aug 9, 2022
9181e25
Merge branch 'main' into forbid-non-futures-in-asyncio-wait
graingert Aug 11, 2022
f973c52
test that asyncio.wait gives the right type error for non-awaitable n…
graingert Aug 12, 2022
0430145
Merge branch 'main' into forbid-non-futures-in-asyncio-wait
graingert Aug 12, 2022
9189603
Merge branch 'main' into forbid-non-futures-in-asyn 8000 cio-wait
graingert Sep 12, 2022
9c325b4
don't deprecate Awaitables in wait yet
graingert Sep 12, 2022
da9f2a2
Merge branch 'main' into forbid-non-futures-in-asyncio-wait
graingert Sep 12, 2022
7e35d9c
remove deprecation docs
graingert Sep 12, 2022
a5c5f4a
remove deprecation assertions from tests for asyncio.wait
graingert Sep 13, 2022
17cbfc9
Update Misc/NEWS.d/next/Library/2022-08-05-11-16-33.gh-issue-95601.hJ…
graingert Sep 13, 2022
69b2a0d
Merge branch 'main' into forbid-non-futures-in-asyncio-wait
graingert Sep 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
clarify awaitable-not-future
  • Loading branch information
@graingert
graingert committed Aug 5, 2022
commit 6661052c9114c7793f632b09e27f4ef0494ffe37
11 changes: 7 additions & 4 deletions Lib/asyncio/tasks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -419,10 +419,13 @@ async def wait(fs, *, timeout=None, return_when=ALL_COMPLETED):
if coroutines.iscoroutine(f):
raise TypeError("Passing coroutines is forbidden, use tasks explicitly.")
Copy link
Member

Choose a reason for hiding this comment

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

What will happened with implicitly created futures added to new_fs? Would it emit a warning?

Copy link
Contributor Author
@graingert graingert Sep 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I removed the warning at @tiran 's request, with a plan to add it in a follow up PR.
I'm happy to put it back in this PR if you'd prefer

if not futures.isfuture(f):
warnings.warn("The explicit passing of awaitable objects to "
"asyncio.wait() is deprecated since Python 3.11, and "
"scheduled for removal in Python 3.14.",
DeprecationWarning, stacklevel=2)
warnings.warn(
Copy link
Contributor

Choose a reason for hiding this comment

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

Use warnings._deprecated?

Copy link
Contributor Author
@graingert graingert Aug 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah I usually would, but I kinda want to try backport the deprecation fix to 3.10

also the wording warnings._deprecated doesn't work too well for changes in behaviour and I didn't want to try and fix it and open a new can of worms there

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hugovk are you already working on a plan to upbraid all the existing warnings.warn(..., DeprecationWarning) to warnings._deprecated ?

there's a slight usability/wording concern when using it for changes in behavior instead of actual full removals
and there also seems to be a desire to use it for totally unscheduled removals: remove=(4, )

in addition do you have a view on backporting deprecation, where it was clear from the code what the intention of the deprecation was, but the deprecation itself had a bug?

Copy link
Member

Choose a reason for hiding this comment

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

@hugovk are you already working on a plan to upbraid all the existing warnings.warn(..., DeprecationWarning) to warnings._deprecated ?

I'm not but that's a good idea. From a quick check, there's not too many, and most are set to be removed in 3.12 anyway.

there's a slight usability/wording concern when using it for changes in behavior instead of actual full removals

Can you alter the wording using the message parameter?

and there also seems to be a desire to use it for totally unscheduled removals: remove=(4, )

Some removals scheduled for 4.0 were from before we knew if 3.10 or 4.0 would follow 3.9 (#80480 (comment)) and in at least one case is shorthand for "sometime after 2.7.final" (#80480 (comment)).

Anyway, we should review any remaining removals originally set for 4.0 (e.g. in new issues or relevant open issues) to decide if they can be removed in an upcoming 3.x.

in addition do you have a view on backporting deprecation, where it was clear from the code what the intention of the deprecation was, but the deprecation itself had a bug?

Generally, I think it's a good idea to increase visibility and help users upgrade in time to reduce friction. It's good to ask release managers when in doubt.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hugovk do you think we have enough here to make a new issue out of this side discussion. Or maybe there's two to four ideas here:

  • Do something about existing deprecation warnings that don't automatically get upgraded to errors
  • Do something about the wording and format-string templating in warnings._deprecated to make it work for even the most subtle behaviour change
  • Determine a backport strategy for when a deprecation warning was applied to the wrong thing
  • Determine a strategy for permanent deprecations, where a feature is a bit broken or an alias of another thing, and people should really use the not broken way or source of the alias, but CPython is unwilling or undecided on actually removing it

Copy link
Member

Choose a reason for hiding this comment

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

Sure! Or at https://discuss.python.org/?

"The explicit passing of awaitable objects that are not futures "
"to asyncio.wait() is deprecated since Python 3.11, and "
"scheduled for removal in Python 3.14.",
DeprecationWarning,
stacklevel=2,
)
new_fs.add(ensure_future(f, loop=loop))
else:
new_fs.add(f)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is getting added to the set with no checks whatsoever. It needs to check _asyncio_future_blocking

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed!

Expand Down
0