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

No prompt to install Python when open an exist nb without Kernel #5202

Merged
merged 3 commits into from
Mar 19, 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
3 changes: 2 additions & 1 deletion src/client/datascience/jupyter/kernels/helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -339,8 +339,9 @@ export function findPreferredKernel(

// If still not found, look for a match based on notebook metadata and interpreter
if (index < 0) {
const hasLanguageInfo = notebookMetadata?.language_info?.name ? true : false;
const nbMetadataLanguage =
!notebookMetadata || isPythonNotebook(notebookMetadata)
!notebookMetadata || isPythonNotebook(notebookMetadata) || !hasLanguageInfo
? PYTHON_LANGUAGE
: (
(notebookMetadata?.kernelspec?.language as string) || notebookMetadata?.language_info?.name
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import type { nbformat } from '@jupyterlab/coreutils/lib/nbformat';
import { inject, injectable, named } from 'inversify';
import { Memento, NotebookCellKind, NotebookDocument } from 'vscode';
import { IExtensionSingleActivationService } from '../../activation/types';
import { IPythonExtensionChecker } from '../../api/types';
import { IVSCodeNotebook } from '../../common/application/types';
import { PYTHON_LANGUAGE } from '../../common/constants';
import { traceWarning } from '../../common/logger';
Expand All @@ -27,6 +28,7 @@ export class NotebookCellLanguageService implements IExtensionSingleActivationSe
constructor(
@inject(IVSCodeNotebook) private readonly vscNotebook: IVSCodeNotebook,
@inject(IDisposableRegistry) private readonly disposables: IDisposableRegistry,
@inject(IPythonExtensionChecker) private readonly pythonExtensionChecker: IPythonExtensionChecker,
@inject(IMemento) @named(GLOBAL_MEMENTO) private readonly globalMemento: Memento
) {}
/**
Expand All @@ -38,7 +40,11 @@ export class NotebookCellLanguageService implements IExtensionSingleActivationSe
metadata?.language_info?.name ||
(metadata?.kernelspec as IJupyterKernelSpec | undefined)?.language ||
this.lastSavedNotebookCellLanguage;
return translateKernelLanguageToMonaco(jupyterLanguage || PYTHON_LANGUAGE);

// Default to python language only if the Python extension is installed.
const defaultLanguage = this.pythonExtensionChecker.isPythonExtensionInstalled ? PYTHON_LANGUAGE : 'plaintext';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this wasn't right.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If user doesn't have a python kernel at all, no point defaulting to Python

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Else later if you save these notebooks and open it, then we assume its a python notebook and ask the user to install Python.

// Note, what ever language is returned here, when the user selects a kernel, the cells (of blank documents) get updated based on that kernel selection.
return translateKernelLanguageToMonaco(jupyterLanguage || defaultLanguage);
}
public async activate() {
this.vscNotebook.onDidSaveNotebookDocument(this.onDidSaveNotebookDocument, this, this.disposables);
Expand Down
7 changes: 2 additions & 5 deletions src/client/datascience/notebook/helpers/helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -107,11 +107,8 @@ export function isPythonNotebook(metadata?: nbformat.INotebookMetadata) {
if (metadata?.language_info?.name && metadata.language_info.name !== PYTHON_LANGUAGE) {
return false;
}
if (kernelSpec?.language && kernelSpec.language !== PYTHON_LANGUAGE) {
return false;
}
// All other notebooks are python notebooks.
return true;
// Valid notebooks will have a language information in the metadata.
return kernelSpec?.language === PYTHON_LANGUAGE;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This too wasn't right, better now

}
/**
* No need to update the notebook metadata just yet.
Expand Down
30 changes: 24 additions & 6 deletions src/client/datascience/notebook/kernelWithMetadata.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

// eslint-disable-next-line @typescript-eslint/no-require-imports
import { join } from 'path';
import { Uri, NotebookCell, NotebookDocument, NotebookKernel as VSCNotebookKernel } from 'vscode';
import { Uri, NotebookCell, NotebookDocument, NotebookKernel as VSCNotebookKernel, notebook } from 'vscode';
import { ICommandManager, IVSCodeNotebook } from '../../common/application/types';
import { disposeAllDisposables } from '../../common/helpers';
import { traceInfo } from '../../common/logger';
Expand All @@ -16,6 +16,7 @@ import { KernelSocketInformation } from '../types';
import { traceCellMessage, trackKernelInfoInNotebookMetadata } from './helpers/helpers';

export class VSCodeNotebookKernelMetadata implements VSCNotebookKernel {
private notebookKernels = new WeakMap<NotebookDocument, IKernel>();
get preloads(): Uri[] {
return [
Uri.file(join(this.context.extensionPath, 'out', 'ipywidgets', 'dist', 'ipywidgets.js')),
Expand Down Expand Up @@ -64,18 +65,29 @@ export class VSCodeNotebookKernelMetadata implements VSCNotebookKernel {
this.commandManager.executeCommand(Commands.NotebookEditorInterruptKernel).then(noop, noop);
}
private updateKernelInfoInNotebookWhenAvailable(kernel: IKernel, doc: NotebookDocument) {
if (this.notebookKernels.get(doc) === kernel) {
return;
}
this.notebookKernels.set(doc, kernel);
let kernelSocket: KernelSocketInformation | undefined;
const handlerDisposables: IDisposable[] = [];

// If the notebook is closed, dispose everything.
notebook.onDidCloseNotebookDocument(
(e) => {
if (e === doc) {
disposeAllDisposables(handlerDisposables);
}
},
this,
handlerDisposables
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Better clean up of handlers.

);
const saveKernelInfo = () => {
const kernelId = kernelSocket?.options.id;
if (!kernelId) {
return;
}
traceInfo(`Updating preferred kernel for remote notebook ${kernelId}`);
this.preferredRemoteKernelIdProvider.storePreferredRemoteKernelId(doc.uri, kernelId).catch(noop);

disposeAllDisposables(handlerDisposables);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was wrong

};

const kernelDisposedDisposable = kernel.onDisposed(() => disposeAllDisposables(handlerDisposables));
Expand All @@ -92,8 +104,14 @@ export class VSCodeNotebookKernelMetadata implements VSCNotebookKernel {
return;
}
trackKernelInfoInNotebookMetadata(doc, kernel.info);
if (kernel.info.status === 'ok' && this.selection.kind === 'startUsingKernelSpec') {
saveKernelInfo();
if (this.selection.kind === 'startUsingKernelSpec') {
if (kernel.info.status === 'ok') {
saveKernelInfo();
} else {
disposeAllDisposables(handlerDisposables);
}
} else {
disposeAllDisposables(handlerDisposables);
}
});

Expand Down
5 changes: 1 addition & 4 deletions src/client/datascience/notebookStorage/vscNotebookModel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -83,15 +83,12 @@ export class VSCodeNotebookModel extends BaseNotebookModel {
return this.document ? this.document.cells.length : this.notebookJson.cells?.length ?? 0;
}
public getNotebookData() {
if (!this.preferredLanguage) {
throw new Error('Preferred Language not initialized');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Its possible to not have a preferred langauge, we'll default to plain text.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: When you create a blank notebook and we return cells with plain text,
& a kernel is selected, then the language of the cells get updated to match the language of the kernel selected.

Tha'ts existing fucntionality.

I.e. if you create a blank notebook and cells are TypeScript, then select Juali kernel, then language of empty cells in blank notebook will change to Julia.

}
return notebookModelToVSCNotebookData(
this.isTrusted,
this.notebookContentWithoutCells,
this.file,
this.notebookJson.cells || [],
this.preferredLanguage,
this.preferredLanguage || 'plaintext',
this.originalJson
);
}
Expand Down
2 changes: 1 addition & 1 deletion src/client/datascience/openNotebookBanner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ export class OpenNotebookBanner implements IExtensionSingleActivationService {
private async openedNotebook(editor: INotebookEditor) {
if (
!this.pythonExtensionChecker.isPythonExtensionInstalled &&
editor.model.metadata &&
editor.model.metadata?.kernelspec &&
isPythonNotebook(editor.model.metadata) &&
!isUntitledFile(editor.file)
) {
Expand Down