Skip to content

Commit

Permalink
Allow users to cancel and esc from env creation (#12883)
Browse files Browse the repository at this point in the history 
* Allow users to cancel and esc from env creation

* Telemtry

* fixes
  • Loading branch information
@DonJayamanne
DonJayamanne authored Feb 21, 2023
1 parent 01f1091 commit 532b316
Show file tree
Hide file tree
Showing 5 changed files with 101 additions and 43 deletions.
1 change: 1 addition & 0 deletions src/gdpr.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
"DATASCIENCE.CREATE_PYTHON_ENVIRONMENT" : {
"reason": {"classification":"SystemMetaData","purpose":"PerformanceAndHealth","comment":"Reason for failure. cancelled - User cancelled the operation (cancellation token cancelled). kernelConnectionNotCreated - Kernel connection not created via the kernel finder.","owner":"donjayamanne"},
"dependenciesInstalled": {"classification":"SystemMetaData","purpose":"FeatureInsight","comment":"Whether the kernel dependencies were installed or not.","owner":"donjayamanne"},
"envType": {"classification":"SystemMetaData","purpose":"FeatureInsight","comment":"Type of the Python environment.","owner":"donjayamanne"},
"${include}": [
"${F1}"
Expand Down
48 changes: 34 additions & 14 deletions src/notebooks/controllers/kernelSource/kernelSelector.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -181,20 +181,16 @@ export class KernelSelector implements IDisposable {
this.categories.set(item, new Set(items));
});

const refreshButton: QuickInputButton = { iconPath: new ThemeIcon('refresh'), tooltip: Common.refresh };
const refreshingButton: QuickInputButton = {
iconPath: new ThemeIcon('loading~spin'),
tooltip: Common.refreshing
};

if (
!this.extensionChecker.isPythonExtensionInstalled &&
this.provider.kind === ContributedKernelFinderKind.LocalPythonEnvironment
) {
this.installPythonExtItems.push(this.installPythonExtension);
}

let quickPickToBeUpdated: QuickPick<CompoundQuickPickItem> | undefined;
const quickPickToBeUpdated: { quickPick: QuickPick<CompoundQuickPickItem> | undefined } = {
quickPick: undefined
};
if (
this.extensionChecker.isPythonExtensionInstalled &&
this.provider.kind === ContributedKernelFinderKind.LocalPythonEnvironment
Expand All @@ -213,14 +209,14 @@ export class KernelSelector implements IDisposable {
) {
if (this.workspace.isTrusted) {
this.installPythonItems.push(this.installPythonItem);
if (quickPickToBeUpdated) {
this.updateQuickPickItems(quickPickToBeUpdated);
if (quickPickToBeUpdated.quickPick) {
this.updateQuickPickItems(quickPickToBeUpdated.quickPick);
}
}
} else if (this.provider.kernels.length) {
this.installPythonItems.length = 0;
if (quickPickToBeUpdated) {
this.updateQuickPickItems(quickPickToBeUpdated);
if (quickPickToBeUpdated.quickPick) {
this.updateQuickPickItems(quickPickToBeUpdated.quickPick);
}
}
};
Expand All @@ -247,6 +243,21 @@ export class KernelSelector implements IDisposable {
);
}
}
return this.selectKernelImpl(quickPickFactory, quickPickToBeUpdated);
}
public async selectKernelImpl(
quickPickFactory: CreateAndSelectItemFromQuickPick,
quickPickToBeUpdated: { quickPick: QuickPick<CompoundQuickPickItem> | undefined }
): Promise<
| { selection: 'controller'; finder: IContributedKernelFinder; connection: KernelConnectionMetadata }
| { selection: 'userPerformedSomeOtherAction' }
| undefined
> {
const refreshButton: QuickInputButton = { iconPath: new ThemeIcon('refresh'), tooltip: Common.refresh };
const refreshingButton: QuickInputButton = {
iconPath: new ThemeIcon('loading~spin'),
tooltip: Common.refreshing
};
const { quickPick, selection } = quickPickFactory({
title: this.provider.title,
items: this.installPythonItems
Expand All @@ -263,7 +274,7 @@ export class KernelSelector implements IDisposable {
}
}
});
quickPickToBeUpdated = quickPick;
quickPickToBeUpdated.quickPick = quickPick;
if (this.provider.status === 'discovering') {
quickPick.busy = true;
}
Expand Down Expand Up @@ -309,6 +320,8 @@ export class KernelSelector implements IDisposable {
} catch (ex) {
if (ex instanceof SomeOtherActionError) {
return { selection: 'userPerformedSomeOtherAction' };
} else if (ex === InputFlowAction.back) {
return this.selectKernelImpl(quickPickFactory, quickPickToBeUpdated);
}
throw ex;
}
Expand All @@ -319,14 +332,21 @@ export class KernelSelector implements IDisposable {
throw InputFlowAction.back;
}
}
private onCreatePythonEnvironment() {
private async onCreatePythonEnvironment() {
const cancellationToken = new CancellationTokenSource();
this.disposables.push(new Disposable(() => cancellationToken.cancel()));
this.disposables.push(cancellationToken);

const creator = new PythonEnvKernelConnectionCreator(this.notebook, cancellationToken.token);
this.disposables.push(creator);
return creator.createPythonEnvFromKernelPicker();
const result = await creator.createPythonEnvFromKernelPicker();
if ('action' in result) {
if (result.action === 'Cancel') {
throw InputFlowAction.cancel;
}
throw InputFlowAction.back;
}
return result.kernelConnection;
}
private updateQuickPickItems(quickPick: QuickPick<CompoundQuickPickItem>) {
quickPick.title = this.provider.title;
Expand Down
54 changes: 39 additions & 15 deletions src/notebooks/controllers/pythonEnvKernelConnectionCreator.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import { IControllerRegistration } from './types';

type CreateEnvironmentResult = {
path: string | undefined;
action?: 'Back' | 'Cancel';
};

/**
Expand Down Expand Up @@ -61,28 +62,40 @@ export class PythonEnvKernelConnectionCreator {
/**
* Creates a Python Environment & then returns the corresponding kernel connection for the newly created python env.
*/
public async createPythonEnvFromKernelPicker() {
public async createPythonEnvFromKernelPicker(): Promise<
| {
kernelConnection: PythonKernelConnectionMetadata;
}
| {
action: 'Back' | 'Cancel';
}
> {
let env: PythonEnvironment | undefined;

env = await this.createPythonEnvironment();
if (this.cancelTokeSource.token.isCancellationRequested || !env) {
const envResult = await this.createPythonEnvironment();
if (this.cancelTokeSource.token.isCancellationRequested || envResult.action) {
sendTelemetryEvent(Telemetry.CreatePythonEnvironment, undefined, { failed: true, reason: 'cancelled' });
return;
return { action: envResult.action || 'Cancel' };
}
if (!envResult.interpreter) {
sendTelemetryEvent(Telemetry.CreatePythonEnvironment, undefined, { failed: true, reason: 'cancelled' });
return { action: 'Cancel' };
}
env = envResult.interpreter;
traceVerbose(`Python Environment created ${env.id}`);

const kernelConnection = await this.waitForPythonKernel(env);
const kernelConnection = await this.waitForPythonKernel(envResult.interpreter);
if (this.cancelTokeSource.token.isCancellationRequested) {
sendTelemetryEvent(Telemetry.CreatePythonEnvironment, undefined, { failed: true, reason: 'cancelled' });
return;
return { action: 'Cancel' };
}
if (!kernelConnection) {
sendTelemetryEvent(Telemetry.CreatePythonEnvironment, undefined, {
failed: true,
reason: 'kernelConnectionNotCreated'
});
traceVerbose(`Python Environment ${env.id} not found as a kernel`);
return;
return { action: 'Cancel' };
}
traceVerbose(`Python Environment ${env.id} found as a kernel ${kernelConnection.kind}:${kernelConnection.id}`);
const dependencyService = ServiceContainer.instance.get<IKernelDependencyService>(IKernelDependencyService);
Expand All @@ -105,8 +118,11 @@ export class PythonEnvKernelConnectionCreator {
);
}

sendTelemetryEvent(Telemetry.CreatePythonEnvironment, undefined, { dependenciesInstalled });
return kernelConnection;
sendTelemetryEvent(Telemetry.CreatePythonEnvironment, undefined, {
dependenciesInstalled,
envType: envResult.interpreter.envType
});
return { kernelConnection };
}
private async waitForPythonKernel(env: PythonEnvironment) {
const kernelFinder = ServiceContainer.instance.get<IKernelFinder>(IKernelFinder);
Expand Down Expand Up @@ -162,7 +178,7 @@ export class PythonEnvKernelConnectionCreator {
return kernel;
});
}
private async createPythonEnvironment() {
private async createPythonEnvironment(): Promise<{ interpreter?: PythonEnvironment; action?: 'Back' | 'Cancel' }> {
const interpreterService = ServiceContainer.instance.get<IInterpreterService>(IInterpreterService);
const cancellation = new CancellationTokenSource();
// While we're busy creating this env ignore other events from python extension
Expand All @@ -171,28 +187,36 @@ export class PythonEnvKernelConnectionCreator {
// we'll end up creating a controller for this venv and that will get selected by vscode
interpreterService.pauseInterpreterDetection(cancellation.token);
try {
const result: CreateEnvironmentResult = await commands.executeCommand('python.createEnvironment');
const result: CreateEnvironmentResult = await commands.executeCommand('python.createEnvironment', {
showBackButton: true
});
if (result?.action === 'Cancel') {
return { action: 'Cancel' };
}
if (result?.action === 'Back') {
return { action: 'Back' };
}
const path = result?.path;
if (this.cancelTokeSource.token.isCancellationRequested) {
return;
return { action: 'Cancel' };
}
if (!path) {
traceWarning(
`Python Environment not created, either user cancelled the creation or there was an error in the Python Extension`
);
return;
return { action: 'Cancel' };
}
this.createdEnvId = path;
traceVerbose(`Python Environment created ${path}`);
const env = await interpreterService.getInterpreterDetails({ path });
if (this.cancelTokeSource.token.isCancellationRequested) {
return;
return { action: 'Cancel' };
}
if (!env) {
traceWarning(`No interpreter details for New Python Environment ${getDisplayPath(Uri.file(path))}`);
}
this.createdEnvId = env?.id;
return env;
return { interpreter: env };
} finally {
cancellation.cancel();
cancellation.dispose();
Expand Down
33 changes: 19 additions & 14 deletions src/notebooks/controllers/pythonEnvKernelConnectionCreator.unit.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -115,15 +115,17 @@ suite('Python Environment Kernel Connection Creator', () => {
});
teardown(() => disposeAllDisposables(disposables));
test('Not does create a Python Env when Python extension fails to create it', async () => {
when(mockedVSCodeNamespaces.commands.executeCommand('python.createEnvironment')).thenResolve(undefined);
when(mockedVSCodeNamespaces.commands.executeCommand('python.createEnvironment', anything())).thenResolve(
undefined
);

const kernel = await pythonEnvKernelConnectionCreator.createPythonEnvFromKernelPicker();

assert.isUndefined(kernel);
const connection = 'kernelConnection' in kernel ? kernel.kernelConnection : undefined;
assert.isUndefined(connection);
});
test('Can cancel after creation of the Environment', async () => {
const newCondaEnvPath = '<workspaceFolder>/.conda';
when(mockedVSCodeNamespaces.commands.executeCommand('python.createEnvironment')).thenResolve({
when(mockedVSCodeNamespaces.commands.executeCommand('python.createEnvironment', anything())).thenResolve({
path: newCondaEnvPath
} as any);
when(localPythonEnvFinder.kernels).thenReturn([venvPythonKernel]);
Expand All @@ -133,14 +135,14 @@ suite('Python Environment Kernel Connection Creator', () => {
return Promise.resolve(newCondaPythonKernel.interpreter);
});

const kernelPromise = pythonEnvKernelConnectionCreator.createPythonEnvFromKernelPicker();

assert.isUndefined(await kernelPromise);
const kernel = await pythonEnvKernelConnectionCreator.createPythonEnvFromKernelPicker();
const connection = 'kernelConnection' in kernel ? kernel.kernelConnection : undefined;
assert.isUndefined(connection);
verify(interpreterService.getInterpreterDetails(deepEqual({ path: newCondaEnvPath }))).once();
});
test('Installs missing dependencies and returns the kernel connection', async () => {
const newCondaEnvPath = '<workspaceFolder>/.conda';
when(mockedVSCodeNamespaces.commands.executeCommand('python.createEnvironment')).thenResolve({
when(mockedVSCodeNamespaces.commands.executeCommand('python.createEnvironment', anything())).thenResolve({
path: newCondaEnvPath
} as any);
when(localPythonEnvFinder.kernels).thenReturn([venvPythonKernel, newCondaPythonKernel]);
Expand All @@ -153,8 +155,11 @@ suite('Python Environment Kernel Connection Creator', () => {
);

const kernel = await pythonEnvKernelConnectionCreator.createPythonEnvFromKernelPicker();

assert.strictEqual(kernel, newCondaPythonKernel);
const kernelConnection = 'kernelConnection' in kernel ? kernel.kernelConnection : undefined;
if (!kernelConnection) {
assert.fail('Kernel Connection not created');
}
assert.strictEqual(kernelConnection, newCondaPythonKernel);
verify(interpreterService.getInterpreterDetails(deepEqual({ path: newCondaEnvPath }))).once();
verify(kernelDependencyService.installMissingDependencies(anything())).once();
const args = capture(kernelDependencyService.installMissingDependencies).first()[0];
Expand All @@ -166,7 +171,7 @@ suite('Python Environment Kernel Connection Creator', () => {
});
test('Abort creation if another controller is selected for the notebook', async () => {
const newCondaEnvPath = '<workspaceFolder>/.conda';
when(mockedVSCodeNamespaces.commands.executeCommand('python.createEnvironment')).thenResolve({
when(mockedVSCodeNamespaces.commands.executeCommand('python.createEnvironment', anything())).thenResolve({
path: newCondaEnvPath
} as any);
when(localPythonEnvFinder.kernels).thenReturn([venvPythonKernel]);
Expand All @@ -183,9 +188,9 @@ suite('Python Environment Kernel Connection Creator', () => {
return Promise.resolve(newCondaPythonKernel.interpreter);
});

const kernelPromise = pythonEnvKernelConnectionCreator.createPythonEnvFromKernelPicker();

assert.isUndefined(await kernelPromise);
const kernel = await pythonEnvKernelConnectionCreator.createPythonEnvFromKernelPicker();
const connection = 'kernelConnection' in kernel ? kernel.kernelConnection : undefined;
assert.isUndefined(connection);
verify(interpreterService.getInterpreterDetails(deepEqual({ path: newCondaEnvPath }))).once();
});
});
8 changes: 8 additions & 0 deletions src/telemetry.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3139,6 +3139,10 @@ export class IEventNamePropertyMapping {
* kernelConnectionNotCreated - Kernel connection not created via the kernel finder.
*/
reason: 'cancelled' | 'kernelConnectionNotCreated';
/**
* Type of the Python environment.
*/
envType?: EnvironmentType;
}
| /* Creation succeeded */ {
/**
Expand All @@ -3159,6 +3163,10 @@ export class IEventNamePropertyMapping {
dependenciesInstalled: {
classification: 'SystemMetaData',
purpose: 'FeatureInsight'
},
envType: {
classification: 'SystemMetaData',
purpose: 'FeatureInsight'
}
}
};
Expand Down

0 comments on commit 532b316

Please sign in to comment.