Skip to content

Fix bug in registering interpreter as kernel and add telemetry for DS events #9052

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.

Sign up for GitHub

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

Merged
Don Jayamanne (DonJayamanne) merged 5 commits into from
Dec 12, 2019
Merged

Fix bug in registering interpreter as kernel and add telemetry for DS events #9052

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
392cf76
Fixed a bug and added telemetry for kernel selections
DonJayamanne Dec 12, 2019
61e1458
Fix tests
DonJayamanne Dec 12, 2019
13ae641
Fix test
DonJayamanne Dec 12, 2019
fedb098
Oops
DonJayamanne Dec 12, 2019
4e52746
Fix imports
DonJayamanne Dec 12, 2019
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
5 changes: 5 additions & 0 deletions src/client/datascience/constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,11 @@ export enum Telemetry {
ConnectFailedJupyter = 'DATASCIENCE.CONNECTFAILEDJUPYTER',
ConnectRemoteFailedJupyter = 'DATASCIENCE.CONNECTREMOTEFAILEDJUPYTER',
ConnectRemoteSelfCertFailedJupyter = 'DATASCIENCE.CONNECTREMOTESELFCERTFAILEDJUPYTER',
RegisterAndUseInterpreterAsKernel = 'DATASCIENCE.REGISTER_AND_USE_INTERPRETER_AS_KERNEL',
UseInterpreterAsKernel = 'DATASCIENCE.USE_INTERPRETER_AS_KERNEL',
UseExistingKernel = 'DATASCIENCE.USE_EXISTING_KERNEL',
SwitchToInterpreterAsKernel = 'DATASCIENCE.SWITCH_TO_INTERPRETER_AS_KERNEL',
SwitchToExistingKernel = 'DATASCIENCE.SWITCH_TO_EXISTING_KERNEL',
SelfCertsMessageEnabled = 'DATASCIENCE.SELFCERTSMESSAGEENABLED',
SelfCertsMessageClose = 'DATASCIENCE.SELFCERTSMESSAGECLOSE',
RemoteAddCode = 'DATASCIENCE.LIVESHARE.ADDCODE',
Expand Down
7 changes: 7 additions & 0 deletions src/client/datascience/jupyter/kernels/kernelSelector.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ import { IInstaller, Product } from '../../../common/types';
import * as localize from '../../../common/utils/localize';
import { noop } from '../../../common/utils/misc';
import { IInterpreterService, PythonInterpreter } from '../../../interpreter/contracts';
import { sendTelemetryEvent } from '../../../telemetry';
import { Telemetry } from '../../constants';
import { IJupyterKernelSpec, IJupyterSessionManager } from '../../types';
import { KernelSelectionProvider } from './kernelSelections';
import { KernelService } from './kernelService';
Expand Down Expand Up @@ -83,6 +85,7 @@ export class KernelSelector {
selection.kernelSpec = await this.kernelService.findMatchingKernelSpec(notebookMetadata?.kernelspec, sessionManager, cancelToken);
if (selection.kernelSpec) {
selection.interpreter = await this.kernelService.findMatchingInterpreter(selection.kernelSpec, cancelToken);
sendTelemetryEvent(Telemetry.UseExistingKernel);
} else {
// No kernel info, hence prmopt to use current interpreter as a kernel.
const activeInterpreter = await this.interpreterService.getActiveInterpreter(undefined);
Expand Down Expand Up @@ -113,8 +116,10 @@ export class KernelSelector {
}
// Check if ipykernel is installed in this kernel.
if (selection.selection.interpreter) {
sendTelemetryEvent(Telemetry.SwitchToInterpreterAsKernel);
return this.useInterpreterAsKernel(selection.selection.interpreter, undefined, session, cancelToken);
} else {
sendTelemetryEvent(Telemetry.SwitchToExistingKernel);
const interpreter = selection.selection.kernelSpec ? await this.kernelService.findMatchingInterpreter(selection.selection.kernelSpec, cancelToken) : undefined;
return { kernelSpec: selection.selection.kernelSpec, interpreter };
}
Expand Down Expand Up @@ -152,6 +157,8 @@ export class KernelSelector {
if (displayNameOfKernelNotFound) {
this.applicationShell.showInformationMessage(localize.DataScience.fallbackToUseActiveInterpeterAsKernel().format(displayNameOfKernelNotFound)).then(noop, noop);
}

sendTelemetryEvent(Telemetry.UseInterpreterAsKernel);
return { kernelSpec, interpreter };
}
traceInfo(`ipykernel installed in ${interpreter.path}, no matching kernel found. Will register kernel.`);
Expand Down
6 changes: 4 additions & 2 deletions src/client/datascience/jupyter/kernels/kernelService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import { sleep } from '../../../common/utils/async';
import { noop } from '../../../common/utils/misc';
import { IEnvironmentActivationService } from '../../../interpreter/activation/types';
import { IInterpreterService, PythonInterpreter } from '../../../interpreter/contracts';
import { captureTelemetry } from '../../../telemetry';
import { captureTelemetry, sendTelemetryEvent } from '../../../telemetry';
import { JupyterCommands, Telemetry } from '../../constants';
import { IJupyterKernelSpec, IJupyterSessionManager } from '../../types';
import { JupyterCommandFinder } from '../jupyterCommandFinder';
Expand Down Expand Up @@ -199,6 +199,7 @@ export class KernelService {
// If a kernelspec already exists for this, then use that.
const found = await this.findMatchingKernelSpec(interpreter, undefined, cancelToken);
if (found) {
sendTelemetryEvent(Telemetry.UseExistingKernel);
return found;
}
return this.registerKernel(interpreter, cancelToken);
Expand Down Expand Up @@ -233,7 +234,7 @@ export class KernelService {
// If we wish to wait for installation to complete, we must provide a cancel token.
const token = new CancellationTokenSource();
const response = await this.installer.promptToInstall(Product.ipykernel, interpreter, wrapCancellationTokens(cancelToken, token.token));
if (response === InstallerResponse.Installed) {
if (response !== InstallerResponse.Installed) {
traceWarning(`Prompted to install ipykernel, however ipykernel not installed in the interpreter ${interpreter.path}. Response ${response}`);
return;
}
Expand Down Expand Up @@ -311,6 +312,7 @@ export class KernelService {
await this.fileSystem.writeFile(kernel.specFile, JSON.stringify(specModel, undefined, 2), { flag: 'w', encoding: 'utf8' });
kernel.metadata = specModel.metadata;

sendTelemetryEvent(Telemetry.RegisterAndUseInterpreterAsKernel);
traceInfo(`Kernel successfully registered for ${interpreter.path} with the name=${name} and spec can be found here ${kernel.specFile}`);
return kernel;
}
Expand Down
5 changes: 5 additions & 0 deletions src/client/telemetry/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1405,6 +1405,11 @@ export interface IEventNamePropertyMapping {
[Telemetry.ConnectRemoteJupyter]: never | undefined;
[Telemetry.ConnectRemoteFailedJupyter]: never | undefined;
[Telemetry.ConnectRemoteSelfCertFailedJupyter]: never | undefined;
[Telemetry.RegisterAndUseInterpreterAsKernel]: never | undefined;
[Telemetry.UseInterpreterAsKernel]: never | undefined;
[Telemetry.UseExistingKernel]: never | undefined;
[Telemetry.SwitchToExistingKernel]: never | undefined;
[Telemetry.SwitchToInterpreterAsKernel]: never | undefined;
[Telemetry.ConvertToPythonFile]: never | undefined;
[Telemetry.CopySourceCode]: never | undefined;
[Telemetry.CreateNewNotebook]: never | undefined;
Expand Down
30 changes: 29 additions & 1 deletion src/test/datascience/jupyter/kernels/kernelService.unit.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import { IFileSystem } from '../../../../client/common/platform/types';
import { PythonExecutionFactory } from '../../../../client/common/process/pythonExecutionFactory';
import { PythonExecutionService } from '../../../../client/common/process/pythonProcess';
import { IPythonExecutionFactory, IPythonExecutionService } from '../../../../client/common/process/types';
import { IInstaller, Product, ReadWrite } from '../../../../client/common/types';
import { IInstaller, InstallerResponse, Product, ReadWrite } from '../../../../client/common/types';
import { Architecture } from '../../../../client/common/utils/platform';
import { JupyterCommands } from '../../../../client/datascience/constants';
import { InterpreterJupyterNotebookCommand } from '../../../../client/datascience/jupyter/jupyterCommand';
Expand Down Expand Up @@ -276,6 +276,34 @@ suite('Data Science - KernelService', () => {
assert.deepEqual(installArgs, ['install', '--user', '--name', kernelName, '--display-name', interpreter.displayName]);
await assert.isRejected(promise, `Kernel not created with the name ${kernelName}, display_name ${interpreter.displayName}. Output is `);
}).timeout(5_000);
test('If ipykernel is not installed, then prompt to install ipykernel', async () => {
when(execService.execModule('ipykernel', anything(), anything())).thenResolve({ stdout: '' });
when(installer.isInstalled(Product.ipykernel, interpreter)).thenResolve(false);
when(installer.promptToInstall(anything(), anything(), anything())).thenResolve(InstallerResponse.Installed);
findMatchingKernelSpecStub.resolves(undefined);

const promise = kernelService.registerKernel(interpreter);

await assert.isRejected(promise);
verify(execService.execModule('ipykernel', anything(), anything())).once();
const installArgs = capture(execService.execModule).first()[1] as string[];
const kernelName = installArgs[3];
assert.deepEqual(installArgs, ['install', '--user', '--name', kernelName, '--display-name', interpreter.displayName]);
await assert.isRejected(promise, `Kernel not created with the name ${kernelName}, display_name ${interpreter.displayName}. Output is `);
verify(installer.promptToInstall(anything(), anything(), anything())).once();
}).timeout(5_000);
test('If ipykernel is not installed, and ipykerne installation is canclled, then do not reigster kernel', async () => {
when(execService.execModule('ipykernel', anything(), anything())).thenResolve({ stdout: '' });
when(installer.isInstalled(Product.ipykernel, interpreter)).thenResolve(false);
when(installer.promptToInstall(anything(), anything(), anything())).thenResolve(InstallerResponse.Ignore);
findMatchingKernelSpecStub.resolves(undefined);

const kernel = await kernelService.registerKernel(interpreter);

assert.isUndefined(kernel);
verify(execService.execModule('ipykernel', anything(), anything())).never();
verify(installer.promptToInstall(anything(), anything(), anything())).once();
}).timeout(5_000);
test('Fail if installed kernel is not an instance of JupyterKernelSpec', async () => {
when(execService.execModule('ipykernel', anything(), anything())).thenResolve({ stdout: '' });
when(installer.isInstalled(Product.ipykernel, interpreter)).thenResolve(true);
Expand Down