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

Upgrade to new lsp middleware concat #8219

Merged
merged 6 commits into from
Nov 10, 2021
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -1980,7 +1980,7 @@
"@jupyterlab/services": "^6.1.17",
"@lumino/widgets": "^1.28.0",
"@nteract/messaging": "^7.0.0",
"@vscode/jupyter-lsp-middleware": "^0.2.19",
"@vscode/jupyter-lsp-middleware": "^0.2.21",
"ansi-to-html": "^0.6.7",
"arch": "^2.1.0",
"bootstrap": "^4.3.1",
Expand Down
34 changes: 10 additions & 24 deletions src/client/datascience/notebook/intellisense/languageServer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,12 @@ import {
NotebookDocument,
workspace,
window,
NotebookConcatTextDocument,
notebooks,
Event,
Uri
Uri,
NotebookCellsChangeEvent
} from 'vscode';
import {
ClientCapabilities,
DocumentSelector,
DynamicFeature,
ExecuteCommandRegistrationOptions,
ExecuteCommandRequest,
Expand Down Expand Up @@ -89,23 +87,6 @@ class NerfedExecuteCommandFeature implements DynamicFeature<ExecuteCommandRegist
}
}

// TODO: Export this api from the lsp middleware instead of just having the type match
const notebookApi = new (class {
public get onDidOpenNotebookDocument(): Event<NotebookDocument> {
return workspace.onDidOpenNotebookDocument;
}
public get onDidCloseNotebookDocument(): Event<NotebookDocument> {
return workspace.onDidCloseNotebookDocument;
}
public get notebookDocuments(): ReadonlyArray<NotebookDocument> {
return workspace.notebookDocuments;
}
public createConcatTextDocument(doc: NotebookDocument, selector?: DocumentSelector): NotebookConcatTextDocument {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
return notebooks.createConcatTextDocument(doc, selector) as any;
}
})();

/**
* This class wraps an instance of the language server (either Pylance or Jedi LSP) per interpreter.
*
Expand All @@ -121,6 +102,7 @@ export class LanguageServer implements Disposable {
private disposables: Disposable[]
) {
this._interpreterId = getInterpreterId(interpreter);
notebooks.onDidChangeNotebookCells(this.onDidChangeNotebookCells, this, disposables);
}

public async dispose() {
Expand Down Expand Up @@ -157,16 +139,15 @@ export class LanguageServer implements Disposable {
const middleware =
middlewareType == 'jupyter'
? createNotebookMiddleware(
notebookApi,
() => languageClient,
() => noop, // Don't trace output. Slows things down too much
NOTEBOOK_SELECTOR,
/.*\.(ipynb|interactive)/m,
interpreter.path,
(uri) => shouldAllowIntellisense(uri, interpreterId, interpreter.path)
)
: createPylanceMiddleware(
() => languageClient,
NOTEBOOK_SELECTOR,
interpreter.path,
(uri) => shouldAllowIntellisense(uri, interpreterId, interpreter.path)
);
Expand Down Expand Up @@ -219,6 +200,11 @@ export class LanguageServer implements Disposable {
}
}

private onDidChangeNotebookCells(e: NotebookCellsChangeEvent) {
// Tell the middleware to refresh its concat document (pylance or notebook)
this.middleware.refresh(e.document);
}

private static async createServerOptions(
interpreter: PythonEnvironment,
cancellationStrategy: FileBasedCancellationStrategy
Expand Down Expand Up @@ -259,7 +245,7 @@ export class LanguageServer implements Disposable {
const bundlePath = path.join(distPath, 'server.bundle.js');
const nonBundlePath = path.join(distPath, 'server.js');
const modulePath = (await fs.pathExists(nonBundlePath)) ? nonBundlePath : bundlePath;
const debugOptions = { execArgv: ['--nolazy', '--inspect=6600'] };
const debugOptions = { execArgv: ['--nolazy', '--inspect=6617'] };

// If the extension is launched in debug mode, then the debug server options are used.
const serverOptions: ServerOptions = {
Expand Down
7 changes: 7 additions & 0 deletions src/client/datascience/notebook/vscodeNotebookController.ts
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,13 @@ export class VSCodeNotebookController implements Disposable {
public async updateNotebookAffinity(notebook: NotebookDocument, affinity: NotebookControllerAffinity) {
traceInfo(`Setting controller affinity for ${getDisplayPath(notebook.uri)} ${this.id}`);
this.controller.updateNotebookAffinity(notebook, affinity);

// Stick in our list of associated documents and indicate controller may have changed.
if (!this.associatedDocuments.has(notebook)) {
this.associatedDocuments.add(notebook);
this._onNotebookControllerSelected.fire({ notebook, controller: this });
Copy link
Contributor

Choose a reason for hiding this comment

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

Setting affinity is not the same as setting it as selected.
You could have a notebook that's associated with kernel A, but the recommended kernel (affinity) is Kernel B.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd consider raising a seprate event and or having a seprate method to get the recommended kernel.
I believe you need this for instances where a kernel isn't seleced, but want to provide the right intellisense, if that's the case a seprate property/event would be better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No this is exactly what I want.

This is what is happening now.

  • User opens a notebook, kernel is associated with document, notebook selection change event fires
  • User closes notebook
  • User reopens notebook. Nothing fires, controller doesn't even know what the selection is.

I think what should happen is the same thing as the first one

  • User opens a notebook, kernel is associated with document, notebook selection change event fires

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I need this so that on second open, the intellisense is using the correct (selected) kernel.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On first open, the reason it works is because the selection change event fires after setting affinity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This code here:

this.associatedDocuments.add(event.notebook);

Which is triggered by this:

this.controller.onDidChangeSelectedNotebooks(this.onDidChangeSelectedNotebooks, this, this.disposables);

On a second open that selection changed event doesn't fire.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok got it, but if you create a brand new notebook, then this event will still get fired even when no kernel is selected.
Today there's a bug in VS Code causing kernels to get auto selecfed, hence this will work.

I'll merge my fix tomorrow into VS Code and then we can re-visit this PR.
I still don't think this is right.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay trying a different way. We can just remember it like VS code does. When it gets set, stick it in a map.

this._onNotebookControllerSelectionChanged.fire();
}
}

// Handle the execution of notebook cell
Expand Down