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

Support for LSP notebooks experiment #19087

Merged
merged 41 commits into from
May 17, 2022
Merged

Conversation

debonte
Copy link

@debonte debonte commented May 7, 2022

Contributes to https://github.com/microsoft/pyrx/issues/2343

When Pylance's LSP notebooks experiment is enabled, the Python extension will be responsible for starting Pylance for notebooks and therefore also needs to be able to provide the notebook's python.exe path to Pylance. Prior to this change, this was always done by Jupyter. This PR exposes a new Python API called registerJupyterPythonPathFunction which enables Jupyter to hook into Pylance's config setting requests and provide the python.exe path for a notebook's kernel.

The default behavior of notebooks is unchanged. The new behavior is only used when the experiment is enabled via the python.pylanceLspNotebooksEnabled setting and the installed versions of both Pylance and Jupyter are new enough to support the experiment.

A quick overview of the changes:

  1. The new LspNotebooksExperiment class determines if the experiment is enabled. It also deals with issues around transitioning into the experiment if Jupyter is installed after Python activates. For example, in this scenario it needs to restart any existing Pylance instances.
  2. I created Jedi and Node implementations of LanguageClientMiddleware, so the code related to the experiment would be separated from the Jedi-related logic.
  3. NodeLanguageClientMiddleware does not install the HidingMiddlewareAddon when the experiment is enabled.
  4. NodeLanguageClientMiddleware also overrides the new getPythonPathOverride method to call the Jupyter code provided via registerJupyterPythonPathFunction and get the kernel's python path.

The related Pylance changes are in this PR: https://github.com/microsoft/pyrx/pull/2382
The related Jupyter changes are in this PR: microsoft/vscode-jupyter#9842

@debonte debonte requested review from kimadeline and rchiodo May 7, 2022 04:46
@karrtikr karrtikr assigned kimadeline and unassigned karrtikr May 7, 2022
Copy link

@kimadeline kimadeline left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want to add a news item for this change? Something like "Experiment support for notebooks when using Pylance"? If not, do add the 'skip news' label to this PR.

src/client/languageServer/watcher.ts Outdated Show resolved Hide resolved
src/client/telemetry/index.ts Show resolved Hide resolved
@kimadeline
Copy link

Also, there will be merge conflicts depending on which PR gets merged in first between this one and #19076.

@debonte debonte added no-changelog No news entry required skip package*.json package.json and package-lock.json don't both need updating labels May 10, 2022
@rchiodo
Copy link

rchiodo commented May 11, 2022

I just thought of something. This whole thing is going to break de-duplication that we do for completions we get back from the jupyter kernel.

Right now, since the jupyter extension creates its own pylance, we can get the completions from pylance and remove those from jupyter that are the same thing.

After this change, the pylance server won't be started by jupyter and we won't be able to ask it directly for completions.

We need some way to find the specific completion provider (or the language client). So I think we'd need yet another API to pass the language client through to jupyter.

We could do that after this though.

This is the line where we ask pylance directly for its completions:
https://github.com/microsoft/vscode-jupyter/blob/8f7f38e2bc664836af236e8e2ad9f1b85130c3c9/src/intellisense/pythonKernelCompletionProvider.node.ts#L222

We'd need some way to get to this language client from jupyter still (well unless VS code has an API to return a specific completion provider but I don't think it does).

@rchiodo
Copy link

rchiodo commented May 11, 2022

I entered this issue here for the dedupe problem:
#19129

@debonte debonte merged commit bf68773 into microsoft:main May 17, 2022
@debonte debonte deleted the lspNotebooks branch May 17, 2022 22:06
wesm pushed a commit to posit-dev/positron that referenced this pull request Mar 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-changelog No news entry required skip package*.json package.json and package-lock.json don't both need updating
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants