Skip to content

Commit d0fbdfd

Browse files
IanMatthewHuffIanMatthewHuff
authored
Update file metadata with notebook interpreter and kernel info (microsoft#8835)
1 parent 751f5b2 commit d0fbdfd

9 files changed

Lines changed: 171 additions & 73 deletions

File tree

‎news/2 Fixes/8223.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Correctly update interpreter and kernel info in the metadata.

‎src/client/datascience/interactive-ipynb/nativeEditor.ts‎

Lines changed: 21 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -283,8 +283,10 @@ export class NativeEditor extends InteractiveBase implements INotebookEditor {
283283

284284
protected submitCode(code: string, file: string, line: number, id?: string, editor?: TextEditor, debug?: boolean): Promise<boolean> {
285285
// When code is executed, update the version number in the metadata.
286-
this.updateVersionInfoInNotebook().ignoreErrors();
287-
return super.submitCode(code, file, line, id, editor, debug);
286+
return super.submitCode(code, file, line, id, editor, debug).then((value) => {
287+
this.updateVersionInfoInNotebook().ignoreErrors();
288+
return value;
289+
});
288290
}
289291

290292
@captureTelemetry(Telemetry.SubmitCellThroughInput, undefined, false)
@@ -439,13 +441,24 @@ export class NativeEditor extends InteractiveBase implements INotebookEditor {
439441
* @memberof NativeEditor
440442
*/
441443
private async updateVersionInfoInNotebook(): Promise<void> {
442-
// Make sure we have notebook json
443-
await this.ensureNotebookJson();
444+
// Get our kernel_info and language_info from the current notebook
445+
const notebook = this.getNotebook();
444446

445-
// Use the active interpreter
446-
const usableInterpreter = await this.jupyterExecution.getUsableJupyterPython();
447-
if (usableInterpreter && usableInterpreter.version && this.notebookJson.metadata && this.notebookJson.metadata.language_info) {
448-
this.notebookJson.metadata.language_info.version = `${usableInterpreter.version.major}.${usableInterpreter.version.minor}.${usableInterpreter.version.patch}`;
447+
if (notebook) {
448+
const [interpreter, kernelSpec] = await Promise.all([notebook.getMatchingInterpreter(), notebook.getKernelSpec()]);
449+
450+
if (interpreter && interpreter.version && this.notebookJson.metadata && this.notebookJson.metadata.language_info) {
451+
this.notebookJson.metadata.language_info.version = interpreter.version.raw;
452+
}
453+
454+
if (kernelSpec && this.notebookJson.metadata && !this.notebookJson.metadata.kernelspec) {
455+
// Add a new spec in this case
456+
this.notebookJson.metadata.kernelspec = { name: kernelSpec.name, display_name: kernelSpec.display_name };
457+
} else if (kernelSpec && this.notebookJson.metadata && this.notebookJson.metadata.kernelspec) {
458+
// Spec exists, just update name and display_name
459+
this.notebookJson.metadata.kernelspec.name = kernelSpec.name;
460+
this.notebookJson.metadata.kernelspec.display_name = kernelSpec.display_name;
461+
}
449462
}
450463
}
451464

‎src/client/datascience/jupyter/jupyterNotebook.ts‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@ import { CodeSnippits, Identifiers, Telemetry } from '../constants';
3131
import {
3232
CellState,
3333
ICell,
34+
IJupyterKernelSpec,
3435
IJupyterSession,
3536
INotebook,
3637
INotebookCompletion,
@@ -470,6 +471,10 @@ export class JupyterNotebookBase implements INotebook {
470471
return this.interpreter;
471472
}
472473

474+
public async getKernelSpec(): Promise<IJupyterKernelSpec | undefined> {
475+
return this.launchInfo.kernelSpec;
476+
}
477+
473478
private finishUncompletedCells() {
474479
const copyPending = [...this.pendingCellSubscriptions];
475480
copyPending.forEach(c => c.cancel());

‎src/client/datascience/jupyter/kernels/jupyterKernelSpec.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ export class JupyterKernelSpec implements IJupyterKernelSpec {
1616
public language: string;
1717
public path: string;
1818
public specFile: string | undefined;
19-
public display_name?: string;
19+
public display_name: string;
2020
// tslint:disable-next-line: no-any
2121
public readonly metadata?: Record<string, any> & { interpreter?: Partial<PythonInterpreter> };
2222

‎src/client/datascience/jupyter/liveshare/guestJupyterNotebook.ts‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ import { PythonInterpreter } from '../../../interpreter/contracts';
1717
import { LiveShare, LiveShareCommands } from '../../constants';
1818
import {
1919
ICell,
20+
IJupyterKernelSpec,
2021
INotebook,
2122
INotebookCompletion,
2223
INotebookExecutionLogger,
@@ -184,6 +185,10 @@ export class GuestJupyterNotebook
184185
return Promise.resolve(undefined);
185186
}
186187

188+
public getKernelSpec(): Promise<IJupyterKernelSpec | undefined> {
189+
return Promise.resolve(undefined);
190+
}
191+
187192
private onServerResponse = (args: Object) => {
188193
const er = args as IExecuteObservableResponse;
189194
traceInfo(`Guest serverResponse ${er.pos} ${er.id}`);

‎src/client/datascience/types.ts‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,7 @@ export interface INotebook extends IAsyncDisposable {
101101
setMatplotLibStyle(useDark: boolean): Promise<void>;
102102
addLogger(logger: INotebookExecutionLogger): void;
103103
getMatchingInterpreter(): Promise<PythonInterpreter | undefined>;
104+
getKernelSpec(): Promise<IJupyterKernelSpec | undefined>;
104105
}
105106

106107
export interface INotebookServerOptions {
@@ -193,16 +194,16 @@ export interface IJupyterKernel {
193194
}
194195

195196
export interface IJupyterKernelSpec extends IAsyncDisposable {
196-
name: string | undefined;
197-
language: string | undefined;
198-
path: string | undefined;
197+
name: string;
198+
language: string;
199+
path: string;
199200
/**
200201
* Kernel display name.
201202
*
202203
* @type {string}
203204
* @memberof IJupyterKernelSpec
204205
*/
205-
readonly display_name?: string;
206+
readonly display_name: string;
206207
/**
207208
* A dictionary of additional attributes about this kernel; used by clients to aid in kernel selection.
208209
* Optionally storing the interpreter information in the metadata (helping extension search for kernels that match an interpereter).

‎src/test/datascience/execution.unit.test.ts‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -127,6 +127,10 @@ class MockJupyterNotebook implements INotebook {
127127
public getMatchingInterpreter(): Promise<PythonInterpreter | undefined> {
128128
return Promise.resolve(undefined);
129129
}
130+
131+
public getKernelSpec(): Promise<IJupyterKernelSpec | undefined> {
132+
return Promise.resolve(undefined);
133+
}
130134
}
131135

132136
// tslint:disable:no-any no-http-string no-multiline-string max-func-body-length

‎src/test/datascience/interactive-ipynb/nativeEditor.unit.test.ts‎

Lines changed: 1 addition & 57 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@ import { expect } from 'chai';
55
import { anything, instance, mock, verify, when } from 'ts-mockito';
66
import { ConfigurationChangeEvent, Disposable, EventEmitter, TextEditor, Uri } from 'vscode';
77

8-
import { nbformat } from '@jupyterlab/coreutils';
98
import * as sinon from 'sinon';
109
import { ApplicationShell } from '../../../client/common/application/applicationShell';
1110
import { CommandManager } from '../../../client/common/application/commandManager';
@@ -25,7 +24,7 @@ import { ConfigurationService } from '../../../client/common/configuration/servi
2524
import { LiveShareApi } from '../../../client/common/liveshare/liveshare';
2625
import { FileSystem } from '../../../client/common/platform/fileSystem';
2726
import { IFileSystem } from '../../../client/common/platform/types';
28-
import { IConfigurationService, Version } from '../../../client/common/types';
27+
import { IConfigurationService } from '../../../client/common/types';
2928
import { CodeCssGenerator } from '../../../client/datascience/codeCssGenerator';
3029
import { DataViewerProvider } from '../../../client/datascience/data-viewing/dataViewerProvider';
3130
import { DataScienceErrorHandler } from '../../../client/datascience/errorHandler/errorHandler';
@@ -54,7 +53,6 @@ import { IInterpreterService } from '../../../client/interpreter/contracts';
5453
import { InterpreterService } from '../../../client/interpreter/interpreterService';
5554
import { createEmptyCell } from '../../../datascience-ui/interactive-common/mainState';
5655
import { waitForCondition } from '../../common';
57-
import { noop } from '../../core';
5856
import { MockMemento } from '../../mocks/mementos';
5957
import { MockStatusProvider } from '../mockStatusProvider';
6058

@@ -421,60 +419,6 @@ suite('Data Science - Native Editor', () => {
421419
expect(newEditor.cells).to.be.lengthOf(3);
422420
});
423421

424-
test('Python version info is not queried on creating a blank editor', async () => {
425-
const file = Uri.parse('file:///Untitled1.ipynb');
426-
427-
// When a cell is executed, then ensure we store the python version info in the notebook data.
428-
when(executionProvider.getUsableJupyterPython()).thenReject();
429-
430-
const editor = createEditor();
431-
await editor.load('', file);
432-
433-
try {
434-
await editor.getContents();
435-
expect(false, 'Did not throw an error');
436-
} catch {
437-
// This should throw an error
438-
noop();
439-
}
440-
});
441-
442-
test('Pyton version info will be updated in notebook when a cell has been executed', async () => {
443-
const file = Uri.parse('file:///foo.ipynb');
444-
445-
const editor = createEditor();
446-
await editor.load(baseFile, file);
447-
expect(await editor.getContents()).to.be.equal(baseFile);
448-
// At the begining version info is NOT in the file (at least not the same as what we are using to run cells).
449-
let contents = JSON.parse(await editor.getContents()) as nbformat.INotebookContent;
450-
expect(contents.metadata!.language_info!.version).to.not.equal('10.11.12');
451-
452-
// When a cell is executed, then ensure we store the python version info in the notebook data.
453-
const version: Version = { build: [], major: 10, minor: 11, patch: 12, prerelease: [], raw: '10.11.12' };
454-
when(executionProvider.getUsableJupyterPython()).thenResolve(({ version } as any));
455-
456-
try {
457-
editor.onMessage(InteractiveWindowMessages.SubmitNewCell, { code: 'hello', id: '1' });
458-
} catch {
459-
// Ignore errors related to running cells, assume that works.
460-
noop();
461-
}
462-
463-
// Wait for the version info to be retrieved (done in the background).
464-
await waitForCondition(async () => {
465-
try {
466-
verify(executionProvider.getUsableJupyterPython()).atLeast(1);
467-
return true;
468-
} catch {
469-
return false;
470-
}
471-
}, 5_000, 'Timeout');
472-
473-
// Verify the version info is in the notbook.
474-
contents = JSON.parse(await editor.getContents()) as nbformat.INotebookContent;
475-
expect(contents.metadata!.language_info!.version).to.equal('10.11.12');
476-
});
477-
478422
test('Opening file with local storage but no global will still open with old contents', async () => {
479423
// This test is really for making sure when a user upgrades to a new extension, we still have their old storage
480424
const file = Uri.parse('file:///foo.ipynb');

‎src/test/datascience/nativeEditor.functional.test.tsx‎

Lines changed: 128 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
// Copyright (c) Microsoft Corporation. All rights reserved.
22
// Licensed under the MIT License.
33
'use strict';
4+
import { nbformat } from '@jupyterlab/coreutils';
45
import { assert, expect, use } from 'chai';
56
import * as chaiAsPromised from 'chai-as-promised';
67
import { ReactWrapper } from 'enzyme';
@@ -491,7 +492,7 @@ for _ in range(50):
491492
ioc = new DataScienceIocContainer();
492493
ioc.registerDataScienceTypes();
493494
}
494-
async function setupFunction(this: Mocha.Context) {
495+
async function setupFunction(this: Mocha.Context, fileContents?: any) {
495496
const wrapperPossiblyUndefined = await setupWebview(ioc);
496497
if (wrapperPossiblyUndefined) {
497498
wrapper = wrapperPossiblyUndefined;
@@ -502,8 +503,8 @@ for _ in range(50):
502503
// This is used in some tests (saving).
503504
const filesystem = new FileSystem();
504505
notebookFile = await filesystem.createTemporaryFile('.ipynb');
505-
await fs.writeFile(notebookFile.filePath, baseFile);
506-
await Promise.all([waitForUpdate(wrapper, NativeEditor, 1), openEditor(ioc, baseFile, notebookFile.filePath)]);
506+
await fs.writeFile(notebookFile.filePath, fileContents ? fileContents : baseFile);
507+
await Promise.all([waitForUpdate(wrapper, NativeEditor, 1), openEditor(ioc, fileContents ? fileContents : baseFile, notebookFile.filePath)]);
507508
} else {
508509
// tslint:disable-next-line: no-invalid-this
509510
this.skip();
@@ -1332,6 +1333,130 @@ for _ in range(50):
13321333
});
13331334
});
13341335

1336+
suite('Update Metadata', () => {
1337+
setup(async function() {
1338+
initIoc();
1339+
1340+
const oldJson: nbformat.INotebookContent = {
1341+
nbformat: 4,
1342+
nbformat_minor: 2,
1343+
cells: [
1344+
{
1345+
cell_type: 'code',
1346+
execution_count: 1,
1347+
metadata: {
1348+
collapsed: true
1349+
},
1350+
outputs: [
1351+
{
1352+
data: {
1353+
'text/plain': [
1354+
'1'
1355+
]
1356+
},
1357+
output_type: 'execute_result',
1358+
execution_count: 1,
1359+
metadata: {}
1360+
}
1361+
],
1362+
source: [
1363+
'a=1\n',
1364+
'a'
1365+
]
1366+
},
1367+
{
1368+
cell_type: 'code',
1369+
execution_count: 2,
1370+
metadata: {},
1371+
outputs: [
1372+
{
1373+
data: {
1374+
'text/plain': [
1375+
'2'
1376+
]
1377+
},
1378+
output_type: 'execute_result',
1379+
execution_count: 2,
1380+
metadata: {}
1381+
}
1382+
],
1383+
source: [
1384+
'b=2\n',
1385+
'b'
1386+
]
1387+
},
1388+
{
1389+
cell_type: 'code',
1390+
execution_count: 3,
1391+
metadata: {},
1392+
outputs: [
1393+
{
1394+
data: {
1395+
'text/plain': [
1396+
'3'
1397+
]
1398+
},
1399+
output_type: 'execute_result',
1400+
execution_count: 3,
1401+
metadata: {}
1402+
}
1403+
],
1404+
source: [
1405+
'c=3\n',
1406+
'c'
1407+
]
1408+
}
1409+
],
1410+
metadata: {
1411+
orig_nbformat: 4,
1412+
kernelspec: {
1413+
display_name: 'JUNK',
1414+
name: 'JUNK'
1415+
},
1416+
language_info: {
1417+
name: 'python',
1418+
version: '1.2.3'
1419+
}
1420+
}
1421+
};
1422+
1423+
// tslint:disable-next-line: no-invalid-this
1424+
await setupFunction.call(this, JSON.stringify(oldJson));
1425+
});
1426+
1427+
test('Update notebook metadata on execution', async () => {
1428+
const notebookProvider = ioc.get<INotebookEditorProvider>(INotebookEditorProvider);
1429+
const editor = notebookProvider.editors[0];
1430+
assert.ok(editor, 'No editor when saving');
1431+
const savedPromise = createDeferred();
1432+
const disp = editor.saved(() => savedPromise.resolve());
1433+
1434+
// add cells, run them and save
1435+
await addCell(wrapper, ioc, 'a=1\na');
1436+
const runAllButton = findButton(wrapper, NativeEditor, 0);
1437+
await waitForMessageResponse(ioc, () => runAllButton!.simulate('click'));
1438+
simulateKeyPressOnCell(1, { code: 's', ctrlKey: true });
1439+
1440+
await savedPromise.promise;
1441+
disp.dispose();
1442+
1443+
// the file has output and execution count
1444+
const fileContent = await fs.readFile(notebookFile.filePath, 'utf8');
1445+
const fileObject = JSON.parse(fileContent);
1446+
1447+
// The version should be updated to something not "1.2.3"
1448+
assert.notEqual(fileObject.metadata.language_info.version, '1.2.3');
1449+
1450+
// Some tests don't have a kernelspec, in which case we should remove it
1451+
// If there is a spec, we should update the name and display name
1452+
const isRollingBuild = process.env ? process.env.VSCODE_PYTHON_ROLLING !== undefined : false;
1453+
if (isRollingBuild && fileObject.metadata.kernelspec) {
1454+
assert.notEqual(fileObject.metadata.kernelspec.display_name, 'JUNK');
1455+
assert.notEqual(fileObject.metadata.kernelspec.name, 'JUNK');
1456+
}
1457+
});
1458+
});
1459+
13351460
suite('Clear Outputs', () => {
13361461
setup(async function() {
13371462
initIoc();

0 commit comments

Comments
 (0)