Maintenance updates

[blink1073]

• Use jupyter_core utilites for run_sync
• Drop nest_asyncio dep
• Fix typing errors

[ccordoba12]

According to the inline comment here, you need to bump the minimal required version of Jupyter-client in pyproject.toml to be >=7.0.

[blink1073]

No, we're pulling this function from jupyter_core now, which I added to the deps.

[ccordoba12]

Ok, I see 👍🏽 Sorry for the noise then.

[ccordoba12]

This is a Windows specific code path for which we don't have tests in Spyder-kernels.

@dalthviz, could you check that this PR doesn't break the Tk backend on Windows, as described in spyder-ide/spyder#17024? (see PR #830, which introduced this fix).

Also @impact27, if you're available, could you take a look at these changes?

[dalthviz]

As far as I tested seems like this PR works with the latest Spyder 5.x branch on Windows with the Tkinter graphics backend and a custom interpreter 👍

[blink1073]

Thanks @dalthviz!

[ccordoba12]

Ok, let's wait until Tuesday to see if @impact27 has time to review this. If not, please go ahead and merge it @blink1073.

[blink1073]

Sounds good!

[impact27]

Looks good to me 👍

[impact27]

From what I understand run_sync uses asyncio so it should work as intended

[blink1073]

That's correct, thanks @impact27!

[ccordoba12]

@blink1073, I'm sorry to say it but these changes unfortunately broke the Tk backend at the end. This was detected by the Spyder tests (not the Spyder-kernels ones) as soon as you released 6.21.0:

https://github.com/spyder-ide/spyder/actions/runs/4045110791/jobs/6957241779#step:13:6965

At first I thought it was a problem with that test, but after checking things manually, I can confirm the backend is broken, i.e. it only displays frozen figures for me.

Furthermore, things work as expected by reverting the changes you made to it. I tried several alternatives to remove the dependency on nest_asyncio, but I was unable to (sorry, I don't know enough asyncio to propose a better fix than the one you made).

For now I suggest to yank 6.21.0 until this is solved. I can also help you to test other PRs.

[ccordoba12]

One more thing: I'm also seeing this warning in our IPython console with 6.21.0

C:\Users\test\AppData\Local\Temp\ipykernel_1804\3694909353.py:1: UserWarning: Starting a Matplotlib GUI outside of the main thread will likely fail.

I mention it in case it can be useful.

[blink1073]

Yanked, thanks for the heads up!

[blink1073]

Could we perhaps add a [tk] extra that also depends on nest-asyncio?

[ccordoba12]

That's a good idea, but the problem is the Tk backend is used a lot for teaching (to create animations with the turtle module) and also to design GUIs because it's simpler than Qt and comes with almost all Python installations (at least that's what we've noticed from reading bug reports in our issue tracker). But without the nest-asyncio dependency, the backend would appear as broken for those users.

I understand you want to get rid of nest-asyncio, but it's a small dependency (a single ~230 lines file) that have no other dependencies. So why don't we just leave it?

[blink1073]

Okay, fair enough. I don't have the bandwidth to track down a more viable solution for the tk backend.

[ccordoba12]

Ok, thanks for understanding @blink1073! I'll push a PR with the necessary changes later today.

[impact27]

I don’t think there will be a way to get rid of nest-asyncio. We want to be able to run scripts that contain asyncio loop in spyder and this is only possible with nest-asyncio. Of course the workaround would be for us to monkey patch ipykernel from spyder-kernels, but that would disable asyncio loops in notebooks.

[davidbrochart]

We shouldn't use nest-asyncio anymore. Is it possible to make spyder an async app?

[impact27]

The problem is that we want to run scripts written by users, and that these scripts might use asyncio, so I don’t really see a way around nest-asyncio

[davidbrochart]

How is that different from ipython/ipykernel? Users can await at the top-level, without needing nest-asyncio.

[impact27]

The situation is:
A user has a python script (script.py) containing:

import asyncio

async def main():
    print('Hello ...')
    await asyncio.sleep(1)
    print('... World!')

asyncio.run(main())

The user can run it with python.
Now the user wants to run it in spyder. This is only possible because of nest-asyncio

if I understand your proposition correctly, this user would have to modify his script so it runs in spyder, and then modify it again so it runs without spyder.

[davidbrochart]

Yes, just like in JupyterLab.
Users can always do the nest-asyncio patching themselves if they want.

[impact27]

This would be a regression from the user perspective. The user has a valid python script that runs in spyder. After the update, spyder would only support a subset of valid python scripts.

[davidbrochart]

But ipykernel is not python, it's python in an environment, which runs an event loop.
Spyder can always run raw python to avoid this kind of issues.

[impact27]

As I said spyder-kernel can apply nest-asyncio, so it doesn’t need to stay in ipykernel.
As I see it, the options are to either degrade the user experience or keep using nest-asyncio. I am not sure I understand why the first option is preferable? Is there some major issue with nest-asyncio?

[davidbrochart]

Is there some major issue with nest-asyncio?

Yes, nest-asyncio patches asyncio, which is bad.

As I see it, the options are to either degrade the user experience

It depends how we see it, there is degradation from the situation where we used nest-asyncio, which should not have happened in the first place. I'm not blaming anyone, I was the first to introduce it to Jupyter, but that was a mistake.

As I said spyder-kernel can apply nest-asyncio, so it doesn’t need to stay in ipykernel.

I would prefer this. However, note that this won't work anymore if/when ipykernel switches to AnyIO.

[impact27]

I would prefer this. However, note that this won't work anymore if/when #1079.

doesn’t AnyIO uses asyncio? If so nest-asyncio should work no?

[davidbrochart]

AnyIO can use asyncio, but it can also use trio, in which case nest-asyncio won't work.

[impact27]

After double checking it turns out I was mistaken, the asyncio script only works on windows when the user uses the tkinter backend, so removing the nest-asyncio is fine as this is mostly not a regression. The tk backend would still need to be fixed though.