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

go back to using nest_asyncio for run_sync utility #353

Conversation

smacke
Copy link

@smacke smacke commented May 25, 2023

This PR appears to fix the issue described in jupyter/notebook#6721, whereby users sporadically encounter zmq message arrived on closed channel errors in the tornado loop. It reverts the threaded implementation of jupyter_core.utils.run_sync and replaces it with the original one based on nest_asyncio.

Actually it uses a patched version of nest_asyncio from here, due to portability issues with the original version.

Unfortunately I don't fully understand why reverting the threaded implementation of run_sync is needed to eliminate these errors, but some users suggest it could have something to do with tornado coroutines not playing nicely when run across different threads: tornadoweb/tornado#2871 (comment)

Test plan: did an editable install locally and verified everything works well, and that all the zmq errors I encountered before are gone.

@smacke smacke force-pushed the run_sync_with_nest_asyncio_fixes_tornado_issues branch from d9a25cb to 11107c7 Compare May 25, 2023 15:40
@smacke smacke force-pushed the run_sync_with_nest_asyncio_fixes_tornado_issues branch from b5a79d9 to 2649b72 Compare May 25, 2023 15:49
@davidbrochart
Copy link
Member

I don't think we want to go back to nest-asyncio, which has its own issues too.
Maybe the long-term solution will be to allow nested event loops in asyncio, see python/cpython#66435.

@davidbrochart
Copy link
Member

Maybe a dumb question, but since the issue seems to be related to Tornado, has anyone tried to run Jupyter Notebook with Jupyverse?

@smacke
Copy link
Author

smacke commented May 25, 2023

I don't think we want to go back to nest-asyncio, which has its own issues too.

Agreed (at least for the long term), the purpose of this PR is mainly to raise awareness around jupyter/notebook#6721 -- I've found that this is usually the best way to pester more knowledgeable folks :)

Maybe a dumb question, but since the issue seems to be related to Tornado, has anyone tried to run Jupyter Notebook with Jupyverse?

