Skip to content

Use python execution service, not general process execution service for non-daemon raw start. #12846

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

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/2 Fixes/12821.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Use the given interpreter for launching the non-daemon python
34 changes: 22 additions & 12 deletions src/client/datascience/kernel-launcher/kernelLauncherDaemon.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
import { ChildProcess } from 'child_process';
import { inject, injectable } from 'inversify';
import { IDisposable } from 'monaco-editor';
import { ObservableExecutionResult } from '../../common/process/types';
import { IPythonExecutionService, ObservableExecutionResult } from '../../common/process/types';
import { Resource } from '../../common/types';
import { noop } from '../../common/utils/misc';
import { PythonInterpreter } from '../../pythonEnvironments/info';
Expand All @@ -27,15 +27,10 @@ export class PythonKernelLauncherDaemon implements IDisposable {
resource: Resource,
kernelSpec: IJupyterKernelSpec,
interpreter?: PythonInterpreter
): Promise<{ observableOutput: ObservableExecutionResult<string>; daemon: IPythonKernelDaemon } | undefined> {
): Promise<{ observableOutput: ObservableExecutionResult<string>; daemon: IPythonKernelDaemon | undefined }> {
const daemon = await this.daemonPool.get(resource, kernelSpec, interpreter);

// The daemon pool can return back a non-IPythonKernelDaemon if daemon service is not supported or for Python 2.
// Use a check for the daemon.start function here before we call it.
if (!daemon.start) {
return undefined;
}

// Check to see if we have the type of kernelspec that we expect
const args = kernelSpec.argv.slice();
const modulePrefixIndex = args.findIndex((item) => item === '-m');
if (modulePrefixIndex === -1) {
Expand All @@ -49,11 +44,26 @@ export class PythonKernelLauncherDaemon implements IDisposable {
const moduleArgs = args.slice(modulePrefixIndex + 2);
const env = kernelSpec.env && Object.keys(kernelSpec.env).length > 0 ? kernelSpec.env : undefined;

const observableOutput = await daemon.start(moduleName, moduleArgs, { env });
if (observableOutput.proc) {
this.processesToDispose.push(observableOutput.proc);
// The daemon pool can return back a non-IPythonKernelDaemon if daemon service is not supported or for Python 2.
// Use a check for the daemon.start function here before we call it.
if (!daemon.start) {
// If we don't have a KernelDaemon here then we have an execution service and should use that to launch
// Typing is a bit funk here, as createDaemon can return an execution service instead of the requested
// daemon class
// tslint:disable-next-line:no-any
const executionService = (daemon as any) as IPythonExecutionService;

const observableOutput = executionService.execModuleObservable(moduleName, moduleArgs, { env });

return { observableOutput, daemon: undefined };
} else {
// In the case that we do have a kernel deamon, just return it
const observableOutput = await daemon.start(moduleName, moduleArgs, { env });
if (observableOutput.proc) {
this.processesToDispose.push(observableOutput.proc);
}
return { observableOutput, daemon };
}
return { observableOutput, daemon };
}
public dispose() {
while (this.processesToDispose.length) {
Expand Down
8 changes: 3 additions & 5 deletions src/client/datascience/kernel-launcher/kernelProcess.ts
Original file line number Diff line number Diff line change
Expand Up @@ -224,13 +224,11 @@ export class KernelProcess implements IKernelProcess {
this.interpreter
);

if (kernelDaemonLaunch) {
this.kernelDaemon = kernelDaemonLaunch.daemon;
exeObs = kernelDaemonLaunch.observableOutput;
}
this.kernelDaemon = kernelDaemonLaunch.daemon;
exeObs = kernelDaemonLaunch.observableOutput;
}

// If we are not python or if we failed with our daemon launch just use the ProcessExecutionFactory
// If we are not python just use the ProcessExecutionFactory
if (!exeObs) {
// First part of argument is always the executable.
const executable = this._kernelSpec.argv[0];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

import { assert } from 'chai';
import { anything, deepEqual, instance, mock, when } from 'ts-mockito';
import { ObservableExecutionResult } from '../../../client/common/process/types';
import { IPythonExecutionService, ObservableExecutionResult } from '../../../client/common/process/types';
import { ReadWrite } from '../../../client/common/types';
import { KernelDaemonPool } from '../../../client/datascience/kernel-launcher/kernelDaemonPool';
import { PythonKernelLauncherDaemon } from '../../../client/datascience/kernel-launcher/kernelLauncherDaemon';
Expand Down Expand Up @@ -61,10 +61,23 @@ suite('Data Science - Kernel Launcher Daemon', () => {
assert.equal(daemonCreationOutput.daemon, instance(kernelDaemon));
}
});
test('If our daemon pool return does not support start, return undefined', async () => {
when(daemonPool.get(anything(), anything(), anything())).thenResolve({} as any);
test('If our daemon pool returns an execution service, then use it and return the daemon as undefined', async () => {
const executionService = mock<IPythonExecutionService>();
when(
executionService.execModuleObservable(
'ipkernel_launcher',
deepEqual(['-f', 'file.json']),
deepEqual({ env: kernelSpec.env })
)
).thenReturn(instance(observableOutputForDaemon));
// Make sure that it doesn't have a start function. Normally the proxy will pretend to have a start function, which we are checking for non-existance
when((executionService as any).start).thenReturn(false);
// Else ts-mockit doesn't allow us to return an instance of a mock as a return value from an async function.
(instance(executionService) as any).then = undefined;
when(daemonPool.get(anything(), anything(), anything())).thenResolve(instance(executionService) as any);
const daemonCreationOutput = await launcher.launch(undefined, kernelSpec, interpreter);

assert.isUndefined(daemonCreationOutput);
assert.equal(daemonCreationOutput.observableOutput, instance(observableOutputForDaemon));
assert.isUndefined(daemonCreationOutput.daemon);
});
});