Skip to content
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

Use native coroutines instead of tornado coroutines #632

Merged
merged 3 commits into from
Apr 24, 2021

Conversation

SylvainCorlay
Copy link
Member

@SylvainCorlay SylvainCorlay commented Apr 9, 2021

  • This fixes Python contextvars don't persist across cells in notebook ipython#11565 because tornado's wrapping of asyncio is a bit aggressive with creating Task objects which causes them to have a different contextvars.Context.
  • However, this is a major change. I think this deserves a careful review - and be considered a backward incompatible change.
  • If/when this goes in, we should also consider completely getting rid of tornado in ipykernel.

cc @afshin @minrk @Carreau.

Note: tests are passing except for the in-process kernel tests, for which we will need to make more functions async.

@SylvainCorlay SylvainCorlay force-pushed the use-native-coroutines branch 6 times, most recently from 4c9ae9c to 3604092 Compare April 10, 2021 08:45
@blink1073 blink1073 added this to the 6.0 milestone Apr 10, 2021
@SylvainCorlay SylvainCorlay force-pushed the use-native-coroutines branch 6 times, most recently from db887a1 to 1e7272b Compare April 10, 2021 12:10
@minrk minrk self-requested a review April 10, 2021 19:49
Copy link
Member

@minrk minrk left a comment

Choose a reason for hiding this comment

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

Makes sense. One minor suggestion that we preserve the maybe_future functionality to allow non-async methods, since most are in fact not async. That's probably the biggest breaking change here.

Since asyncio doesn't have its own maybe_future we can be more simplistic and just use:

result = maybe_async_api()
if inspect.isawaitable(result):
    result = await result

The main difference this has is that concurrent.futures.Future is not awaitable. Everything tornado returns is, though (>=5.0, at lest). I think that's okay, though.

ipykernel/kernelbase.py Outdated Show resolved Hide resolved
ipykernel/kernelbase.py Outdated Show resolved Hide resolved
@SylvainCorlay SylvainCorlay force-pushed the use-native-coroutines branch 3 times, most recently from 11f8217 to 971383d Compare April 14, 2021 11:22
@SylvainCorlay
Copy link
Member Author

I could fix the in-process kernel tests.

@SylvainCorlay
Copy link
Member Author

Also, should I remove other uses of tornado in the code base in this PR?

Copy link
Member

@minrk minrk left a comment

Choose a reason for hiding this comment

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

should I remove other uses of tornado in the code base in this PR?

I think it's sensible to do it in stages, since this is a concrete discrete change.

It looks like the main thing left is our use of queues. However, asyncio's queue is not threadsafe, unlike tornado's, and the whole point of our use of the queues is inter-thread communication. So I think it makes sense to deal with that in its own PR, and keep this one to just async def. Edit: tornado's queues are also not threadsafe. They are only called via add_callback. But I can imagine there being subtle threadsafety issues, since we do use them across threads.

ipykernel/kernelbase.py Outdated Show resolved Hide resolved
ipykernel/kernelbase.py Outdated Show resolved Hide resolved
ipykernel/inprocess/client.py Show resolved Hide resolved
@SylvainCorlay SylvainCorlay force-pushed the use-native-coroutines branch from 971383d to 93a61cd Compare April 15, 2021 22:16
@Carreau
Copy link
Member

Carreau commented Apr 15, 2021

That looks great; with this, capturing of IO and some of the cleanup that has been done recently it would make a compelling release. We may even use the occasion to drop a couple deprecated things.

@SylvainCorlay
Copy link
Member Author

That looks great; with this, capturing of IO and some of the cleanup that has been done recently it would make a compelling release. We may even use the occasion to drop a couple deprecated things.

👍 And debugger support!

Copy link
Contributor

@blink1073 blink1073 left a comment

Choose a reason for hiding this comment

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

👍🏼

@SylvainCorlay SylvainCorlay force-pushed the use-native-coroutines branch 3 times, most recently from a2bfc64 to e6ff9ff Compare April 17, 2021 00:01
@SylvainCorlay
Copy link
Member Author

It looks like the main thing left is our use of queues. However, asyncio's queue is not threadsafe, unlike tornado's, and the whole point of our use of the queues is inter-thread communication. So I think it makes sense to deal with that in its own PR, and keep this one to just async def. Edit: tornado's queues are also not threadsafe. They are only called via add_callback. But I can imagine there being subtle threadsafety issues, since we do use them across threads.

Tornado queues are not thread safe, but it does not mean you can't use them with threads. It only means you can't use their APIs from a different thread where the ioloop runs - except for add_callback which is documented as being threadsafe.

Copy link
Member

@minrk minrk left a comment

Choose a reason for hiding this comment

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

👍 in general. I'd only add that I think we shouldn't deprecate synchronous handlers, so I think we should skip the deprecation message, and shouldn't convert existing do_ methods and message handlers to async if they aren't already. There isn't really a benefit to that that I can see, and it does have a (tiny) performance cost.

ipykernel/kernelbase.py Outdated Show resolved Hide resolved
ipykernel/kernelbase.py Outdated Show resolved Hide resolved
ipykernel/kernelbase.py Outdated Show resolved Hide resolved
ipykernel/kernelbase.py Outdated Show resolved Hide resolved
ipykernel/kernelbase.py Outdated Show resolved Hide resolved
@SylvainCorlay SylvainCorlay force-pushed the use-native-coroutines branch 2 times, most recently from fb46301 to 583dc40 Compare April 23, 2021 16:55
@SylvainCorlay
Copy link
Member Author

SylvainCorlay commented Apr 23, 2021

I addressed all of @minrk comments.

The windows python 3.7 test failure is the same as in master and is probably due to the release of importlib-metadata a few days a go. This is fixed in #649.

@blink1073
Copy link
Contributor

Kicking CI

@blink1073 blink1073 closed this Apr 24, 2021
@blink1073 blink1073 reopened this Apr 24, 2021
@SylvainCorlay SylvainCorlay force-pushed the use-native-coroutines branch from 7b01b09 to 4965355 Compare April 24, 2021 06:28
@minrk minrk merged commit d18d079 into ipython:master Apr 24, 2021
@minrk
Copy link
Member

minrk commented Apr 24, 2021

Nice!

@SylvainCorlay SylvainCorlay deleted the use-native-coroutines branch April 24, 2021 15:08
@SylvainCorlay
Copy link
Member Author

Many thanks to @minrk for the careful review.

@blink1073
Copy link
Contributor

Wow, good work y'all. 🎉

@gsakkis
Copy link

gsakkis commented May 18, 2021

I just upgraded ipykernel to 5.5.5 for this fix but it doesn't seem to be working for me:
JupyterLab
Am I missing something?

EDIT: it turns out this PR is not backported to 5.x releases; it works fine after pip install ipykernel==6.0.0b0

@SylvainCorlay
Copy link
Member Author

Yep, will be in 6.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Python contextvars don't persist across cells in notebook
5 participants