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

Jupyter notebook kernel changes not detected to Pylance #6333

Closed
Tracked by #7364
aroxred opened this issue Jun 19, 2021 · 24 comments · Fixed by #7924
Closed
Tracked by #7364

Jupyter notebook kernel changes not detected to Pylance #6333

aroxred opened this issue Jun 19, 2021 · 24 comments · Fixed by #7924
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug notebook-intellisense Intellisense & other language features in notebook cells for any language verified Verification succeeded
Milestone

Comments

@aroxred
Copy link

aroxred commented Jun 19, 2021

Environment data

  • VS Code version: 1.57.1
  • Jupyter Extension version (available under the Extensions sidebar): v2021.6.999406279
  • Python Extension version (available under the Extensions sidebar): v2021.6.944021595
  • OS (Windows | Mac | Linux distro) and version: Windows 19043
  • Python and/or Anaconda version: 3.8.5
  • Type of virtual environment used (N/A | venv | virtualenv | conda | ...): venv
  • Jupyter server running: Local

Expected behaviour

Upon changing the kernel in the top right Pylance should detect module changes and resolve imports

Code_AIlNLawSya

Actual behaviour

Pylance will only detect kernel changes if you change the interpreter via the command palette

Before changing via command palette
Code_5gyZtvgN7r

After changing
Code_A4F75UVgZw

Imports resolve

Steps to reproduce:

  1. Create a venv and install any pip module foo
  2. Create a notebook and import foo
  3. The pylance import detection will not update via changing kernel
  4. Changing via Python: Select Interpreter in the command palette will update Pylance

Note: code execution will work as if the venv is selected, but highlighting doesn't.
Another note: I'm sure a lot of my lingo I'm using is incorrect, I'm just starting out :)

@aroxred aroxred added the bug Issue identified by VS Code Team member as probable bug label Jun 19, 2021
@aroxred
Copy link
Author

aroxred commented Jun 19, 2021

I've noticed this only started happening after I updated vscode when the kernel selector moved from the bottom left to the top right.

@claudiaregio
Copy link
Contributor

cc/ @savannahostrowski @jakebailey

@joyceerhl
Copy link
Contributor

This is because Pylance resolves imports against the active Python interpreter and not whichever kernel the notebook is running against. Pylance doesn't know about the Jupyter kernel AFAIK.

Not sure if we've already discussed this, but perhaps when the user changes Python kernels, the Jupyter extension can find the interpreter most closely associated with the new environment if any and set that as the active Python interpreter?

@aroxred
Copy link
Author

aroxred commented Jun 19, 2021

I didn't realize that there was a big difference between jupyter kernels and python interpreters. My kernel list and interpreter list is identical.

@rchiodo
Copy link
Contributor

rchiodo commented Jun 21, 2021

We should discuss this with the pylance team to see how we resolve this.

@jakebailey
Copy link
Member

Based on previous meetings, my impression was that you were going to spawn kernel-specific language servers and route jupyter requests though them, rather than the normal LS, avoiding this issue. Has something changed, or have you just had other things on your plate in the meantime?

@rchiodo
Copy link
Contributor

rchiodo commented Jun 21, 2021

Based on previous meetings, my impression was that you were going to spawn kernel-specific language servers and route jupyter requests though them, rather than the normal LS, avoiding this issue. Has something changed, or have you just had other things on your plate in the meantime?

We haven't gotten to this and I'm not sure it was entirely clear this was the plan all along. I think if we all met in person we could discuss the downsides/upsides to that approach. There were some worries pylance wouldn't be happy with us spinning up multiple copies of it.

@greazer greazer added the notebook-intellisense Intellisense & other language features in notebook cells for any language label Aug 3, 2021
@greazer greazer modified the milestones: August 2021, July 2021 Aug 9, 2021
@rchiodo
Copy link
Contributor

rchiodo commented Aug 20, 2021

Crux of problem:

  • One document selector for 'python' files that maps to one single language server.
  • Active interpreter is registered as the 'python' language server (passed to pylance on startup)
  • Always uses this to return intellisense

