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

Notebook controller always defaulting to Pyolite with the new MRU kernel picker #171385

Closed
DonJayamanne opened this issue Jan 16, 2023 · 11 comments · Fixed by #173352
Closed

Notebook controller always defaulting to Pyolite with the new MRU kernel picker #171385

DonJayamanne opened this issue Jan 16, 2023 · 11 comments · Fixed by #173352
Assignees
Labels
author-verification-requested Issues potentially verifiable by issue author bug Issue identified by VS Code Team member as probable bug insiders-released Patch has been released in VS Code Insiders notebook-kernel-picker verified Verification succeeded
Milestone

Comments

@DonJayamanne
Copy link
Contributor

Does this issue occur when all extensions are disabled?: Yes/No

  • VS Code Version: Latest Insiders 1.75
  • OS Version: Mac

Steps to Reproduce:

  1. Install VSCode insiders
  2. Install Julia , pyolite, Jupyter pre-release & python pre-release extension
  3. Ensure MRU is used as the kernel picker
  4. Create a new workspace folder and a new ipynb file with a name such as helo_world1234.ipynb
  5. The kernel that is selected is pyolite
  6. Create a new cell and run the cell and there is no prompt to select a kernel (proving the fact that pyolite is somehow selected by VS Code as the default kernel)

Expected experience - as a user I would be required to select a kernel

@rebornix rebornix added bug Issue identified by VS Code Team member as probable bug notebook-kernel-picker labels Jan 17, 2023
@rebornix rebornix added this to the January 2023 milestone Jan 17, 2023
@vscodenpa vscodenpa added unreleased Patch has not yet been released in VS Code Insiders insiders-released Patch has been released in VS Code Insiders and removed unreleased Patch has not yet been released in VS Code Insiders labels Jan 18, 2023
@DonJayamanne
Copy link
Contributor Author

@rebornix I'm still running into this issue and so is @amunger

@DonJayamanne DonJayamanne reopened this Jan 24, 2023
@amunger
Copy link
Contributor

amunger commented Jan 24, 2023

after updating I'm no longer hitting this

@DonJayamanne
Copy link
Contributor Author

DonJayamanne commented Jan 24, 2023

My scenario:

  • Create a venv
  • Use that venv as the kernel
  • Delete that venv
  • Refresh interpreters
  • At some point after reloading vscode or reopening the notebook, the pyolite kernel is selected

I've seen this happen a lot in the past day and today

@vscodenpa vscodenpa removed the insiders-released Patch has been released in VS Code Insiders label Jan 24, 2023
@DonJayamanne
Copy link
Contributor Author

  • Open notebook a.ipnb
  • Run cell and in the prompt select kernel .venv (no idea why i need to select one when i had already selected this, doesnt matter, we can repro this again)
  • Wait for cell to complete
  • Close notebook
  • Re-load VS Code
  • Open notebook a.ipynb again
  • No kernel is selected, I get Select Kernel for this file.
