-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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'; | ||
|
@@ -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 { | ||
|
@@ -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; | ||
} | ||
|
@@ -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, | ||
|
@@ -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> { | ||
|
@@ -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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the code that's not right. I still don't like this, |
||
updateNotebookMetadata(metadata, this.metadata); | ||
|
||
this._notebookPromise = this.notebookProvider.getOrCreateNotebook({ | ||
identity: this.uri, | ||
resource: this.uri, | ||
|
@@ -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); | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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'); | ||
|
||
|
@@ -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); | ||
} | ||
|
@@ -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, { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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'); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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'; | ||
|
@@ -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 | ||
|
@@ -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; | ||
|
@@ -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( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. 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. | ||
|
There was a problem hiding this comment.
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.