Skip to content

Fixes to blowing away of kernel info & not using right startup info #14295

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 5 commits into from
Oct 7, 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
6 changes: 3 additions & 3 deletions build/ci/templates/test_phases.yml
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ steps:
python -c "import sys;print(sys.executable)"
displayName: 'pip install functional requirements'
condition: and(succeeded(), eq(variables['NeedsPythonFunctionalReqs'], 'true'))

# Add CONDA to the path so anaconda works
#
# This task will only run if variable `NeedsPythonFunctionalReqs` is true.
Expand Down Expand Up @@ -422,7 +422,7 @@ steps:
# Upload the test results to Azure DevOps to facilitate test reporting in their UX.
- task: PublishTestResults@2
displayName: 'Publish functional tests results'
condition: or(contains(variables['TestsToRun'], 'testFunctional'), contains(variables['TestsToRun'], 'testParallelFunctional'))
condition: or(contains(variables['TestsToRun'], 'testFunctional'), contains(variables['TestsToRun'], 'testParallelFunctional'))
inputs:
testResultsFiles: '$(Build.ArtifactStagingDirectory)/test-junit*.xml'
testRunTitle: 'functional-$(Agent.Os)-Py$(pythonVersion)'
Expand All @@ -446,7 +446,7 @@ steps:
- script: |
npm run testDataScience
continueOnError: true
displayName: 'Run DataScience Tests in VSCode Insiders'
displayName: 'Run Native Notebook Tests in VSCode Insiders'
condition: and(succeeded(), contains(variables['TestsToRun'], 'testDataScience'), not(contains(variables['TestsToRun'], 'testDataScienceInVSCode')))
env:
DISPLAY: :10
Expand Down
2 changes: 1 addition & 1 deletion build/ci/vscode-python-pr-validation.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ stages:
'Single Workspace':
TestsToRun: 'testSingleWorkspace'
NeedsPythonTestReqs: true
'DataScience':
'Native Notebook':
TestsToRun: 'testDataScience'
NeedsPythonTestReqs: true
NeedsPythonFunctionalReqs: true
Expand Down
7 changes: 6 additions & 1 deletion src/client/datascience/baseJupyterSession.ts
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,12 @@ export abstract class BaseJupyterSession implements IJupyterSession {
);
}
}

public async requestKernelInfo(): Promise<KernelMessage.IInfoReplyMsg> {
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 the information stored in the metatadata of the notebook.
Today we're handcrafting this, we can instead get this from the kernel.

if (!this.session) {
throw new Error('Cannot request KernelInfo, Session not initialized.');
}
return this.session.kernel.requestKernelInfo();
}
public async changeKernel(kernelConnection: KernelConnectionMetadata, timeoutMS: number): Promise<void> {
let newSession: ISessionWithSocket | undefined;

Expand Down
1 change: 1 addition & 0 deletions src/client/datascience/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -574,6 +574,7 @@ export namespace LiveShareCommands {
export const getUsableJupyterPython = 'getUsableJupyterPython';
export const executeObservable = 'executeObservable';
export const getSysInfo = 'getSysInfo';
export const requestKernelInfo = 'requestKernelInfo';
export const serverResponse = 'serverResponse';
export const catchupRequest = 'catchupRequest';
export const syncRequest = 'synchRequest';
Expand Down
4 changes: 3 additions & 1 deletion src/client/datascience/jupyter/jupyterNotebook.ts
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,9 @@ export class JupyterNotebookBase implements INotebook {
}
}
}