First output
[KernelDetection] start extension activation for jupyter-notebook
[NotebookEditorWidget] warmup file:///Users/donjayamanne/crap/misc/a.ipynb
[NotebookEditorWidget] warmup - webview resolved
[NotebookEditorWidget] warmup - viewport warmed up
[NotebookEditorWidget] warmup - list view model attached, set to visible
[KernelDetection] finish extension activation for jupyter-notebook
[History] getMatchingKernels: 90 kernels available for /Users/donjayamanne/crap/misc/a.ipynb. Selected: undefined. Suggested: undefined
[History] mru: 5 kernels in history, 4 registered already.
[History] getMatchingKernels: 90 kernels available for /Users/donjayamanne/crap/misc/a.ipynb. Selected: undefined. Suggested: undefined
[History] mru: 5 kernels in history, 4 registered already.
[History] getMatchingKernels: 90 kernels available for /Users/donjayamanne/crap/misc/a.ipynb. Selected: undefined. Suggested: undefined
[History] mru: 5 kernels in history, 4 registered already.
[History] getMatchingKernels: 90 kernels available for /Users/donjayamanne/crap/misc/a.ipynb. Selected: undefined. Suggested: undefined
[History] mru: 5 kernels in history, 4 registered already.
[History] getMatchingKernels: 90 kernels available for /Users/donjayamanne/crap/misc/a.ipynb. Selected: undefined. Suggested: undefined
[History] mru: 5 kernels in history, 4 registered already.
[History] getMatchingKernels: 90 kernels available for /Users/donjayamanne/crap/misc/a.ipynb. Selected: undefined. Suggested: undefined
[History] mru: 5 kernels in history, 4 registered already.
[History] getMatchingKernels: 90 kernels available for /Users/donjayamanne/crap/misc/a.ipynb. Selected: .venv (Python 3.9.15). Suggested: undefined
[History] mru: 5 kernels in history, 4 registered already.
[History] getMatchingKernels: 90 kernels available for /Users/donjayamanne/crap/misc/a.ipynb. Selected: .venv (Python 3.9.15). Suggested: undefined
[History] mru: 5 kernels in history, 4 registered already.
[History] getMatchingKernels: 90 kernels available for /Users/donjayamanne/crap/misc/a.ipynb. Selected: .venv (Python 3.9.15). Suggested: undefined
[History] mru: 5 kernels in history, 4 registered already.
[History] getMatchingKernels: 90 kernels available for /Users/donjayamanne/crap/misc/a.ipynb. Selected: .venv (Python 3.9.15). Suggested: undefined
[History] mru: 5 kernels in history, 4 registered already.
[History] getMatchingKernels: 90 kernels available for /Users/donjayamanne/crap/misc/a.ipynb. Selected: .venv (Python 3.9.15). Suggested: undefined
[History] mru: 5 kernels in history, 4 registered already.
[History] getMatchingKernels: 90 kernels available for /Users/donjayamanne/crap/misc/a.ipynb. Selected: .venv (Python 3.9.15). Suggested: undefined
[History] mru: 5 kernels in history, 4 registered already.
[History] getMatchingKernels: 90 kernels available for /Users/donjayamanne/crap/misc/a.ipynb. Selected: .venv (Python 3.9.15). Suggested: undefined
[History] mru: 5 kernels in history, 4 registered already.
[History] getMatchingKernels: 90 kernels available for /Users/donjayamanne/crap/misc/a.ipynb. Selected: .venv (Python 3.9.15). Suggested: undefined
[History] mru: 5 kernels in history, 4 registered already.
[History] getMatchingKernels: 90 kernels available for /Users/donjayamanne/crap/misc/a.ipynb. Selected: .venv (Python 3.9.15). Suggested: undefined
[History] mru: 5 kernels in history, 4 registered already.

Second reload (kernel is not auto selected)
[KernelDetection] start extension activation for jupyter-notebook
[NotebookEditorWidget] warmup file:///Users/donjayamanne/crap/misc/a.ipynb
[NotebookEditorWidget] warmup - webview resolved
[NotebookEditorWidget] warmup - viewport warmed up
[NotebookEditorWidget] warmup - list view model attached, set to visible
[KernelDetection] finish extension activation for jupyter-notebook
[History] getMatchingKernels: 90 kernels available for /Users/donjayamanne/crap/misc/a.ipynb. Selected: undefined. Suggested: undefined
[History] mru: 5 kernels in history, 4 registered already.
[History] getMatchingKernels: 90 kernels available for /Users/donjayamanne/crap/misc/a.ipynb. Selected: undefined. Suggested: undefined
[History] mru: 5 kernels in history, 4 registered already.

@vscodenpa vscodenpa added unreleased Patch has not yet been released in VS Code Insiders insiders-released Patch has been released in VS Code Insiders and removed unreleased Patch has not yet been released in VS Code Insiders labels Jan 25, 2023
@rebornix rebornix added the author-verification-requested Issues potentially verifiable by issue author label Jan 27, 2023
@vscodenpa
Copy link

This bug has been fixed in the latest release of VS Code Insiders!

@DonJayamanne, you can help us out by commenting /verified if things are now working as expected.

