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

Add priority setting for servers handling the same language #588

Merged
merged 5 commits into from
May 17, 2021
Merged
Show file tree
Hide file tree
Changes from 2 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
10 changes: 8 additions & 2 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,8 +1,14 @@
## Changelog

### (unreleased)
### `@krassowski/jupyterlab-lsp 3.7.0` (unreleased)

- Add ability to deactivate Kernel completions or LSP completion through the settings.
- features:

- add ability to deactivate Kernel completions or LSP completion through the settings ([#586], thanks @Carreau)
- allow to set a priority for LSP server, allowing to choose which server to use when multiple servers are installed ([#588])

[#586]: https://github.com/krassowski/jupyterlab-lsp/pull/586
[#588]: https://github.com/krassowski/jupyterlab-lsp/pull/588

### `jupyter-lsp 1.2.0` (2021-04-26)

Expand Down
2 changes: 1 addition & 1 deletion atest/03_Notebook.robot
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ Moving Cells Around
Foreign Extractors
${file} = Set Variable Foreign extractors.ipynb
Configure JupyterLab Plugin
... {"language_servers": {"texlab": {"serverSettings": {"latex.lint.onChange": true}}, "bash-langauge-server": {"bashIde.highlightParsingErrors": true}}}
... {"language_servers": {"texlab": {"serverSettings": {"latex.lint.onChange": true}}, "bash-langauge-server": {"bashIde.highlightParsingErrors": true}}, "pylsp": {"priority": 1000}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

getting long... maybe we hoist to a fixture file?

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, could we have Configure JupyterLab Plugin merge the new settings with the default priorities (unless custom priorities are given)? This should simplify tests that have custom config.

Copy link
Member Author

Choose a reason for hiding this comment

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

Or just move "pylsp": {"priority": 1000} to a fixture file. I think we used to have a json file for frontend settings but cannot see it.

Capture Page Screenshot 10-configured.png
Reset Application State
Setup Notebook Python ${file}
Expand Down
3 changes: 2 additions & 1 deletion docs/Configuring.ipynb
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,8 @@
" `mkdir \"new directory\"` should be split into `[\"mkdir\", \"new directory\"]`;\n",
" If you have Python installed, you can use `shlex.split(\"your command\")` to\n",
" get such an array.\n",
"- the `languages` which the language server will respond to, and\n",
"- the `languages` which the language server will respond to (language names\n",
" should be given in lowercase), and\n",
krassowski marked this conversation as resolved.
Show resolved Hide resolved
"- the schema `version` of the spec (currently `2`)\n",
"- `mime_types` by which the notebooks and files will be matched to the language\n",
" server:\n",
Expand Down
5 changes: 3 additions & 2 deletions docs/Language Servers.ipynb
Original file line number Diff line number Diff line change
Expand Up @@ -137,8 +137,9 @@
"\n",
"These servers have support for notebooks and file editors. The `pyls` and\n",
"`r-languageserver` are well-tested, while `jedi` and `Julia` servers are\n",
"experimental. Please only install one language server per language (`jedi` or\n",
"`pyls` for Python - not both)."
"experimental. If you choose to install multiple language servers for the same\n",
"language, the one with the highest `priority` (which can be set in the _Advanced\n",
"Settings Editor_) will be used."
]
},
{
Expand Down
6 changes: 6 additions & 0 deletions packages/jupyterlab-lsp/schema/plugin.json
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,12 @@
"description": "Configuration to be sent to language server over LSP when initialized: see the specific language server's documentation for more",
"type": "object",
"default": {}
},
"priority": {
"title": "Priority of the server",
"description": "When multiple servers match specific document/language, the server with higher priority will be used",
krassowski marked this conversation as resolved.
Show resolved Hide resolved
"type": "number",
"default": 50
krassowski marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
Expand Down
58 changes: 36 additions & 22 deletions packages/jupyterlab-lsp/src/components/statusbar.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,12 @@ import { LSPConnection } from '../connection';
import { DocumentConnectionManager } from '../connection_manager';
import { SERVER_EXTENSION_404 } from '../errors';
import { LanguageServerManager } from '../manager';
import { ILSPAdapterManager, ILanguageServerManager } from '../tokens';
import {
ILSPAdapterManager,
ILanguageServerManager,
TSessionMap,
TLanguageServerId
} from '../tokens';
import { VirtualDocument, collect_documents } from '../virtual/document';

import { codeCheckIcon, codeClockIcon, codeWarningIcon } from './icons';
Expand Down Expand Up @@ -464,34 +469,44 @@ export namespace LSPStatus {
}, this);
}

get available_servers(): Array<SCHEMA.LanguageServerSession> {
return Array.from(this.language_server_manager.sessions.values());
get available_servers(): TSessionMap {
return this.language_server_manager.sessions;
}

get supported_languages(): Set<string> {
const languages = new Set<string>();
for (let server of this.available_servers) {
for (let server of this.available_servers.values()) {
for (let language of server.spec.languages) {
languages.add(language.toLocaleLowerCase());
}
}
return languages;
}

private is_server_running(server: SCHEMA.LanguageServerSession): boolean {
for (let language of server.spec.languages) {
if (this.detected_languages.has(language.toLocaleLowerCase())) {
private is_server_running(
id: TLanguageServerId,
server: SCHEMA.LanguageServerSession
): boolean {
for (const language of this.detected_languages) {
const matchedServers = this.language_server_manager.getMatchingServers({
language
});
// TODO server.status === "started" ?
// TODO update once multiple servers are allowed
if (matchedServers.length && matchedServers[0] === id) {
return true;
}
}
return false;
}

get documents_by_server(): Map<
SCHEMA.LanguageServerSession,
Map<string, VirtualDocument[]>
> {
let data = new Map();
let data = new Map<
SCHEMA.LanguageServerSession,
Map<string, VirtualDocument[]>
>();
if (!this.adapter?.virtual_editor) {
return data;
}
Expand All @@ -501,19 +516,17 @@ export namespace LSPStatus {

for (let document of documents.values()) {
let language = document.language.toLocaleLowerCase();
let servers = this.available_servers.filter(
server => server.spec.languages.indexOf(language) !== -1
let server_ids = this._connection_manager.language_server_manager.getMatchingServers(
{ language: document.language }
);
if (servers.length > 1) {
console.warn('More than one server per language for', language);
}
if (servers.length === 0) {
if (server_ids.length === 0) {
continue;
}
let server = servers[0];
// For now only use the server with the highest priority
Comment on lines -504 to +525
Copy link
Member Author

Choose a reason for hiding this comment

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

Should probably use the connection + server Identifier here. Also should we restart all connections if preferred language server changed our just add a note that JupyterLab reload is needed for changes to go into effect? Probably the latter

Copy link
Collaborator

Choose a reason for hiding this comment

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

i think wherever possible, settings changes should take effect immediately, since have established the pattern... can see how it would be a tad onerous

Copy link
Member Author

Choose a reason for hiding this comment

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

Right. But I would prefer not to do this now given the plan to rewrite it later to support multiple connections per document which would invalidate the logic; lets consider the priority setting a tool for us to test multiple servers on CI for now, and a user-facing feature once it starts defining priority between multiple servers running in tandem.

let server = this.language_server_manager.sessions.get(server_ids[0]);

if (!data.has(server)) {
data.set(server, new Map<string, VirtualDocument>());
data.set(server, new Map<string, VirtualDocument[]>());
}

let documents_map = data.get(server);
Expand All @@ -529,9 +542,9 @@ export namespace LSPStatus {
}

get servers_available_not_in_use(): Array<SCHEMA.LanguageServerSession> {
return this.available_servers.filter(
server => !this.is_server_running(server)
);
return [...this.available_servers.entries()]
.filter(([id, server]) => !this.is_server_running(id, server))
.map(([id, server]) => server);
}

get detected_languages(): Set<string> {
Expand Down Expand Up @@ -577,10 +590,11 @@ export namespace LSPStatus {

detected_documents.forEach((document, uri) => {
let connection = this._connection_manager.connections.get(uri);
let server_id = this._connection_manager.language_server_manager.getServerId(
let server_ids = this._connection_manager.language_server_manager.getMatchingServers(
{ language: document.language }
);
if (server_id !== null) {

if (server_ids.length !== 0) {
documents_with_known_servers.add(document);
}
if (!connection) {
Expand Down
6 changes: 5 additions & 1 deletion packages/jupyterlab-lsp/src/connection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,17 @@ import * as lsProtocol from 'vscode-languageserver-protocol';

import { until_ready } from './utils';

interface ILSPOptions extends ILspOptions {}
interface ILSPOptions extends ILspOptions {
serverIdentifier?: string;
}

export class LSPConnection extends LspWsConnection {
protected documentsToOpen: IDocumentInfo[];
public serverIdentifier: string;

constructor(options: ILSPOptions) {
super(options);
this.serverIdentifier = options?.serverIdentifier;
this.documentsToOpen = [];
}

Expand Down
48 changes: 34 additions & 14 deletions packages/jupyterlab-lsp/src/connection_manager.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1,14 @@
import { PageConfig, URLExt } from '@jupyterlab/coreutils';
import { Signal } from '@lumino/signaling';
import type * as protocol from 'vscode-languageserver-protocol';

import type * as ConnectionModuleType from './connection';
import {
ILSPLogConsole,
ILanguageServerConfiguration,
ILanguageServerManager,
TLanguageServerConfigurations,
TLanguageServerId
TLanguageServerId,
TServerKeys
} from './tokens';
import { expandDottedPaths, sleep, until_ready } from './utils';
import { IForeignContext, VirtualDocument } from './virtual/document';
Expand Down Expand Up @@ -134,9 +135,14 @@ export class DocumentConnectionManager {
language
);

const language_server_id = this.language_server_manager.getServerId({
const matchingServers = this.language_server_manager.getMatchingServers({
language
});
this.console.debug('Matching servers: ', matchingServers);

// for now use only the server with the highest priority.
const language_server_id =
matchingServers.length === 0 ? null : matchingServers[0];

// lazily load 1) the underlying library (1.5mb) and/or 2) a live WebSocket-
// like connection: either already connected or potentially in the process
Expand All @@ -161,13 +167,21 @@ export class DocumentConnectionManager {
* "serverSettings" keyword in the setting registry. New keywords can
* be added and extra functionality implemented here when needed.
*/
public updateServerConfigurations(allServerSettings: any) {
for (let language_server_id in allServerSettings) {
const parsedSettings = expandDottedPaths(
allServerSettings[language_server_id].serverSettings
);
public updateServerConfigurations(
allServerSettings: TLanguageServerConfigurations
) {
let language_server_id: TServerKeys;
this.language_server_manager.setConfiguration(allServerSettings);

for (language_server_id in allServerSettings) {
if (!allServerSettings.hasOwnProperty(language_server_id)) {
continue;
}
const rawSettings = allServerSettings[language_server_id];

const serverSettings: ILanguageServerConfiguration = {
const parsedSettings = expandDottedPaths(rawSettings.serverSettings);

const serverSettings: protocol.DidChangeConfigurationParams = {
settings: parsedSettings
};

Expand Down Expand Up @@ -351,9 +365,14 @@ export namespace DocumentConnectionManager {
? rootUri
: virtualDocumentsUri;

const language_server_id = Private.getLanguageServerManager().getServerId({
language
});
// for now take the best match only
const matchingServers = Private.getLanguageServerManager().getMatchingServers(
{
language
}
);
const language_server_id =
matchingServers.length === 0 ? null : matchingServers[0];

if (language_server_id === null) {
throw `No language server installed for language ${language}`;
Expand Down Expand Up @@ -425,7 +444,8 @@ namespace Private {
const connection = new LSPConnection({
languageId: language,
serverUri: uris.server,
rootUri: uris.base
rootUri: uris.base,
serverIdentifier: language_server_id
});
// TODO: remove remaining unbounded users of connection.on
connection.setMaxListeners(999);
Expand All @@ -441,7 +461,7 @@ namespace Private {

export function updateServerConfiguration(
language_server_id: TLanguageServerId,
settings: ILanguageServerConfiguration
settings: protocol.DidChangeConfigurationParams
): void {
const connection = _connections.get(language_server_id);
if (connection) {
Expand Down
9 changes: 6 additions & 3 deletions packages/jupyterlab-lsp/src/editor_integration/testutils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ export interface ITestEnvironment {
export class MockLanguageServerManager extends LanguageServerManager {
async fetchSessions() {
this._sessions = new Map();
this._sessions.set('pyls', {
this._sessions.set('pylsp', {
spec: {
languages: ['python']
}
Expand Down Expand Up @@ -111,7 +111,9 @@ export class MockExtension implements ILSPExtension {
this.app = null;
this.feature_manager = new FeatureManager();
this.editor_type_manager = new VirtualEditorManager();
this.language_server_manager = new MockLanguageServerManager({});
this.language_server_manager = new MockLanguageServerManager({
console: new BrowserConsole()
});
this.connection_manager = new DocumentConnectionManager({
language_server_manager: this.language_server_manager,
console: new BrowserConsole()
Expand Down Expand Up @@ -240,7 +242,8 @@ function FeatureSupport<TBase extends TestEnvironmentConstructor>(Base: TBase) {
return new LSPConnection({
languageId: this.document_options.language,
serverUri: 'ws://localhost:8080',
rootUri: 'file:///unit-test'
rootUri: 'file:///unit-test',
serverIdentifier: 'pylsp'
krassowski marked this conversation as resolved.
Show resolved Hide resolved
});
}

Expand Down
6 changes: 4 additions & 2 deletions packages/jupyterlab-lsp/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -146,10 +146,12 @@ export class LSPExtension implements ILSPExtension {
status_bar: IStatusBar | null
) {
const trans = (translator || nullTranslator).load('jupyterlab-lsp');
this.language_server_manager = new LanguageServerManager({});
this.language_server_manager = new LanguageServerManager({
console: this.console.scope('LanguageServerManager')
});
this.connection_manager = new DocumentConnectionManager({
language_server_manager: this.language_server_manager,
console: this.console
console: this.console.scope('DocumentConnectionManager')
});

const statusButtonExtension = new StatusButtonExtension({
Expand Down
Loading