Repository navigation
gh-93896: allow enabling asyncio.Runner calls to asyncio.set_event_loop independantly to loop_factory #94058
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
Closed
graingert
wants to merge
13
commits into
python:main
from
graingert:restore-set-event-loop-policy-to-asyncio-run
Closed
Changes from 1 commit
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
f6b7bab
restore the set_event_loop calls to asyncio.run
graingert 428cc8a
📜🤖 Added by blurb_it.
blurb-it[bot] 7786b44
test that asyncio.Runner does not touch the asyncio policy system
graingert abbd6b4
only check policy loop in tearDown
graingert d113102
Update Lib/asyncio/runners.py
graingert 35e746f
Update Lib/unittest/async_case.py
graingert 489d544
Merge branch 'main' into restore-set-event-loop-policy-to-asyncio-run
graingert 9f0f519
Merge branch 'main' into restore-set-event-loop-policy-to-asyncio-run
graingert 1431a63
Update Misc/NEWS.d/next/Library/2022-06-21-13-08-46.gh-issue-93896.2B…
graingert 45b1af0
Merge branch 'main' into restore-set-event-loop-policy-to-asyncio-run
graingert 28af308
Merge branch 'main' into restore-set-event-loop-policy-to-asyncio-run
graingert 83b1514
Merge branch 'main' into restore-set-event-loop-policy-to-asyncio-run
graingert 5865ac6
Apply suggestions from code review
graingert File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Update Lib/asyncio/runners.py
- Loading branch information
commit d11310246b2d5b40b44454a6473bd91fa5ea1443
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
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.
If this is classified as a bug (clearly it is causing issues), then fixing it shouldn't be blocking. By only implementing this behind an argument we are introducing a new feature which means that this will only be added to 3.12 (and not 3.11).
I would vote for not adding this argument.
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.
I think it's important to maintain the asyncio.Runner support for a high level policy-free use of asyncio, so that the policy system can be deprecated and removed
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.
By adding this option we make it more difficult to deprecate the policy system because then this option will need to be deprecated as well. Users who specify this option will have more to do to migrate.
Uh oh!
There was an error while loading. Please reload this page.
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.
How about I remove the flag from asyncio.Runner and have callers use a wrapper that mutates the policy loop?
I'm proposing this
set_policy_loopflag get deprecated at the same time as the policy system. I could add the flag as deprecated in this PR so users won't add it unless they already have more work to do to migrate from the policy system as a conceptThere 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.
I don't think I understand the downside with enabling this by default. My arguments boil down to:
With this flag disabled, we retain the current behaviour which causes issues. I would guess that most people don't fully understand what it does and will leave it to the default, then potentially run into these issues.
As we've so far discussed, this flag is a fix for an issue which might soon no matter much because it is planned to be deprecated. With this option needing to be explicitly enabled, we introduce more things to deprecate. If, instead, this would be enabled by default then most would not need to change anything as it gets removed after the deprecation period.
I am not super confident in the event loop policy system, so I won't continue arguing if you don't agree with me after this comment - i'll defer to whenever a core dev takes a look at this issue.
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.
It should be opt in for asyncio.Runner
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.
yep: set_policy_loop defaults to False in
asyncio.Runner:def __init__(self, *, debug=None, loop_factory=None, set_policy_loop=False):people using the old
asyncio.runget the old behaviour that uses the policy loop factory and sets the policy loop. People using the newasyncio.Runnerget the new behaviour which does not use the policy loop factory or set the policy loop by default