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-93896: allow enabling asyncio.Runner calls to asyncio.set_event_loop independantly to loop_factory by graingert · Pull Request #94058 · python/cpython

Conversation

@graingert

@graingert graingert commented Jun 21, 2022 •
edited by bedevere-bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

Fixes #93896

  • IsolatedAsyncioTestCase and asyncio.run no-longer call asyncio.set_event_loop #93896

Sorry, something went wrong.

Uh oh!

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

All reactions
@bedevere-bot bedevere-bot added the awaiting review label Jun 21, 2022
@graingert graingert changed the title restore the set_event_loop calls to asyncio.run #93896 restore the set_event_loop calls to asyncio.run Jun 21, 2022
graingert
graingert commented Jun 21, 2022
View reviewed changes
Comment thread Lib/asyncio/runners.py Outdated

@graingert graingert Jun 21, 2022

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.

this opts asyncio.Runner out of the policy system and asyncio.run opts back in

Sorry, something went wrong.

Uh oh!

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

Bluenix2 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@graingert graingert changed the title #93896 restore the set_event_loop calls to asyncio.run #gh-93896 restore the set_event_loop calls to asyncio.run Jun 21, 2022
@graingert graingert changed the title #gh-93896 restore the set_event_loop calls to asyncio.run #gh-93896: restore the set_event_loop calls to asyncio.run Jun 21, 2022
@graingert graingert changed the title #gh-93896: restore the set_event_loop calls to asyncio.run gh-93896: restore the set_event_loop calls to asyncio.run Jun 21, 2022
graingert
graingert commented Jun 21, 2022
View reviewed changes
Comment thread Lib/asyncio/runners.py Resolved

Uh oh!

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

graingert
graingert commented Jun 21, 2022
View reviewed changes
Comment thread Lib/asyncio/runners.py Resolved

Uh oh!

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

graingert and others added 4 commits June 22, 2022 13:52
@graingert
restore the set_event_loop calls to asyncio.run
f6b7bab
Fixes python#93896
@blurb-it @graingert
📜🤖 Added by blurb_it.
428cc8a
@graingert
test that asyncio.Runner does not touch the asyncio policy system
7786b44
@graingert
only check policy loop in tearDown
abbd6b4
@graingert
graingert force-pushed the restore-set-event-loop-policy-to-asyncio-run branch from f21f808 to abbd6b4 Compare June 22, 2022 12:52
@graingert
Update Lib/asyncio/runners.py
d113102
graingert
graingert commented Jun 22, 2022
View reviewed changes
Comment thread Lib/unittest/async_case.py Resolved

Uh oh!

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

@graingert
Update Lib/unittest/async_case.py
35e746f
@graingert
graingert marked this pull request as ready for review June 22, 2022 13:29
@graingert
graingert requested review from 1st1 and asvetlov as code owners June 22, 2022 13:29
@graingert
Merge branch 'main' into restore-set-event-loop-policy-to-asyncio-run
489d544
@graingert graingert mentioned this pull request Jun 22, 2022
Implement asyncio.Runner context manager #91218
Closed
@graingert
Merge branch 'main' into restore-set-event-loop-policy-to-asyncio-run
9f0f519
Bluenix2
Bluenix2 suggested changes Jun 25, 2022
View reviewed changes
Comment thread Lib/asyncio/runners.py
Comment on lines +38 to +39
If set_policy_loop is True, the event loop in the default policy will be
set.

@Bluenix2 Bluenix2 Jun 24, 2022

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.

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.

Sorry, something went wrong.

Uh oh!

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

All reactions

@graingert graingert Jun 26, 2022

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.

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

Sorry, something went wrong.

Uh oh!

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

All reactions

@Bluenix2 Bluenix2 Jun 27, 2022

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.

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.

Sorry, something went wrong.

Uh oh!

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

All reactions

@graingert graingert Jun 27, 2022 •
edited
Loading

Uh oh!

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

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.

How about I remove the flag from asyncio.Runner and have callers use a wrapper that mutates the policy loop?

@contextmanager
def _with_policy_loop(loop):
    try:
        asyncio.set_event_loop(loop)
        yield
    finally:
        asyncio.set_event_loop(None)
runner = asyncio.Runner()
cmgr = _with_policy_loop(runner.get_loop())
try:
    cmgr(runner.run)(corofn())
finally:
    cmgr(runner.close)()

Users who specify this option will have more to do to migrate.

I'm proposing this set_policy_loop flag 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 concept

Sorry, something went wrong.

Uh oh!

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

All reactions

@Bluenix2 Bluenix2 Jun 30, 2022

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.

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.

Sorry, something went wrong.

Uh oh!

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

All reactions

@graingert graingert Jun 30, 2022

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.

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.

It should be opt in for asyncio.Runner

Sorry, something went wrong.

Uh oh!

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

All reactions