Not sure (I haven't tried this) -- I only know that right now, for many people, if they do pip install --upgrade jupyter to get the latest versions of dependencies, and run the "legacy" notebook v6 via jupyter notebook, they get errors like jupyter/notebook#6721

@smacke smacke force-pushed the run_sync_with_nest_asyncio_fixes_tornado_issues branch from b6c7a4a to 8bc645f Compare May 25, 2023 17:23
@jamescooke
Copy link

@davidbrochart I didn't know about Jupyverse (and it looks interesting) but I'm not able to run it because of the following dependency conflict (it seems that asphalt used by the api requires an older version of typeguard, asphalt's requirement here)

There are incompatible versions in the resolved dependencies:
  typeguard>=3.0.2 (from pandera==0.15.1->-r requirements.in (line 19))
  typeguard~=2.0 (from asphalt==4.11.1->jupyverse-api==0.1.6->jupyverse[auth,jupyterlab]==0.1.6->-r requirements.in (line 14))

@davidbrochart
Copy link
Member

I don't have this conflict locally. Which package depends on pandera?

@jamescooke
Copy link

@davidbrochart Sorry I wasn't clear - (and this is off topic for this Issue probably). My large notebooks that have experienced the crashes in jupyter/notebook#6721 use pandera to validate data at load time, it's not a dependency of anything in this repo or Jupyverse 🙇🏻 . The conflict meant that I couldn't follow up on your suggestion to try that server.

@davidbrochart
Copy link
Member

Anyway for Jupyter Notebook, a new Jupyverse plugin should be created. But if Jupyter Notebook v7 is fine for you, this will be easy since Jupyverse already has a RetroLab plugin, which should be very similar.

@smacke
Copy link
Author

smacke commented May 26, 2023

That's great that we have some workarounds. However, I am concerned that the default jupyter installation (with notebook v6) is currently broken by default. E.g. if I create a fresh virtualenv and do:

pip install --upgrade jupyter
jupyter notebook

I can reliably reproduce the issue from jupyter/notebook#6721.

One way to reproduce is to generate a plot in a cell with matplotlib, and then hold down ctrl+enter to keep submitting the cell as fast as possible. This leaves the notebook in an unusable state.

While that particular kind of interaction may be unlikely to occur in practice, others are reporting that restart + run all followed by normal use will trigger it.

And it definitely root causes to this threaded run_sync.

@philippjfr
Copy link

Sorry for chiming in on a PR but I didn't see any actual issue about this on here and did want to re-raise this. As you can see from jupyter/notebook#6721 this is a major issue that is biting quite a few people. While we have put mitigation in place for our projects we still see this issue crop up quite frequently causing major issues for many Panel and HoloViews users. As of right now we can only recommend users pin older versions of jupyter-server and jupyter-client, which does not seem a reasonable long-term solution. Is there anything we can do to work towards a fix here?

@blink1073
Copy link
Contributor

I think the right solution is for notebook<7 to pin, as in this PR: jupyter/notebook#6749. We can't undo the changes in the stdlib that drove us to move away from nest_asyncio.

@davidbrochart
Copy link
Member

I recently found out about greenletio. It looks interesting, especially the ability to use await inside a sync function:

from greenletio import await_

async def async_function():
    pass

def sync_function():
    await_(async_function())

@blink1073
Copy link
Contributor

I agree, it does look promising, we're looking at using it for async PyMongo as well: https://gist.github.com/blink1073/3873afd23977a8ab94a423127f2ca2ce

@davidbrochart
Copy link
Member

The only thing is that it uses greenlet, which is a C extension and probably does some low-level magic, but it works on CPython.
Not sure about PyPy though?

@blink1073
Copy link
Contributor

Good point, I think greenletio would need to support continuelet as well: https://doc.pypy.org/en/latest/stackless.html.

@blink1073
Copy link
Contributor

@davidbrochart
Copy link
Member

Nice!

@smacke
Copy link
Author

smacke commented Jul 14, 2023

We can't undo the changes in the stdlib that drove us to move away from nest_asyncio.

I'd be keen to learn more about this -- has anybody written a tl;dr documenting some of these issues? I came across this extremely long thread but find myself more confused than when I started.

@blink1073
Copy link
Contributor

I don't know of a good tl;dr: here's the thread I was involved in: python/cpython#93453 (comment)

@smacke
Copy link
Author

smacke commented Jul 15, 2023

I don't know of a good tl;dr: here's the thread I was involved in: python/cpython#93453 (comment)

Thanks for the pointer!

However, I am concerned that the default jupyter installation (with notebook v6) is currently broken by default.

I happened to reread this several weeks later, and am now cringing hard. I hope folks on this thread will accept my apologies for the wording. I shouldn't have minimized the (incredible) contributions of Jupyter core contributors, especially not without context behind some of the changes being made.

@davidbrochart
Copy link
Member

Actually greenletio would not help us running async code from a sync function running in an event loop:

import asyncio
from greenletio import await_

async def async_function():
    pass

def sync_function():
    await_(async_function())

async def main():
    sync_function()

asyncio.run(main())

# RuntimeError: await_ cannot be called from the asyncio task

@blink1073
Copy link
Contributor

No, you'd have to have a synchronous class/function hierarchy, and then a parallel async version.

@blink1073
Copy link
Contributor

That's what we plan to do for PyMongo.

@blink1073
Copy link
Contributor

We believe #362 fixes the issue, but in the mean time Notebook 6.5.5 has pinned its use of jupyter_core. jupyter/notebook#6721 (comment). I'm going to close this PR, since we don't want to go back to using nest_asyncio.

@blink1073 blink1073 closed this Sep 27, 2023
@smacke
Copy link
Author

smacke commented Oct 1, 2023

We believe #362 fixes the issue

One way to reproduce is to generate a plot in a cell with matplotlib, and then hold down ctrl+enter to keep submitting the cell as fast as possible. This leaves the notebook in an unusable state.

Unfortunately, I can confirm that the above repro still reproduces the same issue on notebook v6 with the newest version of jupyter_core. Here are the (potentially relevant) libraries I'm using:

jupyter_core==5.3.2
jupyter_client==8.3.1
tornado==6.3.2
notebook==6.5.6

Though, maybe it is not really an issue anymore given the aforementioned pinning of jupyter_core to an older version in notebook v6, or given that notebook v7 is out.

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.

5 participants