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

Support for Jupyter to pass additional install args #16133

Merged
merged 1 commit into from
May 4, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions news/3 Code Health/16131.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Add ability for Jupyter extension to pass addtional installer arguments.
12 changes: 9 additions & 3 deletions src/client/common/installer/condaInstaller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import { inDiscoveryExperiment } from '../experiments/helpers';
import { ExecutionInfo, IConfigurationService, IExperimentService } from '../types';
import { isResource } from '../utils/misc';
import { ModuleInstaller } from './moduleInstaller';
import { InterpreterUri } from './types';
import { InterpreterUri, ModuleInstallFlags } from './types';

/**
* A Python module installer for a conda environment.
Expand Down Expand Up @@ -64,7 +64,7 @@ export class CondaInstaller extends ModuleInstaller {
protected async getExecutionInfo(
moduleName: string,
resource?: InterpreterUri,
isUpgrade?: boolean,
flags: ModuleInstallFlags = 0,
): Promise<ExecutionInfo> {
const condaService = this.serviceContainer.get<ICondaService>(ICondaService);
const condaFile = await condaService.getCondaFile();
Expand All @@ -77,7 +77,7 @@ export class CondaInstaller extends ModuleInstaller {
? this.serviceContainer.get<IComponentAdapter>(IComponentAdapter)
: this.serviceContainer.get<ICondaLocatorService>(ICondaLocatorService);
const info = await condaLocatorService.getCondaEnvironment(pythonPath);
const args = [isUpgrade ? 'update' : 'install'];
const args = [flags & ModuleInstallFlags.upgrade ? 'update' : 'install'];

// Temporarily ensure tensorboard is installed from the conda-forge
// channel since 2.4.1 is not yet available in the default index
Expand All @@ -93,6 +93,12 @@ export class CondaInstaller extends ModuleInstaller {
args.push('--prefix');
args.push(info.path.fileToCommandArgument());
}
if (flags & ModuleInstallFlags.updateDependencies) {
args.push('--update-deps');
}
if (flags & ModuleInstallFlags.reInstall) {
args.push('--force-reinstall');
}
args.push(moduleName);
args.push('-y');
return {
Expand Down
8 changes: 4 additions & 4 deletions src/client/common/installer/moduleInstaller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import { ExecutionInfo, IConfigurationService, IOutputChannel, ModuleNamePurpose
import { Products } from '../utils/localize';
import { isResource } from '../utils/misc';
import { ProductNames } from './productNames';
import { IModuleInstaller, InterpreterUri } from './types';
import { IModuleInstaller, InterpreterUri, ModuleInstallFlags } from './types';

@injectable()
export abstract class ModuleInstaller implements IModuleInstaller {
Expand All @@ -33,7 +33,7 @@ export abstract class ModuleInstaller implements IModuleInstaller {
productOrModuleName: Product | string,
resource?: InterpreterUri,
cancel?: CancellationToken,
isUpgrade?: boolean,
flags?: ModuleInstallFlags,
): Promise<void> {
const name =
typeof productOrModuleName == 'string'
Expand All @@ -48,7 +48,7 @@ export abstract class ModuleInstaller implements IModuleInstaller {
} else {
options.interpreter = resource;
}
const executionInfo = await this.getExecutionInfo(name, resource, isUpgrade);
const executionInfo = await this.getExecutionInfo(name, resource, flags);
const terminalService = this.serviceContainer
.get<ITerminalServiceFactory>(ITerminalServiceFactory)
.getTerminalService(options);
Expand Down Expand Up @@ -132,7 +132,7 @@ export abstract class ModuleInstaller implements IModuleInstaller {
protected abstract getExecutionInfo(
moduleName: string,
resource?: InterpreterUri,
isUpgrade?: boolean,
flags?: ModuleInstallFlags,
): Promise<ExecutionInfo>;
private async processInstallArgs(args: string[], resource?: InterpreterUri): Promise<string[]> {
const indexOfPylint = args.findIndex((arg) => arg.toUpperCase() === 'PYLINT');
Expand Down
11 changes: 8 additions & 3 deletions src/client/common/installer/pipEnvInstaller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import { inDiscoveryExperiment } from '../experiments/helpers';
import { ExecutionInfo, IExperimentService } from '../types';
import { isResource } from '../utils/misc';
import { ModuleInstaller } from './moduleInstaller';
import { InterpreterUri } from './types';
import { InterpreterUri, ModuleInstallFlags } from './types';

export const pipenvName = 'pipenv';

Expand Down Expand Up @@ -61,9 +61,14 @@ export class PipEnvInstaller extends ModuleInstaller {
protected async getExecutionInfo(
moduleName: string,
_resource?: InterpreterUri,
isUpgrade?: boolean,
flags: ModuleInstallFlags = 0,
): Promise<ExecutionInfo> {
const args = [isUpgrade ? 'update' : 'install', moduleName, '--dev'];
// In pipenv the only way to update/upgrade or re-install is update (apart from a complete uninstall and re-install).
const update =
flags & ModuleInstallFlags.reInstall ||
flags & ModuleInstallFlags.updateDependencies ||
flags & ModuleInstallFlags.upgrade;
const args = [update ? 'update' : 'install', moduleName, '--dev'];
if (moduleName === 'black') {
args.push('--pre');
}
Expand Down
20 changes: 14 additions & 6 deletions src/client/common/installer/pipInstaller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import { IPythonExecutionFactory } from '../process/types';
import { ExecutionInfo } from '../types';
import { isResource } from '../utils/misc';
import { ModuleInstaller } from './moduleInstaller';
import { InterpreterUri } from './types';
import { InterpreterUri, ModuleInstallFlags } from './types';

@injectable()
export class PipInstaller extends ModuleInstaller {
Expand All @@ -28,16 +28,24 @@ export class PipInstaller extends ModuleInstaller {
public isSupported(resource?: InterpreterUri): Promise<boolean> {
return this.isPipAvailable(resource);
}
protected async getExecutionInfo(moduleName: string, _resource?: InterpreterUri): Promise<ExecutionInfo> {
const proxyArgs: string[] = [];
protected async getExecutionInfo(
moduleName: string,
_resource?: InterpreterUri,
flags: ModuleInstallFlags = 0,
): Promise<ExecutionInfo> {
const args: string[] = [];
const workspaceService = this.serviceContainer.get<IWorkspaceService>(IWorkspaceService);
const proxy = workspaceService.getConfiguration('http').get('proxy', '');
if (proxy.length > 0) {
proxyArgs.push('--proxy');
proxyArgs.push(proxy);
args.push('--proxy');
args.push(proxy);
}
args.push(...['install', '-U']);
if (flags & ModuleInstallFlags.reInstall) {
args.push('--force-reinstall');
}
return {
args: [...proxyArgs, 'install', '-U', moduleName],
args: [...args, moduleName],
moduleName: 'pip',
};
}
Expand Down
23 changes: 12 additions & 11 deletions src/client/common/installer/productInstaller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ import {
InterpreterUri,
IProductPathService,
IProductService,
ModuleInstallFlags,
} from './types';

export { Product } from '../types';
Expand Down Expand Up @@ -72,7 +73,7 @@ abstract class BaseInstaller {
product: Product,
resource?: InterpreterUri,
cancel?: CancellationToken,
isUpgrade?: boolean,
flags?: ModuleInstallFlags,
): Promise<InstallerResponse> {
// If this method gets called twice, while previous promise has not been resolved, then return that same promise.
// E.g. previous promise is not resolved as a message has been displayed to the user, so no point displaying
Expand All @@ -83,7 +84,7 @@ abstract class BaseInstaller {
if (BaseInstaller.PromptPromises.has(key)) {
return BaseInstaller.PromptPromises.get(key)!;
}
const promise = this.promptToInstallImplementation(product, resource, cancel, isUpgrade);
const promise = this.promptToInstallImplementation(product, resource, cancel, flags);
BaseInstaller.PromptPromises.set(key, promise);
promise.then(() => BaseInstaller.PromptPromises.delete(key)).ignoreErrors();
promise.catch(() => BaseInstaller.PromptPromises.delete(key)).ignoreErrors();
Expand All @@ -95,7 +96,7 @@ abstract class BaseInstaller {
product: Product,
resource?: InterpreterUri,
cancel?: CancellationToken,
isUpgrade?: boolean,
flags?: ModuleInstallFlags,
): Promise<InstallerResponse> {
if (product === Product.unittest) {
return InstallerResponse.Installed;
Expand All @@ -112,7 +113,7 @@ abstract class BaseInstaller {
}

await installer
.installModule(product, resource, cancel, isUpgrade)
.installModule(product, resource, cancel, flags)
.catch((ex) => traceError(`Error in installing the product '${ProductNames.get(product)}', ${ex}`));

return this.isInstalled(product, resource).then((isInstalled) => {
Expand Down Expand Up @@ -207,7 +208,7 @@ abstract class BaseInstaller {
product: Product,
resource?: InterpreterUri,
cancel?: CancellationToken,
isUpgrade?: boolean,
flags?: ModuleInstallFlags,
): Promise<InstallerResponse>;

protected getExecutableNameFromSettings(product: Product, resource?: Uri): string {
Expand Down Expand Up @@ -349,7 +350,7 @@ class DataScienceInstaller extends BaseInstaller {
product: Product,
interpreterUri?: InterpreterUri,
cancel?: CancellationToken,
isUpgrade?: boolean,
flags?: ModuleInstallFlags,
): Promise<InstallerResponse> {
// Precondition
if (isResource(interpreterUri)) {
Expand Down Expand Up @@ -390,7 +391,7 @@ class DataScienceInstaller extends BaseInstaller {
}

await installerModule
.installModule(product, interpreter, cancel, isUpgrade)
.installModule(product, interpreter, cancel, flags)
.catch((ex) => traceError(`Error in installing the module '${moduleName}', ${ex}`));

return this.isInstalled(product, interpreter).then((isInstalled) => {
Expand Down Expand Up @@ -447,15 +448,15 @@ export class ProductInstaller implements IInstaller {
product: Product,
resource?: InterpreterUri,
cancel?: CancellationToken,
isUpgrade?: boolean,
flags?: ModuleInstallFlags,
): Promise<InstallerResponse> {
const currentInterpreter = isResource(resource)
? await this.interpreterService.getActiveInterpreter(resource)
: resource;
if (!currentInterpreter) {
return InstallerResponse.Ignore;
}
return this.createInstaller(product).promptToInstall(product, resource, cancel, isUpgrade);
return this.createInstaller(product).promptToInstall(product, resource, cancel, flags);
}

public async isProductVersionCompatible(
Expand All @@ -470,9 +471,9 @@ export class ProductInstaller implements IInstaller {
product: Product,
resource?: InterpreterUri,
cancel?: CancellationToken,
isUpgrade?: boolean,
flags?: ModuleInstallFlags,
): Promise<InstallerResponse> {
return this.createInstaller(product).install(product, resource, cancel, isUpgrade);
return this.createInstaller(product).install(product, resource, cancel, flags);
}

public async isInstalled(product: Product, resource?: InterpreterUri): Promise<boolean> {
Expand Down
10 changes: 8 additions & 2 deletions src/client/common/installer/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ export interface IModuleInstaller {
product: string,
resource?: InterpreterUri,
cancel?: CancellationToken,
isUpgrade?: boolean,
flags?: ModuleInstallFlags,
): Promise<void>;
/**
* Installs a Product
Expand All @@ -44,7 +44,7 @@ export interface IModuleInstaller {
product: Product,
resource?: InterpreterUri,
cancel?: CancellationToken,
isUpgrade?: boolean,
flags?: ModuleInstallFlags,
): Promise<void>;
isSupported(resource?: InterpreterUri): Promise<boolean>;
}
Expand Down Expand Up @@ -76,3 +76,9 @@ export const IExtensionBuildInstaller = Symbol('IExtensionBuildInstaller');
export interface IExtensionBuildInstaller {
install(): Promise<void>;
}

export enum ModuleInstallFlags {
upgrade = 1,
updateDependencies = 2,
reInstall = 4,
}
6 changes: 3 additions & 3 deletions src/client/common/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import {
import { LanguageServerType } from '../activation/types';
import { LogLevel } from '../logging/levels';
import type { ExtensionChannels } from './insidersBuild/types';
import type { InterpreterUri } from './installer/types';
import type { InterpreterUri, ModuleInstallFlags } from './installer/types';
import { EnvironmentVariables } from './variables/types';
import { ITestingSettings } from '../testing/configuration/types';

Expand Down Expand Up @@ -122,13 +122,13 @@ export interface IInstaller {
product: Product,
resource?: InterpreterUri,
cancel?: CancellationToken,
isUpgrade?: boolean,
flags?: ModuleInstallFlags,
): Promise<InstallerResponse>;
install(
product: Product,
resource?: InterpreterUri,
cancel?: CancellationToken,
isUpgrade?: boolean,
flags?: ModuleInstallFlags,
): Promise<InstallerResponse>;
isInstalled(product: Product, resource?: InterpreterUri): Promise<boolean>;
isProductVersionCompatible(
Expand Down
13 changes: 11 additions & 2 deletions src/client/jupyter/jupyterIntegration.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import { CancellationToken, Disposable, Event, Extension, Memento, Uri } from 'v
import * as lsp from 'vscode-languageserver-protocol';
import { ILanguageServerCache, ILanguageServerConnection } from '../activation/types';
import { JUPYTER_EXTENSION_ID } from '../common/constants';
import { InterpreterUri } from '../common/installer/types';
import { InterpreterUri, ModuleInstallFlags } from '../common/installer/types';
import {
GLOBAL_MEMENTO,
IExperimentService,
Expand Down Expand Up @@ -179,7 +179,16 @@ export class JupyterExtensionIntegration {
product: JupyterProductToInstall,
resource?: InterpreterUri,
cancel?: CancellationToken,
): Promise<InstallerResponse> => this.installer.install(ProductMapping[product], resource, cancel),
reInstallAndUpdate?: boolean,
): Promise<InstallerResponse> =>
this.installer.install(
ProductMapping[product],
resource,
cancel,
reInstallAndUpdate === true
? ModuleInstallFlags.updateDependencies | ModuleInstallFlags.reInstall
: undefined,
),
getDebuggerPath: async () => dirname(getDebugpyPackagePath()),
getInterpreterPathSelectedForJupyterServer: () =>
this.globalState.get<string | undefined>('INTERPRETER_PATH_SELECTED_FOR_JUPYTER_SERVER'),
Expand Down
5 changes: 4 additions & 1 deletion src/client/tensorBoard/tensorBoardSession.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ import { EventName } from '../telemetry/constants';
import { ImportTracker } from '../telemetry/importTracker';
import { TensorBoardPromptSelection, TensorBoardSessionStartResult } from './constants';
import { IMultiStepInputFactory } from '../common/utils/multiStepInput';
import { ModuleInstallFlags } from '../common/installer/types';

enum Messages {
JumpToSource = 'jump_to_source',
Expand Down Expand Up @@ -236,7 +237,9 @@ export class TensorBoardSession {
Product.tensorboard,
interpreter,
installerToken,
tensorboardInstallStatus === ProductInstallStatus.NeedsUpgrade,
tensorboardInstallStatus === ProductInstallStatus.NeedsUpgrade
? ModuleInstallFlags.upgrade
: undefined,
),
);
}
Expand Down
22 changes: 17 additions & 5 deletions src/test/common/installer/moduleInstaller.unit.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,11 @@ import { ModuleInstaller } from '../../../client/common/installer/moduleInstalle
import { PipEnvInstaller, pipenvName } from '../../../client/common/installer/pipEnvInstaller';
import { PipInstaller } from '../../../client/common/installer/pipInstaller';
import { ProductInstaller } from '../../../client/common/installer/productInstaller';
import { IInstallationChannelManager, IModuleInstaller } from '../../../client/common/installer/types';
import {
IInstallationChannelManager,
IModuleInstaller,
ModuleInstallFlags,
} from '../../../client/common/installer/types';
import { IFileSystem } from '../../../client/common/platform/types';
import { ITerminalService, ITerminalServiceFactory } from '../../../client/common/terminal/types';
import {
Expand Down Expand Up @@ -340,7 +344,7 @@ suite('Module Installer', () => {
async function installModuleAndVerifyCommand(
command: string,
expectedArgs: string[],
isUpgrade?: boolean,
flags?: ModuleInstallFlags,
) {
terminalService
.setup((t) =>
Expand All @@ -353,7 +357,7 @@ suite('Module Installer', () => {
.returns(() => Promise.resolve())
.verifiable(TypeMoq.Times.once());

await installer.installModule(product.value, resource, undefined, isUpgrade);
await installer.installModule(product.value, resource, undefined, flags);
terminalService.verifyAll();
}

Expand Down Expand Up @@ -604,7 +608,11 @@ suite('Module Installer', () => {
if (moduleName === 'black') {
expectedArgs.push('--pre');
}
await installModuleAndVerifyCommand(pipenvName, expectedArgs, isUpgrade);
await installModuleAndVerifyCommand(
pipenvName,
expectedArgs,
isUpgrade ? ModuleInstallFlags.upgrade : undefined,
);
});
});
}
Expand All @@ -625,7 +633,11 @@ suite('Module Installer', () => {
}
expectedArgs.push(moduleName);
expectedArgs.push('-y');
await installModuleAndVerifyCommand(condaExecutable, expectedArgs, isUpgrade);
await installModuleAndVerifyCommand(
condaExecutable,
expectedArgs,
isUpgrade ? ModuleInstallFlags.upgrade : undefined,
);
});
});
}
Expand Down
Loading