From 6be7c8ded75d45139968f7ef7a2a09850493f83d Mon Sep 17 00:00:00 2001 From: Ian Huff Date: Tue, 29 Sep 2020 14:49:09 -0700 Subject: [PATCH 01/14] basic work in progress --- .vscode/launch.json | 3 +++ .vscode/settings.json | 2 +- .../jupyterInterpreterDependencyService.ts | 27 +++++++++++++++++++ ...erInterpreterSubCommandExecutionService.ts | 3 +++ .../datascience/notebook.functional.test.ts | 2 +- 5 files changed, 35 insertions(+), 2 deletions(-) diff --git a/.vscode/launch.json b/.vscode/launch.json index 4eab7ac182a7..65adfd5c46ff 100644 --- a/.vscode/launch.json +++ b/.vscode/launch.json @@ -250,6 +250,7 @@ "--recursive", "--colors", // "--grep", "", + "--grep", "IANHU", "--timeout=300000", "--exit" ], @@ -258,10 +259,12 @@ "XVSC_PYTHON_DS_UI_BROWSER": "1", // Remove `X` prefix to test with real python (for DS functional tests). "XVSCODE_PYTHON_ROLLING": "1", + "VSCODE_PYTHON_ROLLING": "1", // Remove 'X' to turn on all logging in the debug output "XVSC_PYTHON_FORCE_LOGGING": "1", // Remove `X` prefix and update path to test with real python interpreter (for DS functional tests). "XCI_PYTHON_PATH": "", + "CI_PYTHON_PATH": "/Users/ianhuff/opt/miniconda3/envs/functionalTestEnv/bin/python", // Remove 'X' prefix to dump output for debugger. Directory has to exist prior to launch "XDEBUGPY_LOG_DIR": "${workspaceRoot}/tmp/Debug_Output" }, diff --git a/.vscode/settings.json b/.vscode/settings.json index b2dc13efe26e..9f729bd8127e 100644 --- a/.vscode/settings.json +++ b/.vscode/settings.json @@ -45,7 +45,7 @@ "source.fixAll.eslint": true, "source.fixAll.tslint": true }, - "python.languageServer": "Microsoft", + "python.languageServer": "Pylance", "python.analysis.logLevel": "Trace", "python.analysis.downloadChannel": "beta", "python.linting.pylintEnabled": false, diff --git a/src/client/datascience/jupyter/interpreter/jupyterInterpreterDependencyService.ts b/src/client/datascience/jupyter/interpreter/jupyterInterpreterDependencyService.ts index d951338264bf..1ce06b6481d4 100644 --- a/src/client/datascience/jupyter/interpreter/jupyterInterpreterDependencyService.ts +++ b/src/client/datascience/jupyter/interpreter/jupyterInterpreterDependencyService.ts @@ -4,6 +4,7 @@ 'use strict'; import { inject, injectable } from 'inversify'; +import { parse, SemVer } from 'semver'; import { CancellationToken } from 'vscode'; import { IApplicationShell } from '../../../common/application/types'; import { Cancellation, createPromiseFromCancellation, wrapCancellationTokens } from '../../../common/cancellation'; @@ -241,6 +242,32 @@ export class JupyterInterpreterDependencyService { return installed; } + // IANHU: Combine with kernelspec version code? + public async getNbConvertVersion( + interpreter: PythonEnvironment, + _token?: CancellationToken + ): Promise { + const command = this.commandFactory.createInterpreterCommand( + JupyterCommands.ConvertCommand, + 'jupyter', + ['-m', 'jupyter', 'nbconvert'], + interpreter, + false + ); + + const result = await command.exec(['--version'], { throwOnStdErr: true }); + + // IANHU: Shared code with dataViewerDependencyService.ts for Pandas + // IANHU: Helper function and unit test? + const versionMatch = /^\s*(\d+)\.(\d+)\.(.+)\s*$/.exec(result.stdout); + if (versionMatch && versionMatch.length > 2) { + const major = parseInt(versionMatch[1], 10); + const minor = parseInt(versionMatch[2], 10); + const build = parseInt(versionMatch[3], 10); + return parse(`${major}.${minor}.${build}`, true) ?? undefined; + } + } + /** * Gets a list of the dependencies not installed, dependencies that are required to launch the jupyter notebook server. * diff --git a/src/client/datascience/jupyter/interpreter/jupyterInterpreterSubCommandExecutionService.ts b/src/client/datascience/jupyter/interpreter/jupyterInterpreterSubCommandExecutionService.ts index e65c08a4c544..f8e05947e6b3 100644 --- a/src/client/datascience/jupyter/interpreter/jupyterInterpreterSubCommandExecutionService.ts +++ b/src/client/datascience/jupyter/interpreter/jupyterInterpreterSubCommandExecutionService.ts @@ -168,6 +168,9 @@ export class JupyterInterpreterSubCommandExecutionService } } + // Check what version of nbconvert we are working with here. We need different templates for 6.0.0+ + const nbConvertVersion = this.jupyterDependencyService.getNbConvertVersion(interpreter, token); + const daemon = await this.pythonExecutionFactory.createDaemon({ daemonModule: JupyterDaemonModule, pythonPath: interpreter.path diff --git a/src/test/datascience/notebook.functional.test.ts b/src/test/datascience/notebook.functional.test.ts index aeec51594400..f0247f9bda60 100644 --- a/src/test/datascience/notebook.functional.test.ts +++ b/src/test/datascience/notebook.functional.test.ts @@ -626,7 +626,7 @@ suite('DataScience notebook tests', () => { } }); - runTest('Export/Import', async () => { + runTest('IANHU Export/Import', async () => { // Get a bunch of test cells (use our test cells from the react controls) const testFolderPath = path.join(EXTENSION_ROOT_DIR, 'src', 'test', 'datascience'); const testState = generateTestState(testFolderPath); From ba1c13b6e2ac3dc181c04d6af52f5190c0e2752a Mon Sep 17 00:00:00 2001 From: Ian Huff Date: Tue, 29 Sep 2020 15:14:12 -0700 Subject: [PATCH 02/14] code building, needs cleanup and testing --- ...erInterpreterSubCommandExecutionService.ts | 16 ++++--- .../datascience/jupyter/jupyterExecution.ts | 3 +- .../jupyter/jupyterExecutionFactory.ts | 3 +- .../datascience/jupyter/jupyterImporter.ts | 39 +++++++++++----- .../liveshare/guestJupyterExecution.ts | 46 ++++++++++++++----- src/client/datascience/types.ts | 9 +++- 6 files changed, 83 insertions(+), 33 deletions(-) diff --git a/src/client/datascience/jupyter/interpreter/jupyterInterpreterSubCommandExecutionService.ts b/src/client/datascience/jupyter/interpreter/jupyterInterpreterSubCommandExecutionService.ts index f8e05947e6b3..f98bc9d2502f 100644 --- a/src/client/datascience/jupyter/interpreter/jupyterInterpreterSubCommandExecutionService.ts +++ b/src/client/datascience/jupyter/interpreter/jupyterInterpreterSubCommandExecutionService.ts @@ -5,6 +5,7 @@ import { inject, injectable, named } from 'inversify'; import * as path from 'path'; +import { SemVer } from 'semver'; import { CancellationToken, Uri } from 'vscode'; import { Cancellation } from '../../../common/cancellation'; import { traceError, traceInfo, traceWarning } from '../../../common/logger'; @@ -76,12 +77,18 @@ export class JupyterInterpreterSubCommandExecutionService } return this.jupyterDependencyService.areDependenciesInstalled(interpreter, token); } - public async isExportSupported(token?: CancellationToken): Promise { + //public async isExportSupported(token?: CancellationToken): Promise { + public async isExportSupported(token?: CancellationToken): Promise { const interpreter = await this.jupyterInterpreter.getSelectedInterpreter(token); if (!interpreter) { - return false; + return; + } + + if (await this.jupyterDependencyService.isExportSupported(interpreter, token)) { + return this.jupyterDependencyService.getNbConvertVersion(interpreter, token); } - return this.jupyterDependencyService.isExportSupported(interpreter, token); + + // return this.jupyterDependencyService.isExportSupported(interpreter, token); } public async getReasonForJupyterNotebookNotBeingSupported(token?: CancellationToken): Promise { let interpreter = await this.jupyterInterpreter.getSelectedInterpreter(token); @@ -168,9 +175,6 @@ export class JupyterInterpreterSubCommandExecutionService } } - // Check what version of nbconvert we are working with here. We need different templates for 6.0.0+ - const nbConvertVersion = this.jupyterDependencyService.getNbConvertVersion(interpreter, token); - const daemon = await this.pythonExecutionFactory.createDaemon({ daemonModule: JupyterDaemonModule, pythonPath: interpreter.path diff --git a/src/client/datascience/jupyter/jupyterExecution.ts b/src/client/datascience/jupyter/jupyterExecution.ts index 476685dff607..c580d0566f63 100644 --- a/src/client/datascience/jupyter/jupyterExecution.ts +++ b/src/client/datascience/jupyter/jupyterExecution.ts @@ -2,6 +2,7 @@ // Licensed under the MIT License. 'use strict'; import * as path from 'path'; +import { SemVer } from 'semver'; import * as uuid from 'uuid/v4'; import { CancellationToken, CancellationTokenSource, Event, EventEmitter, Uri } from 'vscode'; @@ -123,7 +124,7 @@ export class JupyterExecutionBase implements IJupyterExecution { } @reportAction(ReportableAction.CheckingIfImportIsSupported) - public async isImportSupported(cancelToken?: CancellationToken): Promise { + public async isImportSupported(cancelToken?: CancellationToken): Promise { // See if we can find the command nbconvert return this.jupyterInterpreterService.isExportSupported(cancelToken); } diff --git a/src/client/datascience/jupyter/jupyterExecutionFactory.ts b/src/client/datascience/jupyter/jupyterExecutionFactory.ts index 9888b3147ab3..f71ffa29f980 100644 --- a/src/client/datascience/jupyter/jupyterExecutionFactory.ts +++ b/src/client/datascience/jupyter/jupyterExecutionFactory.ts @@ -2,6 +2,7 @@ // Licensed under the MIT License. 'use strict'; import { inject, injectable, named } from 'inversify'; +import { SemVer } from 'semver'; import { CancellationToken, Event, EventEmitter, Uri } from 'vscode'; import { IApplicationShell, ILiveShareApi, IWorkspaceService } from '../../common/application/types'; @@ -117,7 +118,7 @@ export class JupyterExecutionFactory implements IJupyterExecution, IAsyncDisposa return execution.getNotebookError(); } - public async isImportSupported(cancelToken?: CancellationToken): Promise { + public async isImportSupported(cancelToken?: CancellationToken): Promise { const execution = await this.executionFactory.get(); return execution.isImportSupported(cancelToken); } diff --git a/src/client/datascience/jupyter/jupyterImporter.ts b/src/client/datascience/jupyter/jupyterImporter.ts index cc0016992b83..e662dcdef38d 100644 --- a/src/client/datascience/jupyter/jupyterImporter.ts +++ b/src/client/datascience/jupyter/jupyterImporter.ts @@ -26,7 +26,7 @@ import { export class JupyterImporter implements INotebookImporter { public isDisposed: boolean = false; // Template that changes markdown cells to have # %% [markdown] in the comments - private readonly nbconvertTemplateFormat = + private readonly nbconvert5TemplateFormat = // tslint:disable-next-line:no-multiline-string `{%- extends 'null.tpl' -%} {% block codecell %} @@ -37,9 +37,22 @@ export class JupyterImporter implements INotebookImporter { {% block input %}{{ cell.source | ipython2python }}{% endblock input %} {% block markdowncell scoped %}{0} [markdown] {{ cell.source | comment_lines }} +{% endblock markdowncell %}`; + private readonly nbconvert6TemplateFormat = + // tslint:disable-next-line:no-multiline-string + `{%- extends 'null.j2' -%} +{% block codecell %} +{0} +{{ super() }} +{% endblock codecell %} +{% block in_prompt %}{% endblock in_prompt %} +{% block input %}{{ cell.source | ipython2python }}{% endblock input %} +{% block markdowncell scoped %}{0} [markdown] +{{ cell.source | comment_lines }} {% endblock markdowncell %}`; - private templatePromise: Promise; + private template5Promise: Promise; + private template6Promise: Promise; constructor( @inject(IDataScienceFileSystem) private fs: IDataScienceFileSystem, @@ -51,11 +64,13 @@ export class JupyterImporter implements INotebookImporter { @inject(IJupyterInterpreterDependencyManager) private readonly dependencyManager: IJupyterInterpreterDependencyManager ) { - this.templatePromise = this.createTemplateFile(); + this.template5Promise = this.createTemplateFile(this.nbconvert5TemplateFormat); + this.template6Promise = this.createTemplateFile(this.nbconvert6TemplateFormat); } public async importFromFile(sourceFile: Uri): Promise { - const template = await this.templatePromise; + const template5 = await this.template5Promise; + const template6 = await this.template6Promise; // If the user has requested it, add a cd command to the imported file so that relative paths still work const settings = this.configuration.getSettings(); @@ -69,9 +84,14 @@ export class JupyterImporter implements INotebookImporter { await this.dependencyManager.installMissingDependencies(); } + const nbConvertVersion = await this.jupyterExecution.isImportSupported(); // Use the jupyter nbconvert functionality to turn the notebook into a python file - if (await this.jupyterExecution.isImportSupported()) { - let fileOutput: string = await this.jupyterExecution.importNotebook(sourceFile, template); + if (nbConvertVersion) { + // nbconvert 5 and 6 use a different base template file + let fileOutput: string = await this.jupyterExecution.importNotebook( + sourceFile, + nbConvertVersion.major >= 6 ? template6 : template5 + ); if (fileOutput.includes('get_ipython()')) { fileOutput = this.addIPythonImport(fileOutput); } @@ -153,7 +173,7 @@ export class JupyterImporter implements INotebookImporter { } } - private async createTemplateFile(): Promise { + private async createTemplateFile(baseTemplate: string): Promise { // Create a temp file on disk const file = await this.fs.createTemporaryLocalFile('.tpl'); @@ -162,10 +182,7 @@ export class JupyterImporter implements INotebookImporter { try { // Save this file into our disposables so the temp file goes away this.disposableRegistry.push(file); - await this.fs.appendLocalFile( - file.filePath, - this.nbconvertTemplateFormat.format(this.defaultCellMarker) - ); + await this.fs.appendLocalFile(file.filePath, baseTemplate.format(this.defaultCellMarker)); // Now we should have a template that will convert return file.filePath; diff --git a/src/client/datascience/jupyter/liveshare/guestJupyterExecution.ts b/src/client/datascience/jupyter/liveshare/guestJupyterExecution.ts index 6e585661ed32..f4abb5c2b895 100644 --- a/src/client/datascience/jupyter/liveshare/guestJupyterExecution.ts +++ b/src/client/datascience/jupyter/liveshare/guestJupyterExecution.ts @@ -2,6 +2,7 @@ // Licensed under the MIT License. 'use strict'; import { injectable } from 'inversify'; +import { SemVer } from 'semver'; import * as uuid from 'uuid/v4'; import { CancellationToken } from 'vscode'; @@ -72,10 +73,31 @@ export class GuestJupyterExecution extends LiveShareParticipantGuest( } public async isNotebookSupported(cancelToken?: CancellationToken): Promise { - return this.checkSupported(LiveShareCommands.isNotebookSupported, cancelToken); + //return this.checkSupported(LiveShareCommands.isNotebookSupported, cancelToken); + const service = await this.waitForService(); + + // Make a remote call on the proxy + if (service) { + const result = await service.request(LiveShareCommands.isNotebookSupported, [], cancelToken); + return result as boolean; + } + + return false; } - public isImportSupported(cancelToken?: CancellationToken): Promise { - return this.checkSupported(LiveShareCommands.isImportSupported, cancelToken); + public async isImportSupported(cancelToken?: CancellationToken): Promise { + //return this.checkSupported(LiveShareCommands.isImportSupported, cancelToken); + const service = await this.waitForService(); + + // Make a remote call on the proxy + if (service) { + const result = await service.request(LiveShareCommands.isImportSupported, [], cancelToken); + + if (result) { + return result as SemVer; + } + } + + return; } public isSpawnSupported(_cancelToken?: CancellationToken): Promise { return Promise.resolve(false); @@ -145,15 +167,15 @@ export class GuestJupyterExecution extends LiveShareParticipantGuest( return this.serverCache.get(options); } - private async checkSupported(command: string, cancelToken?: CancellationToken): Promise { - const service = await this.waitForService(); + //private async checkSupported(command: string, cancelToken?: CancellationToken): Promise { + //const service = await this.waitForService(); - // Make a remote call on the proxy - if (service) { - const result = await service.request(command, [], cancelToken); - return result as boolean; - } + //// Make a remote call on the proxy + //if (service) { + //const result = await service.request(command, [], cancelToken); + //return result as boolean; + //} - return false; - } + //return false; + //} } diff --git a/src/client/datascience/types.ts b/src/client/datascience/types.ts index b651af2b7750..8f68474dbad5 100644 --- a/src/client/datascience/types.ts +++ b/src/client/datascience/types.ts @@ -7,6 +7,7 @@ import type { Kernel, KernelMessage } from '@jupyterlab/services/lib/kernel'; import type { JSONObject } from '@phosphor/coreutils'; import { WriteStream } from 'fs-extra'; import { Observable } from 'rxjs/Observable'; +import { SemVer } from 'semver'; import { CancellationToken, CodeLens, @@ -282,7 +283,8 @@ export const IJupyterExecution = Symbol('IJupyterExecution'); export interface IJupyterExecution extends IAsyncDisposable { serverStarted: Event; isNotebookSupported(cancelToken?: CancellationToken): Promise; - isImportSupported(cancelToken?: CancellationToken): Promise; + //isImportSupported(cancelToken?: CancellationToken): Promise; + isImportSupported(cancelToken?: CancellationToken): Promise; isSpawnSupported(cancelToken?: CancellationToken): Promise; connectToNotebookServer( options?: INotebookServerOptions, @@ -994,12 +996,15 @@ export interface IJupyterSubCommandExecutionService { isNotebookSupported(cancelToken?: CancellationToken): Promise; /** * Checks whether exporting of ipynb is supported. + * If exporting is supported return the version of nbconvert available + * otherwise undefined. * * @param {CancellationToken} [cancelToken] * @returns {Promise} * @memberof IJupyterSubCommandExecutionService */ - isExportSupported(cancelToken?: CancellationToken): Promise; + //isExportSupported(cancelToken?: CancellationToken): Promise; + isExportSupported(cancelToken?: CancellationToken): Promise; /** * Error message indicating why jupyter notebook isn't supported. * From 89729eb354fa3656371facb4b2490c46e9d06bfc Mon Sep 17 00:00:00 2001 From: Ian Huff Date: Tue, 29 Sep 2020 20:25:30 -0700 Subject: [PATCH 03/14] parse version check and updated file --- src/client/datascience/common.ts | 12 ++++++++++++ .../data-viewing/dataViewerDependencyService.ts | 10 +++------- .../jupyterInterpreterDependencyService.ts | 16 +++++++++------- ...pyterInterpreterSubCommandExecutionService.ts | 1 + .../datascience/jupyter/jupyterImporter.ts | 2 +- 5 files changed, 26 insertions(+), 15 deletions(-) diff --git a/src/client/datascience/common.ts b/src/client/datascience/common.ts index efa0a104d7e7..06b1da13b837 100644 --- a/src/client/datascience/common.ts +++ b/src/client/datascience/common.ts @@ -3,6 +3,7 @@ 'use strict'; import type { nbformat } from '@jupyterlab/coreutils'; import * as os from 'os'; +import { parse, SemVer } from 'semver'; import { Memento, Uri } from 'vscode'; import { splitMultilineString } from '../../datascience-ui/common'; import { traceError, traceInfo } from '../common/logger'; @@ -188,3 +189,14 @@ export async function getRealPath( } } } + +// For the given string parse it out to a SemVer or return undefined +export function parseSemVer(versionString: string): SemVer | undefined { + const versionMatch = /^\s*(\d+)\.(\d+)\.(.+)\s*$/.exec(versionString); + if (versionMatch && versionMatch.length > 2) { + const major = parseInt(versionMatch[1], 10); + const minor = parseInt(versionMatch[2], 10); + const build = parseInt(versionMatch[3], 10); + return parse(`${major}.${minor}.${build}`, true) ?? undefined; + } +} diff --git a/src/client/datascience/data-viewing/dataViewerDependencyService.ts b/src/client/datascience/data-viewing/dataViewerDependencyService.ts index 90bf35756a81..7e4230dc5981 100644 --- a/src/client/datascience/data-viewing/dataViewerDependencyService.ts +++ b/src/client/datascience/data-viewing/dataViewerDependencyService.ts @@ -14,6 +14,7 @@ import { IInstaller, InstallerResponse, Product } from '../../common/types'; import { Common, DataScience } from '../../common/utils/localize'; import { PythonEnvironment } from '../../pythonEnvironments/info'; import { sendTelemetryEvent } from '../../telemetry'; +import { parseSemVer } from '../common'; import { Telemetry } from '../constants'; const minimumSupportedPandaVersion = '0.20.0'; @@ -104,13 +105,8 @@ export class DataViewerDependencyService { throwOnStdErr: true, token }); - const versionMatch = /^\s*(\d+)\.(\d+)\.(.+)\s*$/.exec(result.stdout); - if (versionMatch && versionMatch.length > 2) { - const major = parseInt(versionMatch[1], 10); - const minor = parseInt(versionMatch[2], 10); - const build = parseInt(versionMatch[3], 10); - return parse(`${major}.${minor}.${build}`, true) ?? undefined; - } + + return parseSemVer(result.stdout); } catch (ex) { traceWarning('Failed to get version of Pandas to use Data Viewer', ex); return; diff --git a/src/client/datascience/jupyter/interpreter/jupyterInterpreterDependencyService.ts b/src/client/datascience/jupyter/interpreter/jupyterInterpreterDependencyService.ts index 1ce06b6481d4..8d98600115c0 100644 --- a/src/client/datascience/jupyter/interpreter/jupyterInterpreterDependencyService.ts +++ b/src/client/datascience/jupyter/interpreter/jupyterInterpreterDependencyService.ts @@ -15,6 +15,7 @@ import { Common, DataScience } from '../../../common/utils/localize'; import { noop } from '../../../common/utils/misc'; import { PythonEnvironment } from '../../../pythonEnvironments/info'; import { sendTelemetryEvent } from '../../../telemetry'; +import { parseSemVer } from '../../common'; import { HelpLinks, JupyterCommands, Telemetry } from '../../constants'; import { reportAction } from '../../progress/decorator'; import { ReportableAction } from '../../progress/types'; @@ -257,15 +258,16 @@ export class JupyterInterpreterDependencyService { const result = await command.exec(['--version'], { throwOnStdErr: true }); + return parseSemVer(result.stdout); // IANHU: Shared code with dataViewerDependencyService.ts for Pandas // IANHU: Helper function and unit test? - const versionMatch = /^\s*(\d+)\.(\d+)\.(.+)\s*$/.exec(result.stdout); - if (versionMatch && versionMatch.length > 2) { - const major = parseInt(versionMatch[1], 10); - const minor = parseInt(versionMatch[2], 10); - const build = parseInt(versionMatch[3], 10); - return parse(`${major}.${minor}.${build}`, true) ?? undefined; - } + //const versionMatch = /^\s*(\d+)\.(\d+)\.(.+)\s*$/.exec(result.stdout); + //if (versionMatch && versionMatch.length > 2) { + //const major = parseInt(versionMatch[1], 10); + //const minor = parseInt(versionMatch[2], 10); + //const build = parseInt(versionMatch[3], 10); + //return parse(`${major}.${minor}.${build}`, true) ?? undefined; + //} } /** diff --git a/src/client/datascience/jupyter/interpreter/jupyterInterpreterSubCommandExecutionService.ts b/src/client/datascience/jupyter/interpreter/jupyterInterpreterSubCommandExecutionService.ts index f98bc9d2502f..df7689addcff 100644 --- a/src/client/datascience/jupyter/interpreter/jupyterInterpreterSubCommandExecutionService.ts +++ b/src/client/datascience/jupyter/interpreter/jupyterInterpreterSubCommandExecutionService.ts @@ -183,6 +183,7 @@ export class JupyterInterpreterSubCommandExecutionService const args = template ? [file.fsPath, '--to', 'python', '--stdout', '--template', template] : [file.fsPath, '--to', 'python', '--stdout']; + // Ignore stderr, as nbconvert writes conversion result to stderr. // stdout contains the generated python code. return daemon diff --git a/src/client/datascience/jupyter/jupyterImporter.ts b/src/client/datascience/jupyter/jupyterImporter.ts index e662dcdef38d..070fb825e5eb 100644 --- a/src/client/datascience/jupyter/jupyterImporter.ts +++ b/src/client/datascience/jupyter/jupyterImporter.ts @@ -40,7 +40,7 @@ export class JupyterImporter implements INotebookImporter { {% endblock markdowncell %}`; private readonly nbconvert6TemplateFormat = // tslint:disable-next-line:no-multiline-string - `{%- extends 'null.j2' -%} + `{%- extends 'base/null.j2' -%} {% block codecell %} {0} {{ super() }} From 2c13198a7029d99e5b1bac570ad5690f9f61049f Mon Sep 17 00:00:00 2001 From: Ian Huff Date: Tue, 29 Sep 2020 20:48:52 -0700 Subject: [PATCH 04/14] refactor tpl file some --- .../datascience/jupyter/jupyterImporter.ts | 39 +++++++++---------- 1 file changed, 19 insertions(+), 20 deletions(-) diff --git a/src/client/datascience/jupyter/jupyterImporter.ts b/src/client/datascience/jupyter/jupyterImporter.ts index 070fb825e5eb..a144b232dc98 100644 --- a/src/client/datascience/jupyter/jupyterImporter.ts +++ b/src/client/datascience/jupyter/jupyterImporter.ts @@ -26,11 +26,11 @@ import { export class JupyterImporter implements INotebookImporter { public isDisposed: boolean = false; // Template that changes markdown cells to have # %% [markdown] in the comments - private readonly nbconvert5TemplateFormat = + private readonly nbconvertBaseTemplateFormat = // tslint:disable-next-line:no-multiline-string - `{%- extends 'null.tpl' -%} + `{%- extends '{0}' -%} {% block codecell %} -{0} +{1} {{ super() }} {% endblock codecell %} {% block in_prompt %}{% endblock in_prompt %} @@ -38,19 +38,8 @@ export class JupyterImporter implements INotebookImporter { {% block markdowncell scoped %}{0} [markdown] {{ cell.source | comment_lines }} {% endblock markdowncell %}`; - private readonly nbconvert6TemplateFormat = - // tslint:disable-next-line:no-multiline-string - `{%- extends 'base/null.j2' -%} -{% block codecell %} -{0} -{{ super() }} -{% endblock codecell %} -{% block in_prompt %}{% endblock in_prompt %} -{% block input %}{{ cell.source | ipython2python }}{% endblock input %} -{% block markdowncell scoped %}{0} [markdown] -{{ cell.source | comment_lines }} -{% endblock markdowncell %}`; - + private readonly nbconvert5Null = 'null.tpl'; + private readonly nbconvert6Null = 'base/null.j2'; private template5Promise: Promise; private template6Promise: Promise; @@ -64,8 +53,12 @@ export class JupyterImporter implements INotebookImporter { @inject(IJupyterInterpreterDependencyManager) private readonly dependencyManager: IJupyterInterpreterDependencyManager ) { - this.template5Promise = this.createTemplateFile(this.nbconvert5TemplateFormat); - this.template6Promise = this.createTemplateFile(this.nbconvert6TemplateFormat); + //this.template5Promise = this.createTemplateFile(this.nbconvert5TemplateFormat); + //this.template6Promise = this.createTemplateFile(this.nbconvert6TemplateFormat); + + // Create a template file for both nbconvert 5 and 6 in case user updates or changes python interpreter + this.template5Promise = this.createTemplateFile(false); + this.template6Promise = this.createTemplateFile(true); } public async importFromFile(sourceFile: Uri): Promise { @@ -173,7 +166,7 @@ export class JupyterImporter implements INotebookImporter { } } - private async createTemplateFile(baseTemplate: string): Promise { + private async createTemplateFile(nbconvert6: boolean): Promise { // Create a temp file on disk const file = await this.fs.createTemporaryLocalFile('.tpl'); @@ -182,7 +175,13 @@ export class JupyterImporter implements INotebookImporter { try { // Save this file into our disposables so the temp file goes away this.disposableRegistry.push(file); - await this.fs.appendLocalFile(file.filePath, baseTemplate.format(this.defaultCellMarker)); + await this.fs.appendLocalFile( + file.filePath, + this.nbconvertBaseTemplateFormat.format( + nbconvert6 ? this.nbconvert6Null : this.nbconvert5Null, + this.defaultCellMarker + ) + ); // Now we should have a template that will convert return file.filePath; From 7d28992bdab332c6ac5a85edd879aedec02b3ced Mon Sep 17 00:00:00 2001 From: Ian Huff Date: Wed, 30 Sep 2020 03:41:06 -0700 Subject: [PATCH 05/14] cleanup for review --- .../data-viewing/dataViewerDependencyService.ts | 2 +- .../jupyterInterpreterDependencyService.ts | 12 +----------- ...upyterInterpreterSubCommandExecutionService.ts | 4 +--- .../jupyter/liveshare/guestJupyterExecution.ts | 15 --------------- 4 files changed, 3 insertions(+), 30 deletions(-) diff --git a/src/client/datascience/data-viewing/dataViewerDependencyService.ts b/src/client/datascience/data-viewing/dataViewerDependencyService.ts index 7e4230dc5981..67e2dc1d1224 100644 --- a/src/client/datascience/data-viewing/dataViewerDependencyService.ts +++ b/src/client/datascience/data-viewing/dataViewerDependencyService.ts @@ -4,7 +4,7 @@ 'use strict'; import { inject, injectable } from 'inversify'; -import { parse, SemVer } from 'semver'; +import { SemVer } from 'semver'; import { CancellationToken } from 'vscode'; import { IApplicationShell } from '../../common/application/types'; import { Cancellation, createPromiseFromCancellation, wrapCancellationTokens } from '../../common/cancellation'; diff --git a/src/client/datascience/jupyter/interpreter/jupyterInterpreterDependencyService.ts b/src/client/datascience/jupyter/interpreter/jupyterInterpreterDependencyService.ts index 8d98600115c0..c93626f704d1 100644 --- a/src/client/datascience/jupyter/interpreter/jupyterInterpreterDependencyService.ts +++ b/src/client/datascience/jupyter/interpreter/jupyterInterpreterDependencyService.ts @@ -4,7 +4,7 @@ 'use strict'; import { inject, injectable } from 'inversify'; -import { parse, SemVer } from 'semver'; +import { SemVer } from 'semver'; import { CancellationToken } from 'vscode'; import { IApplicationShell } from '../../../common/application/types'; import { Cancellation, createPromiseFromCancellation, wrapCancellationTokens } from '../../../common/cancellation'; @@ -243,7 +243,6 @@ export class JupyterInterpreterDependencyService { return installed; } - // IANHU: Combine with kernelspec version code? public async getNbConvertVersion( interpreter: PythonEnvironment, _token?: CancellationToken @@ -259,15 +258,6 @@ export class JupyterInterpreterDependencyService { const result = await command.exec(['--version'], { throwOnStdErr: true }); return parseSemVer(result.stdout); - // IANHU: Shared code with dataViewerDependencyService.ts for Pandas - // IANHU: Helper function and unit test? - //const versionMatch = /^\s*(\d+)\.(\d+)\.(.+)\s*$/.exec(result.stdout); - //if (versionMatch && versionMatch.length > 2) { - //const major = parseInt(versionMatch[1], 10); - //const minor = parseInt(versionMatch[2], 10); - //const build = parseInt(versionMatch[3], 10); - //return parse(`${major}.${minor}.${build}`, true) ?? undefined; - //} } /** diff --git a/src/client/datascience/jupyter/interpreter/jupyterInterpreterSubCommandExecutionService.ts b/src/client/datascience/jupyter/interpreter/jupyterInterpreterSubCommandExecutionService.ts index df7689addcff..491e9897b1c9 100644 --- a/src/client/datascience/jupyter/interpreter/jupyterInterpreterSubCommandExecutionService.ts +++ b/src/client/datascience/jupyter/interpreter/jupyterInterpreterSubCommandExecutionService.ts @@ -77,18 +77,16 @@ export class JupyterInterpreterSubCommandExecutionService } return this.jupyterDependencyService.areDependenciesInstalled(interpreter, token); } - //public async isExportSupported(token?: CancellationToken): Promise { public async isExportSupported(token?: CancellationToken): Promise { const interpreter = await this.jupyterInterpreter.getSelectedInterpreter(token); if (!interpreter) { return; } + // If nbconvert is there check and return the version if (await this.jupyterDependencyService.isExportSupported(interpreter, token)) { return this.jupyterDependencyService.getNbConvertVersion(interpreter, token); } - - // return this.jupyterDependencyService.isExportSupported(interpreter, token); } public async getReasonForJupyterNotebookNotBeingSupported(token?: CancellationToken): Promise { let interpreter = await this.jupyterInterpreter.getSelectedInterpreter(token); diff --git a/src/client/datascience/jupyter/liveshare/guestJupyterExecution.ts b/src/client/datascience/jupyter/liveshare/guestJupyterExecution.ts index f4abb5c2b895..e18c4aa91019 100644 --- a/src/client/datascience/jupyter/liveshare/guestJupyterExecution.ts +++ b/src/client/datascience/jupyter/liveshare/guestJupyterExecution.ts @@ -85,7 +85,6 @@ export class GuestJupyterExecution extends LiveShareParticipantGuest( return false; } public async isImportSupported(cancelToken?: CancellationToken): Promise { - //return this.checkSupported(LiveShareCommands.isImportSupported, cancelToken); const service = await this.waitForService(); // Make a remote call on the proxy @@ -96,8 +95,6 @@ export class GuestJupyterExecution extends LiveShareParticipantGuest( return result as SemVer; } } - - return; } public isSpawnSupported(_cancelToken?: CancellationToken): Promise { return Promise.resolve(false); @@ -166,16 +163,4 @@ export class GuestJupyterExecution extends LiveShareParticipantGuest( public async getServer(options?: INotebookServerOptions): Promise { return this.serverCache.get(options); } - - //private async checkSupported(command: string, cancelToken?: CancellationToken): Promise { - //const service = await this.waitForService(); - - //// Make a remote call on the proxy - //if (service) { - //const result = await service.request(command, [], cancelToken); - //return result as boolean; - //} - - //return false; - //} } From f096cf5649584214966084de050e4d1ce0ed33b4 Mon Sep 17 00:00:00 2001 From: Ian Huff Date: Wed, 30 Sep 2020 04:01:40 -0700 Subject: [PATCH 06/14] importer cleanup and remove test code --- .vscode/launch.json | 3 -- .../datascience/jupyter/jupyterImporter.ts | 37 ++++++++++--------- .../datascience/notebook.functional.test.ts | 2 +- 3 files changed, 21 insertions(+), 21 deletions(-) diff --git a/.vscode/launch.json b/.vscode/launch.json index 65adfd5c46ff..4eab7ac182a7 100644 --- a/.vscode/launch.json +++ b/.vscode/launch.json @@ -250,7 +250,6 @@ "--recursive", "--colors", // "--grep", "", - "--grep", "IANHU", "--timeout=300000", "--exit" ], @@ -259,12 +258,10 @@ "XVSC_PYTHON_DS_UI_BROWSER": "1", // Remove `X` prefix to test with real python (for DS functional tests). "XVSCODE_PYTHON_ROLLING": "1", - "VSCODE_PYTHON_ROLLING": "1", // Remove 'X' to turn on all logging in the debug output "XVSC_PYTHON_FORCE_LOGGING": "1", // Remove `X` prefix and update path to test with real python interpreter (for DS functional tests). "XCI_PYTHON_PATH": "", - "CI_PYTHON_PATH": "/Users/ianhuff/opt/miniconda3/envs/functionalTestEnv/bin/python", // Remove 'X' prefix to dump output for debugger. Directory has to exist prior to launch "XDEBUGPY_LOG_DIR": "${workspaceRoot}/tmp/Debug_Output" }, diff --git a/src/client/datascience/jupyter/jupyterImporter.ts b/src/client/datascience/jupyter/jupyterImporter.ts index a144b232dc98..e38da8c0e553 100644 --- a/src/client/datascience/jupyter/jupyterImporter.ts +++ b/src/client/datascience/jupyter/jupyterImporter.ts @@ -40,8 +40,8 @@ export class JupyterImporter implements INotebookImporter { {% endblock markdowncell %}`; private readonly nbconvert5Null = 'null.tpl'; private readonly nbconvert6Null = 'base/null.j2'; - private template5Promise: Promise; - private template6Promise: Promise; + private template5Promise?: Promise; + private template6Promise?: Promise; constructor( @inject(IDataScienceFileSystem) private fs: IDataScienceFileSystem, @@ -52,19 +52,9 @@ export class JupyterImporter implements INotebookImporter { @inject(IPlatformService) private readonly platform: IPlatformService, @inject(IJupyterInterpreterDependencyManager) private readonly dependencyManager: IJupyterInterpreterDependencyManager - ) { - //this.template5Promise = this.createTemplateFile(this.nbconvert5TemplateFormat); - //this.template6Promise = this.createTemplateFile(this.nbconvert6TemplateFormat); - - // Create a template file for both nbconvert 5 and 6 in case user updates or changes python interpreter - this.template5Promise = this.createTemplateFile(false); - this.template6Promise = this.createTemplateFile(true); - } + ) {} public async importFromFile(sourceFile: Uri): Promise { - const template5 = await this.template5Promise; - const template6 = await this.template6Promise; - // If the user has requested it, add a cd command to the imported file so that relative paths still work const settings = this.configuration.getSettings(); let directoryChange: string | undefined; @@ -81,10 +71,23 @@ export class JupyterImporter implements INotebookImporter { // Use the jupyter nbconvert functionality to turn the notebook into a python file if (nbConvertVersion) { // nbconvert 5 and 6 use a different base template file - let fileOutput: string = await this.jupyterExecution.importNotebook( - sourceFile, - nbConvertVersion.major >= 6 ? template6 : template5 - ); + // Create and select the correct one + let template: string | undefined; + if (nbConvertVersion.major >= 6) { + if (!this.template6Promise) { + this.template6Promise = this.createTemplateFile(true); + } + + template = await this.template6Promise; + } else { + if (!this.template5Promise) { + this.template5Promise = this.createTemplateFile(false); + } + + template = await this.template5Promise; + } + + let fileOutput: string = await this.jupyterExecution.importNotebook(sourceFile, template); if (fileOutput.includes('get_ipython()')) { fileOutput = this.addIPythonImport(fileOutput); } diff --git a/src/test/datascience/notebook.functional.test.ts b/src/test/datascience/notebook.functional.test.ts index f0247f9bda60..aeec51594400 100644 --- a/src/test/datascience/notebook.functional.test.ts +++ b/src/test/datascience/notebook.functional.test.ts @@ -626,7 +626,7 @@ suite('DataScience notebook tests', () => { } }); - runTest('IANHU Export/Import', async () => { + runTest('Export/Import', async () => { // Get a bunch of test cells (use our test cells from the react controls) const testFolderPath = path.join(EXTENSION_ROOT_DIR, 'src', 'test', 'datascience'); const testState = generateTestState(testFolderPath); From f410e2c31c93e64172f28bd3de2793e62b69f759 Mon Sep 17 00:00:00 2001 From: Ian Huff Date: Wed, 30 Sep 2020 04:02:40 -0700 Subject: [PATCH 07/14] revert settings.json accidental change --- .vscode/settings.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.vscode/settings.json b/.vscode/settings.json index 9f729bd8127e..b2dc13efe26e 100644 --- a/.vscode/settings.json +++ b/.vscode/settings.json @@ -45,7 +45,7 @@ "source.fixAll.eslint": true, "source.fixAll.tslint": true }, - "python.languageServer": "Pylance", + "python.languageServer": "Microsoft", "python.analysis.logLevel": "Trace", "python.analysis.downloadChannel": "beta", "python.linting.pylintEnabled": false, From c37703425c4cbfdac99076b5a0fe4a9a03d10738 Mon Sep 17 00:00:00 2001 From: Ian Huff Date: Wed, 30 Sep 2020 04:06:03 -0700 Subject: [PATCH 08/14] remove commented out code --- .../datascience/jupyter/liveshare/guestJupyterExecution.ts | 1 - src/client/datascience/types.ts | 6 ------ 2 files changed, 7 deletions(-) diff --git a/src/client/datascience/jupyter/liveshare/guestJupyterExecution.ts b/src/client/datascience/jupyter/liveshare/guestJupyterExecution.ts index e18c4aa91019..d74d1499faf1 100644 --- a/src/client/datascience/jupyter/liveshare/guestJupyterExecution.ts +++ b/src/client/datascience/jupyter/liveshare/guestJupyterExecution.ts @@ -73,7 +73,6 @@ export class GuestJupyterExecution extends LiveShareParticipantGuest( } public async isNotebookSupported(cancelToken?: CancellationToken): Promise { - //return this.checkSupported(LiveShareCommands.isNotebookSupported, cancelToken); const service = await this.waitForService(); // Make a remote call on the proxy diff --git a/src/client/datascience/types.ts b/src/client/datascience/types.ts index 8f68474dbad5..14a240c6e844 100644 --- a/src/client/datascience/types.ts +++ b/src/client/datascience/types.ts @@ -283,7 +283,6 @@ export const IJupyterExecution = Symbol('IJupyterExecution'); export interface IJupyterExecution extends IAsyncDisposable { serverStarted: Event; isNotebookSupported(cancelToken?: CancellationToken): Promise; - //isImportSupported(cancelToken?: CancellationToken): Promise; isImportSupported(cancelToken?: CancellationToken): Promise; isSpawnSupported(cancelToken?: CancellationToken): Promise; connectToNotebookServer( @@ -998,12 +997,7 @@ export interface IJupyterSubCommandExecutionService { * Checks whether exporting of ipynb is supported. * If exporting is supported return the version of nbconvert available * otherwise undefined. - * - * @param {CancellationToken} [cancelToken] - * @returns {Promise} - * @memberof IJupyterSubCommandExecutionService */ - //isExportSupported(cancelToken?: CancellationToken): Promise; isExportSupported(cancelToken?: CancellationToken): Promise; /** * Error message indicating why jupyter notebook isn't supported. From 9490d4453da2ea3c3a6441d221826bbae22d3088 Mon Sep 17 00:00:00 2001 From: Ian Huff Date: Wed, 30 Sep 2020 04:08:02 -0700 Subject: [PATCH 09/14] add news --- news/2 Fixes/14169.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 news/2 Fixes/14169.md diff --git a/news/2 Fixes/14169.md b/news/2 Fixes/14169.md new file mode 100644 index 000000000000..5588237a9faf --- /dev/null +++ b/news/2 Fixes/14169.md @@ -0,0 +1 @@ +Support nbconvert version 6+ for exporting notebooks to python code. \ No newline at end of file From 08d52e4473c056f090220080591b0a0c58c420ee Mon Sep 17 00:00:00 2001 From: Ian Huff Date: Wed, 30 Sep 2020 04:45:20 -0700 Subject: [PATCH 10/14] unit test updates --- .vscode/launch.json | 1 + src/test/datascience/execution.unit.test.ts | 14 ++++++++++++-- ...erpreterSubCommandExecutionService.unit.test.ts | 2 +- 3 files changed, 14 insertions(+), 3 deletions(-) diff --git a/.vscode/launch.json b/.vscode/launch.json index 4eab7ac182a7..d09519b3edcc 100644 --- a/.vscode/launch.json +++ b/.vscode/launch.json @@ -209,6 +209,7 @@ "--recursive", "--colors", //"--grep", "", + "--grep", "IANHU", "--timeout=300000" ], "outFiles": ["${workspaceFolder}/out/**/*.js", "!${workspaceFolder}/**/node_modules**/*"], diff --git a/src/test/datascience/execution.unit.test.ts b/src/test/datascience/execution.unit.test.ts index 78203130f51f..cdeffc0f6c53 100644 --- a/src/test/datascience/execution.unit.test.ts +++ b/src/test/datascience/execution.unit.test.ts @@ -1000,6 +1000,14 @@ suite('Jupyter Execution', async () => { return []; } ); + when(dependencyService.getNbConvertVersion(anything(), anything())).thenCall( + async (interpreter: PythonEnvironment) => { + if (interpreter === missingNotebookPython) { + return undefined; + } + return new SemVer('1.1.1'); + } + ); const oldStore = mock(JupyterInterpreterOldCacheStateStore); when(oldStore.getCachedInterpreterPath()).thenReturn(); const jupyterInterpreterService = mock(JupyterInterpreterService); @@ -1047,7 +1055,8 @@ suite('Jupyter Execution', async () => { const jupyterExecutionFactory = createExecution(workingPython); await assert.eventually.equal(jupyterExecutionFactory.isNotebookSupported(), true, 'Notebook not supported'); - await assert.eventually.equal(jupyterExecutionFactory.isImportSupported(), true, 'Import not supported'); + const nbConvertVer = await jupyterExecutionFactory.isImportSupported(); + assert.isTrue(nbConvertVer?.compare('1.1.1') === 0); const usableInterpreter = await jupyterExecutionFactory.getUsableJupyterPython(); assert.isOk(usableInterpreter, 'Usable interpreter not found'); await assert.isFulfilled(jupyterExecutionFactory.connectToNotebookServer(), 'Should be able to start a server'); @@ -1062,7 +1071,8 @@ suite('Jupyter Execution', async () => { ); await assert.eventually.equal(jupyterExecutionFactory.isNotebookSupported(), true, 'Notebook not supported'); - await assert.eventually.equal(jupyterExecutionFactory.isImportSupported(), true, 'Import not supported'); + const nbConvertVer = await jupyterExecutionFactory.isImportSupported(); + assert.isTrue(nbConvertVer?.compare('1.1.1') === 0); const usableInterpreter = await jupyterExecutionFactory.getUsableJupyterPython(); assert.isOk(usableInterpreter, 'Usable interpreter not found'); await assert.isFulfilled(jupyterExecutionFactory.connectToNotebookServer(), 'Should be able to start a server'); diff --git a/src/test/datascience/jupyter/interpreter/jupyterInterpreterSubCommandExecutionService.unit.test.ts b/src/test/datascience/jupyter/interpreter/jupyterInterpreterSubCommandExecutionService.unit.test.ts index a95b8b8f79a4..19b076df0de4 100644 --- a/src/test/datascience/jupyter/interpreter/jupyterInterpreterSubCommandExecutionService.unit.test.ts +++ b/src/test/datascience/jupyter/interpreter/jupyterInterpreterSubCommandExecutionService.unit.test.ts @@ -102,7 +102,7 @@ suite('DataScience - Jupyter InterpreterSubCommandExecutionService', () => { }); test('Export is not supported', async () => { const isSupported = await jupyterInterpreterExecutionService.isExportSupported(undefined); - assert.isFalse(isSupported); + assert.isUndefined(isSupported); }); test('Jupyter cannot be started because no interpreter has been selected', async () => { when(interperterService.getActiveInterpreter(undefined)).thenResolve(undefined); From 83ff6e992ac19df993a6f35af8e0fbcefa6bfdf5 Mon Sep 17 00:00:00 2001 From: Ian Huff Date: Wed, 30 Sep 2020 04:46:57 -0700 Subject: [PATCH 11/14] revert comment --- .vscode/launch.json | 1 - 1 file changed, 1 deletion(-) diff --git a/.vscode/launch.json b/.vscode/launch.json index d09519b3edcc..4eab7ac182a7 100644 --- a/.vscode/launch.json +++ b/.vscode/launch.json @@ -209,7 +209,6 @@ "--recursive", "--colors", //"--grep", "", - "--grep", "IANHU", "--timeout=300000" ], "outFiles": ["${workspaceFolder}/out/**/*.js", "!${workspaceFolder}/**/node_modules**/*"], From d7e98d36acdb06c9a31cf09e928628f26b70c451 Mon Sep 17 00:00:00 2001 From: Ian Huff Date: Wed, 30 Sep 2020 13:22:48 -0700 Subject: [PATCH 12/14] IJupyterExecution getImportPackageVersion rename --- src/client/datascience/constants.ts | 2 +- src/client/datascience/export/exportDependencyChecker.ts | 4 ++-- src/client/datascience/jupyter/jupyterExecution.ts | 2 +- src/client/datascience/jupyter/jupyterExecutionFactory.ts | 4 ++-- src/client/datascience/jupyter/jupyterImporter.ts | 4 ++-- .../datascience/jupyter/liveshare/guestJupyterExecution.ts | 4 ++-- .../datascience/jupyter/liveshare/hostJupyterExecution.ts | 6 +++--- src/client/datascience/types.ts | 2 +- src/test/datascience/execution.unit.test.ts | 4 ++-- src/test/datascience/notebook.functional.test.ts | 2 +- 10 files changed, 17 insertions(+), 17 deletions(-) diff --git a/src/client/datascience/constants.ts b/src/client/datascience/constants.ts index f168c60438e5..ee17840a2c39 100644 --- a/src/client/datascience/constants.ts +++ b/src/client/datascience/constants.ts @@ -569,7 +569,7 @@ export namespace LiveShare { export namespace LiveShareCommands { export const isNotebookSupported = 'isNotebookSupported'; - export const isImportSupported = 'isImportSupported'; + export const getImportPackageVersion = 'getImportPackageVersion'; export const connectToNotebookServer = 'connectToNotebookServer'; export const getUsableJupyterPython = 'getUsableJupyterPython'; export const executeObservable = 'executeObservable'; diff --git a/src/client/datascience/export/exportDependencyChecker.ts b/src/client/datascience/export/exportDependencyChecker.ts index 0ec028999e51..e357d63ed020 100644 --- a/src/client/datascience/export/exportDependencyChecker.ts +++ b/src/client/datascience/export/exportDependencyChecker.ts @@ -17,9 +17,9 @@ export class ExportDependencyChecker { // Before we try the import, see if we don't support it, if we don't give a chance to install dependencies const reporter = this.progressReporter.createProgressIndicator(`Exporting to ${format}`); try { - if (!(await this.jupyterExecution.isImportSupported())) { + if (!(await this.jupyterExecution.getImportPackageVersion())) { await this.dependencyManager.installMissingDependencies(); - if (!(await this.jupyterExecution.isImportSupported())) { + if (!(await this.jupyterExecution.getImportPackageVersion())) { throw new Error(localize.DataScience.jupyterNbConvertNotSupported()); } } diff --git a/src/client/datascience/jupyter/jupyterExecution.ts b/src/client/datascience/jupyter/jupyterExecution.ts index c580d0566f63..9bb1e7d48934 100644 --- a/src/client/datascience/jupyter/jupyterExecution.ts +++ b/src/client/datascience/jupyter/jupyterExecution.ts @@ -124,7 +124,7 @@ export class JupyterExecutionBase implements IJupyterExecution { } @reportAction(ReportableAction.CheckingIfImportIsSupported) - public async isImportSupported(cancelToken?: CancellationToken): Promise { + public async getImportPackageVersion(cancelToken?: CancellationToken): Promise { // See if we can find the command nbconvert return this.jupyterInterpreterService.isExportSupported(cancelToken); } diff --git a/src/client/datascience/jupyter/jupyterExecutionFactory.ts b/src/client/datascience/jupyter/jupyterExecutionFactory.ts index f71ffa29f980..a2d2b636e92b 100644 --- a/src/client/datascience/jupyter/jupyterExecutionFactory.ts +++ b/src/client/datascience/jupyter/jupyterExecutionFactory.ts @@ -118,9 +118,9 @@ export class JupyterExecutionFactory implements IJupyterExecution, IAsyncDisposa return execution.getNotebookError(); } - public async isImportSupported(cancelToken?: CancellationToken): Promise { + public async getImportPackageVersion(cancelToken?: CancellationToken): Promise { const execution = await this.executionFactory.get(); - return execution.isImportSupported(cancelToken); + return execution.getImportPackageVersion(cancelToken); } public async isSpawnSupported(cancelToken?: CancellationToken): Promise { const execution = await this.executionFactory.get(); diff --git a/src/client/datascience/jupyter/jupyterImporter.ts b/src/client/datascience/jupyter/jupyterImporter.ts index e38da8c0e553..eaecc62e2d9e 100644 --- a/src/client/datascience/jupyter/jupyterImporter.ts +++ b/src/client/datascience/jupyter/jupyterImporter.ts @@ -63,11 +63,11 @@ export class JupyterImporter implements INotebookImporter { } // Before we try the import, see if we don't support it, if we don't give a chance to install dependencies - if (!(await this.jupyterExecution.isImportSupported())) { + if (!(await this.jupyterExecution.getImportPackageVersion())) { await this.dependencyManager.installMissingDependencies(); } - const nbConvertVersion = await this.jupyterExecution.isImportSupported(); + const nbConvertVersion = await this.jupyterExecution.getImportPackageVersion(); // Use the jupyter nbconvert functionality to turn the notebook into a python file if (nbConvertVersion) { // nbconvert 5 and 6 use a different base template file diff --git a/src/client/datascience/jupyter/liveshare/guestJupyterExecution.ts b/src/client/datascience/jupyter/liveshare/guestJupyterExecution.ts index d74d1499faf1..3ebcf0606669 100644 --- a/src/client/datascience/jupyter/liveshare/guestJupyterExecution.ts +++ b/src/client/datascience/jupyter/liveshare/guestJupyterExecution.ts @@ -83,12 +83,12 @@ export class GuestJupyterExecution extends LiveShareParticipantGuest( return false; } - public async isImportSupported(cancelToken?: CancellationToken): Promise { + public async getImportPackageVersion(cancelToken?: CancellationToken): Promise { const service = await this.waitForService(); // Make a remote call on the proxy if (service) { - const result = await service.request(LiveShareCommands.isImportSupported, [], cancelToken); + const result = await service.request(LiveShareCommands.getImportPackageVersion, [], cancelToken); if (result) { return result as SemVer; diff --git a/src/client/datascience/jupyter/liveshare/hostJupyterExecution.ts b/src/client/datascience/jupyter/liveshare/hostJupyterExecution.ts index 422963c45ba7..72adb1dd8ff0 100644 --- a/src/client/datascience/jupyter/liveshare/hostJupyterExecution.ts +++ b/src/client/datascience/jupyter/liveshare/hostJupyterExecution.ts @@ -122,7 +122,7 @@ export class HostJupyterExecution // Register handlers for all of the supported remote calls if (service) { service.onRequest(LiveShareCommands.isNotebookSupported, this.onRemoteIsNotebookSupported); - service.onRequest(LiveShareCommands.isImportSupported, this.onRemoteIsImportSupported); + service.onRequest(LiveShareCommands.getImportPackageVersion, this.onRemoteGetImportPackageVersion); service.onRequest(LiveShareCommands.connectToNotebookServer, this.onRemoteConnectToNotebookServer); service.onRequest(LiveShareCommands.getUsableJupyterPython, this.onRemoteGetUsableJupyterPython); } @@ -153,9 +153,9 @@ export class HostJupyterExecution return this.isNotebookSupported(cancellation); }; - private onRemoteIsImportSupported = (_args: any[], cancellation: CancellationToken): Promise => { + private onRemoteGetImportPackageVersion = (_args: any[], cancellation: CancellationToken): Promise => { // Just call local - return this.isImportSupported(cancellation); + return this.getImportPackageVersion(cancellation); }; private onRemoteConnectToNotebookServer = async ( diff --git a/src/client/datascience/types.ts b/src/client/datascience/types.ts index 14a240c6e844..4dafce7c0de7 100644 --- a/src/client/datascience/types.ts +++ b/src/client/datascience/types.ts @@ -283,7 +283,7 @@ export const IJupyterExecution = Symbol('IJupyterExecution'); export interface IJupyterExecution extends IAsyncDisposable { serverStarted: Event; isNotebookSupported(cancelToken?: CancellationToken): Promise; - isImportSupported(cancelToken?: CancellationToken): Promise; + getImportPackageVersion(cancelToken?: CancellationToken): Promise; isSpawnSupported(cancelToken?: CancellationToken): Promise; connectToNotebookServer( options?: INotebookServerOptions, diff --git a/src/test/datascience/execution.unit.test.ts b/src/test/datascience/execution.unit.test.ts index cdeffc0f6c53..1d1c66e70442 100644 --- a/src/test/datascience/execution.unit.test.ts +++ b/src/test/datascience/execution.unit.test.ts @@ -1055,7 +1055,7 @@ suite('Jupyter Execution', async () => { const jupyterExecutionFactory = createExecution(workingPython); await assert.eventually.equal(jupyterExecutionFactory.isNotebookSupported(), true, 'Notebook not supported'); - const nbConvertVer = await jupyterExecutionFactory.isImportSupported(); + const nbConvertVer = await jupyterExecutionFactory.getImportPackageVersion(); assert.isTrue(nbConvertVer?.compare('1.1.1') === 0); const usableInterpreter = await jupyterExecutionFactory.getUsableJupyterPython(); assert.isOk(usableInterpreter, 'Usable interpreter not found'); @@ -1071,7 +1071,7 @@ suite('Jupyter Execution', async () => { ); await assert.eventually.equal(jupyterExecutionFactory.isNotebookSupported(), true, 'Notebook not supported'); - const nbConvertVer = await jupyterExecutionFactory.isImportSupported(); + const nbConvertVer = await jupyterExecutionFactory.getImportPackageVersion(); assert.isTrue(nbConvertVer?.compare('1.1.1') === 0); const usableInterpreter = await jupyterExecutionFactory.getUsableJupyterPython(); assert.isOk(usableInterpreter, 'Usable interpreter not found'); diff --git a/src/test/datascience/notebook.functional.test.ts b/src/test/datascience/notebook.functional.test.ts index aeec51594400..03d20a90d50e 100644 --- a/src/test/datascience/notebook.functional.test.ts +++ b/src/test/datascience/notebook.functional.test.ts @@ -875,7 +875,7 @@ suite('DataScience notebook tests', () => { ); assert.ok( await testCancelableMethod( - (t: CancellationToken) => jupyterExecution.isImportSupported(t), + (t: CancellationToken) => jupyterExecution.getImportPackageVersion(t), 'Cancel did not cancel isImport after {0}ms', true ) From 18a6a27310ac7d928c7c8998a734f294eb662724 Mon Sep 17 00:00:00 2001 From: Ian Huff Date: Wed, 30 Sep 2020 13:25:47 -0700 Subject: [PATCH 13/14] SubExecutionService getExportPackageVersion rename --- .../jupyterInterpreterSubCommandExecutionService.ts | 2 +- src/client/datascience/jupyter/jupyterExecution.ts | 2 +- src/client/datascience/types.ts | 3 +-- .../jupyterInterpreterSubCommandExecutionService.unit.test.ts | 2 +- 4 files changed, 4 insertions(+), 5 deletions(-) diff --git a/src/client/datascience/jupyter/interpreter/jupyterInterpreterSubCommandExecutionService.ts b/src/client/datascience/jupyter/interpreter/jupyterInterpreterSubCommandExecutionService.ts index 491e9897b1c9..8f9e6379130c 100644 --- a/src/client/datascience/jupyter/interpreter/jupyterInterpreterSubCommandExecutionService.ts +++ b/src/client/datascience/jupyter/interpreter/jupyterInterpreterSubCommandExecutionService.ts @@ -77,7 +77,7 @@ export class JupyterInterpreterSubCommandExecutionService } return this.jupyterDependencyService.areDependenciesInstalled(interpreter, token); } - public async isExportSupported(token?: CancellationToken): Promise { + public async getExportPackageVersion(token?: CancellationToken): Promise { const interpreter = await this.jupyterInterpreter.getSelectedInterpreter(token); if (!interpreter) { return; diff --git a/src/client/datascience/jupyter/jupyterExecution.ts b/src/client/datascience/jupyter/jupyterExecution.ts index 9bb1e7d48934..b0f3b495bf06 100644 --- a/src/client/datascience/jupyter/jupyterExecution.ts +++ b/src/client/datascience/jupyter/jupyterExecution.ts @@ -126,7 +126,7 @@ export class JupyterExecutionBase implements IJupyterExecution { @reportAction(ReportableAction.CheckingIfImportIsSupported) public async getImportPackageVersion(cancelToken?: CancellationToken): Promise { // See if we can find the command nbconvert - return this.jupyterInterpreterService.isExportSupported(cancelToken); + return this.jupyterInterpreterService.getExportPackageVersion(cancelToken); } public isSpawnSupported(cancelToken?: CancellationToken): Promise { diff --git a/src/client/datascience/types.ts b/src/client/datascience/types.ts index 4dafce7c0de7..e97bcada4539 100644 --- a/src/client/datascience/types.ts +++ b/src/client/datascience/types.ts @@ -994,11 +994,10 @@ export interface IJupyterSubCommandExecutionService { */ isNotebookSupported(cancelToken?: CancellationToken): Promise; /** - * Checks whether exporting of ipynb is supported. * If exporting is supported return the version of nbconvert available * otherwise undefined. */ - isExportSupported(cancelToken?: CancellationToken): Promise; + getExportPackageVersion(cancelToken?: CancellationToken): Promise; /** * Error message indicating why jupyter notebook isn't supported. * diff --git a/src/test/datascience/jupyter/interpreter/jupyterInterpreterSubCommandExecutionService.unit.test.ts b/src/test/datascience/jupyter/interpreter/jupyterInterpreterSubCommandExecutionService.unit.test.ts index 19b076df0de4..e6704bd35fb4 100644 --- a/src/test/datascience/jupyter/interpreter/jupyterInterpreterSubCommandExecutionService.unit.test.ts +++ b/src/test/datascience/jupyter/interpreter/jupyterInterpreterSubCommandExecutionService.unit.test.ts @@ -101,7 +101,7 @@ suite('DataScience - Jupyter InterpreterSubCommandExecutionService', () => { assert.isFalse(isSupported); }); test('Export is not supported', async () => { - const isSupported = await jupyterInterpreterExecutionService.isExportSupported(undefined); + const isSupported = await jupyterInterpreterExecutionService.getExportPackageVersion(undefined); assert.isUndefined(isSupported); }); test('Jupyter cannot be started because no interpreter has been selected', async () => { From 144434674371c39cf0a3297667f598fa84dd7a05 Mon Sep 17 00:00:00 2001 From: Ian Huff Date: Wed, 30 Sep 2020 13:46:53 -0700 Subject: [PATCH 14/14] surface an error if converted file is zero size --- package.nls.json | 2 +- src/client/common/utils/localize.ts | 2 +- .../jupyterInterpreterSubCommandExecutionService.ts | 11 ++++++++++- 3 files changed, 12 insertions(+), 3 deletions(-) diff --git a/package.nls.json b/package.nls.json index b1b7007f43d8..d916bd2600e3 100644 --- a/package.nls.json +++ b/package.nls.json @@ -31,7 +31,7 @@ "DataScience.openExportFileYes": "Yes", "DataScience.openExportFileNo": "No", "DataScience.failedExportMessage": "Export failed.", - "DataScience.exportFailedGeneralMessage": "Export failed. Please check the 'Python' [output](command:python.viewOutput) panel for further details.", + "DataScience.exportFailedGeneralMessage": "Please check the 'Python' [output](command:python.viewOutput) panel for further details.", "DataScience.exportToPDFDependencyMessage": "If you have not installed xelatex (TeX) you will need to do so before you can export to PDF, for further instructions go to https://nbconvert.readthedocs.io/en/latest/install.html#installing-tex. \r\nTo avoid installing xelatex (TeX) you might want to try exporting to HTML and using your browsers \"Print to PDF\" feature.", "DataScience.launchNotebookTrustPrompt": "A notebook could execute harmful code when opened. Some outputs have been hidden. Do you trust this notebook? [Learn more.](https://aka.ms/trusted-notebooks)", "DataScience.launchNotebookTrustPrompt.yes": "Trust", diff --git a/src/client/common/utils/localize.ts b/src/client/common/utils/localize.ts index 4a4981f8ae63..089cb95d873e 100644 --- a/src/client/common/utils/localize.ts +++ b/src/client/common/utils/localize.ts @@ -892,7 +892,7 @@ export namespace DataScience { export const openExportFileNo = localize('DataScience.openExportFileNo', 'No'); export const exportFailedGeneralMessage = localize( 'DataScience.exportFailedGeneralMessage', - `Export failed. Please check the 'Python' [output](command:python.viewOutput) panel for further details.` + `Please check the 'Python' [output](command:python.viewOutput) panel for further details.` ); export const exportToPDFDependencyMessage = localize( 'DataScience.exportToPDFDependencyMessage', diff --git a/src/client/datascience/jupyter/interpreter/jupyterInterpreterSubCommandExecutionService.ts b/src/client/datascience/jupyter/interpreter/jupyterInterpreterSubCommandExecutionService.ts index 8f9e6379130c..905cd6eb42bc 100644 --- a/src/client/datascience/jupyter/interpreter/jupyterInterpreterSubCommandExecutionService.ts +++ b/src/client/datascience/jupyter/interpreter/jupyterInterpreterSubCommandExecutionService.ts @@ -186,7 +186,16 @@ export class JupyterInterpreterSubCommandExecutionService // stdout contains the generated python code. return daemon .execModule('jupyter', ['nbconvert'].concat(args), { throwOnStdErr: false, encoding: 'utf8', token }) - .then((output) => output.stdout); + .then((output) => { + // We can't check stderr (as nbconvert puts diag output there) but we need to verify here that we actually + // converted something. If it's zero size then just raise an error + if (output.stdout === '') { + traceError('nbconvert zero size output'); + throw new Error(output.stderr); + } else { + return output.stdout; + } + }); } public async openNotebook(notebookFile: string): Promise { const interpreter = await this.getSelectedInterpreterAndThrowIfNotAvailable();