If things still don't seem right, please ensure you're on version 07d6f5b of Insiders (today's or later - you can use Help: About in the command palette to check), and leave a comment letting us know what isn't working as expected.

Happy Coding!

@DonJayamanne
Copy link
Contributor Author

Still running into this.

  • Open notebook select kernel
  • Run a cell
  • Close notebook
  • Reload vscode
  • Open the notebook and we have Select Kernel displayed

Here is the Notebook logs

[KernelDetection] start extension activation for jupyter-notebook
[NotebookEditorWidget] warmup file:///Users/donjayamanne/crap/misc/a.ipynb
[NotebookEditorWidget] warmup - webview resolved
[NotebookEditorWidget] warmup - viewport warmed up
[NotebookEditorWidget] warmup - list view model attached, set to visible
[KernelDetection] finish extension activation for jupyter-notebook
[History] getMatchingKernels: 91 kernels available for /Users/donjayamanne/crap/misc/a.ipynb. Selected: undefined. Suggested: undefined
[History] mru: 5 kernels in history, 4 registered already.
[History] getMatchingKernels: 91 kernels available for /Users/donjayamanne/crap/misc/a.ipynb. Selected: undefined. Suggested: undefined
[History] mru: 5 kernels in history, 4 registered already.

Yes the controllers have been created and regsietered

here are the jupyter logs confirming the fact that the controllers are registered

debug 12:51:07.659: Creating notebook controller for startUsingPythonInterpreter & view jupyter-notebook (id='.jvsc74a57bd08d5ba0844302807eb036461f209c43f4b480158b851d7a5b69faf22bf4995b18./Users/donjayamanne/crap/.venv/python./Users/donjayamanne/crap/.venv/python.-m#ipykernel_launcher') with name '.venv (Python 3.9.15)'
debug 12:51:07.660: Creating notebook controller for startUsingPythonInterpreter & view interactive (id='.jvsc74a57bd08d5ba0844302807eb036461f209c43f4b480158b851d7a5b69faf22bf4995b18./Users/donjayamanne/crap/.venv/python./Users/donjayamanne/crap/.venv/python.-m#ipykernel_launcher') with name '.venv (Python 3.9.15)'

@DonJayamanne DonJayamanne reopened this Jan 27, 2023
@DonJayamanne DonJayamanne added the verification-found Issue verification failed label Jan 27, 2023
@vscodenpa vscodenpa removed the insiders-released Patch has been released in VS Code Insiders label Jan 27, 2023
@rebornix rebornix modified the milestones: January 2023, February 2023 Jan 27, 2023
@rebornix rebornix removed verification-found Issue verification failed author-verification-requested Issues potentially verifiable by issue author labels Jan 27, 2023
@DonJayamanne
Copy link
Contributor Author

DonJayamanne commented Jan 27, 2023

  • Create a new ipynb file on disc
  • We see Select Kernel
  • Type into the cell and then automatically Pyolite is selected as the active kernel
[History] getMatchingKernels: 91 kernels available for /Users/donjayamanne/crap/misc/newRandomx1.ipynb. Selected: undefined. Suggested: undefined
[History] mru: 5 kernels in history, 4 registered already.
[History] getMatchingKernels: 91 kernels available for /Users/donjayamanne/crap/misc/newRandomx1.ipynb. Selected: undefined. Suggested: undefined
[History] mru: 5 kernels in history, 4 registered already.
[NotebookEditorWidget] warmup file:///Users/donjayamanne/crap/misc/newRandomx1.ipynb
[NotebookEditorWidget] warmup - webview resolved
[History] getMatchingKernels: 91 kernels available for /Users/donjayamanne/crap/misc/newRandomx1.ipynb. Selected: undefined. Suggested: undefined
[History] mru: 5 kernels in history, 4 registered already.
[History] getMatchingKernels: 91 kernels available for /Users/donjayamanne/crap/misc/newRandomx1.ipynb. Selected: undefined. Suggested: undefined
[History] mru: 5 kernels in history, 4 registered already.
[NotebookEditorWidget] warmup - viewport warmed up
[NotebookEditorWidget] warmup - list view model attached, set to visible
[History] getMatchingKernels: 91 kernels available for /Users/donjayamanne/crap/misc/newRandomx1.ipynb. Selected: undefined. Suggested: undefined
[History] mru: 5 kernels in history, 4 registered already.
[History] getMatchingKernels: 91 kernels available for /Users/donjayamanne/crap/misc/newRandomx1.ipynb. Selected: undefined. Suggested: undefined
[History] mru: 5 kernels in history, 4 registered already.
[History] getMatchingKernels: 91 kernels available for /Users/donjayamanne/crap/misc/newRandomx1.ipynb. Selected: Pyolite. Suggested: undefined
[History] mru: 5 kernels in history, 4 registered already.
[History] getMatchingKernels: 91 kernels available for /Users/donjayamanne/crap/misc/newRandomx1.ipynb. Selected: Pyolite. Suggested: undefined
[History] mru: 5 kernels in history, 4 registered already.
[History] getMatchingKernels: 91 kernels available for /Users/donjayamanne/crap/misc/newRandomx1.ipynb. Selected: Pyolite. Suggested: undefined
[History] mru: 5 kernels in history, 4 registered already.
[History] getMatchingKernels: 91 kernels available for /Users/donjayamanne/crap/misc/newRandomx1.ipynb. Selected: Pyolite. Suggested: undefined
[History] mru: 5 kernels in history, 4 registered already.

@rebornix
Copy link
Member

@DonJayamanne and I debugged together, and here are our findings. Basically it's a race condition but it should be prevented.

  • With 1fabb19, everyone a notebook is closed, the selected kernel is removed from notebook binding. This means we are no longer saving the selected kernel to storage.
  • The selected kernel is stored in editor view state
  • When we open a notebook, we will restore kernel from view state. If the kernel is not registered yet, then it skips this step.

In @DonJayamanne 's case, he has 90+ kernels, registering them takes long time. When reloading the window, the editor tries to restore the kernel, but can't find a matching kernel, thus it skips restoring the selected kernel.

I guess removing selected kernel from notebook binding for all notebooks is not expected from the commit title "Keep selected kernel from untitled notebook when saving".

@roblourens
Copy link
Member

I hardly remember this change at all but I'll take a look!

@roblourens
Copy link
Member

roblourens commented Feb 2, 2023

I think the change you're referring to is for

Also fix caching the previously selected kernel for Untitled-1 after saving it

Basically, remove the notebook binding for the untitled document when it's closed when saving and switching to the new saved notebook. I think this should be ok, it's treating the notebookBindings list as active notebook bindings.

everyone a notebook is closed, the selected kernel is removed from notebook binding. This means we are no longer saving the selected kernel to storage.

I'm actually not seeing that- I see that when I close a notebook, first the NotebookEditorWidget gets its editor view state which includes the selected kernel id, then second, we close the notebook textmodel and fire the event that it was closed, and the kernel service removes the kernel from this._notebookBindings.

When we open a notebook, we will restore kernel from view state. If the kernel is not registered yet, then it skips this step.

This is an issue and I can repro it with my kernels if I do this

  • Open notebook, select kernel
  • Close notebook
  • Reload window
  • Reopen same notebook: sometimes, view state restore happens before kernels are done registering, and so the previous kernel is not selected

I suggest letting the view state restore set the previous kernel id into this._notebookBindings even before that kernel exists. When the kernel is registered, it will be set up correctly.

Looking over the code, I'm not sure whether it will be an issue somewhere for this._notebookBindings to have a binding to a kernel that does not exist? I think it's ok? I've been testing this and also testing the case where we restore a kernel id that is never actually registered at all due to being deleted, and that seems to be handled fine.

roblourens added a commit that referenced this issue Feb 3, 2023
…lose

And also only persist the selected kernel in the editor view state for untitled notebook editors
Fix #171385
roblourens added a commit that referenced this issue Feb 3, 2023
…lose (#173352)

And also only persist the selected kernel in the editor view state for untitled notebook editors
Fix #171385
@vscodenpa vscodenpa added unreleased Patch has not yet been released in VS Code Insiders insiders-released Patch has been released in VS Code Insiders and removed unreleased Patch has not yet been released in VS Code Insiders labels Feb 3, 2023
c-claeys pushed a commit to c-claeys/vscode that referenced this issue Feb 16, 2023
…lose (microsoft#173352)

And also only persist the selected kernel in the editor view state for untitled notebook editors
Fix microsoft#171385
@connor4312
Copy link
Member

@DonJayamanne can you verify this?

@rzhao271 rzhao271 added the author-verification-requested Issues potentially verifiable by issue author label Feb 23, 2023
@amunger amunger added the verified Verification succeeded label Feb 23, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Mar 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
author-verification-requested Issues potentially verifiable by issue author bug Issue identified by VS Code Team member as probable bug insiders-released Patch has been released in VS Code Insiders notebook-kernel-picker verified Verification succeeded
Projects
None yet
7 participants