Conversation
|
|
||
| try: | ||
| from jupyter_client.utils import run_sync # requires 7.0+ | ||
| except ImportError: | ||
| run_sync = None # type:ignore |
There was a problem hiding this comment.
According to the inline comment here, you need to bump the minimal required version of Jupyter-client in pyproject.toml to be >=7.0.
There was a problem hiding this comment.
No, we're pulling this function from jupyter_core now, which I added to the deps.
There was a problem hiding this comment.
Ok, I see 👍🏽 Sorry for the noise then.
| loop = asyncio.get_event_loop() | ||
| try: | ||
| loop.run_until_complete(self.func()) | ||
| run_sync(self.func)() |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 👍
There was a problem hiding this comment.
Ok, let's wait until Tuesday to see if @impact27 has time to review this. If not, please go ahead and merge it @blink1073.
There was a problem hiding this comment.
From what I understand run_sync uses asyncio so it should work as intended
|
@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 For now I suggest to yank 6.21.0 until this is solved. I can also help you to test other PRs. |
|
One more thing: I'm also seeing this warning in our IPython console with 6.21.0 I mention it in case it can be useful. |
|
Yanked, thanks for the heads up! |
|
Could we perhaps add a |
|
That's a good idea, but the problem is the Tk backend is used a lot for teaching (to create animations with the I understand you want to get rid of |
|
Okay, fair enough. I don't have the bandwidth to track down a more viable solution for the tk backend. |
|
Ok, thanks for understanding @blink1073! I'll push a PR with the necessary changes later today. |
|
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. |
|
We shouldn't use |
|
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 |
|
How is that different from ipython/ipykernel? Users can |
|
The situation is: The user can run it with python. 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. |
|
Yes, just like in JupyterLab. |
|
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. |
|
But ipykernel is not python, it's python in an environment, which runs an event loop. |
|
As I said spyder-kernel can apply nest-asyncio, so it doesn’t need to stay in ipykernel. |
Yes,
It depends how we see it, there is degradation from the situation where we used
I would prefer this. However, note that this won't work anymore if/when ipykernel switches to AnyIO. |
doesn’t AnyIO uses asyncio? If so nest-asyncio should work no? |
|
AnyIO can use |
|
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 |
run_syncnest_asynciodep