Skip to content

Commit

Permalink
Prevent unnecessary activation of the Python extension (#5199)
Browse files Browse the repository at this point in the history
  • Loading branch information
DonJayamanne authored Mar 19, 2021
1 parent 0afaa42 commit 1c4fad8
Show file tree
Hide file tree
Showing 21 changed files with 191 additions and 106 deletions.
1 change: 0 additions & 1 deletion .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -912,7 +912,6 @@ module.exports = {
'src/client/datascience/notebookAndInteractiveTracker.ts',
'src/client/datascience/statusProvider.ts',
'src/client/datascience/jupyter/interpreter/jupyterInterpreterSelectionCommand.ts',
'src/client/datascience/jupyter/interpreter/jupyterInterpreterStateStore.ts',
'src/client/datascience/jupyter/interpreter/jupyterInterpreterOldCacheStateStore.ts',
'src/client/datascience/jupyter/interpreter/jupyterInterpreterSelector.ts',
'src/client/datascience/jupyter/interpreter/jupyterInterpreterService.ts',
Expand Down
1 change: 1 addition & 0 deletions news/2 Fixes/5193.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Prevent unnecessary activation of the Python extension.
2 changes: 1 addition & 1 deletion src/client/activation/activationManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ export class ExtensionActivationManager implements IExtensionActivationManager {
this.activatedWorkspaces.add(key);

// Get latest interpreter list in the background.
if (this.extensionChecker.isPythonExtensionInstalled) {
if (this.extensionChecker.isPythonExtensionActive) {
this.interpreterService.getInterpreters(resource).ignoreErrors();
}

Expand Down
88 changes: 65 additions & 23 deletions src/client/api/pythonApi.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,13 +49,33 @@ import {
@injectable()
export class PythonApiProvider implements IPythonApiProvider {
private readonly api = createDeferred<PythonApi>();
private readonly didActivatePython = new EventEmitter<void>();
public get onDidActivatePythonExtension() {
return this.didActivatePython.event;
}

private initialized?: boolean;
private hooksRegistered?: boolean;

constructor(
@inject(IExtensions) private readonly extensions: IExtensions,
@inject(IDisposableRegistry) private readonly disposables: IDisposableRegistry,
@inject(IPythonExtensionChecker) private extensionChecker: IPythonExtensionChecker
) {}
) {
const previouslyInstalled = this.extensionChecker.isPythonExtensionInstalled;
if (!previouslyInstalled) {
this.extensions.onDidChange(
async () => {
if (this.extensionChecker.isPythonExtensionInstalled) {
await this.registerHooks();
}
},
this,
this.disposables
);
}
this.disposables.push(this.didActivatePython);
}

public getApi(): Promise<PythonApi> {
this.init().catch(noop);
Expand All @@ -78,11 +98,23 @@ export class PythonApiProvider implements IPythonApiProvider {
if (!pythonExtension) {
await this.extensionChecker.showPythonExtensionInstallRequiredPrompt();
} else {
if (!pythonExtension.isActive) {
await pythonExtension.activate();
}
pythonExtension.exports.jupyter.registerHooks();
await this.registerHooks();
}
}
private async registerHooks() {
if (this.hooksRegistered) {
return;
}
const pythonExtension = this.extensions.getExtension<{ jupyter: { registerHooks(): void } }>(PythonExtension);
if (!pythonExtension) {
return;
}
this.hooksRegistered = true;
if (!pythonExtension.isActive) {
await pythonExtension.activate();
this.didActivatePython.fire();
}
pythonExtension.exports.jupyter.registerHooks();
}
}

Expand All @@ -106,6 +138,9 @@ export class PythonExtensionChecker implements IPythonExtensionChecker {
public get isPythonExtensionInstalled() {
return this.extensions.getExtension(this.pythonExtensionId) !== undefined;
}
public get isPythonExtensionActive() {
return this.extensions.getExtension(this.pythonExtensionId)?.isActive === true;
}

public async showPythonExtensionInstallRequiredPrompt(): Promise<void> {
if (this.waitingOnInstallPrompt) {
Expand Down Expand Up @@ -292,24 +327,17 @@ export class InterpreterService implements IInterpreterService {
) {}

public get onDidChangeInterpreter(): Event<void> {
if (this.extensionChecker.isPythonExtensionInstalled && !this.eventHandlerAdded) {
this.apiProvider
.getApi()
.then((api) => {
if (!this.eventHandlerAdded) {
this.eventHandlerAdded = true;
api.onDidChangeInterpreter(
() => {
// Clear our cache of active interpreters.
this.workspaceCachedActiveInterpreter.clear();
this.didChangeInterpreter.fire();
},
this,
this.disposables
);
}
})
.catch(noop);
if (this.extensionChecker.isPythonExtensionInstalled) {
if (this.extensionChecker.isPythonExtensionActive && !this.eventHandlerAdded) {
this.hookupOnDidChangeInterpreterEvent();
}
if (!this.extensionChecker.isPythonExtensionActive) {
this.apiProvider.onDidActivatePythonExtension(
this.hookupOnDidChangeInterpreterEvent,
this,
this.disposables
);
}
}
return this.didChangeInterpreter.event;
}
Expand Down Expand Up @@ -345,4 +373,18 @@ export class InterpreterService implements IInterpreterService {
return undefined;
}
}
private hookupOnDidChangeInterpreterEvent() {
if (this.eventHandlerAdded) {
return;
}
this.apiProvider
.getApi()
.then((api) => {
if (!this.eventHandlerAdded) {
this.eventHandlerAdded = true;
api.onDidChangeInterpreter(() => this.didChangeInterpreter.fire(), this, this.disposables);
}
})
.catch(noop);
}
}
2 changes: 2 additions & 0 deletions src/client/api/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,14 @@ export interface ILanguageServer extends Disposable {

export const IPythonApiProvider = Symbol('IPythonApi');
export interface IPythonApiProvider {
onDidActivatePythonExtension: Event<void>;
getApi(): Promise<PythonApi>;
setApi(api: PythonApi): void;
}
export const IPythonExtensionChecker = Symbol('IPythonExtensionChecker');
export interface IPythonExtensionChecker {
readonly isPythonExtensionInstalled: boolean;
readonly isPythonExtensionActive: boolean;
showPythonExtensionInstallRequiredPrompt(): Promise<void>;
showPythonExtensionInstallRecommendedPrompt(): Promise<void>;
}
Expand Down
7 changes: 4 additions & 3 deletions src/client/datascience/activation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import { JupyterDaemonModule, Telemetry } from './constants';
import { ActiveEditorContextService } from './commands/activeEditorContext';
import { JupyterInterpreterService } from './jupyter/interpreter/jupyterInterpreterService';
import { KernelDaemonPreWarmer } from './kernel-launcher/kernelDaemonPreWarmer';
import { INotebookCreationTracker, INotebookEditorProvider } from './types';
import { INotebookCreationTracker, INotebookEditorProvider, IRawNotebookSupportedService } from './types';

@injectable()
export class Activation implements IExtensionSingleActivationService {
Expand All @@ -27,6 +27,7 @@ export class Activation implements IExtensionSingleActivationService {
@inject(IDisposableRegistry) private readonly disposables: IDisposableRegistry,
@inject(ActiveEditorContextService) private readonly contextService: ActiveEditorContextService,
@inject(KernelDaemonPreWarmer) private readonly daemonPoolPrewarmer: KernelDaemonPreWarmer,
@inject(IRawNotebookSupportedService) private readonly rawSupported: IRawNotebookSupportedService,
@inject(INotebookCreationTracker)
private readonly tracker: INotebookCreationTracker,
@inject(IPythonExtensionChecker) private readonly extensionChecker: IPythonExtensionChecker
Expand All @@ -44,7 +45,7 @@ export class Activation implements IExtensionSingleActivationService {
this.PreWarmDaemonPool().ignoreErrors();
sendTelemetryEvent(Telemetry.OpenNotebookAll);

if (this.extensionChecker.isPythonExtensionInstalled) {
if (!this.rawSupported.supported() && this.extensionChecker.isPythonExtensionInstalled) {
// Warm up our selected interpreter for the extension
this.jupyterInterpreterService.setInitialInterpreter().ignoreErrors();
}
Expand All @@ -61,7 +62,7 @@ export class Activation implements IExtensionSingleActivationService {
@debounceAsync(500)
@swallowExceptions('Failed to pre-warm daemon pool')
private async PreWarmDaemonPool() {
if (!this.extensionChecker.isPythonExtensionInstalled) {
if (!this.extensionChecker.isPythonExtensionActive) {
// Skip prewarm if no python extension
return;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import { inject, injectable, named } from 'inversify';
import { Memento } from 'vscode';
import { IExtensionSingleActivationService } from '../../../activation/types';
import { IPythonApiProvider, IPythonExtensionChecker } from '../../../api/types';
import { GLOBAL_MEMENTO, IMemento } from '../../../common/types';
import { GLOBAL_MEMENTO, IDisposableRegistry, IMemento } from '../../../common/types';
import { noop } from '../../../common/utils/misc';

const key = 'INTERPRETER_PATH_SELECTED_FOR_JUPYTER_SERVER';
Expand Down Expand Up @@ -45,25 +45,35 @@ export class JupyterInterpreterStateStore {

@injectable()
export class MigrateJupyterInterpreterStateService implements IExtensionSingleActivationService {
private settingsMigrated?: boolean;
constructor(
@inject(IPythonApiProvider) private readonly api: IPythonApiProvider,
@inject(IMemento) @named(GLOBAL_MEMENTO) private readonly memento: Memento,
@inject(IPythonExtensionChecker) private readonly checker: IPythonExtensionChecker
@inject(IPythonExtensionChecker) private readonly checker: IPythonExtensionChecker,
@inject(IDisposableRegistry) private readonly disposables: IDisposableRegistry
) {}

// Migrate the interpreter path selected for Jupyter server from the Python extension's globalState memento
public async activate() {
this.activateBackground().catch(noop);
this.api.onDidActivatePythonExtension(this.activateBackground, this, this.disposables);
}
public async activateBackground() {
// Migrate in the background.
// Python extension will not activate unless Jupyter activates, and here we're waiting for Python.
// Hence end in deadlock (caught in smoke test).
if (!this.memento.get(key) && this.checker.isPythonExtensionInstalled) {
const api = await this.api.getApi();
const data = api.getInterpreterPathSelectedForJupyterServer();
await this.memento.update(key, data);
await this.memento.update(keySelected, true);
if (!this.memento.get(key) && this.checker.isPythonExtensionActive) {
await this.migrateSettings();
}
}
private async migrateSettings() {
if (this.settingsMigrated) {
return;
}
this.settingsMigrated = true;
const api = await this.api.getApi();
const data = api.getInterpreterPathSelectedForJupyterServer();
await this.memento.update(key, data);
await this.memento.update(keySelected, true);
}
}
29 changes: 22 additions & 7 deletions src/client/datascience/kernel-launcher/kernelDaemonPreWarmer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,9 @@ import { PYTHON_LANGUAGE } from '../../common/constants';
import '../../common/extensions';
import { IConfigurationService, IDisposableRegistry, Resource } from '../../common/types';
import { swallowExceptions } from '../../common/utils/decorators';
import { isJupyterKernel } from '../notebook/helpers/helpers';
import { isUntitledFile } from '../../common/utils/misc';
import { isPythonKernelConnection } from '../jupyter/kernels/helpers';
import { getNotebookMetadata, isJupyterKernel, isPythonNotebook } from '../notebook/helpers/helpers';
import {
IInteractiveWindowProvider,
INotebookCreationTracker,
Expand Down Expand Up @@ -40,7 +42,7 @@ export class KernelDaemonPreWarmer {
// If not, don't bother with prewarming
// Also respect the disable autostart setting to not do any prewarming for the user
if (
!(await this.rawNotebookSupported.supported()) ||
!this.rawNotebookSupported.supported() ||
this.configService.getSettings().disableJupyterAutoStart ||
!this.extensionChecker.isPythonExtensionInstalled
) {
Expand All @@ -54,14 +56,20 @@ export class KernelDaemonPreWarmer {

this.disposables.push(this.vscodeNotebook.onDidOpenNotebookDocument(this.onDidOpenNotebookDocument, this));

if (this.notebookEditorProvider.editors.length > 0 || this.interactiveProvider.windows.length > 0) {
if (
this.extensionChecker.isPythonExtensionActive &&
(this.notebookEditorProvider.editors.length > 0 || this.interactiveProvider.windows.length > 0)
) {
await this.preWarmKernelDaemonPool();
}
await this.preWarmDaemonPoolIfNecesary();
await this.preWarmDaemonPoolIfNecessary();
}
private async preWarmDaemonPoolIfNecesary() {
private async preWarmDaemonPoolIfNecessary() {
// This is only for python, so prewarm just if we've seen python recently in this workspace
if (this.shouldPreWarmDaemonPool(this.usageTracker.lastPythonNotebookCreated)) {
if (
this.shouldPreWarmDaemonPool(this.usageTracker.lastPythonNotebookCreated) &&
this.extensionChecker.isPythonExtensionActive
) {
await this.preWarmKernelDaemonPool();
}
}
Expand All @@ -79,9 +87,16 @@ export class KernelDaemonPreWarmer {

// Handle opening of native documents
private async onDidOpenNotebookDocument(doc: NotebookDocument): Promise<void> {
// It could be anything, lets not make any assumptions.
if (isUntitledFile(doc.uri)) {
return;
}
const kernel = this.vscodeNotebook.notebookEditors.find((item) => item.document === doc)?.kernel;
const isPythonKernel = isJupyterKernel(kernel) ? isPythonKernelConnection(kernel.selection) : false;
const notebookMetadata = isPythonNotebook(getNotebookMetadata(doc));
if (
isJupyterKernel(kernel) ||
isPythonKernel ||
notebookMetadata ||
doc.cells.some((cell: NotebookCell) => {
return cell.document.languageId === PYTHON_LANGUAGE;
})
Expand Down
7 changes: 5 additions & 2 deletions src/client/datascience/kernel-launcher/localKernelFinder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,8 @@ import {
} from '../jupyter/kernels/types';
import { IJupyterKernelSpec } from '../types';
import { ILocalKernelFinder } from './types';
import { tryGetRealPath } from '../common';
import { getResourceType, tryGetRealPath } from '../common';
import { isPythonNotebook } from '../notebook/helpers/helpers';

const winJupyterPath = path.join('AppData', 'Roaming', 'jupyter', 'kernels');
const linuxJupyterPath = path.join('.local', 'share', 'jupyter', 'kernels');
Expand Down Expand Up @@ -90,12 +91,14 @@ export class LocalKernelFinder implements ILocalKernelFinder {
try {
// Get list of all of the specs
const kernels = await this.listKernels(resource, cancelToken);
const isPythonNbOrInteractiveWindow =
isPythonNotebook(option) || getResourceType(resource) === 'interactive';

// Always include the interpreter in the search if we can
const interpreter =
option && isInterpreter(option)
? option
: resource && this.extensionChecker.isPythonExtensionInstalled
: resource && isPythonNbOrInteractiveWindow && this.extensionChecker.isPythonExtensionInstalled
? await this.interpreterService.getActiveInterpreter(resource)
: undefined;

Expand Down
Loading

0 comments on commit 1c4fad8

Please sign in to comment.