Skip to content

Commit

Permalink
connected jupyter, got rid of lsp notebook exp checks, interactive no…
Browse files Browse the repository at this point in the history
…tebook check and etc.
  • Loading branch information
heejaechang committed Mar 9, 2023
1 parent 8f0c3d5 commit 4b5b66d
Show file tree
Hide file tree
Showing 12 changed files with 72 additions and 147 deletions.
2 changes: 1 addition & 1 deletion src/client/activation/languageClientMiddleware.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ export class LanguageClientMiddleware extends LanguageClientMiddlewareBase {
);
}

protected shouldCreateHidingMiddleware(jupyterDependencyManager: IJupyterExtensionDependencyManager): boolean {
private shouldCreateHidingMiddleware(jupyterDependencyManager: IJupyterExtensionDependencyManager): boolean {
return jupyterDependencyManager && jupyterDependencyManager.isJupyterExtensionInstalled;
}

Expand Down
4 changes: 0 additions & 4 deletions src/client/activation/node/analysisOptions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import { IExperimentService } from '../../common/types';

import { LanguageServerAnalysisOptionsBase } from '../common/analysisOptions';
import { ILanguageServerOutputChannel } from '../types';
import { LspNotebooksExperiment } from './lspNotebooksExperiment';
import { traceWarn } from '../../logging';

const EDITOR_CONFIG_SECTION = 'editor';
Expand All @@ -22,7 +21,6 @@ export class NodeLanguageServerAnalysisOptions extends LanguageServerAnalysisOpt
lsOutputChannel: ILanguageServerOutputChannel,
workspace: IWorkspaceService,
private readonly experimentService: IExperimentService,
private readonly lspNotebooksExperiment: LspNotebooksExperiment,
) {
super(lsOutputChannel, workspace);
}
Expand All @@ -36,8 +34,6 @@ export class NodeLanguageServerAnalysisOptions extends LanguageServerAnalysisOpt
return ({
experimentationSupport: true,
trustedWorkspaceSupport: true,
lspNotebooksSupport: this.lspNotebooksExperiment.isInNotebooksExperiment(),
lspInteractiveWindowSupport: this.lspNotebooksExperiment.isInNotebooksExperimentWithInteractiveWindowSupport(),
autoIndentSupport: await this.isAutoIndentEnabled(),
} as unknown) as LanguageClientOptions;
}
Expand Down
25 changes: 7 additions & 18 deletions src/client/activation/node/languageClientMiddleware.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,40 +36,29 @@ export class NodeLanguageClientMiddleware extends LanguageClientMiddleware {
this.jupyterExtensionIntegration = serviceContainer.get<JupyterExtensionIntegration>(
JupyterExtensionIntegration,
);
if (!this.notebookAddon && this.lspNotebooksExperiment.isInNotebooksExperimentWithInteractiveWindowSupport()) {
if (!this.notebookAddon) {
this.notebookAddon = new LspInteractiveWindowMiddlewareAddon(
this.getClient,
this.jupyterExtensionIntegration,
);
}
}

protected shouldCreateHidingMiddleware(jupyterDependencyManager: IJupyterExtensionDependencyManager): boolean {
return (
super.shouldCreateHidingMiddleware(jupyterDependencyManager) &&
!this.lspNotebooksExperiment.isInNotebooksExperiment()
);
}

protected async onExtensionChange(jupyterDependencyManager: IJupyterExtensionDependencyManager): Promise<void> {
if (jupyterDependencyManager && jupyterDependencyManager.isJupyterExtensionInstalled) {
await this.lspNotebooksExperiment.onJupyterInstalled();
}

if (this.lspNotebooksExperiment.isInNotebooksExperimentWithInteractiveWindowSupport()) {
if (!this.notebookAddon) {
this.notebookAddon = new LspInteractiveWindowMiddlewareAddon(
this.getClient,
this.jupyterExtensionIntegration,
);
}
} else {
super.onExtensionChange(jupyterDependencyManager);
if (!this.notebookAddon) {
this.notebookAddon = new LspInteractiveWindowMiddlewareAddon(
this.getClient,
this.jupyterExtensionIntegration,
);
}
}

protected async getPythonPathOverride(uri: Uri | undefined): Promise<string | undefined> {
if (!uri || !this.lspNotebooksExperiment.isInNotebooksExperiment()) {
if (!uri) {
return undefined;
}

Expand Down
7 changes: 4 additions & 3 deletions src/client/activation/node/languageServerProxy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
LanguageClientOptions,
} from 'vscode-languageclient/node';

import { Extension } from 'vscode';
import { IExperimentService, IExtensions, IInterpreterPathService, Resource } from '../../common/types';
import { IEnvironmentVariablesProvider } from '../../common/variables/types';
import { PythonEnvironment } from '../../pythonEnvironments/info';
Expand Down Expand Up @@ -95,7 +96,7 @@ export class NodeLanguageServerProxy implements ILanguageServerProxy {
const extension = await this.getPylanceExtension();
this.lsVersion = extension?.packageJSON.version || '0';

const api = extension?.exports as PylanceApi | undefined;
const api = extension?.exports;
if (api && api.client && api.client.isEnabled()) {
this.pylanceApi = api;
await api.client.start();
Expand Down Expand Up @@ -220,8 +221,8 @@ export class NodeLanguageServerProxy implements ILanguageServerProxy {
);
}

private async getPylanceExtension() {
const extension = this.extensions.getExtension(PYLANCE_EXTENSION_ID);
private async getPylanceExtension(): Promise<Extension<PylanceApi> | undefined> {
const extension = this.extensions.getExtension<PylanceApi>(PYLANCE_EXTENSION_ID);
if (!extension) {
return undefined;
}
Expand Down
94 changes: 11 additions & 83 deletions src/client/activation/node/lspNotebooksExperiment.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.
import { inject, injectable } from 'inversify';
import * as semver from 'semver';
import { Disposable, extensions } from 'vscode';
import { IConfigurationService, IDisposableRegistry } from '../../common/types';
import { sendTelemetryEvent } from '../../telemetry';
Expand All @@ -10,7 +9,6 @@ import { JUPYTER_EXTENSION_ID, PYLANCE_EXTENSION_ID } from '../../common/constan
import { IExtensionSingleActivationService, LanguageServerType } from '../types';
import { traceLog, traceVerbose } from '../../logging';
import { IJupyterExtensionDependencyManager } from '../../common/application/types';
import { ILanguageServerWatcher } from '../../languageServer/types';
import { IServiceContainer } from '../../ioc/types';
import { sleep } from '../../common/utils/async';
import { JupyterExtensionIntegration } from '../../jupyter/jupyterIntegration';
Expand All @@ -23,9 +21,7 @@ export class LspNotebooksExperiment implements IExtensionSingleActivationService

private isJupyterInstalled = false;

private isInExperiment: boolean | undefined;

private supportsInteractiveWindow: boolean | undefined;
private isUsingPylance: boolean | undefined;

constructor(
@inject(IServiceContainer) private readonly serviceContainer: IServiceContainer,
Expand All @@ -50,101 +46,33 @@ export class LspNotebooksExperiment implements IExtensionSingleActivationService
return;
}

if (LspNotebooksExperiment.jupyterSupportsNotebooksExperiment()) {
await this.waitForJupyterToRegisterPythonPathFunction();
this.updateExperimentSupport();
}
await this.waitForJupyterToRegisterPythonPathFunction();
this.updateExperimentSupport();

this.isJupyterInstalled = true;
}

public isInNotebooksExperiment(): boolean {
return this.isInExperiment ?? false;
}

public isInNotebooksExperimentWithInteractiveWindowSupport(): boolean {
return this.supportsInteractiveWindow ?? false;
}

private updateExperimentSupport(): void {
const wasInExperiment = this.isInExperiment;
const isInTreatmentGroup = true;
const languageServerType = this.configurationService.getSettings().languageServer;

this.isInExperiment = false;
this.isUsingPylance = false;
if (languageServerType !== LanguageServerType.Node) {
traceLog(`LSP Notebooks experiment is disabled -- not using Pylance`);
traceLog(`LSP Notebooks is disabled -- not using Pylance`);
} else if (!LspNotebooksExperiment.isJupyterInstalled()) {
traceLog(`LSP Notebooks experiment is disabled -- Jupyter disabled or not installed`);
} else if (!LspNotebooksExperiment.jupyterSupportsNotebooksExperiment()) {
traceLog(`LSP Notebooks experiment is disabled -- Jupyter does not support experiment`);
traceLog(`LSP Notebooks is disabled -- Jupyter disabled or not installed`);
} else if (!LspNotebooksExperiment.isPylanceInstalled()) {
traceLog(`LSP Notebooks experiment is disabled -- Pylance disabled or not installed`);
} else if (!LspNotebooksExperiment.pylanceSupportsNotebooksExperiment()) {
traceLog(`LSP Notebooks experiment is disabled -- Pylance does not support experiment`);
} else if (!isInTreatmentGroup) {
traceLog(`LSP Notebooks experiment is disabled -- not in treatment group`);
// to avoid scorecard SRMs, we're also triggering the telemetry for users who meet
// the criteria to experience LSP notebooks, but may be in the control group.
sendTelemetryEvent(EventName.PYTHON_EXPERIMENTS_LSP_NOTEBOOKS);
traceLog(`LSP Notebooks is disabled -- Pylance disabled or not installed`);
} else {
this.isInExperiment = true;
traceLog(`LSP Notebooks experiment is enabled`);
this.isUsingPylance = true;
traceLog(`LSP Notebooks is enabled`);
sendTelemetryEvent(EventName.PYTHON_EXPERIMENTS_LSP_NOTEBOOKS);
}

this.supportsInteractiveWindow = false;
if (!this.isInExperiment) {
traceLog(`LSP Notebooks interactive window support is disabled -- not in LSP Notebooks experiment`);
} else if (!LspNotebooksExperiment.jupyterSupportsLspInteractiveWindow()) {
traceLog(`LSP Notebooks interactive window support is disabled -- Jupyter is not new enough`);
} else if (!LspNotebooksExperiment.pylanceSupportsLspInteractiveWindow()) {
traceLog(`LSP Notebooks interactive window support is disabled -- Pylance is not new enough`);
if (!this.isUsingPylance) {
traceLog(`LSP Notebooks interactive window support is disabled -- not using Pylance`);
} else {
this.supportsInteractiveWindow = true;
traceLog(`LSP Notebooks interactive window support is enabled`);
}

// Our "in experiment" status can only change from false to true. That's possible if Pylance
// or Jupyter is installed after Python is activated. A true to false transition would require
// either Pylance or Jupyter to be uninstalled or downgraded after Python activated, and that
// would require VS Code to be reloaded before the new extension version could be used.
if (wasInExperiment === false && this.isInExperiment === true) {
const watcher = this.serviceContainer.get<ILanguageServerWatcher>(ILanguageServerWatcher);
if (watcher) {
watcher.restartLanguageServers();
}
}
}

private static jupyterSupportsNotebooksExperiment(): boolean {
const jupyterVersion = extensions.getExtension(JUPYTER_EXTENSION_ID)?.packageJSON.version;
return (
jupyterVersion && (semver.gt(jupyterVersion, '2022.5.1001411044') || semver.patch(jupyterVersion) === 100)
);
}

private static pylanceSupportsNotebooksExperiment(): boolean {
const pylanceVersion = extensions.getExtension(PYLANCE_EXTENSION_ID)?.packageJSON.version;
return (
pylanceVersion &&
(semver.gte(pylanceVersion, '2022.5.3-pre.1') || semver.prerelease(pylanceVersion)?.includes('dev'))
);
}

private static jupyterSupportsLspInteractiveWindow(): boolean {
const jupyterVersion = extensions.getExtension(JUPYTER_EXTENSION_ID)?.packageJSON.version;
return (
jupyterVersion && (semver.gt(jupyterVersion, '2022.7.1002041057') || semver.patch(jupyterVersion) === 100)
);
}

private static pylanceSupportsLspInteractiveWindow(): boolean {
const pylanceVersion = extensions.getExtension(PYLANCE_EXTENSION_ID)?.packageJSON.version;
return (
pylanceVersion &&
(semver.gte(pylanceVersion, '2022.7.51') || semver.prerelease(pylanceVersion)?.includes('dev'))
);
}

private async waitForJupyterToRegisterPythonPathFunction(): Promise<void> {
Expand Down
19 changes: 19 additions & 0 deletions src/client/activation/node/pylanceApi.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,29 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.
import {
CancellationToken,
CompletionContext,
CompletionItem,
CompletionList,
Position,
TextDocument,
Uri,
} from 'vscode';

export interface PylanceApi {
client?: {
isEnabled(): boolean;
start(): Promise<void>;
stop(): Promise<void>;
};
notebook?: {
registerJupyterPythonPathFunction(func: (uri: Uri) => Promise<string | undefined>): void;
registerGetNotebookUriForTextDocumentUriFunction(func: (textDocumentUri: Uri) => Uri | undefined): void;
getCompletionItems(
document: TextDocument,
position: Position,
context: CompletionContext,
token: CancellationToken,
): Promise<CompletionItem[] | CompletionList | undefined>;
};
}
31 changes: 29 additions & 2 deletions src/client/jupyter/jupyterIntegration.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import { dirname } from 'path';
import { CancellationToken, Event, Extension, Memento, Uri } from 'vscode';
import type { SemVer } from 'semver';
import { IWorkspaceService } from '../common/application/types';
import { JUPYTER_EXTENSION_ID } from '../common/constants';
import { JUPYTER_EXTENSION_ID, PYLANCE_EXTENSION_ID } from '../common/constants';
import { InterpreterUri, ModuleInstallFlags } from '../common/installer/types';
import {
GLOBAL_MEMENTO,
Expand All @@ -34,7 +34,7 @@ import {
} from '../interpreter/contracts';
import { PythonEnvironment } from '../pythonEnvironments/info';
import { IDataViewerDataProvider, IJupyterUriProvider } from './types';

import { PylanceApi } from '../activation/node/pylanceApi';
/**
* This allows Python extension to update Product enum without breaking Jupyter.
* I.e. we have a strict contract, else using numbers (in enums) is bound to break across products.
Expand Down Expand Up @@ -184,6 +184,8 @@ type JupyterExtensionApi = {
export class JupyterExtensionIntegration {
private jupyterExtension: Extension<JupyterExtensionApi> | undefined;

private pylanceExtension: Extension<PylanceApi> | undefined;

private jupyterPythonPathFunction: ((uri: Uri) => Promise<string | undefined>) | undefined;

private getNotebookUriForTextDocumentUriFunction: ((textDocumentUri: Uri) => Uri | undefined) | undefined;
Expand Down Expand Up @@ -300,6 +302,16 @@ export class JupyterExtensionIntegration {
}

private async getExtensionApi(): Promise<JupyterExtensionApi | undefined> {
if (!this.pylanceExtension) {
const pylanceExtension = this.extensions.getExtension<PylanceApi>(PYLANCE_EXTENSION_ID);

if (pylanceExtension && !pylanceExtension.isActive) {
await pylanceExtension.activate();
}

this.pylanceExtension = pylanceExtension;
}

if (!this.jupyterExtension) {
const jupyterExtension = this.extensions.getExtension<JupyterExtensionApi>(JUPYTER_EXTENSION_ID);
if (!jupyterExtension) {
Expand All @@ -316,8 +328,18 @@ export class JupyterExtensionIntegration {
return undefined;
}

private getPylanceApi(): PylanceApi | undefined {
const api = this.pylanceExtension?.exports;
return api && api.notebook && api.client && api.client.isEnabled() ? api : undefined;
}

private registerJupyterPythonPathFunction(func: (uri: Uri) => Promise<string | undefined>) {
this.jupyterPythonPathFunction = func;

const api = this.getPylanceApi();
if (api) {
api.notebook!.registerJupyterPythonPathFunction(func);
}
}

public getJupyterPythonPathFunction(): ((uri: Uri) => Promise<string | undefined>) | undefined {
Expand All @@ -326,6 +348,11 @@ export class JupyterExtensionIntegration {

public registerGetNotebookUriForTextDocumentUriFunction(func: (textDocumentUri: Uri) => Uri | undefined): void {
this.getNotebookUriForTextDocumentUriFunction = func;

const api = this.getPylanceApi();
if (api) {
api.notebook!.registerGetNotebookUriForTextDocumentUriFunction(func);
}
}

public getGetNotebookUriForTextDocumentUriFunction(): ((textDocumentUri: Uri) => Uri | undefined) | undefined {
Expand Down
3 changes: 0 additions & 3 deletions src/client/languageServer/pylanceLSExtensionManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import { promptForPylanceInstall } from '../activation/common/languageServerChan
import { NodeLanguageServerAnalysisOptions } from '../activation/node/analysisOptions';
import { NodeLanguageClientFactory } from '../activation/node/languageClientFactory';
import { NodeLanguageServerProxy } from '../activation/node/languageServerProxy';
import { LspNotebooksExperiment } from '../activation/node/lspNotebooksExperiment';
import { NodeLanguageServerManager } from '../activation/node/manager';
import { ILanguageServerOutputChannel } from '../activation/types';
import { IApplicationShell, ICommandManager, IWorkspaceService } from '../common/application/types';
Expand Down Expand Up @@ -49,13 +48,11 @@ export class PylanceLSExtensionManager implements IDisposable, ILanguageServerEx
fileSystem: IFileSystem,
private readonly extensions: IExtensions,
readonly applicationShell: IApplicationShell,
lspNotebooksExperiment: LspNotebooksExperiment,
) {
this.analysisOptions = new NodeLanguageServerAnalysisOptions(
outputChannel,
workspaceService,
experimentService,
lspNotebooksExperiment,
);
this.clientFactory = new NodeLanguageClientFactory(fileSystem, extensions);
this.serverProxy = new NodeLanguageServerProxy(
Expand Down
3 changes: 0 additions & 3 deletions src/client/languageServer/watcher.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ import { JediLSExtensionManager } from './jediLSExtensionManager';
import { NoneLSExtensionManager } from './noneLSExtensionManager';
import { PylanceLSExtensionManager } from './pylanceLSExtensionManager';
import { ILanguageServerExtensionManager, ILanguageServerWatcher } from './types';
import { LspNotebooksExperiment } from '../activation/node/lspNotebooksExperiment';

@injectable()
/**
Expand Down Expand Up @@ -63,7 +62,6 @@ export class LanguageServerWatcher implements IExtensionActivationService, ILang
@inject(IFileSystem) private readonly fileSystem: IFileSystem,
@inject(IExtensions) private readonly extensions: IExtensions,
@inject(IApplicationShell) readonly applicationShell: IApplicationShell,
@inject(LspNotebooksExperiment) private readonly lspNotebooksExperiment: LspNotebooksExperiment,
@inject(IDisposableRegistry) readonly disposables: IDisposableRegistry,
) {
this.workspaceInterpreters = new Map();
Expand Down Expand Up @@ -244,7 +242,6 @@ export class LanguageServerWatcher implements IExtensionActivationService, ILang
this.fileSystem,
this.extensions,
this.applicationShell,
this.lspNotebooksExperiment,
);
break;
case LanguageServerType.None:
Expand Down
Loading

0 comments on commit 4b5b66d

Please sign in to comment.