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

Prevent unnecessary activation of the Python extension #5199

Merged
merged 14 commits into from
Mar 19, 2021
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>();
DonJayamanne marked this conversation as resolved.
Show resolved Hide resolved
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(
Copy link
Contributor Author

@DonJayamanne DonJayamanne Mar 19, 2021

Choose a reason for hiding this comment

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

If the extension is subsequently installed, then register the hooks

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();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add event handlers only if extension has been activated, and add it later once python loads.

}
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>;
Copy link
Member

Choose a reason for hiding this comment

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

Not fully sure if I'm suggesting this. But does this seem to you like it should be on IPythonExtensionChecker? That has everything relevant to checking if the extension is installed or active.

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 can do, its the PythonApiProvider that activates the Python extension. So only PythonAPIProvider knows if the python extension was activated or not.
VS Code doesn't fire events when extensions load.
Here we're only interested in situations when we load the python extension & other parts of the code can then react to that.

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