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

NB Convert 6.0 support for export #14177

Merged
merged 14 commits into from
Sep 30, 2020
1 change: 1 addition & 0 deletions news/2 Fixes/14169.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Support nbconvert version 6+ for exporting notebooks to python code.
12 changes: 12 additions & 0 deletions src/client/datascience/common.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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;
}
}
2 changes: 1 addition & 1 deletion src/client/datascience/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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';
Expand Down Expand Up @@ -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;
Expand Down
4 changes: 2 additions & 2 deletions src/client/datascience/export/exportDependencyChecker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
'use strict';

import { inject, injectable } from 'inversify';
import { SemVer } from 'semver';
import { CancellationToken } from 'vscode';
import { IApplicationShell } from '../../../common/application/types';
import { Cancellation, createPromiseFromCancellation, wrapCancellationTokens } from '../../../common/cancellation';
Expand All @@ -14,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';
Expand Down Expand Up @@ -241,6 +243,23 @@ export class JupyterInterpreterDependencyService {
return installed;
}

public async getNbConvertVersion(
interpreter: PythonEnvironment,
_token?: CancellationToken
): Promise<SemVer | undefined> {
const command = this.commandFactory.createInterpreterCommand(
JupyterCommands.ConvertCommand,
'jupyter',
['-m', 'jupyter', 'nbconvert'],
interpreter,
false
);

const result = await command.exec(['--version'], { throwOnStdErr: true });

return parseSemVer(result.stdout);
}

