Skip to content

Commit 1db75b5

Browse files
rchiodorchiodo
authored
Handle failures with conda activation (microsoft#4669)
For #4424 <!-- If an item below does not apply to you, then go ahead and check it off as "done" and strikethrough the text, e.g.: - [x] ~Has unit tests & system/integration tests~ --> - [x] Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR) - [ ] Title summarizes what is changing - [x] Has a [news entry](https://github.com/Microsoft/vscode-python/tree/master/news) file (remember to thank yourself!) - [x] Has sufficient logging. - [ ] Has telemetry for enhancements. - [ ] Unit tests & system/integration tests are added/updated - [ ] [Test plan](https://github.com/Microsoft/vscode-python/blob/master/.github/test_plan.md) is updated as appropriate - [ ] [`package-lock.json`](https://github.com/Microsoft/vscode-python/blob/master/package-lock.json) has been regenerated by running `npm install` (if dependencies have changed) - [ ] The wiki is updated with any design decisions/details.
1 parent 97444ef commit 1db75b5

12 files changed

Lines changed: 101 additions & 61 deletions

File tree

‎news/3 Code Health/4424.md‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1,2 @@
1-
Add more logging to diagnose issues getting the Python Interactive window to show up
1+
Add more logging to diagnose issues getting the Python Interactive window to show up.
2+
Add checks for conda activation never finishing.

‎package.nls.json‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -96,6 +96,7 @@
9696
"DataScience.notebookCheckForImportDontAskAgain": "Don't Ask Again",
9797
"DataScience.notebookCheckForImportTitle": "Do you want to import the Jupyter Notebook into Python code?",
9898
"DataScience.jupyterNotSupported": "Running cells requires Jupyter notebooks to be installed.",
99+
"DataScience.jupyterNotSupportedBecauseOfEnvironment": "Activating {0} to run Jupyter failed with {1}.",
99100
"DataScience.jupyterNbConvertNotSupported": "Importing notebooks requires Jupyter nbconvert to be installed.",
100101
"DataScience.jupyterLaunchNoURL": "Failed to find the URL of the launched Jupyter notebook server",
101102
"DataScience.jupyterLaunchTimedOut": "The Jupyter notebook server failed to launch in time",

‎src/client/common/process/pythonExecutionFactory.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ export class PythonExecutionFactory implements IPythonExecutionFactory {
3232
return new PythonExecutionService(this.serviceContainer, processService, pythonPath);
3333
}
3434
public async createActivatedEnvironment(options: ExecutionFactoryCreateWithEnvironmentOptions): Promise<IPythonExecutionService> {
35-
const envVars = await this.activationHelper.getActivatedEnvironmentVariables(options.resource, options.interpreter);
35+
const envVars = await this.activationHelper.getActivatedEnvironmentVariables(options.resource, options.interpreter, options.allowEnvironmentFetchExceptions);
3636
const hasEnvVars = envVars && Object.keys(envVars).length > 0;
3737
sendTelemetryEvent(EventName.PYTHON_INTERPRETER_ACTIVATION_ENVIRONMENT_VARIABLES, undefined, { hasEnvVars });
3838
if (!hasEnvVars) {

‎src/client/common/process/types.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,7 @@ export type ExecutionFactoryCreationOptions = {
6060
export type ExecutionFactoryCreateWithEnvironmentOptions = {
6161
resource?: Uri;
6262
interpreter?: PythonInterpreter;
63+
allowEnvironmentFetchExceptions?: boolean;
6364
};
6465
export interface IPythonExecutionFactory {
6566
create(options: ExecutionFactoryCreationOptions): Promise<IPythonExecutionService>;

‎src/client/common/utils/localize.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,7 @@ export namespace DataScience {
7979
export const notebookCheckForImportNo = localize('DataScience.notebookCheckForImportNo', 'Later');
8080
export const notebookCheckForImportDontAskAgain = localize('DataScience.notebookCheckForImportDontAskAgain', 'Don\'t Ask Again');
8181
export const jupyterNotSupported = localize('DataScience.jupyterNotSupported', 'Jupyter is not installed');
82+
export const jupyterNotSupportedBecauseOfEnvironment = localize('DataScience.jupyterNotSupportedBecauseOfEnvironment', 'Activating {0} to run Jupyter failed with {1}');
8283
export const jupyterNbConvertNotSupported = localize('DataScience.jupyterNbConvertNotSupported', 'Jupyter nbconvert is not installed');
8384
export const jupyterLaunchTimedOut = localize('DataScience.jupyterLaunchTimedOut', 'The Jupyter notebook server failed to launch in time');
8485
export const jupyterLaunchNoURL = localize('DataScience.jupyterLaunchNoURL', 'Failed to find the URL of the launched Jupyter notebook server');

‎src/client/datascience/history.ts‎

Lines changed: 34 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ import { IFileSystem } from '../common/platform/types';
2727
import { IConfigurationService, IDisposable, IDisposableRegistry, ILogger } from '../common/types';
2828
import { createDeferred, Deferred } from '../common/utils/async';
2929
import * as localize from '../common/utils/localize';
30-
import { IInterpreterService } from '../interpreter/contracts';
30+
import { IInterpreterService, PythonInterpreter } from '../interpreter/contracts';
3131
import { captureTelemetry, sendTelemetryEvent } from '../telemetry';
3232
import { EditorContexts, Identifiers, Telemetry } from './constants';
3333
import { HistoryMessageListener } from './historyMessageListener';
@@ -815,21 +815,12 @@ export class History implements IHistory {
815815
}
816816
}
817817

818-
private load = async (): Promise<void> => {
819-
// Status depends upon if we're about to connect to existing server or not.
820-
const status = (await this.jupyterExecution.getServer(await this.historyProvider.getNotebookOptions())) ?
821-
this.setStatus(localize.DataScience.connectingToJupyter()) : this.setStatus(localize.DataScience.startingJupyter());
822-
823-
// Check to see if we support ipykernel or not
818+
private async checkUsable() : Promise<boolean> {
819+
let activeInterpreter : PythonInterpreter | undefined;
824820
try {
821+
activeInterpreter = await this.interpreterService.getActiveInterpreter();
825822
const usableInterpreter = await this.jupyterExecution.getUsableJupyterPython();
826-
if (!usableInterpreter) {
827-
// Not loading anymore
828-
status.dispose();
829-
830-
// Nobody is useable, throw an exception
831-
throw new JupyterInstallError(localize.DataScience.jupyterNotSupported(), localize.DataScience.pythonInteractiveHelpLink());
832-
} else {
823+
if (usableInterpreter) {
833824
// See if the usable interpreter is not our active one. If so, show a warning
834825
// Only do this if not the guest in a liveshare session
835826
const api = await this.liveShare.getApi();
@@ -845,6 +836,35 @@ export class History implements IHistory {
845836
}
846837
}
847838

839+
return usableInterpreter ? true : false;
840+
841+
} catch (e) {
842+
// Can't find a usable interpreter, show the error.
843+
if (activeInterpreter) {
844+
const displayName = activeInterpreter.displayName ? activeInterpreter.displayName : activeInterpreter.path;
845+
throw new Error(localize.DataScience.jupyterNotSupportedBecauseOfEnvironment().format(displayName, e.toString()));
846+
} else {
847+
throw new JupyterInstallError(localize.DataScience.jupyterNotSupported(), localize.DataScience.pythonInteractiveHelpLink());
848+
}
849+
}
850+
}
851+
852+
private load = async (): Promise<void> => {
853+
// Status depends upon if we're about to connect to existing server or not.
854+
const status = (await this.jupyterExecution.getServer(await this.historyProvider.getNotebookOptions())) ?
855+
this.setStatus(localize.DataScience.connectingToJupyter()) : this.setStatus(localize.DataScience.startingJupyter());
856+
857+
// Check to see if we support ipykernel or not
858+
try {
859+
const usable = await this.checkUsable();
860+
if (!usable) {
861+
// Not loading anymore
862+
status.dispose();
863+
864+
// Indicate failing.
865+
throw new JupyterInstallError(localize.DataScience.jupyterNotSupported(), localize.DataScience.pythonInteractiveHelpLink());
866+
}
867+
848868
// Get the web panel to show first
849869
await this.loadWebPanel();
850870

‎src/client/datascience/jupyter/jupyterCommand.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,7 @@ class InterpreterJupyterCommand implements IJupyterCommand {
7171
constructor(args: string[], pythonExecutionFactory: IPythonExecutionFactory, interpreter: PythonInterpreter) {
7272
this.requiredArgs = args;
7373
this.interpreterPromise = Promise.resolve(interpreter);
74-
this.pythonLauncher = pythonExecutionFactory.createActivatedEnvironment({ resource: undefined, interpreter });
74+
this.pythonLauncher = pythonExecutionFactory.createActivatedEnvironment({ resource: undefined, interpreter, allowEnvironmentFetchExceptions: true });
7575
}
7676

7777
public interpreter() : Promise<PythonInterpreter | undefined> {

‎src/client/datascience/jupyter/jupyterExecution.ts‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -356,7 +356,8 @@ export class JupyterExecutionBase implements IJupyterExecution {
356356
const bestInterpreter = await this.getUsableJupyterPython(cancelToken);
357357
if (bestInterpreter) {
358358
const newOptions: SpawnOptions = { mergeStdOutErr: true, token: cancelToken };
359-
const launcher = await this.executionFactory.createActivatedEnvironment({ resource: undefined, interpreter: bestInterpreter });
359+
const launcher = await this.executionFactory.createActivatedEnvironment(
360+
{ resource: undefined, interpreter: bestInterpreter, allowEnvironmentFetchExceptions: true });
360361
const file = path.join(EXTENSION_ROOT_DIR, 'pythonFiles', 'datascience', 'getServerInfo.py');
361362
const serverInfoString = await launcher.exec([file], newOptions);
362363

@@ -732,14 +733,14 @@ export class JupyterExecutionBase implements IJupyterExecution {
732733
}
733734
}
734735

735-
// Return result
736+
// Return results
736737
return this.commands.hasOwnProperty(command) ? this.commands[command] : undefined;
737738
}
738739

739740
private doesModuleExist = async (module: string, interpreter: PythonInterpreter, cancelToken?: CancellationToken): Promise<boolean> => {
740741
if (interpreter && interpreter !== null) {
741742
const newOptions: SpawnOptions = { throwOnStdErr: true, encoding: 'utf8', token: cancelToken };
742-
const pythonService = await this.executionFactory.createActivatedEnvironment({ resource: undefined, interpreter });
743+
const pythonService = await this.executionFactory.createActivatedEnvironment({ resource: undefined, interpreter, allowEnvironmentFetchExceptions: true });
743744
try {
744745
// Special case for ipykernel
745746
const actualModule = module === JupyterCommands.KernelCreateCommand ? module : 'jupyter';

‎src/client/interpreter/activation/service.ts‎

Lines changed: 39 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -6,15 +6,14 @@ import '../../common/extensions';
66
import { inject, injectable } from 'inversify';
77
import * as path from 'path';
88

9-
import { LogOptions, traceDecorators, traceVerbose } from '../../common/logger';
9+
import { LogOptions, traceDecorators, traceError, traceVerbose } from '../../common/logger';
1010
import { IPlatformService } from '../../common/platform/types';
1111
import { IProcessServiceFactory } from '../../common/process/types';
1212
import { ITerminalHelper } from '../../common/terminal/types';
1313
import { ICurrentProcess, IDisposable, Resource } from '../../common/types';
1414
import {
1515
cacheResourceSpecificInterpreterData,
16-
clearCachedResourceSpecificIngterpreterData,
17-
swallowExceptions
16+
clearCachedResourceSpecificIngterpreterData
1817
} from '../../common/utils/decorators';
1918
import { OSType } from '../../common/utils/platform';
2019
import { IEnvironmentVariablesProvider } from '../../common/variables/types';
@@ -26,6 +25,7 @@ import { IEnvironmentActivationService } from './types';
2625

2726
const getEnvironmentPrefix = 'e8b39361-0157-4923-80e1-22d70d46dee6';
2827
const cacheDuration = 10 * 60 * 1000;
28+
const getEnvironmentTimeout = 30000;
2929

3030
// The shell under which we'll execute activation scripts.
3131
const defaultShells = {
@@ -51,39 +51,52 @@ export class EnvironmentActivationService implements IEnvironmentActivationServi
5151
this.disposables.forEach(d => d.dispose());
5252
}
5353
@traceDecorators.verbose('getActivatedEnvironmentVariables', LogOptions.Arguments)
54-
@swallowExceptions('getActivatedEnvironmentVariables')
5554
@captureTelemetry(EventName.PYTHON_INTERPRETER_ACTIVATION_ENVIRONMENT_VARIABLES, { failed: false }, true)
5655
@cacheResourceSpecificInterpreterData('ActivatedEnvironmentVariables', cacheDuration)
57-
public async getActivatedEnvironmentVariables(resource: Resource, interpreter?: PythonInterpreter): Promise<NodeJS.ProcessEnv | undefined> {
56+
public async getActivatedEnvironmentVariables(resource: Resource, interpreter?: PythonInterpreter, allowExceptions?: boolean): Promise<NodeJS.ProcessEnv | undefined> {
5857
const shell = defaultShells[this.platform.osType];
5958
if (!shell) {
6059
return;
6160
}
6261

63-
const activationCommands = await this.helper.getEnvironmentActivationShellCommands(resource, interpreter);
64-
traceVerbose(`Activation Commands received ${activationCommands}`);
65-
if (!activationCommands || !Array.isArray(activationCommands) || activationCommands.length === 0) {
66-
return;
67-
}
62+
try {
63+
const activationCommands = await this.helper.getEnvironmentActivationShellCommands(resource, interpreter);
64+
traceVerbose(`Activation Commands received ${activationCommands}`);
65+
if (!activationCommands || !Array.isArray(activationCommands) || activationCommands.length === 0) {
66+
return;
67+
}
68+
69+
// Run the activate command collect the environment from it.
70+
const activationCommand = this.fixActivationCommands(activationCommands).join(' && ');
71+
const processService = await this.processServiceFactory.create(resource);
72+
const customEnvVars = await this.envVarsService.getEnvironmentVariables(resource);
73+
const hasCustomEnvVars = Object.keys(customEnvVars).length;
74+
const env = hasCustomEnvVars ? customEnvVars : this.currentProcess.env;
75+
traceVerbose(`${hasCustomEnvVars ? 'Has' : 'No'} Custom Env Vars`);
76+
77+
// In order to make sure we know where the environment output is,
78+
// put in a dummy echo we can look for
79+
const printEnvPyFile = path.join(EXTENSION_ROOT_DIR, 'pythonFiles', 'printEnvVariables.py');
80+
const command = `${activationCommand} && echo '${getEnvironmentPrefix}' && python ${printEnvPyFile.fileToCommandArgument()}`;
81+
traceVerbose(`Activating Environment to capture Environment variables, ${command}`);
6882

69-
// Run the activate command collect the environment from it.
70-
const activationCommand = this.fixActivationCommands(activationCommands).join(' && ');
71-
const processService = await this.processServiceFactory.create(resource);
72-
const customEnvVars = await this.envVarsService.getEnvironmentVariables(resource);
73-
const hasCustomEnvVars = Object.keys(customEnvVars).length;
74-
const env = hasCustomEnvVars ? customEnvVars : this.currentProcess.env;
75-
traceVerbose(`${hasCustomEnvVars ? 'Has' : 'No'} Custom Env Vars`);
83+
// Conda activate can hang on certain systems. Fail after 30 seconds.
84+
// See the discussion from hidesoon in this issue: https://github.com/Microsoft/vscode-python/issues/4424
85+
// His issue is conda never finishing during activate. This is a conda issue, but we
86+
// should at least tell the user.
87+
const result = await processService.shellExec(command, { env, shell, timeout: getEnvironmentTimeout });
88+
if (result.stderr && result.stderr.length > 0) {
89+
throw new Error(`StdErr from ShellExec, ${result.stderr}`);
90+
}
91+
return this.parseEnvironmentOutput(result.stdout);
92+
} catch (e) {
93+
traceError('getActivatedEnvironmentVariables', e);
7694

77-
// In order to make sure we know where the environment output is,
78-
// put in a dummy echo we can look for
79-
const printEnvPyFile = path.join(EXTENSION_ROOT_DIR, 'pythonFiles', 'printEnvVariables.py');
80-
const command = `${activationCommand} && echo '${getEnvironmentPrefix}' && python ${printEnvPyFile.fileToCommandArgument()}`;
81-
traceVerbose(`Activating Environment to capture Environment variables, ${command}`);
82-
const result = await processService.shellExec(command, { env, shell });
83-
if (result.stderr && result.stderr.length > 0) {
84-
throw new Error(`StdErr from ShellExec, ${result.stderr}`);
95+
// Some callers want this to bubble out, others don't
96+
if (allowExceptions) {
97+
throw e;
98+
}
8599
}
86-
return this.parseEnvironmentOutput(result.stdout);
87100
}
88101
protected onDidEnvironmentVariablesChange(affectedResource: Resource) {
89102
clearCachedResourceSpecificIngterpreterData('ActivatedEnvironmentVariables', affectedResource);

‎src/client/interpreter/activation/types.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,5 +8,5 @@ import { PythonInterpreter } from '../contracts';
88

99
export const IEnvironmentActivationService = Symbol('IEnvironmentActivationService');
1010
export interface IEnvironmentActivationService {
11-
getActivatedEnvironmentVariables(resource: Resource, interpreter?: PythonInterpreter): Promise<NodeJS.ProcessEnv | undefined>;
11+
getActivatedEnvironmentVariables(resource: Resource, interpreter?: PythonInterpreter, allowExceptions?: boolean): Promise<NodeJS.ProcessEnv | undefined>;
1212
}

0 commit comments

Comments
 (0)