Skip to content

Fix Quick Create package installation failure handling with retry flow #569

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

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions src/common/localize.ts
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,14 @@ export namespace VenvManagerStrings {
export const quickCreateDescription = l10n.t('Create a virtual environment in the workspace root');
export const customize = l10n.t('Custom');
export const customizeDescription = l10n.t('Choose python version, location, packages, name, etc.');

// New strings for retry functionality
export const packageInstallationFailedTitle = l10n.t('Package installation failed');
export const packageInstallationFailedMessage = l10n.t('Environment was created successfully, but package installation failed. Would you like to select packages to install?');
export const retryPackageInstallation = l10n.t('Select Packages');
export const environmentCreationFailedTitle = l10n.t('Environment creation failed');
export const environmentCreationFailedMessage = l10n.t('Failed to create virtual environment. Would you like to try again?');
export const retryEnvironmentCreation = l10n.t('Retry');
}

export namespace SysManagerStrings {
Expand Down
93 changes: 91 additions & 2 deletions src/managers/builtin/venvManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ import { PYTHON_EXTENSION_ID } from '../../common/constants';
import { VenvManagerStrings } from '../../common/localize';
import { traceError } from '../../common/logging';
import { createDeferred, Deferred } from '../../common/utils/deferred';
import { showErrorMessage, withProgress } from '../../common/window.apis';
import { showErrorMessage, showInformationMessage, withProgress } from '../../common/window.apis';
import { findParentIfFile } from '../../features/envCommands';
import { NativePythonFinder } from '../common/nativePythonFinder';
import { getLatest, shortVersion, sortEnvironments } from '../common/utils';
Expand All @@ -51,6 +51,7 @@ import {
setVenvForWorkspace,
setVenvForWorkspaces,
} from './venvUtils';
import { getWorkspacePackagesToInstall } from './pipUtils';

export class VenvManager implements EnvironmentManager {
private collection: PythonEnvironment[] = [];
Expand Down Expand Up @@ -143,7 +144,7 @@ export class VenvManager implements EnvironmentManager {
let environment: PythonEnvironment | undefined = undefined;
if (options?.quickCreate) {
if (this.globalEnv && this.globalEnv.version.startsWith('3.')) {
environment = await quickCreateVenv(
const result = await quickCreateVenv(
this.nativeFinder,
this.api,
this.log,
Expand All @@ -152,6 +153,28 @@ export class VenvManager implements EnvironmentManager {
venvRoot,
options?.additionalPackages,
);

if (result.environmentCreated) {
environment = result.environment;

// Handle package installation failure
if (!result.packagesInstalled && result.packageInstallationError) {
this.log.warn(`Quick Create: Package installation failed: ${result.packageInstallationError.message}`);
// Show notification with retry option for package installation
setImmediate(async () => {
await this.showPackageInstallationFailureNotification(environment!, venvRoot);
});
}
} else {
// Environment creation failed completely
this.log.error(`Quick Create: Environment creation failed: ${result.environmentCreationError?.message}`);
// Show notification with retry option for entire environment creation
setImmediate(async () => {
await this.showEnvironmentCreationFailureNotification(scope, options);
});
showErrorMessage(VenvManagerStrings.venvCreateFailed);
return;
}
} else if (!this.globalEnv) {
this.log.error('No base python found');
showErrorMessage(VenvManagerStrings.venvErrorNoBasePython);
Expand Down Expand Up @@ -578,4 +601,70 @@ export class VenvManager implements EnvironmentManager {
});
return projects;
}

/**
* Handles retry package installation flow after quick create.
*/
private async handlePackageInstallationRetry(
environment: PythonEnvironment,
venvRoot: Uri,
): Promise<void> {
try {
const project = this.api.getPythonProject(venvRoot);
const packages = await getWorkspacePackagesToInstall(
this.api,
{ showSkipOption: true, install: [] },
project ? [project] : undefined,
environment,
this.log,
);

if (packages && (packages.install.length > 0 || packages.uninstall.length > 0)) {
await this.api.managePackages(environment, {
upgrade: false,
install: packages.install,
uninstall: packages.uninstall,
});
this.log.info('Package installation retry succeeded');
}
} catch (error) {
this.log.error(`Package installation retry failed: ${error}`);
showErrorMessage(l10n.t('Package installation failed again: {0}', String(error)));
}
}

/**
* Shows notification for package installation failure with retry option.
*/
private async showPackageInstallationFailureNotification(
environment: PythonEnvironment,
venvRoot: Uri,
): Promise<void> {
const result = await showInformationMessage(
VenvManagerStrings.packageInstallationFailedMessage,
VenvManagerStrings.retryPackageInstallation,
);

if (result === VenvManagerStrings.retryPackageInstallation) {
await this.handlePackageInstallationRetry(environment, venvRoot);
}
}

/**
* Shows notification for environment creation failure with retry option.
*/
private async showEnvironmentCreationFailureNotification(
scope: CreateEnvironmentScope,
options: CreateEnvironmentOptions | undefined,
): Promise<void> {
const result = await showInformationMessage(
VenvManagerStrings.environmentCreationFailedMessage,
VenvManagerStrings.retryEnvironmentCreation,
);

if (result === VenvManagerStrings.retryEnvironmentCreation) {
// Retry the entire create flow
await this.create(scope, options);
}
}
}
117 changes: 103 additions & 14 deletions src/managers/builtin/venvUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,41 @@ import { resolveSystemPythonEnvironmentPath } from './utils';
export const VENV_WORKSPACE_KEY = `${ENVS_EXTENSION_ID}:venv:WORKSPACE_SELECTED`;
export const VENV_GLOBAL_KEY = `${ENVS_EXTENSION_ID}:venv:GLOBAL_SELECTED`;

/**
* Result of environment creation operation.
*/
export interface CreateEnvironmentResult {
/**
* The created environment, if successful.
*/
environment?: PythonEnvironment;

/**
* Whether environment creation succeeded.
*/
environmentCreated: boolean;

/**
* Whether package installation succeeded (only relevant if environmentCreated is true).
*/
packagesInstalled: boolean;

/**
* Error that occurred during environment creation.
*/
environmentCreationError?: Error;

/**
* Error that occurred during package installation.
*/
packageInstallationError?: Error;

/**
* Packages that were attempted to be installed.
*/
attemptedPackages?: PipPackages;
}

export async function clearVenvCache(): Promise<void> {
const keys = [VENV_WORKSPACE_KEY, VENV_GLOBAL_KEY];
const state = await getWorkspacePersistentState();
Expand Down Expand Up @@ -281,7 +316,7 @@ async function createWithProgress(
venvRoot: Uri,
envPath: string,
packages?: PipPackages,
) {
): Promise<CreateEnvironmentResult> {
const pythonPath =
os.platform() === 'win32' ? path.join(envPath, 'Scripts', 'python.exe') : path.join(envPath, 'bin', 'python');

Expand All @@ -290,7 +325,14 @@ async function createWithProgress(
location: ProgressLocation.Notification,
title: VenvManagerStrings.venvCreating,
},
async () => {
async (): Promise<CreateEnvironmentResult> => {
let env: PythonEnvironment | undefined;
let environmentCreated = false;
let packagesInstalled = false;
let environmentCreationError: Error | undefined;
let packageInstallationError: Error | undefined;

// Step 1: Create the virtual environment
try {
const useUv = await isUvInstalled(log);
if (basePython.execInfo?.run.executable) {
Expand All @@ -315,20 +357,48 @@ async function createWithProgress(
}

const resolved = await nativeFinder.resolve(pythonPath);
const env = api.createPythonEnvironmentItem(await getPythonInfo(resolved), manager);
if (packages && (packages.install.length > 0 || packages.uninstall.length > 0)) {
env = api.createPythonEnvironmentItem(await getPythonInfo(resolved), manager);
environmentCreated = true;
log.info(`Virtual environment created successfully: ${envPath}`);
} catch (e) {
environmentCreationError = e instanceof Error ? e : new Error(String(e));
log.error(`Failed to create virtual environment: ${environmentCreationError.message}`);
return {
environmentCreated: false,
packagesInstalled: false,
environmentCreationError,
attemptedPackages: packages,
};
}

// Step 2: Install packages if environment was created successfully
if (env && packages && (packages.install.length > 0 || packages.uninstall.length > 0)) {
try {
await api.managePackages(env, {
upgrade: false,
install: packages?.install,
uninstall: packages?.uninstall ?? [],
install: packages.install,
uninstall: packages.uninstall ?? [],
});
packagesInstalled = true;
log.info('Packages installed successfully');
} catch (e) {
packageInstallationError = e instanceof Error ? e : new Error(String(e));
log.error(`Failed to install packages: ${packageInstallationError.message}`);
// Note: environment is still created, just packages failed
}
return env;
} catch (e) {
log.error(`Failed to create virtual environment: ${e}`);
showErrorMessage(VenvManagerStrings.venvCreateFailed);
return;
} else {
// No packages to install, consider this successful
packagesInstalled = true;
}

return {
environment: env,
environmentCreated,
packagesInstalled,
environmentCreationError,
packageInstallationError,
attemptedPackages: packages,
};
},
);
}
Expand Down Expand Up @@ -361,7 +431,7 @@ export async function quickCreateVenv(
baseEnv: PythonEnvironment,
venvRoot: Uri,
additionalPackages?: string[],
): Promise<PythonEnvironment | undefined> {
): Promise<CreateEnvironmentResult> {
const project = api.getPythonProject(venvRoot);

sendTelemetryEvent(EventNames.VENV_CREATION, undefined, { creationType: 'quick' });
Expand Down Expand Up @@ -408,7 +478,15 @@ export async function createPythonVenv(
if (customize === undefined) {
return;
} else if (customize === false) {
return quickCreateVenv(nativeFinder, api, log, manager, sortedEnvs[0], venvRoot, options.additionalPackages);
const result = await quickCreateVenv(nativeFinder, api, log, manager, sortedEnvs[0], venvRoot, options.additionalPackages);
if (!result.environmentCreated) {
showErrorMessage(VenvManagerStrings.venvCreateFailed);
return;
}
if (!result.packagesInstalled && result.packageInstallationError) {
log.warn(`Package installation failed during quick create: ${result.packageInstallationError.message}`);
}
return result.environment;
} else {
sendTelemetryEvent(EventNames.VENV_CREATION, undefined, { creationType: 'custom' });
}
Expand Down Expand Up @@ -450,10 +528,21 @@ export async function createPythonVenv(
const allPackages = [];
allPackages.push(...(packages?.install ?? []), ...(options.additionalPackages ?? []));

return await createWithProgress(nativeFinder, api, log, manager, basePython, venvRoot, envPath, {
const result = await createWithProgress(nativeFinder, api, log, manager, basePython, venvRoot, envPath, {
install: allPackages,
uninstall: [],
});

if (!result.environmentCreated) {
showErrorMessage(VenvManagerStrings.venvCreateFailed);
return;
}

if (!result.packagesInstalled && result.packageInstallationError) {
log.warn(`Package installation failed: ${result.packageInstallationError.message}`);
}

return result.environment;
}

export async function removeVenv(environment: PythonEnvironment, log: LogOutputChannel): Promise<boolean> {
Expand Down
66 changes: 66 additions & 0 deletions src/test/managers/builtin/venvUtils.unit.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
import assert from 'assert';
import { CreateEnvironmentResult } from '../../../managers/builtin/venvUtils';

suite('VenvUtils Tests', () => {
suite('CreateEnvironmentResult', () => {
test('should properly represent successful environment and package creation', () => {
const result: CreateEnvironmentResult = {
environment: undefined, // Mock environment would go here
environmentCreated: true,
packagesInstalled: true,
attemptedPackages: { install: ['pytest'], uninstall: [] },
};

assert.strictEqual(result.environmentCreated, true);
assert.strictEqual(result.packagesInstalled, true);
assert.strictEqual(result.environmentCreationError, undefined);
assert.strictEqual(result.packageInstallationError, undefined);
assert.deepStrictEqual(result.attemptedPackages?.install, ['pytest']);
});

test('should properly represent environment creation success but package installation failure', () => {
const packageError = new Error('Failed to install packages');
const result: CreateEnvironmentResult = {
environment: undefined, // Mock environment would go here
environmentCreated: true,
packagesInstalled: false,
packageInstallationError: packageError,
attemptedPackages: { install: ['conflicting-package'], uninstall: [] },
};

assert.strictEqual(result.environmentCreated, true);
assert.strictEqual(result.packagesInstalled, false);
assert.strictEqual(result.environmentCreationError, undefined);
assert.strictEqual(result.packageInstallationError, packageError);
assert.deepStrictEqual(result.attemptedPackages?.install, ['conflicting-package']);
});

test('should properly represent complete environment creation failure', () => {
const envError = new Error('Failed to create environment');
const result: CreateEnvironmentResult = {
environmentCreated: false,
packagesInstalled: false,
environmentCreationError: envError,
attemptedPackages: { install: ['some-package'], uninstall: [] },
};

assert.strictEqual(result.environmentCreated, false);
assert.strictEqual(result.packagesInstalled, false);
assert.strictEqual(result.environment, undefined);
assert.strictEqual(result.environmentCreationError, envError);
assert.strictEqual(result.packageInstallationError, undefined);
});

test('should handle case with no packages to install', () => {
const result: CreateEnvironmentResult = {
environment: undefined, // Mock environment would go here
environmentCreated: true,
packagesInstalled: true, // No packages means "successful"
};

assert.strictEqual(result.environmentCreated, true);
assert.strictEqual(result.packagesInstalled, true);
assert.strictEqual(result.attemptedPackages, undefined);
});
});
});