-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Add support for "conda run" in PythonExecutionFactory #8259
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
Changes from all commits
66468a0
4fda3b0
73bee3c
d68445e
de9dddb
16e5542
6d91b33
2646ea2
8296bd0
389bb59
a79252a
bdaa375
865a216
e6b37c3
e294b4d
5d8a1d3
82eafec
9b8f948
780538e
0068e0d
637be5f
ad42ee8
5255e45
6a0ab56
2a43052
3b304be
1fea07e
1361103
d47072d
fe5601b
45160bf
7319345
3e972ac
4f7cc14
1719adf
1b3eb47
2ffe187
1df9f8a
faf1f39
6a772bf
938ebd6
689bf1f
42ba9ff
923dea2
365c92a
77fac1f
fd4ddd6
6edb1da
cf3ba57
5fb07a4
61d685a
55f96e9
77d94a1
68216fa
ee42991
3442437
a1ae719
6d176bc
eb7625b
5dbd096
9c5724e
38c4321
75076e4
a4f0da5
b6b3fd3
610fb01
eddcf15
269c54b
394c85f
0f93c43
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
Use "conda run" (instead of using the "python.pythonPath" setting directly) when executing | ||
Python and an Anaconda environment is selected. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
// Copyright (c) Microsoft Corporation. All rights reserved. | ||
// Licensed under the MIT License. | ||
import { injectable } from 'inversify'; | ||
import { CondaEnvironmentInfo } from '../../interpreter/contracts'; | ||
import { IServiceContainer } from '../../ioc/types'; | ||
import { PythonExecutionService } from './pythonProcess'; | ||
import { IProcessService, PythonExecutionInfo } from './types'; | ||
|
||
@injectable() | ||
export class CondaExecutionService extends PythonExecutionService { | ||
constructor( | ||
serviceContainer: IServiceContainer, | ||
procService: IProcessService, | ||
pythonPath: string, | ||
private readonly condaFile: string, | ||
private readonly condaEnvironment: CondaEnvironmentInfo | ||
) { | ||
super(serviceContainer, procService, pythonPath); | ||
} | ||
|
||
public getExecutionInfo(args: string[]): PythonExecutionInfo { | ||
const executionArgs = this.condaEnvironment.name !== '' ? ['-n', this.condaEnvironment.name] : ['-p', this.condaEnvironment.path]; | ||
|
||
return { command: this.condaFile, args: ['run', ...executionArgs, 'python', ...args] }; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,6 +18,7 @@ import { | |
IProcessService, | ||
IPythonExecutionService, | ||
ObservableExecutionResult, | ||
PythonExecutionInfo, | ||
PythonVersionInfo, | ||
SpawnOptions | ||
} from './types'; | ||
|
@@ -41,7 +42,8 @@ export class PythonExecutionService implements IPythonExecutionService { | |
// See these two bugs: | ||
// https://github.com/microsoft/vscode-python/issues/7569 | ||
// https://github.com/microsoft/vscode-python/issues/7760 | ||
const jsonValue = await waitForPromise(this.procService.exec(this.pythonPath, [file], { mergeStdOutErr: true }), 5000) | ||
const { command, args } = this.getExecutionInfo([file]); | ||
const jsonValue = await waitForPromise(this.procService.exec(command, args, { mergeStdOutErr: true }), 5000) | ||
.then(output => output ? output.stdout.trim() : '--timed out--'); // --timed out-- should cause an exception | ||
|
||
let json: { versionInfo: PythonVersionInfo; sysPrefix: string; sysVersion: string; is64Bit: boolean }; | ||
|
@@ -69,29 +71,41 @@ export class PythonExecutionService implements IPythonExecutionService { | |
if (await this.fileSystem.fileExists(this.pythonPath)) { | ||
kimadeline marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return this.pythonPath; | ||
kimadeline marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
return this.procService.exec(this.pythonPath, ['-c', 'import sys;print(sys.executable)'], { throwOnStdErr: true }) | ||
.then(output => output.stdout.trim()); | ||
|
||
const { command, args } = this.getExecutionInfo(['-c', 'import sys;print(sys.executable)']); | ||
return this.procService.exec(command, args, { throwOnStdErr: true }).then(output => output.stdout.trim()); | ||
} | ||
public async isModuleInstalled(moduleName: string): Promise<boolean> { | ||
return this.procService.exec(this.pythonPath, ['-c', `import ${moduleName}`], { throwOnStdErr: true }) | ||
const { command, args } = this.getExecutionInfo(['-c', `import ${moduleName}`]); | ||
return this.procService.exec(command, args, { throwOnStdErr: true }) | ||
.then(() => true).catch(() => false); | ||
} | ||
|
||
public getExecutionInfo(args: string[]): PythonExecutionInfo { | ||
return { command: this.pythonPath, args }; | ||
} | ||
|
||
public execObservable(args: string[], options: SpawnOptions): ObservableExecutionResult<string> { | ||
const opts: SpawnOptions = { ...options }; | ||
// Cannot use this.getExecutionInfo() until 'conda run' can be run without buffering output. | ||
// See https://github.com/microsoft/vscode-python/issues/8473 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please ensure you use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because today we use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Further to this, So, anywhere We have a couple of options:
Finally, i'd prefer if these were done as separate PRs (issues), as opposed to doing them all in one go in a large PR...I.e. first implement conda exec service, then another, then integrate in a few places, etc... Else it just feels too large (to review) @karthiknadig /cc There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Lets talk |
||
return this.procService.execObservable(this.pythonPath, args, opts); | ||
} | ||
public execModuleObservable(moduleName: string, args: string[], options: SpawnOptions): ObservableExecutionResult<string> { | ||
const opts: SpawnOptions = { ...options }; | ||
// Cannot use this.getExecutionInfo() until 'conda run' can be run without buffering output. | ||
// See https://github.com/microsoft/vscode-python/issues/8473 | ||
return this.procService.execObservable(this.pythonPath, ['-m', moduleName, ...args], opts); | ||
} | ||
public async exec(args: string[], options: SpawnOptions): Promise<ExecutionResult<string>> { | ||
const opts: SpawnOptions = { ...options }; | ||
return this.procService.exec(this.pythonPath, args, opts); | ||
const executable = this.getExecutionInfo(args); | ||
return this.procService.exec(executable.command, executable.args, opts); | ||
} | ||
public async execModule(moduleName: string, args: string[], options: SpawnOptions): Promise<ExecutionResult<string>> { | ||
const opts: SpawnOptions = { ...options }; | ||
const result = await this.procService.exec(this.pythonPath, ['-m', moduleName, ...args], opts); | ||
const executable = this.getExecutionInfo(['-m', moduleName, ...args]); | ||
const result = await this.procService.exec(executable.command, executable.args, opts); | ||
|
||
// If a module is not installed we'll have something in stderr. | ||
if (moduleName && ErrorUtils.outputHasModuleNotInstalledError(moduleName!, result.stderr)) { | ||
|
Uh oh!
There was an error while loading. Please reload this page.