Document selector decides language client to use. Document selector isn't specific enough to pick a different language client based on interpreter.

Implementation thoughts:

Thought 1:

  • Make document selector for python specific to only python files (those ending in .py?)
  • Make new selector for every open notebook
  • Create in python/jupyter extension a new language client per notebook (where selector is that specific file name)
  • Means pylance running per notebook

Thought 2:

  • Make document selector for python specific to only python files
  • Make new selector for notebook files (ipynb and language python)
  • Register providers manually and multiplex to a language client connection?

Thought 3:

  • Make document selector for python specific to only python files
  • Make new selector for notebook files (ipynb and language python)
  • Make new language server exe that does multiplexing internally for every request based on mapping between uri for request and interpreter. Under the covers it sends the requests to one of many pylance servers.

Thought 4:

  • Modify pylance to understand interpreters are not per instance but per file.
  • Pass file to interpreter map to pylance

@DavidKutu DavidKutu removed their assignment Aug 20, 2021
@rchiodo rchiodo self-assigned this Aug 20, 2021
@jakebailey
Copy link
Member

jakebailey commented Aug 23, 2021

We can't make the document selector specific to only Python files, at least not by extension; the selector needs to be "any and all files marked as python" in order to handle untitled files, scripts wihtout .py, sage files, etc.

@greazer greazer modified the milestones: August 2021, September 2021 Aug 27, 2021
@rchiodo
Copy link
Contributor

rchiodo commented Aug 30, 2021