/**
* Gets a list of the dependencies not installed, dependencies that are required to launch the jupyter notebook server.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -76,12 +77,16 @@ export class JupyterInterpreterSubCommandExecutionService
}
return this.jupyterDependencyService.areDependenciesInstalled(interpreter, token);
}
public async isExportSupported(token?: CancellationToken): Promise<boolean> {
public async getExportPackageVersion(token?: CancellationToken): Promise<SemVer | undefined> {
const interpreter = await this.jupyterInterpreter.getSelectedInterpreter(token);
if (!interpreter) {
return false;
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<string> {
let interpreter = await this.jupyterInterpreter.getSelectedInterpreter(token);
Expand Down Expand Up @@ -176,6 +181,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
IanMatthewHuff marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
5 changes: 3 additions & 2 deletions src/client/datascience/jupyter/jupyterExecution.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand Down Expand Up @@ -123,9 +124,9 @@ export class JupyterExecutionBase implements IJupyterExecution {
}

@reportAction(ReportableAction.CheckingIfImportIsSupported)
public async isImportSupported(cancelToken?: CancellationToken): Promise<boolean> {
public async getImportPackageVersion(cancelToken?: CancellationToken): Promise<SemVer | undefined> {
// See if we can find the command nbconvert
return this.jupyterInterpreterService.isExportSupported(cancelToken);
return this.jupyterInterpreterService.getExportPackageVersion(cancelToken);
}

public isSpawnSupported(cancelToken?: CancellationToken): Promise<boolean> {
Expand Down
5 changes: 3 additions & 2 deletions src/client/datascience/jupyter/jupyterExecutionFactory.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -117,9 +118,9 @@ export class JupyterExecutionFactory implements IJupyterExecution, IAsyncDisposa
return execution.getNotebookError();
}

public async isImportSupported(cancelToken?: CancellationToken): Promise<boolean> {
public async getImportPackageVersion(cancelToken?: CancellationToken): Promise<SemVer | undefined> {
const execution = await this.executionFactory.get();
return execution.isImportSupported(cancelToken);
return execution.getImportPackageVersion(cancelToken);
}
public async isSpawnSupported(cancelToken?: CancellationToken): Promise<boolean> {
const execution = await this.executionFactory.get();
Expand Down
47 changes: 33 additions & 14 deletions src/client/datascience/jupyter/jupyterImporter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,20 +26,22 @@ 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 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 %}
{% block input %}{{ cell.source | ipython2python }}{% endblock input %}
{% block markdowncell scoped %}{0} [markdown]
{{ cell.source | comment_lines }}
{% endblock markdowncell %}`;

private templatePromise: Promise<string | undefined>;
private readonly nbconvert5Null = 'null.tpl';
private readonly nbconvert6Null = 'base/null.j2';
private template5Promise?: Promise<string | undefined>;
private template6Promise?: Promise<string | undefined>;

constructor(
@inject(IDataScienceFileSystem) private fs: IDataScienceFileSystem,
Expand All @@ -50,13 +52,9 @@ export class JupyterImporter implements INotebookImporter {
@inject(IPlatformService) private readonly platform: IPlatformService,
@inject(IJupyterInterpreterDependencyManager)
private readonly dependencyManager: IJupyterInterpreterDependencyManager
) {
this.templatePromise = this.createTemplateFile();
}
) {}
Copy link
Member Author

Choose a reason for hiding this comment

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

I moved the construction of the template files to the point where they were needed instead of construction. Didn't see the point of slowing down activation for a feature that's probably not a common every day use scenario. Plus triggering an export is already a user action with an expected wait time, so seems fine to build the file then.


public async importFromFile(sourceFile: Uri): Promise<string> {
const template = await this.templatePromise;

// 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;
Expand All @@ -65,12 +63,30 @@ 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.getImportPackageVersion();
// Use the jupyter nbconvert functionality to turn the notebook into a python file
if (await this.jupyterExecution.isImportSupported()) {
if (nbConvertVersion) {
Copy link
Member Author

Choose a reason for hiding this comment

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

This could be a bit simpler if we didn't worry about users changing the jupyter interpreter while running. But I figured that might be a possibility. So I tested that this does support moving your Jupyter interpreter to one with nbconvert 5 to one that uses nbconvert 6 and back without restarting.

// nbconvert 5 and 6 use a different base template file
// 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);
IanMatthewHuff marked this conversation as resolved.
Show resolved Hide resolved
if (fileOutput.includes('get_ipython()')) {
fileOutput = this.addIPythonImport(fileOutput);
Expand Down Expand Up @@ -153,7 +169,7 @@ export class JupyterImporter implements INotebookImporter {
}
}

private async createTemplateFile(): Promise<string | undefined> {
private async createTemplateFile(nbconvert6: boolean): Promise<string | undefined> {
// Create a temp file on disk
const file = await this.fs.createTemporaryLocalFile('.tpl');

Expand All @@ -164,7 +180,10 @@ export class JupyterImporter implements INotebookImporter {
this.disposableRegistry.push(file);
await this.fs.appendLocalFile(
file.filePath,
this.nbconvertTemplateFormat.format(this.defaultCellMarker)
this.nbconvertBaseTemplateFormat.format(
nbconvert6 ? this.nbconvert6Null : this.nbconvert5Null,
this.defaultCellMarker
)
);

// Now we should have a template that will convert
Expand Down
36 changes: 21 additions & 15 deletions src/client/datascience/jupyter/liveshare/guestJupyterExecution.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand Down Expand Up @@ -72,10 +73,27 @@ export class GuestJupyterExecution extends LiveShareParticipantGuest(
}

public async isNotebookSupported(cancelToken?: CancellationToken): Promise<boolean> {
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<boolean> {
return this.checkSupported(LiveShareCommands.isImportSupported, cancelToken);
public async getImportPackageVersion(cancelToken?: CancellationToken): Promise<SemVer | undefined> {
const service = await this.waitForService();

// Make a remote call on the proxy
if (service) {
const result = await service.request(LiveShareCommands.getImportPackageVersion, [], cancelToken);

if (result) {
return result as SemVer;
}
}
}
public isSpawnSupported(_cancelToken?: CancellationToken): Promise<boolean> {
return Promise.resolve(false);
Expand Down Expand Up @@ -144,16 +162,4 @@ export class GuestJupyterExecution extends LiveShareParticipantGuest(
public async getServer(options?: INotebookServerOptions): Promise<INotebookServer | undefined> {
return this.serverCache.get(options);
}

private async checkSupported(command: string, cancelToken?: CancellationToken): Promise<boolean> {
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;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down Expand Up @@ -153,9 +153,9 @@ export class HostJupyterExecution
return this.isNotebookSupported(cancellation);
};

private onRemoteIsImportSupported = (_args: any[], cancellation: CancellationToken): Promise<any> => {
private onRemoteGetImportPackageVersion = (_args: any[], cancellation: CancellationToken): Promise<any> => {
// Just call local
return this.isImportSupported(cancellation);
return this.getImportPackageVersion(cancellation);
};

private onRemoteConnectToNotebookServer = async (
Expand Down
Loading