Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation

@graingert
Copy link
Contributor

@graingertgraingert commentedJun 21, 2022
edited by bedevere-bot
Loading

@graingertgraingert changed the titlerestore the set_event_loop calls to asyncio.run#93896 restore the set_event_loop calls to asyncio.runJun 21, 2022
Copy link
ContributorAuthor

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

Bluenix2 reacted with thumbs up emoji
@graingertgraingert changed the title#93896 restore the set_event_loop calls to asyncio.run#gh-93896 restore the set_event_loop calls to asyncio.runJun 21, 2022
@graingertgraingert changed the title#gh-93896 restore the set_event_loop calls to asyncio.run#gh-93896: restore the set_event_loop calls to asyncio.runJun 21, 2022
@graingertgraingert changed the title#gh-93896: restore the set_event_loop calls to asyncio.rungh-93896: restore the set_event_loop calls to asyncio.runJun 21, 2022
@graingertgraingertforce-pushed therestore-set-event-loop-policy-to-asyncio-run branch fromf21f808 toabbd6b4CompareJune 22, 2022 12:52
@graingertgraingert marked this pull request as ready for reviewJune 22, 2022 13:29
Comment on lines +38 to +39
If set_policy_loop is True, the event loop in the default policy will be
set.
Copy link
Contributor

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.

Copy link
ContributorAuthor

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

Copy link
Contributor

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 itmore 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.

Copy link
ContributorAuthor

@graingertgraingertJun 27, 2022
edited
Loading

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?

@contextmanagerdef_with_policy_loop(loop):try:asyncio.set_event_loop(loop)yieldfinally: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 thisset_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

Copy link
Contributor

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.

Copy link
ContributorAuthor

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

Copy link
ContributorAuthor

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 inasyncio.Runner:def __init__(self, *, debug=None, loop_factory=None, set_policy_loop=False):

people using the oldasyncio.run get the old behaviour that uses the policy loop factory and sets the policy loop. People using the newasyncio.Runner get the new behaviour which does not use the policy loop factory or set the policy loop by default

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.
Copy link
Contributor

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.

@encukouencukou requested a review frompablogsalJuly 4, 2022 17:19
@graingertgraingert changed the titlegh-93896: restore the set_event_loop calls to asyncio.rungh-93896: allow enabling asyncio.Runner calls to asyncio.set_event_loop independantly to loop_factoryJul 6, 2022
@graingert
Copy link
ContributorAuthor

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

Comment on lines +144 to +145
ifself._set_policy_loop:
events.set_event_loop(None)
Copy link
ContributorAuthor

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

Copy link
Contributor

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.

Copy link
ContributorAuthor

@graingertgraingertJul 6, 2022
edited
Loading

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

withRunner()asrunner1,Runner()asrunner2:runner1.run(fn())# calls set_event_looprunner2.run(fn())# also calls set_event_loop# I'd expect the event loop to be unset here

Copy link
Contributor

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 yourloop_factory otherwise for existing code, its better to keep it set for existing code not usingasyncio.Runner.

Copy link
Contributor

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.

Copy link
ContributorAuthor

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

@graingert
Copy link
ContributorAuthor

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

@graingertgraingert deleted the restore-set-event-loop-policy-to-asyncio-run branchJuly 6, 2022 16:01
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@kumaraditya303kumaraditya303kumaraditya303 left review comments

@1st11st1Awaiting requested review from 1st11st1 is a code owner

@asvetlovasvetlovAwaiting requested review from asvetlovasvetlov is a code owner

@pablogsalpablogsalAwaiting requested review from pablogsal

+1 more reviewer

@Bluenix2Bluenix2Bluenix2 requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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

4 participants

@graingert@Bluenix2@kumaraditya303@bedevere-bot

[8]ページ先頭

©2009-2025 Movatter.jp