Skip to content

Ensure we do not switch kernel if already the same #13297

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

Merged
merged 2 commits into from
Aug 6, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
8 changes: 4 additions & 4 deletions src/client/datascience/jupyter/kernels/kernel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ export class Kernel implements IKernel {
if (this._notebook) {
return this._notebook.getKernelSpec();
}
return this._metadata.kernelSpec || this._metadata.kernelModel;
return this.metadata.kernelSpec || this.metadata.kernelModel;
}
get onStatusChanged(): Event<ServerStatus> {
return this._onStatusChanged.event;
Expand Down Expand Up @@ -63,7 +63,7 @@ export class Kernel implements IKernel {
private restarting?: Deferred<void>;
constructor(
public readonly uri: Uri,
private readonly _metadata: KernelSelection,
public readonly metadata: Readonly<KernelSelection>,
private readonly notebookProvider: INotebookProvider,
private readonly disposables: IDisposableRegistry,
private readonly waitForIdleTimeoutMs: number,
Expand Down Expand Up @@ -93,8 +93,8 @@ export class Kernel implements IKernel {
const metadata = ((getDefaultNotebookContent().metadata || {}) as unknown) as nbformat.INotebookMetadata;
updateNotebookMetadata(
metadata,
this._metadata.interpreter,
this._metadata.kernelSpec || this._metadata.kernelModel
this.metadata.interpreter,
this.metadata.kernelSpec || this.metadata.kernelModel
);

this._notebookPromise = this.notebookProvider.getOrCreateNotebook({
Expand Down
6 changes: 2 additions & 4 deletions src/client/datascience/jupyter/kernels/kernelProvider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

'use strict';

import * as fastDeepEqual from 'fast-deep-equal';
Copy link
Author

Choose a reason for hiding this comment

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

Using this instead of using JSON.stringify as all i'm after is equality of data.

import { inject, injectable } from 'inversify';
import { Uri } from 'vscode';
import { traceWarning } from '../../../common/logger';
Expand All @@ -25,10 +26,7 @@ export class KernelProvider {
}
public getOrCreate(uri: Uri, options: KernelOptions): IKernel | undefined {
const existingKernelInfo = this.kernelsByUri.get(uri.toString());
if (
existingKernelInfo &&
JSON.stringify(existingKernelInfo.options.metadata) === JSON.stringify(options.metadata)
) {
if (existingKernelInfo && fastDeepEqual(existingKernelInfo.options.metadata, options.metadata)) {
return existingKernelInfo.kernel;
}

Expand Down
1 change: 1 addition & 0 deletions src/client/datascience/jupyter/kernels/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ export interface IKernelSelectionUsage {
export interface IKernel extends IAsyncDisposable {
readonly uri: Uri;
readonly kernelSpec?: IJupyterKernelSpec | LiveKernelModel;
readonly metadata: Readonly<KernelSelection>;
Copy link
Author

Choose a reason for hiding this comment

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

This is what i'd like to solve with the meeting I have booked.
I need the metadata property as well as the kernelSpec property.
Ideally we should have just one property that describes all of this and use that everywhere.
One type to rule them all 😄

Leaving for now, as i need to solve this issue, but will address this once we have a type or approach to generalize this into a single entity.

readonly onStatusChanged: Event<ServerStatus>;
readonly onDisposed: Event<void>;
readonly onRestarted: Event<void>;
Expand Down
3 changes: 2 additions & 1 deletion src/client/datascience/notebook/helpers/executionHelpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

import type { nbformat } from '@jupyterlab/coreutils';
import type { KernelMessage } from '@jupyterlab/services';
import * as fastDeepEqual from 'fast-deep-equal';
import { NotebookCell, NotebookCellRunState, NotebookDocument } from 'vscode';
import { createErrorOutput } from '../../../../datascience-ui/common/cellFactory';
import { createIOutputFromCellOutputs, createVSCCellOutputsFromOutputs, translateErrorOutput } from './helpers';
Expand Down Expand Up @@ -106,7 +107,7 @@ export function updateCellOutput(vscCell: NotebookCell, outputs: nbformat.IOutpu
}
// Compare outputs (at the end of the day everything is serializable).
// Hence this is a safe comparison.
if (vscCell.outputs.length === newOutput.length && JSON.stringify(vscCell.outputs) === JSON.stringify(newOutput)) {
if (vscCell.outputs.length === newOutput.length && fastDeepEqual(vscCell.outputs, newOutput)) {
return;
}
vscCell.outputs = newOutput;
Expand Down
19 changes: 17 additions & 2 deletions src/client/datascience/notebook/kernelProvider.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

import * as fastDeepEqual from 'fast-deep-equal';
import { inject, injectable } from 'inversify';
import { CancellationToken, Event, EventEmitter } from 'vscode';
import {
Expand All @@ -13,6 +14,7 @@ import { IVSCodeNotebook } from '../../common/application/types';
import { createPromiseFromCancellation } from '../../common/cancellation';
import { IDisposableRegistry } from '../../common/types';
import { noop } from '../../common/utils/misc';
import { KernelProvider } from '../jupyter/kernels/kernelProvider';
import { KernelSelectionProvider } from '../jupyter/kernels/kernelSelections';
import { KernelSelector } from '../jupyter/kernels/kernelSelector';
import { KernelSwitcher } from '../jupyter/kernels/kernelSwitcher';
Expand All @@ -22,6 +24,7 @@ import { INotebook, INotebookProvider } from '../types';
import { getNotebookMetadata, isJupyterNotebook, updateKernelInNotebookMetadata } from './helpers/helpers';
import { NotebookKernel } from './notebookKernel';
import { INotebookContentProvider, INotebookExecutionService } from './types';

@injectable()
export class VSCodeKernelPickerProvider implements NotebookKernelProvider {
public get onDidChangeKernels(): Event<void> {
Expand All @@ -33,6 +36,7 @@ export class VSCodeKernelPickerProvider implements NotebookKernelProvider {
@inject(INotebookExecutionService) private readonly execution: INotebookExecutionService,
@inject(KernelSelectionProvider) private readonly kernelSelectionProvider: KernelSelectionProvider,
@inject(KernelSelector) private readonly kernelSelector: KernelSelector,
@inject(KernelProvider) private readonly kernelProvider: KernelProvider,
@inject(IVSCodeNotebook) private readonly notebook: IVSCodeNotebook,
@inject(INotebookStorageProvider) private readonly storageProvider: INotebookStorageProvider,
@inject(INotebookProvider) private readonly notebookProvider: INotebookProvider,
Expand Down Expand Up @@ -87,14 +91,14 @@ export class VSCodeKernelPickerProvider implements NotebookKernelProvider {
if (
preferredKernel.kernelSpec &&
item.kernelSpec &&
JSON.stringify(preferredKernel.kernelSpec) === JSON.stringify(item.kernelSpec)
fastDeepEqual(preferredKernel.kernelSpec, item.kernelSpec)
) {
return true;
}
if (
preferredKernel.kernelModel &&
item.kernelModel &&
JSON.stringify(preferredKernel.kernelModel) === JSON.stringify(item.kernelModel)
fastDeepEqual(preferredKernel.kernelModel, item.kernelModel)
) {
return true;
}
Expand Down Expand Up @@ -130,12 +134,23 @@ export class VSCodeKernelPickerProvider implements NotebookKernelProvider {
// Possibly closed or different kernel picked.
return;
}

const model = await this.storageProvider.getOrCreateModel(document.uri);
if (!model || !model.isTrusted) {
// If a model is not trusted, we cannot change the kernel (this results in changes to notebook metadata).
// This is because we store selected kernel in the notebook metadata.
return;
}

// Check what the existing kernel is.
const existingKernel = this.kernelProvider.get(document.uri);
if (existingKernel && fastDeepEqual(existingKernel.metadata, newKernelInfo.kernel.selection)) {
return;
}

// Make this the new kernel (calling this method will associate the new kernel with this Uri).
this.kernelProvider.getOrCreate(document.uri, { metadata: newKernelInfo.kernel.selection });

// Change kernel and update metadata.
const notebook = await this.notebookProvider.getOrCreateNotebook({
resource: document.uri,
Expand Down
2 changes: 1 addition & 1 deletion src/client/datascience/notebook/notebookKernel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ export class NotebookKernel implements VSCNotebookKernel {
constructor(
public readonly label: string,
public readonly description: string,
private readonly selection: KernelSelection,
public readonly selection: Readonly<KernelSelection>,
public readonly isPreferred: boolean,
private readonly execution: INotebookExecutionService,
private readonly kernelSelectionUsage: IKernelSelectionUsage
Expand Down