After some more discussion, the tentative plan is:

  • Modify python extension middleware to swallow all of the LS requests for notebooks.
    • This allows the document selector for python LS to only apply to python files/scripts (we can't just use .py)
  • Modify some other code to register a LS for each notebook as they open
  • Move the middleware concat code to this other code (it still needs to concat the cells into one file)
  • The register for each notebook would essentially launch a pylance/jedi LSP for each notebook

@rchiodo
Copy link
Contributor

rchiodo commented Aug 30, 2021

Additionally the location for this will likely be in the Jupyter extension (or this npm module: https://github.com/microsoft/vscode-jupyter-lsp-middleware)

@jakebailey
Copy link
Member

jakebailey commented Aug 31, 2021

One deal breaker here may be commands; more than one LS shouldn't be registering as the handler for the same commands, but Pylance uses all sorts of commands for various purposes. Starting them up separately may cause VS Code to complain, or certain actions (some completions, quick fixes, our fancier extraction code action) to break.

@rchiodo
Copy link
Contributor

rchiodo commented Sep 7, 2021

Implementation ideas:

  • Change Middleware layer constructor to a factory that takes the ctor arguments. This returns one of two middleware options
    • Python based one that is just for swallowing events
    • Notebook based one that is per notebook
  • Python Middleware
    • if (isNotebookCell(event.document.uri)
    • Then do nothing
  • Notebook Middleware?
    • Should this be middleware? Might be easiest to port that way.
    • LanguageClient code will do the heavy lifting of registering itself and starting pylance

@jakebailey
Copy link
Member

Another option that we could consider is just making the "real fix" and making the LS notebook aware, at least for LSs that are known to support them. We'd need to spec out the details of how to ask for the kernel's interpreter, figure out how to move the contact doc, and then modify Pylance internally to reconstruct and maybe spawn multiple faked internal workspaces, but that may be the "correct" long term solution. Depends on how hard the above turns out to be in the end, I guess.

@rchiodo
Copy link
Contributor

rchiodo commented Sep 14, 2021

After implementing a language server per notebook, I'm ending up with about 100MBs allocated per notebook running (what pylance on my machine seems to keep in memory).

So 10 notebooks open is 1GB of memory reserved. Seems like a lot. It would be a lot less if DocumentSelector could use a function instead of a pattern for matching (then I could have one server per interpreter in use)

@rchiodo
Copy link
Contributor

rchiodo commented Sep 14, 2021

Alternative ideas

  1. Modify DocumentSelector to take a function (not sure if this works as it likely needs to go back to VS code for some stuff)
  2. Make a language server that multiplexes (seems silly, should just have pylance do this)
  3. Register all of the providers manually and have the multiplexing happen there (this is a maintenance headache. Have to reimplement a bunch of parts of vscode-langaugeclient and might get it wrong)
  4. Figure out what all the memory being consumed is and fix it?
  5. Instead of using next in middleware, call into the appropriate client instead. getClient() method could be changed to take a URI.

@rchiodo
Copy link
Contributor

rchiodo commented Sep 14, 2021

Expanding on option 5:

  • Selector is just for notebooks (extension ending in ipynb) and language python.
  • Create dummy language client for all notebooks. It wires up the middleware
  • Create language client for every interpreter needed. Selector is nothing (so no matches). No middleware necessary.
  • Request comes in on dummy language client, middleware redirects to one of the other clients.

Responses come back on wrong client? Wouldn't work if there's some state in the client before calling the middleware.

@rchiodo
Copy link
Contributor

rchiodo commented Sep 14, 2021

Another idea:

Create a language server for every interpreter that starts (default to active).
Map all notebook cells to all language clients (all have same selector)
Swallow results in middleware for the ones not active? (active is set by URI of incoming data)
Something like:

    public provideCompletionItem(
        document: TextDocument,
        position: Position,
        context: CompletionContext,
        token: CancellationToken,
        next: ProvideCompletionItemsSignature
    ) {
        if (isMyNotebookCell(document.uri)) {
            const newDoc = this.converter.toOutgoingDocument(document);
            const newPos = this.converter.toOutgoingPosition(document, position);
            const result = next(newDoc, newPos, context, token);
            if (isThenable(result)) {
                return result.then(this.converter.toIncomingCompletions.bind(this.converter, document));
            }
            return this.converter.toIncomingCompletions(document, result);
        }
    }

Where isMyNotebookCell checks URI against known matching URIs for the interpreter for a middleware/client
Same principle as the python extension having its items swallowed.

Maybe pass isMyNotebookCell into the middleware as a function (so can be applied by jupyter extension which knows the mapping)

@rchiodo
Copy link
Contributor

rchiodo commented Oct 15, 2021

This requires updates to both the jupyter extension and the python extension (python extension was providing notebook intellisense before this change).

PR for python is still in progress: microsoft/vscode-python#17734

@DonJayamanne
Copy link
Contributor

Woo hoo..

@humbertosamora
Copy link

Hello, folks.

I've found a workaround for this issue. I've to configure python.analysis.extraPaths in settings.json of the current project in VSCode:
{
"python.analysis.extraPaths": [ "path_to_venv_site_pakages" ],
}

I'm working in a local project with a conda venv enviroment, so the config looks like:
{
"python.analysis.extraPaths": [ "D:\Users\hs\Envs\udemyDSPy\lib\site-packages" ],
}
As the config is specific to project, we can change this in other projects to point the right venv packages import location.

*Note.: I'm using VSCode 1.64.2 version.

@GF-Huang
Copy link

GF-Huang commented Jul 22, 2022

Not work when I use a remote server and remote kernel. Import success, but Pylance could not be resolved.

image

  • Jupyter v2022.6.1201981810
  • Python v2022.10.1
  • Pylance v2022.7.40
  • VSCode 1.69.2

@rchiodo
Copy link
Contributor

rchiodo commented Jul 22, 2022

In a remote situation, pylance isn't on the remote machine so it doesn't know anything about the remote environment. In that situation pylance only knows about system modules.

@braindevices
Copy link

braindevices commented Jul 13, 2023

Is this #13907 a worsen regression? Now manual change the interpreter fails to fix it in vscode 1.80

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 29, 2023
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-intellisense Intellisense & other language features in notebook cells for any language verified Verification succeeded
Projects
None yet
Development

Successfully merging a pull request may close this issue.