Skip to content

Conversation

@minrk
Copy link
Member

@minrk minrk commented Dec 1, 2021

This fixes the problems introduced in #13269 by following the pattern in #13289. Using get_running_loop is only a valid way to get the event loop from inside a coroutine, which is not what's happening here.

I don't really understand why asyncio.get_event_loop is deprecated and policy.get_event_loop is not, but that is the case, at least for now.

await doesn't work in 7.30 because of this, so this should perhaps be in 7.30.1

@minrk minrk force-pushed the policy_get_event_loop branch from 766864b to 3d4c571 Compare December 1, 2021 08:28
@minrk minrk added this to the 7.31 milestone Dec 1, 2021
@minrk minrk force-pushed the policy_get_event_loop branch from 3d4c571 to 2b2e0e8 Compare December 1, 2021 10:49
get_running_loop is only valid from inside a coroutine

EventLoopPolicy.get_event_loop is _not_ deprecated
@minrk minrk force-pushed the policy_get_event_loop branch from 2b2e0e8 to 3147552 Compare December 1, 2021 11:11
@minrk
Copy link
Member Author

minrk commented Dec 1, 2021

This is running into various problems with invalid assumptions about asyncio in the script magics (#13348)

@minrk minrk force-pushed the policy_get_event_loop branch from cf4cc6a to 5404b27 Compare December 1, 2021 11:37
leave other problems with asyncio implementation for another PR
@minrk minrk force-pushed the policy_get_event_loop branch from 5404b27 to 0fbbb2b Compare December 1, 2021 11:45
@minrk
Copy link
Member Author

minrk commented Dec 1, 2021

In two commits because the first commit should be backported to 7.x, while the second is only for the 8.0 updates to script magics.

@minrk
Copy link
Member Author

minrk commented Dec 1, 2021

FWIW, referring to the Python patch deprecating get_event_loop, it is an accident that policy.get_event_loop is not also deprecated. So this and #13289 is only an extremely temporary measure.

We should move to our own handle on the event loop, and remove all calls to get_event_loop and set_event_loop (including the policy methods). That should be 8.0-only, I think.

@Carreau Carreau merged commit e343cd4 into ipython:master Dec 1, 2021
@lumberbot-app
Copy link
Contributor

lumberbot-app bot commented Dec 1, 2021

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

  1. Checkout backport branch and update it.
git checkout 7.x
git pull
  1. Cherry pick the first parent branch of the this PR on top of the older branch:
git cherry-pick -m1 e343cd4e5872a1fef8ba7f7d111f37315e21cd28
  1. You will likely have some merge/cherry-pick conflict here, fix them and commit:
git commit -am 'Backport PR #13349: get_running_loop is only valid in coroutines'
  1. Push to a named branch:
git push YOURFORK 7.x:auto-backport-of-pr-13349-on-7.x
  1. Create a PR against branch 7.x, I would have named this PR:

"Backport PR #13349 on branch 7.x (get_running_loop is only valid in coroutines)"

And apply the correct labels and milestones.

Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon!

Remember to remove the Still Needs Manual Backport label once the PR gets merged.

If these instructions are inaccurate, feel free to suggest an improvement.

@lumberbot-app lumberbot-app bot added the Still Needs Manual Backport Added My MrMeeseeks when a backport fails. Help by backporting it, solving conflicts, send PR. label Dec 1, 2021
Carreau pushed a commit to Carreau/ipython that referenced this pull request Dec 1, 2021
This only pickes up the first commit of the PR
Carreau pushed a commit to Carreau/ipython that referenced this pull request Dec 1, 2021
This only pickes up the first commit of the PR
Carreau added a commit that referenced this pull request Dec 1, 2021
Backport PR #13349: get_running_loop is only valid in coroutines
@minrk minrk deleted the policy_get_event_loop branch December 2, 2021 11:37
@Carreau Carreau modified the milestones: 7.31, 7.30.1 Dec 2, 2021
@Carreau Carreau removed the Still Needs Manual Backport Added My MrMeeseeks when a backport fails. Help by backporting it, solving conflicts, send PR. label Apr 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants