Skip to content
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

Pass resource to track Kernel Metadata against a resource #4760

Merged
merged 11 commits into from
Feb 11, 2021
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
15 changes: 1 addition & 14 deletions .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -232,8 +232,7 @@ module.exports = {
'src/test/datascience/mockCode2ProtocolConverter.ts',
'src/test/datascience/mockFileSystem.ts',
'src/test/datascience/interactive-common/trustService.unit.test.ts',
'src/test/datascience/interactive-common/notebookProvider.unit.test.ts',
'src/test/datascience/interactive-common/notebookServerProvider.unit.test.ts',
'src/test/datascience/interactive-common/',
'src/test/datascience/interactive-common/trustCommandHandler.unit.test.ts',
'src/test/datascience/mockStatusProvider.ts',
'src/test/datascience/extensionapi/exampleextension/ms-toolsai-test/webpack.config.js',
Expand All @@ -253,8 +252,6 @@ module.exports = {
'src/test/datascience/preWarmVariables.unit.test.ts',
'src/test/datascience/remoteTestHelpers.ts',
'src/test/datascience/mockWorkspaceFolder.ts',
'src/test/datascience/mockJupyterSession.ts',
'src/test/datascience/jupyterUriProviderRegistration.functional.test.ts',
'src/test/datascience/mockJupyterRequest.ts',
'src/test/datascience/inputHistory.unit.test.ts',
'src/test/datascience/jupyterHelpers.ts',
Expand All @@ -271,12 +268,10 @@ module.exports = {
'src/test/datascience/mockProtocol2CodeConverter.ts',
'src/test/datascience/editor-integration/helpers.ts',
'src/test/datascience/editor-integration/cellhashprovider.unit.test.ts',
'src/test/datascience/editor-integration/gotocell.functional.test.ts',
'src/test/datascience/editor-integration/codelensprovider.unit.test.ts',
'src/test/datascience/editor-integration/codewatcher.unit.test.ts',
'src/test/datascience/jupyterPasswordConnect.unit.test.ts',
'src/test/datascience/testHelpers.tsx',
'src/test/datascience/notebook.functional.test.ts',
'src/test/datascience/mockLanguageClient.ts',
'src/test/datascience/errorHandler.functional.test.tsx',
'src/test/datascience/notebook/notebookStorage.unit.test.ts',
Expand All @@ -300,7 +295,6 @@ module.exports = {
'src/test/datascience/intellisense.unit.test.ts',
'src/test/datascience/markdownManipulation.unit.test.ts',
'src/test/datascience/interactivePanel.functional.test.tsx',
'src/test/datascience/variableTestHelpers.ts',
'src/test/datascience/testPersistentStateFactory.ts',
'src/test/datascience/jupyter/interpreter/jupyterInterpreterSubCommandExecutionService.unit.test.ts',
'src/test/datascience/jupyter/interpreter/jupyterInterpreterDependencyService.unit.test.ts',
Expand Down Expand Up @@ -901,12 +895,10 @@ module.exports = {
'src/client/datascience/kernel-launcher/kernelDaemonPreWarmer.ts',
'src/client/datascience/ipywidgets/localWidgetScriptSourceProvider.ts',
'src/client/datascience/ipywidgets/ipyWidgetScriptSourceProvider.ts',
'src/client/datascience/ipywidgets/ipyWidgetScriptSource.ts',
'src/client/datascience/ipywidgets/cdnWidgetScriptSourceProvider.ts',
'src/client/datascience/ipywidgets/types.ts',
'src/client/datascience/ipywidgets/remoteWidgetScriptSourceProvider.ts',
'src/client/datascience/ipywidgets/constants.ts',
'src/client/datascience/ipywidgets/ipyWidgetMessageDispatcher.ts',
'src/client/datascience/ipywidgets/ipywidgetHandler.ts',
'src/client/datascience/ipywidgets/ipyWidgetMessageDispatcherFactory.ts',
'src/client/datascience/themeFinder.ts',
Expand Down Expand Up @@ -951,7 +943,6 @@ module.exports = {
'src/client/datascience/notebookStorage/nativeEditorStorage.ts',
'src/client/datascience/notebookStorage/factory.ts',
'src/client/datascience/notebookStorage/types.ts',
'src/client/datascience/notebookStorage/nativeEditorProvider.ts',
'src/client/datascience/debugLocationTracker.ts',
'src/client/datascience/plotting/plotViewerMessageListener.ts',
'src/client/datascience/plotting/types.ts',
Expand All @@ -972,7 +963,6 @@ module.exports = {
'src/client/datascience/editor-integration/codelensprovider.ts',
'src/client/datascience/editor-integration/cellhashprovider.ts',
'src/client/datascience/commands/commandLineSelector.ts',
'src/client/datascience/commands/notebookCommands.ts',
'src/client/datascience/commands/exportCommands.ts',
'src/client/datascience/cellFactory.ts',
'src/client/datascience/notebook/notebookEditorCompatibilitySupport.ts',
Expand Down Expand Up @@ -1003,7 +993,6 @@ module.exports = {
'src/client/datascience/jupyter/interpreter/jupyterInterpreterService.ts',
'src/client/datascience/jupyter/kernels/jupyterKernelSpec.ts',
'src/client/datascience/jupyter/jupyterExecutionFactory.ts',
'src/client/datascience/jupyter/serverPreload.ts',
'src/client/datascience/jupyter/jupyterRequest.ts',
'src/client/datascience/jupyter/commandLineSelector.ts',
'src/client/datascience/jupyter/jupyterDebugger.ts',
Expand All @@ -1014,8 +1003,6 @@ module.exports = {
'src/client/datascience/jupyter/liveshare/utils.ts',
'src/client/datascience/jupyter/liveshare/guestJupyterSessionManagerFactory.ts',
'src/client/datascience/jupyter/liveshare/types.ts',
'src/client/datascience/jupyter/liveshare/serverCache.ts',
'src/client/datascience/jupyter/liveshare/guestJupyterExecution.ts',
'src/client/datascience/jupyter/debuggerVariableRegistration.ts',
'src/client/datascience/jupyter/jupyterConnection.ts',
'src/client/datascience/jupyter/jupyterPasswordConnect.ts',
Expand Down
12 changes: 9 additions & 3 deletions src/client/datascience/baseJupyterSession.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import { CancellationToken } from 'vscode-jsonrpc';
import { ServerStatus } from '../../datascience-ui/interactive-common/mainState';
import { WrappedError } from '../common/errors/types';
import { traceError, traceInfo, traceWarning } from '../common/logger';
import { Resource } from '../common/types';
import { sleep, waitForPromise } from '../common/utils/async';
import * as localize from '../common/utils/localize';
import { noop } from '../common/utils/misc';
Expand All @@ -22,6 +23,7 @@ import { kernelConnectionMetadataHasKernelSpec } from './jupyter/kernels/helpers
import { JupyterKernelPromiseFailedError } from './jupyter/kernels/jupyterKernelPromiseFailedError';
import { getKernelConnectionId, KernelConnectionMetadata } from './jupyter/kernels/types';
import { suppressShutdownErrors } from './raw-kernel/rawKernel';
import { trackKernelResourceInformation } from './telemetry/telemetry';
import { IJupyterSession, ISessionWithSocket, KernelSocketInformation } from './types';

/**
Expand All @@ -34,7 +36,7 @@ import { IJupyterSession, ISessionWithSocket, KernelSocketInformation } from './
export class JupyterSessionStartError extends WrappedError {
constructor(originalException: Error) {
super(originalException.message, originalException);
sendTelemetryEvent(Telemetry.StartSessionFailedJupyter);
sendTelemetryEvent(Telemetry.StartSessionFailedJupyter, undefined, undefined, originalException, true);
}
}

Expand Down Expand Up @@ -145,9 +147,12 @@ export abstract class BaseJupyterSession implements IJupyterSession {
}
return this.session.kernel.requestKernelInfo();
}
public async changeKernel(kernelConnection: KernelConnectionMetadata, timeoutMS: number): Promise<void> {
public async changeKernel(
resource: Resource,
kernelConnection: KernelConnectionMetadata,
timeoutMS: number
): Promise<void> {
let newSession: ISessionWithSocket | undefined;

// If we are already using this kernel in an active session just return back
const currentKernelSpec =
this.kernelConnectionMetadata && kernelConnectionMetadataHasKernelSpec(this.kernelConnectionMetadata)
Expand All @@ -163,6 +168,7 @@ export abstract class BaseJupyterSession implements IJupyterSession {
}
}

trackKernelResourceInformation(resource, { kernelConnection });
newSession = await this.createNewKernelSession(kernelConnection, timeoutMS);

// This is just like doing a restart, kill the old session (and the old restart session), and start new ones
Expand Down
2 changes: 1 addition & 1 deletion src/client/datascience/commands/activeEditorContext.ts
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ export class ActiveEditorContextService implements IExtensionSingleActivationSer
private updateContextOfActiveNotebookKernel(activeEditor?: INotebookEditor) {
if (activeEditor) {
this.notebookProvider
.getOrCreateNotebook({ identity: activeEditor.file, getOnly: true })
.getOrCreateNotebook({ identity: activeEditor.file, resource: activeEditor.file, getOnly: true })
.then((nb) => {
if (activeEditor === this.notebookEditorProvider.activeEditor) {
const canStart = nb && nb.status !== ServerStatus.NotStarted && activeEditor.model?.isTrusted;
Expand Down
8 changes: 6 additions & 2 deletions src/client/datascience/commands/notebookCommands.ts
Original file line number Diff line number Diff line change
Expand Up @@ -74,11 +74,15 @@ export class NotebookCommands implements IDisposable {
}
if (options.identity) {
// Make sure we have a connection or we can't get remote kernels.
const connection = await this.notebookProvider.connect({ getOnly: false, disableUI: false });
const connection = await this.notebookProvider.connect({
getOnly: false,
disableUI: false,
resource: options.resource
});

// Select a new kernel using the connection information
const kernel = await this.kernelSelector.selectJupyterKernel(
options.identity,
options.resource,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bug fix.

connection,
connection?.type || this.notebookProvider.type,
options.currentKernelDisplayName
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -931,7 +931,12 @@ export class IntellisenseProvider implements IInteractiveWindowListener {

private async getNotebook(token: CancellationToken): Promise<INotebook | undefined> {
return this.notebookIdentity
? this.notebookProvider.getOrCreateNotebook({ identity: this.notebookIdentity, getOnly: true, token })
? this.notebookProvider.getOrCreateNotebook({
identity: this.notebookIdentity,
resource: this.resource,
getOnly: true,
token
})
: undefined;
}

Expand Down
13 changes: 10 additions & 3 deletions src/client/datascience/interactive-common/interactiveBase.ts
Original file line number Diff line number Diff line change
Expand Up @@ -903,7 +903,10 @@ export abstract class InteractiveBase extends WebviewPanelHost<IInteractiveWindo

protected async createNotebookIfProviderConnectionExists(): Promise<void> {
// Check to see if we are already connected to our provider
const providerConnection = await this.notebookProvider.connect({ getOnly: true });
const providerConnection = await this.notebookProvider.connect({
getOnly: true,
resource: this.owningResource
});

if (providerConnection) {
try {
Expand Down Expand Up @@ -931,7 +934,7 @@ export abstract class InteractiveBase extends WebviewPanelHost<IInteractiveWindo
serverUri !== Settings.JupyterServerLocalLaunch &&
!this.configService.getSettings(this.owningResource).disableJupyterAutoStart
) {
serverConnection = await this.notebookProvider.connect({ disableUI: true });
serverConnection = await this.notebookProvider.connect({ disableUI: true, resource: this.owningResource });
}
let displayName =
serverConnection?.displayName ||
Expand Down Expand Up @@ -985,7 +988,11 @@ export abstract class InteractiveBase extends WebviewPanelHost<IInteractiveWindo
// Make sure we're loaded first.
try {
traceInfo('Waiting for jupyter server and web panel ...');
const serverConnection = await this.notebookProvider.connect({ getOnly: false, disableUI: false });
const serverConnection = await this.notebookProvider.connect({
getOnly: false,
disableUI: false,
resource: this.owningResource
});
if (serverConnection) {
await this.ensureNotebook(serverConnection);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import { CancellationToken, ConfigurationTarget, EventEmitter, Uri } from 'vscod
import { IApplicationShell } from '../../common/application/types';
import { CancellationError, wrapCancellationTokens } from '../../common/cancellation';
import { traceInfo } from '../../common/logger';
import { IConfigurationService } from '../../common/types';
import { IConfigurationService, Resource } from '../../common/types';
import * as localize from '../../common/utils/localize';
import { noop } from '../../common/utils/misc';
import { IInterpreterService } from '../../interpreter/contracts';
Expand Down Expand Up @@ -95,6 +95,7 @@ export class NotebookServerProvider implements IJupyterServerProvider {

private async startServer(
options: {
resource: Resource;
metadata?: nbformat.INotebookMetadata;
kernelConnection?: KernelConnectionMetadata;
},
Expand Down Expand Up @@ -215,6 +216,7 @@ export class NotebookServerProvider implements IJupyterServerProvider {
}

private async getNotebookServerOptions(options: {
resource: Resource;
metadata?: nbformat.INotebookMetadata;
kernelConnection?: KernelConnectionMetadata;
}): Promise<INotebookServerOptions> {
Expand All @@ -230,6 +232,7 @@ export class NotebookServerProvider implements IJupyterServerProvider {

return {
uri: serverURI,
resource: options.resource,
skipUsingDefaultConfig: !useDefaultConfig,
purpose: Identifiers.HistoryPurpose,
kernelConnection: options.kernelConnection,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -304,7 +304,10 @@ export class InteractiveWindowCommandListener implements IDataScienceCommandList
try {
const settings = this.configuration.getSettings(document.uri);
// Create a new notebook
notebook = await this.notebookProvider.getOrCreateNotebook({ identity: createExportInteractiveIdentity() });
notebook = await this.notebookProvider.getOrCreateNotebook({
identity: createExportInteractiveIdentity(),
resource: file
});
// If that works, then execute all of the cells.
const cells = Array.prototype.concat(
...(await Promise.all(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -389,6 +389,7 @@ export class IPyWidgetMessageDispatcher implements IIPyWidgetMessageDispatcher {
if (this.notebookIdentity && !this.notebook) {
this.notebook = await this.notebookProvider.getOrCreateNotebook({
identity: this.notebookIdentity,
resource: this.notebookIdentity,
getOnly: true
});
}
Expand Down
1 change: 1 addition & 0 deletions src/client/datascience/ipywidgets/ipyWidgetScriptSource.ts
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,7 @@ export class IPyWidgetScriptSource implements ILocalResourceUriConverter {
if (!this.notebook) {
this.notebook = await this.notebookProvider.getOrCreateNotebook({
identity: this.identity,
resource: this.identity,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is resource the same as identity? Or does it not matter in this context?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doesn't matter in this context (basically use the best we have, majority of the time identify = resoruce, except in interactive window when you open it without a correspoding py file or when you have multiples.

disableUI: true,
getOnly: true
});
Expand Down
15 changes: 12 additions & 3 deletions src/client/datascience/jupyter/jupyterExecution.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import { PythonEnvironment } from '../../pythonEnvironments/info';
import { captureTelemetry, sendTelemetryEvent } from '../../telemetry';
import { JupyterSessionStartError } from '../baseJupyterSession';
import { Commands, Identifiers, Telemetry } from '../constants';
import { trackKernelResourceInformation } from '../telemetry/telemetry';
import {
IJupyterConnection,
IJupyterExecution,
Expand Down Expand Up @@ -192,7 +193,7 @@ export class JupyterExecutionBase implements IJupyterExecution {
const sessionManager = await sessionManagerFactory.create(connection);
try {
kernelConnectionMetadata = await this.kernelSelector.getPreferredKernelForRemoteConnection(
undefined,
options?.resource,
sessionManager,
options?.metadata,
cancelToken
Expand All @@ -211,7 +212,12 @@ export class JupyterExecutionBase implements IJupyterExecution {
purpose: options ? options.purpose : uuid(),
disableUI: !allowUI
};

// If we were not provided a kernel connection, this means we changed the connection here.
if (!options?.kernelConnection) {
trackKernelResourceInformation(options?.resource, {
kernelConnection: launchInfo.kernelConnectionMetadata
});
}
// eslint-disable-next-line no-constant-condition
while (true) {
try {
Expand All @@ -238,14 +244,17 @@ export class JupyterExecutionBase implements IJupyterExecution {
const selection = await this.appShell.showErrorMessage(message, selectKernel, cancel);
if (selection === selectKernel) {
const kernelInterpreter = await this.kernelSelector.selectLocalKernel(
undefined,
options?.resource,
'jupyter',
new StopWatch(),
cancelToken,
getDisplayNameOrNameOfKernelConnection(launchInfo.kernelConnectionMetadata)
);
if (kernelInterpreter) {
launchInfo.kernelConnectionMetadata = kernelInterpreter;
trackKernelResourceInformation(options?.resource, {
kernelConnection: launchInfo.kernelConnectionMetadata
});
continue;
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/client/datascience/jupyter/jupyterNotebook.ts
Original file line number Diff line number Diff line change
Expand Up @@ -685,7 +685,7 @@ export class JupyterNotebookBase implements INotebook {
this.ranInitialSetup = false;

// Change the kernel on the session
await this.session.changeKernel(connectionMetadata, timeoutMS);
await this.session.changeKernel(this.resource, connectionMetadata, timeoutMS);

// Change our own kernel spec
// Only after session was successfully created.
Expand Down
2 changes: 2 additions & 0 deletions src/client/datascience/jupyter/jupyterNotebookProvider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ export class JupyterNotebookProvider implements IJupyterNotebookProvider {
const server = await this.serverProvider.getOrCreateServer({
getOnly: options.getOnly,
disableUI: options.disableUI,
resource: options.resource,
token: options.token
});

Expand All @@ -56,6 +57,7 @@ export class JupyterNotebookProvider implements IJupyterNotebookProvider {
getOnly: options.getOnly,
disableUI: options.disableUI,
token: options.token,
resource: options.resource,
metadata: options.metadata,
kernelConnection: options.kernelConnection
});
Expand Down
Loading