From af9d2bea0b3a235a4836f3bbca64d7220a43e484 Mon Sep 17 00:00:00 2001 From: Daniel Rodriguez Date: Mon, 18 Jul 2022 19:24:05 -0400 Subject: [PATCH 1/2] Refactoring based on https://github.com/microsoft/vscode-jupyter/pull/10825#discussion_r923809738 --- package.nls.json | 4 +- src/platform/common/utils/localize.ts | 4 +- .../dataViewerDependencyService.unit.test.ts | 4 +- ...ndencyServiceInterpreter.node.unit.test.ts | 10 +- ...rDependencyServiceKernel.node.unit.test.ts | 4 +- .../baseDataViewerDependencyImplementation.ts | 81 +++++++++++-- ...DataViewerDependencyImplementation.node.ts | 111 +++++------------- ...ernelDataViewerDependencyImplementation.ts | 61 ++-------- 8 files changed, 131 insertions(+), 148 deletions(-) diff --git a/package.nls.json b/package.nls.json index 034d2ef889e..296119706c1 100644 --- a/package.nls.json +++ b/package.nls.json @@ -673,8 +673,8 @@ }, "DataScience.serverNotStarted": "Not Started", "DataScience.localJupyterServer": "local", - "DataScience.pandasTooOldForViewingFormat": "Python package 'pandas' is version {0}. Version 0.20 or greater is required for viewing data.", - "DataScience.pandasRequiredForViewing": "Python package 'pandas' is required for viewing data. Please ensure you have installed 'pandas' version {0} or above.", + "DataScience.pandasTooOldForViewingFormat": "Python package 'pandas' is version {0}. Version {1} or greater is required for viewing data.", + "DataScience.pandasRequiredForViewing": "Python package 'pandas' version {0} (or above) is required for viewing data.", "DataScience.failedToGetVersionOfPandas": "Failed to get version of Pandas to use the Data Viewer.", "DataScience.valuesColumn": "values", "DataScience.liveShareInvalid": "One or more guests in the session do not have the Jupyter [extension](https://marketplace.visualstudio.com/itemdetails?itemName=ms-toolsai.jupyter) installed.\r\nYour Live Share session cannot continue and will be closed.", diff --git a/src/platform/common/utils/localize.ts b/src/platform/common/utils/localize.ts index 5d1f29bd72c..d604bed5428 100644 --- a/src/platform/common/utils/localize.ts +++ b/src/platform/common/utils/localize.ts @@ -800,7 +800,7 @@ export namespace DataScience { key: 'DataScience.pandasTooOldForViewingFormat', comment: ["{Locked='pandas'", 'This is the name of the pandas package'] }, - "Python package 'pandas' is version {0}. Version 0.20 or greater is required for viewing data." + "Python package 'pandas' is version {0}. Version {1} or greater is required for viewing data." ); export const pandasRequiredForViewing = () => localize( @@ -808,7 +808,7 @@ export namespace DataScience { key: 'DataScience.pandasRequiredForViewing', comment: ["{Locked='pandas'", 'This is the name of the pandas package'] }, - "Python package 'pandas' is required for viewing data. Please ensure you have installed 'pandas' version {0} or above." + "Python package 'pandas' version {0} (or above) is required for viewing data." ); export const valuesColumn = () => localize('DataScience.valuesColumn', 'values'); export const liveShareInvalid = () => diff --git a/src/test/datascience/data-viewing/dataViewerDependencyService.unit.test.ts b/src/test/datascience/data-viewing/dataViewerDependencyService.unit.test.ts index aaf7d1b69ea..fef55f7c601 100644 --- a/src/test/datascience/data-viewing/dataViewerDependencyService.unit.test.ts +++ b/src/test/datascience/data-viewing/dataViewerDependencyService.unit.test.ts @@ -80,7 +80,7 @@ suite('DataScience - DataViewerDependencyService (IKernel, Web)', () => { const resultPromise = dependencyService.checkAndInstallMissingDependencies(kernel); await assert.isRejected( resultPromise, - DataScience.pandasTooOldForViewingFormat().format('0.20.'), + DataScience.pandasTooOldForViewingFormat().format('0.20.', pandasMinimumVersionSupportedByVariableViewer), 'Failed to identify too old pandas' ); assert.deepEqual( @@ -98,7 +98,7 @@ suite('DataScience - DataViewerDependencyService (IKernel, Web)', () => { const resultPromise = dependencyService.checkAndInstallMissingDependencies(kernel); await assert.isRejected( resultPromise, - DataScience.pandasTooOldForViewingFormat().format('0.10.'), + DataScience.pandasTooOldForViewingFormat().format('0.10.', pandasMinimumVersionSupportedByVariableViewer), 'Failed to identify too old pandas' ); assert.deepEqual( diff --git a/src/test/datascience/data-viewing/dataViewerDependencyServiceInterpreter.node.unit.test.ts b/src/test/datascience/data-viewing/dataViewerDependencyServiceInterpreter.node.unit.test.ts index bdd426cdfa2..64aedb03f01 100644 --- a/src/test/datascience/data-viewing/dataViewerDependencyServiceInterpreter.node.unit.test.ts +++ b/src/test/datascience/data-viewing/dataViewerDependencyServiceInterpreter.node.unit.test.ts @@ -72,7 +72,10 @@ suite('DataScience - DataViewerDependencyService (PythonEnvironment, Node)', () const promise = dependencyService.checkAndInstallMissingDependencies(interpreter); - await assert.isRejected(promise, DataScience.pandasTooOldForViewingFormat().format('0.20.')); + await assert.isRejected( + promise, + DataScience.pandasTooOldForViewingFormat().format('0.20.', pandasMinimumVersionSupportedByVariableViewer) + ); }); test('Throw exception if pandas is installed and version is < 0.20', async () => { when( @@ -81,7 +84,10 @@ suite('DataScience - DataViewerDependencyService (PythonEnvironment, Node)', () const promise = dependencyService.checkAndInstallMissingDependencies(interpreter); - await assert.isRejected(promise, DataScience.pandasTooOldForViewingFormat().format('0.10.')); + await assert.isRejected( + promise, + DataScience.pandasTooOldForViewingFormat().format('0.10.', pandasMinimumVersionSupportedByVariableViewer) + ); }); test('Prompt to install pandas and install pandas', async () => { when( diff --git a/src/test/datascience/data-viewing/dataViewerDependencyServiceKernel.node.unit.test.ts b/src/test/datascience/data-viewing/dataViewerDependencyServiceKernel.node.unit.test.ts index 2e684c9b284..35616a8b680 100644 --- a/src/test/datascience/data-viewing/dataViewerDependencyServiceKernel.node.unit.test.ts +++ b/src/test/datascience/data-viewing/dataViewerDependencyServiceKernel.node.unit.test.ts @@ -98,7 +98,7 @@ suite('DataScience - DataViewerDependencyService (IKernel, Node)', () => { const resultPromise = dependencyService.checkAndInstallMissingDependencies(kernel); await assert.isRejected( resultPromise, - DataScience.pandasTooOldForViewingFormat().format('0.20.'), + DataScience.pandasTooOldForViewingFormat().format('0.20.', pandasMinimumVersionSupportedByVariableViewer), 'Failed to identify too old pandas' ); assert.deepEqual( @@ -116,7 +116,7 @@ suite('DataScience - DataViewerDependencyService (IKernel, Node)', () => { const resultPromise = dependencyService.checkAndInstallMissingDependencies(kernel); await assert.isRejected( resultPromise, - DataScience.pandasTooOldForViewingFormat().format('0.10.'), + DataScience.pandasTooOldForViewingFormat().format('0.10.', pandasMinimumVersionSupportedByVariableViewer), 'Failed to identify too old pandas' ); assert.deepEqual( diff --git a/src/webviews/extension-side/dataviewer/baseDataViewerDependencyImplementation.ts b/src/webviews/extension-side/dataviewer/baseDataViewerDependencyImplementation.ts index f621187bd96..160fbaf8a14 100644 --- a/src/webviews/extension-side/dataviewer/baseDataViewerDependencyImplementation.ts +++ b/src/webviews/extension-side/dataviewer/baseDataViewerDependencyImplementation.ts @@ -9,24 +9,89 @@ import { IKernel } from '../../../kernels/types'; import { IDataViewerDependencyService } from './types'; import { pandasMinimumVersionSupportedByVariableViewer } from './constants'; import { PythonEnvironment } from '../../../platform/pythonEnvironments/info'; +import { parseSemVer } from '../../../platform/common/utils'; +import { SemVer } from 'semver'; +import { captureTelemetry, sendTelemetryEvent, Telemetry } from '../../../telemetry'; +import { ProductNames } from '../../../kernels/installer/productNames'; +import { Product } from '../../../kernels/installer/types'; +import { CancellationToken, CancellationTokenSource } from 'vscode'; +import { Cancellation } from '../../../platform/common/cancellation'; +import { traceWarning } from '../../../platform/logging'; /** * base class of the data viewer dependency implementation. */ -export abstract class BaseDataViewerDependencyImplementation implements IDataViewerDependencyService { +export abstract class BaseDataViewerDependencyImplementation implements IDataViewerDependencyService { constructor(private readonly applicationShell: IApplicationShell, private isCodeSpace: boolean) {} abstract checkAndInstallMissingDependencies(executionEnvironment: IKernel | PythonEnvironment): Promise; - protected async promptInstall(): Promise { + protected abstract _getVersion(executer: TExecuter, token: CancellationToken): Promise; + protected abstract _doInstall(executer: TExecuter, tokenSource: CancellationTokenSource): Promise; + + protected async getVersion(executer: TExecuter, token: CancellationToken): Promise { + try { + const version = await this._getVersion(executer, token); + return typeof version === 'string' ? parseSemVer(version) : version; + } catch (e) { + traceWarning(DataScience.failedToGetVersionOfPandas(), e.message); + return; + } + } + + @captureTelemetry(Telemetry.PythonModuleInstall, { + action: 'displayed', + moduleName: ProductNames.get(Product.pandas)! + }) + protected async promptInstall( + executer: TExecuter, + tokenSource: CancellationTokenSource, + version?: string + ): Promise { + console.log({ version }); + let message = version + ? DataScience.pandasTooOldForViewingFormat().format(version, pandasMinimumVersionSupportedByVariableViewer) + : DataScience.pandasRequiredForViewing().format(pandasMinimumVersionSupportedByVariableViewer); + let selection = this.isCodeSpace ? Common.install() - : await this.applicationShell.showErrorMessage( - DataScience.pandasRequiredForViewing().format(pandasMinimumVersionSupportedByVariableViewer), - { modal: true }, - Common.install() - ); + : await this.applicationShell.showErrorMessage(message, { modal: true }, Common.install()); + + if (selection === Common.install()) { + await this._doInstall(executer, tokenSource); + } else { + console.log('User cancelled'); + sendTelemetryEvent(Telemetry.UserDidNotInstallPandas); + throw new Error(message); + } + } + + protected async checkOrInstall(executer: TExecuter): Promise { + const tokenSource = new CancellationTokenSource(); + + try { + const pandasVersion = await this.getVersion(executer, tokenSource.token); + + if (Cancellation.isCanceled(tokenSource.token)) { + sendTelemetryEvent(Telemetry.PandasInstallCanceled); + return; + } - return selection === Common.install(); + console.log({ pandasVersion }); + if (pandasVersion) { + if (pandasVersion.compare(pandasMinimumVersionSupportedByVariableViewer) > 0) { + sendTelemetryEvent(Telemetry.PandasOK); + return; + } + sendTelemetryEvent(Telemetry.PandasTooOld); + // Warn user that we cannot start because pandas is too old. + const versionStr = `${pandasVersion.major}.${pandasVersion.minor}.${pandasVersion.build}`; + await this.promptInstall(executer, tokenSource, versionStr); + } + sendTelemetryEvent(Telemetry.PandasNotInstalled); + await this.promptInstall(executer, tokenSource); + } finally { + tokenSource.dispose(); + } } } diff --git a/src/webviews/extension-side/dataviewer/interpreterDataViewerDependencyImplementation.node.ts b/src/webviews/extension-side/dataviewer/interpreterDataViewerDependencyImplementation.node.ts index 4bfc658be69..9da1de67bd4 100644 --- a/src/webviews/extension-side/dataviewer/interpreterDataViewerDependencyImplementation.node.ts +++ b/src/webviews/extension-side/dataviewer/interpreterDataViewerDependencyImplementation.node.ts @@ -3,26 +3,20 @@ 'use strict'; -import { SemVer } from 'semver'; import { CancellationToken, CancellationTokenSource } from 'vscode'; -import { ProductNames } from '../../../kernels/installer/productNames'; import { IInstaller, Product, InstallerResponse } from '../../../kernels/installer/types'; import { IApplicationShell } from '../../../platform/common/application/types'; import { Cancellation, createPromiseFromCancellation } from '../../../platform/common/cancellation'; -import { traceWarning } from '../../../platform/logging'; import { IPythonExecutionFactory } from '../../../platform/common/process/types.node'; -import { parseSemVer } from '../../../platform/common/utils'; -import { DataScience } from '../../../platform/common/utils/localize'; import { IInterpreterService } from '../../../platform/interpreter/contracts'; import { PythonEnvironment } from '../../../platform/pythonEnvironments/info'; import { sendTelemetryEvent, Telemetry } from '../../../telemetry'; -import { pandasMinimumVersionSupportedByVariableViewer } from './constants'; import { BaseDataViewerDependencyImplementation } from './baseDataViewerDependencyImplementation'; /** * Uses the Python interpreter to manage dependencies of a Data Viewer. */ -export class InterpreterDataViewerDependencyImplementation extends BaseDataViewerDependencyImplementation { +export class InterpreterDataViewerDependencyImplementation extends BaseDataViewerDependencyImplementation { constructor( private readonly installer: IInstaller, private pythonFactory: IPythonExecutionFactory, @@ -33,48 +27,23 @@ export class InterpreterDataViewerDependencyImplementation extends BaseDataViewe super(applicationShell, isCodeSpace); } - public async checkAndInstallMissingDependencies(interpreter: PythonEnvironment): Promise { - sendTelemetryEvent(Telemetry.DataViewerUsingInterpreter); - - const tokenSource = new CancellationTokenSource(); - try { - const pandasVersion = await this.getVersion(interpreter, tokenSource.token); - - if (Cancellation.isCanceled(tokenSource.token)) { - sendTelemetryEvent(Telemetry.PandasInstallCanceled); - return; - } - - if (pandasVersion) { - if (pandasVersion.compare(pandasMinimumVersionSupportedByVariableViewer) > 0) { - sendTelemetryEvent(Telemetry.PandasOK); - return; - } - sendTelemetryEvent(Telemetry.PandasTooOld); - // Warn user that we cannot start because pandas is too old. - const versionStr = `${pandasVersion.major}.${pandasVersion.minor}.${pandasVersion.build}`; - throw new Error(DataScience.pandasTooOldForViewingFormat().format(versionStr)); - } - - sendTelemetryEvent(Telemetry.PandasNotInstalled); - await this.installMissingDependencies(interpreter, tokenSource); - } finally { - tokenSource.dispose(); - } - } - - private async installMissingDependencies( + protected async _getVersion( interpreter: PythonEnvironment, - tokenSource: CancellationTokenSource - ): Promise { - sendTelemetryEvent(Telemetry.PythonModuleInstall, undefined, { - action: 'displayed', - moduleName: ProductNames.get(Product.pandas)!, - pythonEnvType: interpreter?.envType + token?: CancellationToken + ): Promise { + const launcher = await this.pythonFactory.createActivatedEnvironment({ + resource: undefined, + interpreter, + allowEnvironmentFetchExceptions: true }); + const result = await launcher.exec(['-c', 'import pandas;print(pandas.__version__)'], { + throwOnStdErr: true, + token + }); + return result.stdout; + } - const doInstall = await this.promptInstall(); - + protected async _doInstall(interpreter: PythonEnvironment, tokenSource: CancellationTokenSource): Promise { // All data science dependencies require an interpreter to be passed in // Default to the active interpreter if no interpreter is available const interpreterToInstallDependenciesInto = @@ -84,44 +53,24 @@ export class InterpreterDataViewerDependencyImplementation extends BaseDataViewe return; } - if (doInstall) { - const cancellationPromise = createPromiseFromCancellation({ - cancelAction: 'resolve', - defaultValue: InstallerResponse.Ignore, - token: tokenSource.token - }); - // Always pass a cancellation token to `install`, to ensure it waits until the module is installed. - const response = await Promise.race([ - this.installer.install(Product.pandas, interpreterToInstallDependenciesInto, tokenSource), - cancellationPromise - ]); - if (response === InstallerResponse.Installed) { - sendTelemetryEvent(Telemetry.UserInstalledPandas); - } - } else { - sendTelemetryEvent(Telemetry.UserDidNotInstallPandas); - throw new Error( - DataScience.pandasRequiredForViewing().format(pandasMinimumVersionSupportedByVariableViewer) - ); + const cancellationPromise = createPromiseFromCancellation({ + cancelAction: 'resolve', + defaultValue: InstallerResponse.Ignore, + token: tokenSource.token + }); + // Always pass a cancellation token to `install`, to ensure it waits until the module is installed. + const response = await Promise.race([ + this.installer.install(Product.pandas, interpreterToInstallDependenciesInto, tokenSource), + cancellationPromise + ]); + if (response === InstallerResponse.Installed) { + sendTelemetryEvent(Telemetry.UserInstalledPandas); } } - private async getVersion(interpreter: PythonEnvironment, token?: CancellationToken): Promise { - const launcher = await this.pythonFactory.createActivatedEnvironment({ - resource: undefined, - interpreter, - allowEnvironmentFetchExceptions: true - }); - try { - const result = await launcher.exec(['-c', 'import pandas;print(pandas.__version__)'], { - throwOnStdErr: true, - token - }); + public async checkAndInstallMissingDependencies(interpreter: PythonEnvironment): Promise { + sendTelemetryEvent(Telemetry.DataViewerUsingInterpreter); - return parseSemVer(result.stdout); - } catch (ex) { - traceWarning('Failed to get version of Pandas to use Data Viewer', ex); - return; - } + await this.checkOrInstall(interpreter); } } diff --git a/src/webviews/extension-side/dataviewer/kernelDataViewerDependencyImplementation.ts b/src/webviews/extension-side/dataviewer/kernelDataViewerDependencyImplementation.ts index 8a60447c201..f6f25c350a7 100644 --- a/src/webviews/extension-side/dataviewer/kernelDataViewerDependencyImplementation.ts +++ b/src/webviews/extension-side/dataviewer/kernelDataViewerDependencyImplementation.ts @@ -3,17 +3,12 @@ 'use strict'; -import { SemVer } from 'semver'; -import { ProductNames } from '../../../kernels/installer/productNames'; -import { Product } from '../../../kernels/installer/types'; import { traceWarning } from '../../../platform/logging'; import { DataScience } from '../../../platform/common/utils/localize'; import { EnvironmentType } from '../../../platform/pythonEnvironments/info'; import { sendTelemetryEvent, Telemetry } from '../../../telemetry'; import { executeSilently } from '../../../kernels/helpers'; import { IKernel, IKernelConnectionSession } from '../../../kernels/types'; -import { parseSemVer } from '../../../platform/common/utils'; -import { pandasMinimumVersionSupportedByVariableViewer } from './constants'; import { BaseDataViewerDependencyImplementation } from './baseDataViewerDependencyImplementation'; export const kernelGetPandasVersion = @@ -36,7 +31,7 @@ function kernelHasSession(kernel: IKernel): kernel is IKernelWithSession { /** * Uses the Kernel to manage the dependencies of a Data Viewer. */ -export class KernelDataViewerDependencyImplementation extends BaseDataViewerDependencyImplementation { +export class KernelDataViewerDependencyImplementation extends BaseDataViewerDependencyImplementation { protected async execute(command: string, kernel: IKernelWithSession): Promise<(string | undefined)[]> { const outputs = await executeSilently(kernel.session, command); const error = outputs.find((item) => item.output_type === 'error'); @@ -46,37 +41,20 @@ export class KernelDataViewerDependencyImplementation extends BaseDataViewerDepe return outputs.map((item) => item.text?.toString()); } - protected async getVersion(kernel: IKernelWithSession): Promise { - try { - const outputs = await this.execute(kernelGetPandasVersion, kernel); - return outputs.map((text) => (text ? parseSemVer(text.toString()) : undefined)).find((item) => item); - } catch (e) { - traceWarning(DataScience.failedToGetVersionOfPandas(), e.message); - return; - } + protected async _getVersion(kernel: IKernelWithSession): Promise { + const outputs = await this.execute(kernelGetPandasVersion, kernel); + return outputs.map((text) => (text ? text.toString() : undefined)).find((item) => item); } - private async installMissingDependencies(kernel: IKernelWithSession): Promise { - sendTelemetryEvent(Telemetry.PythonModuleInstall, undefined, { - action: 'displayed', - moduleName: ProductNames.get(Product.pandas)! - }); - + protected async _doInstall(kernel: IKernelWithSession): Promise { const command = `${kernelPackaging(kernel)} install pandas`; - if (await this.promptInstall()) { - try { - await this.execute(command, kernel); - sendTelemetryEvent(Telemetry.UserInstalledPandas); - } catch (e) { - sendTelemetryEvent(Telemetry.UserInstalledPandas, undefined, undefined, e); - throw new Error(DataScience.failedToInstallPandas()); - } - } else { - sendTelemetryEvent(Telemetry.UserDidNotInstallPandas); - throw new Error( - DataScience.pandasRequiredForViewing().format(pandasMinimumVersionSupportedByVariableViewer) - ); + try { + await this.execute(command, kernel); + sendTelemetryEvent(Telemetry.UserInstalledPandas); + } catch (e) { + sendTelemetryEvent(Telemetry.UserInstalledPandas, undefined, undefined, e); + throw new Error(DataScience.failedToInstallPandas()); } } @@ -88,21 +66,6 @@ export class KernelDataViewerDependencyImplementation extends BaseDataViewerDepe throw new Error('No no active kernel session.'); } - const pandasVersion = await this.getVersion(kernel); - - if (pandasVersion) { - if (pandasVersion.compare(pandasMinimumVersionSupportedByVariableViewer) > 0) { - sendTelemetryEvent(Telemetry.PandasOK); - return; - } - sendTelemetryEvent(Telemetry.PandasTooOld); - // Warn user that we cannot start because pandas is too old. - const versionStr = `${pandasVersion.major}.${pandasVersion.minor}.${pandasVersion.build}`; - throw new Error(DataScience.pandasTooOldForViewingFormat().format(versionStr)); - } - - sendTelemetryEvent(Telemetry.PandasNotInstalled); - - await this.installMissingDependencies(kernel); + await this.checkOrInstall(kernel); } } From d59afc3e81ce16ecbcb0f6d7bf43f631f6bb35a1 Mon Sep 17 00:00:00 2001 From: Daniel Rodriguez Date: Tue, 19 Jul 2022 12:00:25 -0400 Subject: [PATCH 2/2] removed console.logs --- .../dataviewer/baseDataViewerDependencyImplementation.ts | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/webviews/extension-side/dataviewer/baseDataViewerDependencyImplementation.ts b/src/webviews/extension-side/dataviewer/baseDataViewerDependencyImplementation.ts index 160fbaf8a14..add42a7b8e4 100644 --- a/src/webviews/extension-side/dataviewer/baseDataViewerDependencyImplementation.ts +++ b/src/webviews/extension-side/dataviewer/baseDataViewerDependencyImplementation.ts @@ -48,7 +48,6 @@ export abstract class BaseDataViewerDependencyImplementation implemen tokenSource: CancellationTokenSource, version?: string ): Promise { - console.log({ version }); let message = version ? DataScience.pandasTooOldForViewingFormat().format(version, pandasMinimumVersionSupportedByVariableViewer) : DataScience.pandasRequiredForViewing().format(pandasMinimumVersionSupportedByVariableViewer); @@ -60,7 +59,6 @@ export abstract class BaseDataViewerDependencyImplementation implemen if (selection === Common.install()) { await this._doInstall(executer, tokenSource); } else { - console.log('User cancelled'); sendTelemetryEvent(Telemetry.UserDidNotInstallPandas); throw new Error(message); } @@ -77,7 +75,6 @@ export abstract class BaseDataViewerDependencyImplementation implemen return; } - console.log({ pandasVersion }); if (pandasVersion) { if (pandasVersion.compare(pandasMinimumVersionSupportedByVariableViewer) > 0) { sendTelemetryEvent(Telemetry.PandasOK);