public async requestKernelInfo(): Promise<KernelMessage.IInfoReplyMsg> {
return this.session.requestKernelInfo();
}
public get onSessionStatusChanged(): Event<ServerStatus> {
if (!this.onStatusChangedEvent) {
this.onStatusChangedEvent = new EventEmitter<ServerStatus>();
Expand Down
20 changes: 14 additions & 6 deletions src/client/datascience/jupyter/kernels/kernel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
'use strict';

import { nbformat } from '@jupyterlab/coreutils';
import { KernelMessage } from '@jupyterlab/services';
import { Observable } from 'rxjs/Observable';
import { Subject } from 'rxjs/Subject';
import * as uuid from 'uuid/v4';
Expand All @@ -18,11 +19,10 @@ import {
} from 'vscode';
import { ServerStatus } from '../../../../datascience-ui/interactive-common/mainState';
import { IApplicationShell, ICommandManager, IVSCodeNotebook } from '../../../common/application/types';
import { traceError } from '../../../common/logger';
import { traceError, traceWarning } from '../../../common/logger';
import { IDisposableRegistry } from '../../../common/types';
import { createDeferred, Deferred } from '../../../common/utils/async';
import { noop } from '../../../common/utils/misc';
import { IInterpreterService } from '../../../interpreter/contracts';
import { CodeSnippets } from '../../constants';
import { getDefaultNotebookContent, updateNotebookMetadata } from '../../notebookStorage/baseModel';
import {
Expand Down Expand Up @@ -51,6 +51,10 @@ export class Kernel implements IKernel {
get onDisposed(): Event<void> {
return this._onDisposed.event;
}
private _info?: KernelMessage.IInfoReplyMsg['content'];
get info(): KernelMessage.IInfoReplyMsg['content'] | undefined {
return this._info;
}
get status(): ServerStatus {
return this.notebook?.status ?? ServerStatus.NotStarted;
}
Expand Down Expand Up @@ -79,7 +83,6 @@ export class Kernel implements IKernel {
private readonly disposables: IDisposableRegistry,
private readonly launchTimeout: number,
commandManager: ICommandManager,
interpreterService: IInterpreterService,
private readonly errorHandler: IDataScienceErrorHandler,
editorProvider: INotebookEditorProvider,
private readonly kernelProvider: IKernelProvider,
Expand All @@ -90,12 +93,12 @@ export class Kernel implements IKernel {
this.kernelExecution = new KernelExecution(
kernelProvider,
commandManager,
interpreterService,
errorHandler,
editorProvider,
kernelSelectionUsage,
appShell,
vscNotebook
vscNotebook,
metadata
);
}
public async executeCell(cell: NotebookCell): Promise<void> {
Expand Down Expand Up @@ -124,8 +127,9 @@ export class Kernel implements IKernel {
} else {
await this.validate(this.uri);
const metadata = ((getDefaultNotebookContent().metadata || {}) as unknown) as nbformat.INotebookMetadata;
// Create a dummy notebook metadata & update the metadata before starting the notebook (required to ensure we fetch & start the right kernel).
// Lower layers of code below getOrCreateNotebook searches for kernels again using the metadata.
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 the code that's not right.
I've created an issue for this (possibly closed).
Basically we're doing all of the searching for kernels & finding the kernel information, etc..
However, when starting a notebook, we have to pass the notebook metadata as that is what's used by NotebookProvider.getOrCreateNotebook to start the real notebook/kernel.
However this is based on the notebook metadata, & we're not passing that in here, instead we pass the kernel connection metatadata. & the work around here is to generate a dummy object that looks like a notebook metadata & hydrate that with information from the kernel connection.

I still don't like this,
Basically we do all of the search, translate to kernelConnection & then we build another object from that & lower down we search for the right kernel using the information in what's supposed to be the metadata in the ipynb file.

updateNotebookMetadata(metadata, this.metadata);

this._notebookPromise = this.notebookProvider.getOrCreateNotebook({
identity: this.uri,
resource: this.uri,
Expand Down Expand Up @@ -232,6 +236,10 @@ export class Kernel implements IKernel {
if (isPythonKernelConnection(this.metadata)) {
await this.notebook.setLaunchingFile(this.uri.fsPath);
}
await this.notebook
.requestKernelInfo()
.then((item) => (this._info = item.content))
.catch(traceWarning.bind('Failed to request KernelInfo'));
await this.notebook.waitForIdle(this.launchTimeout);
}

Expand Down
17 changes: 4 additions & 13 deletions src/client/datascience/jupyter/kernels/kernelExecution.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,12 @@ import { NotebookCell, NotebookCellRunState, NotebookDocument } from 'vscode';
import { IApplicationShell, ICommandManager, IVSCodeNotebook } from '../../../common/application/types';
import { IDisposable } from '../../../common/types';
import { noop } from '../../../common/utils/misc';
import { IInterpreterService } from '../../../interpreter/contracts';
import { captureTelemetry } from '../../../telemetry';
import { Commands, Telemetry, VSCodeNativeTelemetry } from '../../constants';
import { MultiCancellationTokenSource } from '../../notebook/helpers/multiCancellationToken';
import { IDataScienceErrorHandler, INotebook, INotebookEditorProvider } from '../../types';
import { CellExecution, CellExecutionFactory } from './cellExecution';
import type { IKernel, IKernelProvider, IKernelSelectionUsage } from './types';
import type { IKernel, IKernelProvider, IKernelSelectionUsage, KernelConnectionMetadata } from './types';
// tslint:disable-next-line: no-var-requires no-require-imports
const vscodeNotebookEnums = require('vscode') as typeof import('vscode-proposed');

Expand All @@ -35,12 +34,12 @@ export class KernelExecution implements IDisposable {
constructor(
private readonly kernelProvider: IKernelProvider,
private readonly commandManager: ICommandManager,
private readonly interpreterService: IInterpreterService,
errorHandler: IDataScienceErrorHandler,
editorProvider: INotebookEditorProvider,
readonly kernelSelectionUsage: IKernelSelectionUsage,
readonly appShell: IApplicationShell,
readonly vscNotebook: IVSCodeNotebook
readonly vscNotebook: IVSCodeNotebook,
readonly metadata: Readonly<KernelConnectionMetadata>
) {
this.executionFactory = new CellExecutionFactory(errorHandler, editorProvider, appShell, vscNotebook);
}
Expand Down Expand Up @@ -142,15 +141,7 @@ export class KernelExecution implements IDisposable {
await this.validateKernel(document);
let kernel = this.kernelProvider.get(document.uri);
if (!kernel) {
const activeInterpreter = await this.interpreterService.getActiveInterpreter(document.uri);
kernel = this.kernelProvider.getOrCreate(document.uri, {
Copy link
Author

Choose a reason for hiding this comment

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

This was wrong, we were hand crafting the kernel connection metadata instead of using the one provided.

Copy link
Author

@DonJayamanne DonJayamanne Oct 7, 2020

Choose a reason for hiding this comment

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

Basically starting a kernel from a kernel spec would never have worked with the old code (for native notebooks).

metadata: {
interpreter: activeInterpreter!,
kernelModel: undefined,
kernelSpec: undefined,
kind: 'startUsingPythonInterpreter'
}
});
kernel = this.kernelProvider.getOrCreate(document.uri, { metadata: this.metadata });
}
if (!kernel) {
throw new Error('Unable to create a Kernel to run cell');
Expand Down
3 changes: 0 additions & 3 deletions src/client/datascience/jupyter/kernels/kernelProvider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import { Uri } from 'vscode';
import { IApplicationShell, ICommandManager, IVSCodeNotebook } from '../../../common/application/types';
import { traceInfo, traceWarning } from '../../../common/logger';
import { IAsyncDisposableRegistry, IConfigurationService, IDisposableRegistry } from '../../../common/types';
import { IInterpreterService } from '../../../interpreter/contracts';
import { IDataScienceErrorHandler, INotebookEditorProvider, INotebookProvider } from '../../types';
import { Kernel } from './kernel';
import { KernelSelector } from './kernelSelector';
Expand All @@ -24,7 +23,6 @@ export class KernelProvider implements IKernelProvider {
@inject(INotebookProvider) private notebookProvider: INotebookProvider,
@inject(IConfigurationService) private configService: IConfigurationService,
@inject(ICommandManager) private readonly commandManager: ICommandManager,
@inject(IInterpreterService) private readonly interpreterService: IInterpreterService,
@inject(IDataScienceErrorHandler) private readonly errorHandler: IDataScienceErrorHandler,
@inject(INotebookEditorProvider) private readonly editorProvider: INotebookEditorProvider,
@inject(KernelSelector) private readonly kernelSelectionUsage: IKernelSelectionUsage,
Expand All @@ -50,7 +48,6 @@ export class KernelProvider implements IKernelProvider {
this.disposables,
waitForIdleTimeout,
this.commandManager,
this.interpreterService,
this.errorHandler,
this.editorProvider,
this,
Expand Down
7 changes: 6 additions & 1 deletion src/client/datascience/jupyter/kernels/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

'use strict';

import type { Session } from '@jupyterlab/services';
import type { KernelMessage, Session } from '@jupyterlab/services';
import type { Observable } from 'rxjs/Observable';
import type { CancellationToken, Event, QuickPickItem, Uri } from 'vscode';
import { NotebookCell, NotebookDocument } from '../../../../../types/vscode-proposed';
Expand Down Expand Up @@ -130,6 +130,11 @@ export interface IKernel extends IAsyncDisposable {
readonly onRestarted: Event<void>;
readonly status: ServerStatus;
readonly disposed: boolean;
/**
* Kernel information, used to save in ipynb in the metadata.
* Crucial for non-python notebooks, else we save the incorrect information.
*/
readonly info?: KernelMessage.IInfoReplyMsg['content'];
readonly kernelSocket: Observable<KernelSocketInformation | undefined>;
start(): Promise<void>;
interrupt(): Promise<InterruptResult>;
Expand Down
10 changes: 10 additions & 0 deletions src/client/datascience/jupyter/liveshare/guestJupyterNotebook.ts
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,16 @@ export class GuestJupyterNotebook
}
}

public async requestKernelInfo(): Promise<KernelMessage.IInfoReplyMsg> {
// This is a special case. Ask the shared server
const service = await this.waitForService();
if (service) {
return service.request(LiveShareCommands.getSysInfo, []);
} else {
throw new Error('No LiveShare service to get KernelInfo');
}
}

public async getCompletion(
_cellCode: string,
_offsetInCode: number,
Expand Down
27 changes: 23 additions & 4 deletions src/client/datascience/notebook/helpers/helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ import { JupyterNotebookView } from '../constants';
// tslint:disable-next-line: no-var-requires no-require-imports
const vscodeNotebookEnums = require('vscode') as typeof import('vscode-proposed');
// tslint:disable-next-line: no-require-imports
import { KernelMessage } from '@jupyterlab/services';
// tslint:disable-next-line: no-require-imports
import cloneDeep = require('lodash/cloneDeep');
import { isUntitledFile } from '../../../common/utils/misc';
import { KernelConnectionMetadata } from '../../jupyter/kernels/types';
Expand Down Expand Up @@ -54,7 +56,10 @@ export function isJupyterNotebook(option: NotebookDocument | string) {
}
}

const kernelInformationForNotebooks = new WeakMap<NotebookDocument, KernelConnectionMetadata | undefined>();
const kernelInformationForNotebooks = new WeakMap<
NotebookDocument,
{ metadata?: KernelConnectionMetadata | undefined; kernelInfo?: KernelMessage.IInfoReplyMsg['content'] }
>();

export function getNotebookMetadata(document: NotebookDocument): nbformat.INotebookMetadata | undefined {
// tslint:disable-next-line: no-any
Expand All @@ -70,8 +75,9 @@ export function getNotebookMetadata(document: NotebookDocument): nbformat.INoteb
notebookContent = { ...content, metadata: { ...metadata, language_info } } as any;
}
notebookContent = cloneDeep(notebookContent);
if (kernelInformationForNotebooks.has(document)) {
updateNotebookMetadata(notebookContent.metadata, kernelInformationForNotebooks.get(document));
const data = kernelInformationForNotebooks.get(document);
if (data && data.metadata) {
updateNotebookMetadata(notebookContent.metadata, data.metadata, data.kernelInfo);
}

return notebookContent.metadata;
Expand All @@ -88,7 +94,20 @@ export function updateKernelInNotebookMetadata(
document: NotebookDocument,
kernelConnection: KernelConnectionMetadata | undefined
) {
kernelInformationForNotebooks.set(document, kernelConnection);
const data = { ...(kernelInformationForNotebooks.get(document) || {}) };
data.metadata = kernelConnection;
kernelInformationForNotebooks.set(document, data);
}
export function updateKernelInfoInNotebookMetadata(
Copy link
Author

Choose a reason for hiding this comment

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

We keep track of the kernel information, just as we keep track of the notebook metadata & update it only when writing to disc.
Using same (existing) technique.

I.e. the contents (metadata) get computed only at the time data is written to disc

document: NotebookDocument,
kernelInfo: KernelMessage.IInfoReplyMsg['content']
) {
if (kernelInformationForNotebooks.get(document)?.kernelInfo === kernelInfo) {
return;
}
const data = { ...(kernelInformationForNotebooks.get(document) || {}) };
data.kernelInfo = kernelInfo;
kernelInformationForNotebooks.set(document, data);
}
/**
* Converts a NotebookModel into VSCode friendly format.
Expand Down
Loading