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.

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

Jump to bottom

Stop storing interpreter info in ipynb file #12523

Merged
merged 1 commit into from
Jan 10, 2023
Merged

Stop storing interpreter info in ipynb file #12523

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
14 changes: 12 additions & 2 deletions src/interactive-window/interactiveWindow.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,13 @@ import { traceError, traceInfoIfCI, traceWarning } from '../platform/logging';
import { IFileSystem } from '../platform/common/platform/types';
import uuid from 'uuid/v4';

import { IConfigurationService, InteractiveWindowMode, IsWebExtension, Resource } from '../platform/common/types';
import {
IConfigurationService,
IFeaturesManager,
InteractiveWindowMode,
IsWebExtension,
Resource
} from '../platform/common/types';
import { noop } from '../platform/common/utils/misc';
import {
IKernel,
Expand Down Expand Up @@ -903,7 +909,11 @@ export class InteractiveWindow implements IInteractiveWindowLoadable {
// Pull out the metadata from our active notebook
const metadata: nbformat.INotebookMetadata = { orig_nbformat: defaultNotebookFormat.major };
if (kernel) {
await updateNotebookMetadata(metadata, kernel.kernelConnectionMetadata);
await updateNotebookMetadata(
this.serviceContainer.get<IFeaturesManager>(IFeaturesManager),
metadata,
kernel.kernelConnectionMetadata
);
}

let defaultFileName;
Expand Down
19 changes: 15 additions & 4 deletions src/kernels/execution/helpers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import {
kernelConnectionMetadataHasKernelModel,
getKernelRegistrationInfo
} from '../helpers';
import { IFeaturesManager } from '../../platform/common/types';

export enum CellOutputMimeTypes {
error = 'application/vnd.code.notebook.error',
Expand Down Expand Up @@ -654,6 +655,7 @@ export function hasErrorOutput(outputs: readonly NotebookCellOutput[]) {

// eslint-disable-next-line complexity
export async function updateNotebookMetadata(
featureManager: IFeaturesManager,
metadata?: nbformat.INotebookMetadata,
kernelConnection?: KernelConnectionMetadata,
kernelInfo?: Partial<KernelMessage.IInfoReplyMsg['content']>
Expand Down Expand Up @@ -756,7 +758,12 @@ export async function updateNotebookMetadata(
// since name might be python3 in both scenarios and they might have the same python version so the
// check above with language info would not see them as changed.
const interpreter = getInterpreterFromKernelConnectionMetadata(kernelConnection);
const interpreterHash = interpreter?.uri ? await getInterpreterHash({ uri: interpreter?.uri }) : undefined;
const interpreterHash =
featureManager.features.kernelPickerType === 'Stable'
? interpreter?.uri
? await getInterpreterHash({ uri: interpreter?.uri })
: undefined
: undefined;
const metadataInterpreter: undefined | { hash?: string } =
'interpreter' in metadata // In the past we'd store interpreter.hash directly under metadata, but now we store it under metadata.vscode.
? (metadata.interpreter as undefined | { hash?: string })
Expand All @@ -774,7 +781,7 @@ export async function updateNotebookMetadata(
language: PYTHON_LANGUAGE,
display_name: displayName
};
if (getKernelRegistrationInfo(kernelSpec)) {
if (featureManager.features.kernelPickerType === 'Stable' && getKernelRegistrationInfo(kernelSpec)) {
// For kernels generated by us, store the interpreter information.
// We'll leave the kernel spec name as `python3`, this way it will work even in Jupyter or the like.
// Else if user opens a notebook that works in jupter,
Expand All @@ -784,10 +791,14 @@ export async function updateNotebookMetadata(
hash: await getInterpreterHash(kernelConnection.interpreter)
}
};
if ('interpreter' in metadata) {
delete metadata['interpreter'];
} else {
if ('vscode' in metadata) {
delete metadata['vscode'];
}
}
if ('interpreter' in metadata) {
delete metadata['interpreter'];
}
Comment on lines +794 to +801
Copy link
Member

Choose a reason for hiding this comment

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

The if is redundant:

Suggested change
} else {
if ('vscode' in metadata) {
delete metadata['vscode'];
}
}
if ('interpreter' in metadata) {
delete metadata['interpreter'];
}
} else {
delete metadata['vscode'];
}
delete metadata['interpreter'];

}
} else if (kernelSpecOrModel && !metadata.kernelspec) {
const originalNameFromOriginalSpecFile = kernelSpecOrModel.metadata?.vscode?.originalSpecFile
Expand Down
3 changes: 2 additions & 1 deletion src/notebooks/controllers/controllerRegistration.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -441,7 +441,8 @@ export class ControllerRegistration implements IControllerRegistration, IExtensi
this.serviceContainer.get<IBrowserService>(IBrowserService),
this.extensionChecker,
this.serviceContainer,
this.serviceContainer.get<ConnectionDisplayDataProvider>(ConnectionDisplayDataProvider)
this.serviceContainer.get<ConnectionDisplayDataProvider>(ConnectionDisplayDataProvider),
this.featuresManager
);
// Hook up to if this NotebookController is selected or de-selected
const controllerDisposables: IDisposable[] = [];
Expand Down
23 changes: 17 additions & 6 deletions src/notebooks/controllers/vscodeNotebookController.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,8 @@ import {
IDisplayOptions,
IDisposable,
IDisposableRegistry,
IExtensionContext
IExtensionContext,
IFeaturesManager
} from '../../platform/common/types';
import { createDeferred } from '../../platform/common/utils/async';
import { DataScience, Common } from '../../platform/common/utils/localize';
Expand Down Expand Up @@ -162,7 +163,8 @@ export class VSCodeNotebookController implements Disposable, IVSCodeNotebookCont
browser: IBrowserService,
extensionChecker: IPythonExtensionChecker,
serviceContainer: IServiceContainer,
displayDataProvider: ConnectionDisplayDataProvider
displayDataProvider: ConnectionDisplayDataProvider,
featureManager: IFeaturesManager
): IVSCodeNotebookController {
return new VSCodeNotebookController(
kernelConnection,
Expand All @@ -181,7 +183,8 @@ export class VSCodeNotebookController implements Disposable, IVSCodeNotebookCont
browser,
extensionChecker,
serviceContainer,
displayDataProvider
displayDataProvider,
featureManager
);
}
constructor(
Expand All @@ -201,7 +204,8 @@ export class VSCodeNotebookController implements Disposable, IVSCodeNotebookCont
private readonly browser: IBrowserService,
private readonly extensionChecker: IPythonExtensionChecker,
private serviceContainer: IServiceContainer,
private readonly displayDataProvider: ConnectionDisplayDataProvider
private readonly displayDataProvider: ConnectionDisplayDataProvider,
private readonly featureManager: IFeaturesManager
) {
disposableRegistry.push(this);
displayDataProvider.onDidChange(
Expand Down Expand Up @@ -623,6 +627,7 @@ export class VSCodeNotebookController implements Disposable, IVSCodeNotebookCont
return;
}
await updateNotebookDocumentMetadata(
this.featureManager,
doc,
this.documentManager,
kernel.kernelConnectionMetadata,
Expand Down Expand Up @@ -667,7 +672,12 @@ export class VSCodeNotebookController implements Disposable, IVSCodeNotebookCont
}

// Before we start the notebook, make sure the metadata is set to this new kernel.
await updateNotebookDocumentMetadata(document, this.documentManager, selectedKernelConnectionMetadata);
await updateNotebookDocumentMetadata(
this.featureManager,
document,
this.documentManager,
selectedKernelConnectionMetadata
);

if (document.notebookType === InteractiveWindowView) {
// Possible its an interactive window, in that case we'll create the kernel manually.
Expand Down Expand Up @@ -704,13 +714,14 @@ export class VSCodeNotebookController implements Disposable, IVSCodeNotebookCont
}

async function updateNotebookDocumentMetadata(
featureManager: IFeaturesManager,
document: NotebookDocument,
editManager: IDocumentManager,
kernelConnection?: KernelConnectionMetadata,
kernelInfo?: Partial<KernelMessage.IInfoReplyMsg['content']>
) {
let metadata = getNotebookMetadata(document) || { orig_nbformat: 3 };
const { changed } = await updateNotebookMetadata(metadata, kernelConnection, kernelInfo);
const { changed } = await updateNotebookMetadata(featureManager, metadata, kernelConnection, kernelInfo);
if (changed) {
const edit = new WorkspaceEdit();
// Create a clone.
Expand Down
Loading