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 change events are not firing when changing a kernel #15965

Closed
rchiodo opened this issue Dec 1, 2023 · 15 comments
Closed

Notebook change events are not firing when changing a kernel #15965

rchiodo opened this issue Dec 1, 2023 · 15 comments
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug notebook-execution Kernels issues (start/restart/switch/execution, install ipykernel)
Milestone

Comments

@rchiodo
Copy link
Contributor

rchiodo commented Dec 1, 2023

Does this issue occur when all extensions are disabled?: No, requires the Jupyter extension

  • VS Code Version: Latest insiders
  • OS Version: Windows 11

Steps to Reproduce:

  1. Git clone https://github.com/rchiodo/test-notebook-change (example extension)
  2. Build the extension
  3. Git clone https://github.com/bokeh/tutorial
  4. Setup a python environment with bokeh in it
  5. Install the Jupyter extension and the Python extension
  6. Launch the extension from step 1
  7. Open the bokeh tutorial folder
  8. Open one of the notebooks
  9. Select a kernel and pick one that uses the bokeh environment
  10. Notice the change output in the extension debug console
  11. Open another notebook
  12. Clear console output for debugging extension
  13. Pick that kernel selected in the last notebook

Expected result:
Kernel change notification happens

Actual result:
No changes detected.

This seems to be causing this issue here:
microsoft/pylance-release#5172

@rchiodo
Copy link
Contributor Author

rchiodo commented Dec 1, 2023

I tried this same thing with just a plain folder and a plain notebook and I can't reproduce it there. It only reproduces with this bokeh example for me.

@vscodenpa vscodenpa added the triage-needed Issue needs to be triaged label Dec 1, 2023
@meganrogge meganrogge removed the triage-needed Issue needs to be triaged label Dec 4, 2023
@meganrogge meganrogge assigned DonJayamanne and unassigned meganrogge Dec 4, 2023
@DonJayamanne DonJayamanne added bug Issue identified by VS Code Team member as probable bug and removed bug Issue identified by VS Code Team member as probable bug labels Dec 4, 2023
@DonJayamanne DonJayamanne assigned rebornix and unassigned DonJayamanne Dec 4, 2023
@rebornix rebornix added bug Issue identified by VS Code Team member as probable bug notebook-execution Kernels issues (start/restart/switch/execution, install ipykernel) labels Dec 5, 2023
@rebornix rebornix added this to the On Deck milestone Dec 5, 2023
@rchiodo
Copy link
Contributor Author

rchiodo commented Jan 25, 2024

I can do this with a simple notebook now if I'm using anaconda, although maybe that's just a coincidence. Not sure how anaconda could change the timing.

@rjra2611
Copy link

Facing the same issue, it seems to work again if I downgrade to VSCode 1.84.0-insider didn't test it on 1.84.0-stable. Do we have any timeline for the resolution? Thanks

@rebornix
Copy link
Member

rebornix commented Jul 4, 2024

My understanding is the reason we emit content change event on kernel change is the kernel spec metadata is modified. If the kernel spec metadata is not changed (e.g., switching from one python 3.12 to another 3.12), there is no document metadata change so it will not emit any content change either.

IIRC Pylance gets the python interpreter path from Jupyter extension, I wonder if this can be an event from Jupyter itself.

@rchiodo
Copy link
Contributor Author

rchiodo commented Jul 5, 2024

Pylance gets the python interpreter path from Jupyter extension, I wonder if this can be an event from Jupyter itself.

This works too. We'd have to send a custom message, although we'd probably just send the notebook did change message

@rchiodo
Copy link
Contributor Author

rchiodo commented Jul 5, 2024

It may not be relevant but if we had a custom message, wouldn't that mess up other extensions that might be relying on tracking kernel changes?

@imadjarov-quantinuum
Copy link

I can confirm this is still an issue on all latest releases, as I reported here microsoft/pylance-release#6285

Every time I want to open a Jupyter notebook for which I haven't already chosen a kernel, I have to

  1. Open notebook
  2. Choose kernel
  3. Close notebook
  4. Re-open notebook

Only then does the language server update. Fairly workflow disruptive.

@rchiodo
Copy link
Contributor Author

rchiodo commented Aug 27, 2024

Easiest reproduction for me:

  • Create a venv using global python, switch to this kernel
  • Create a kernel following global python
  • Switch back and forth

No messages sent about kernel change

@DonJayamanne
Copy link
Contributor

DonJayamanne commented Aug 28, 2024

@rchiodo
From what I can tell the Pylance extension has the following API

registerJupyterPythonPathFunction(func: (uri: Uri) => Promise<string | undefined>): void;

& from my understanding, pylance will call the registered function func to get the Path to the Python env.
However when a kernel changes, Pylance is not aware of this, hence the above function isn't invoked.
Is that correct?

If thats the case, then I'm proposing we create a new API as follows (or a new one):

registerJupyterPythonPathProvider(provider: JupyterPythonPathProvider);

interface JupyterPythonPathProvider {
    onDidChange: Event<Uri>;
    getPythonPath(uri: Uri): Promise<string | undefined>;
}

@DonJayamanne
Copy link
Contributor

DonJayamanne commented Aug 28, 2024

@karthiknadig
Rather than Pylance having to provide an API for Python to hook into, why not
Python provide an API instead
E.g. why not change getActiveEnvironmentPath to handle Resource that can be Notebook Uris.
If its a Notebook Uri, then return the Python env from Jupyter extension API,

This way, other LS will benefit from this as well, & any tool wanting the Python env used in a Jupyter notebook will be able to get it via a unifieid API.
Sure the code is async, in that case we can return undefined and once the value is resolved, we can trigger the onDidActiveInterpreterChangedEvent
Which can contain the Uri of the notebook.

I think that will make it simpler, and more accessible to other extensions as well.

@DonJayamanne
Copy link
Contributor

Moving this issue to VS Code-Jupyter extension, as this is not generic to VS Code.

@DonJayamanne DonJayamanne transferred this issue from microsoft/vscode Aug 28, 2024
@karthiknadig
Copy link
Member

This should be possible with the new API I am working on. This will have an API that can pass any Uri and get a python for it. We can discuss if that can fit here.

@rchiodo
Copy link
Contributor Author

rchiodo commented Aug 28, 2024

I disagree about putting this in the python extension (tracking kernel changes). Other extensions likely need to know when a kernel changes, regardless of whether or not the kernel is in a jupyter notebook, or in a different kind of notebook. Additionally, the python extension would only track changes to 'python' kernels. What if the user switches to a C# kernel? (and the C# kernel has the same issue with multiple kernels not causing a did change event).

The root cause of the problem is that onDidChangeNotebookDocument event doesn't fire unless the underlying text model changes. But jupyter notebooks are backed by more than one model - the file on disk, and the current kernel stored somewhere else (I assume in memento storage). I think the correct fix here is to fire the event when either changes.

@rchiodo
Copy link
Contributor Author

rchiodo commented Aug 28, 2024

I think we can make this a lot better with the fix I just proposed. display_name is saved in the ipynb file, so theoretically changing it should cause a kernel change event.

"metadata": {
  "kernelspec": {
   "display_name": ".venv",
   "language": "python",
   "name": "python3"
  },

@DonJayamanne
Copy link
Contributor

Closing as this PR was merged #15967
Will create a separate issue for a simpler API to get this information via Python Api

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue identified by VS Code Team member as probable bug notebook-execution Kernels issues (start/restart/switch/execution, install ipykernel)
Projects
None yet
Development

No branches or pull requests

8 participants