@graingert graingert Jun 30, 2022

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.

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.run get the old behaviour that uses the policy loop factory and sets the policy loop. People using the new asyncio.Runner get the new behaviour which does not use the policy loop factory or set the policy loop by default

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread Lib/asyncio/runners.py
Comment on lines +183 to +185
This function always creates a new event loop from the default policy sets
it in the event loop policy and closes it at the end and sets the policy
loop to None.

@Bluenix2 Bluenix2 Jun 24, 2022

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.

This long sentence is quite wordy and without knowing the change I would probably not understand what it means.

I would probably suggest not making this change (referring to this very sentence). To most, setting the loop in the event loop policy is probably synonymous to actually creating and running a loop.

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread Misc/NEWS.d/next/Library/2022-06-21-13-08-46.gh-issue-93896.2BsYCX.rst Resolved

Uh oh!

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

@bedevere-bot bedevere-bot added awaiting core review and removed awaiting review labels Jun 25, 2022
graingert and others added 3 commits June 25, 2022 15:25
@graingert @Bluenix2
Update Misc/NEWS.d/next/Library/2022-06-21-13-08-46.gh-issue-93896.2B…
1431a63
…sYCX.rst

Co-authored-by: Bluenix <[email protected]>
@graingert
Merge branch 'main' into restore-set-event-loop-policy-to-asyncio-run
45b1af0
@graingert
Merge branch 'main' into restore-set-event-loop-policy-to-asyncio-run
28af308
@encukou
encukou requested a review from pablogsal July 4, 2022 17:19
@graingert
Merge branch 'main' into restore-set-event-loop-policy-to-asyncio-run
83b1514
graingert
graingert commented Jul 6, 2022
View reviewed changes
Comment thread Lib/asyncio/runners.py Resolved

Uh oh!

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

graingert
graingert commented Jul 6, 2022
View reviewed changes
Comment thread Lib/asyncio/runners.py Resolved

Uh oh!

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

@graingert
Apply suggestions from code review
5865ac6
@graingert graingert changed the title gh-93896: restore the set_event_loop calls to asyncio.run gh-93896: allow enabling asyncio.Runner calls to asyncio.set_event_loop independantly to loop_factory Jul 6, 2022
@graingert

graingert commented Jul 6, 2022

Copy link
Copy Markdown
Contributor Author

I've merged the changes from #94593 so I'm now proposing this as a feature change instead of a bug fix

All reactions

Sorry, something went wrong.

Uh oh!

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

graingert
graingert commented Jul 6, 2022
View reviewed changes
Comment thread Lib/asyncio/runners.py
Comment on lines +144 to +145
if self._set_policy_loop:
events.set_event_loop(None)

@graingert graingert Jul 6, 2022

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.

@kumaraditya303 it looks like your PR is missing some calls to set_event_loop here

Sorry, something went wrong.

Uh oh!

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

All reactions

@kumaraditya303 kumaraditya303 Jul 6, 2022

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.

asyncio.Runner should be used a context manager and its close method removes the set event loop so not required here.

Sorry, something went wrong.

Uh oh!

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

All reactions

@graingert graingert Jul 6, 2022 •
edited
Loading

Uh oh!

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

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 issue being when Runner.run finishes the loop should be unset

with Runner() as runner1, Runner() as runner2:
    runner1.run(fn())  # calls set_event_loop
    runner2.run(fn())  # also calls set_event_loop
    # I'd expect the event loop to be unset here

Sorry, something went wrong.

Uh oh!

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

All reactions

@kumaraditya303 kumaraditya303 Jul 6, 2022

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.

I'd expect the event loop to be unset here

That happens only if you supply your loop_factory otherwise for existing code, its better to keep it set for existing code not using asyncio.Runner.

Sorry, something went wrong.

Uh oh!

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

All reactions

@kumaraditya303 kumaraditya303 Jul 6, 2022

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.

I would expect the event loop to be set until the context manager exit which is what we have currently.

Sorry, something went wrong.

Uh oh!

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

All reactions

@graingert graingert Jul 6, 2022

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.

Ah I see what you mean

Sorry, something went wrong.

Uh oh!

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

All reactions
@graingert

graingert commented Jul 6, 2022

Copy link
Copy Markdown
Contributor Author

I'll close this PR for now. It might make sense to re-introduce the set_policy_loop kwarg for 3.12 if the policy system is deprecated

All reactions

Sorry, something went wrong.

Uh oh!

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

@graingert graingert closed this Jul 6, 2022
@graingert
graingert deleted the restore-set-event-loop-policy-to-asyncio-run branch July 6, 2022 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kumaraditya303 kumaraditya303 kumaraditya303 left review comments
@1st1 1st1 Awaiting requested review from 1st1 1st1 is a code owner
@asvetlov asvetlov Awaiting requested review from asvetlov asvetlov is a code owner
@pablogsal pablogsal Awaiting requested review from pablogsal
+1 more reviewer
@Bluenix2 Bluenix2 Bluenix2 requested changes
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

awaiting core review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

IsolatedAsyncioTestCase and asyncio.run no-longer call asyncio.set_event_loop

Uh oh!

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

4 participants

@graingert @Bluenix2 @kumaraditya303 @bedevere-bot
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.