Skip to content

Commit e18d09b

Browse files
IanMatthewHuffIan Huff
IanMatthewHuff
and
Ian Huff
authored
don't try to prewarm an execution service (microsoft#13377)
* don't try to prewarm an execution service * remove funky old cast * update unit tests Co-authored-by: Ian Huff <ianhuff@ravikun-dev2.redmond.corp.microsoft.com>
1 parent c06f2d3 commit e18d09b

7 files changed

Lines changed: 44 additions & 20 deletions

File tree

‎news/2 Fixes/13258.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Prevent daemon from trying to prewarm an execution service.

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,7 @@ export class PythonExecutionFactory implements IPythonExecutionFactory {
7676

7777
public async createDaemon<T extends IPythonDaemonExecutionService | IDisposable>(
7878
options: DaemonExecutionFactoryCreationOptions
79-
): Promise<T> {
79+
): Promise<T | IPythonExecutionService> {
8080
const pythonPath = options.pythonPath
8181
? options.pythonPath
8282
: this.configService.getSettings(options.resource).pythonPath;
@@ -93,7 +93,7 @@ export class PythonExecutionFactory implements IPythonExecutionFactory {
9393
});
9494
// No daemon support in Python 2.7 or during shutdown
9595
if (!interpreterService || (interpreter?.version && interpreter.version.major < 3)) {
96-
return (activatedProcPromise! as unknown) as T;
96+
return activatedProcPromise;
9797
}
9898

9999
// Ensure we do not start multiple daemons for the same interpreter.

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -154,7 +154,7 @@ export interface IPythonExecutionFactory {
154154
*/
155155
createDaemon<T extends IPythonDaemonExecutionService | IDisposable>(
156156
options: DaemonExecutionFactoryCreationOptions
157-
): Promise<T>;
157+
): Promise<T | IPythonExecutionService>;
158158
createActivatedEnvironment(options: ExecutionFactoryCreateWithEnvironmentOptions): Promise<IPythonExecutionService>;
159159
createCondaExecutionService(
160160
pythonPath: string,

‎src/client/datascience/kernel-launcher/kernelDaemonPool.ts‎

Lines changed: 34 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ import { inject, injectable } from 'inversify';
77
import { IWorkspaceService } from '../../common/application/types';
88
import { traceError } from '../../common/logger';
99

10-
import { IPythonExecutionFactory } from '../../common/process/types';
10+
import { IPythonExecutionFactory, IPythonExecutionService } from '../../common/process/types';
1111
import { IDisposable, Resource } from '../../common/types';
1212
import { noop } from '../../common/utils/misc';
1313
import { IEnvironmentVariablesProvider } from '../../common/variables/types';
@@ -23,7 +23,7 @@ type IKernelDaemonInfo = {
2323
workspaceResource: Resource;
2424
workspaceFolderIdentifier: string;
2525
interpreterPath: string;
26-
daemon: Promise<IPythonKernelDaemon>;
26+
daemon: Promise<IPythonKernelDaemon | IPythonExecutionService>;
2727
};
2828

2929
@injectable()
@@ -68,7 +68,7 @@ export class KernelDaemonPool implements IDisposable {
6868
resource: Resource,
6969
kernelSpec: IJupyterKernelSpec,
7070
interpreter?: PythonInterpreter
71-
): Promise<IPythonKernelDaemon> {
71+
): Promise<IPythonKernelDaemon | IPythonExecutionService> {
7272
const pythonPath = interpreter?.path || kernelSpec.argv[0];
7373
// If we have environment variables in the kernel.json, then its not we support.
7474
// Cuz there's no way to know before hand what kernelspec can be used, hence no way to know what envs are required.
@@ -104,14 +104,26 @@ export class KernelDaemonPool implements IDisposable {
104104
dedicated: true,
105105
resource
106106
});
107-
daemon.then((d) => this.disposables.push(d)).catch(noop);
107+
daemon
108+
.then((d) => {
109+
if ('dispose' in d) {
110+
this.disposables.push(d);
111+
}
112+
})
113+
.catch(noop);
108114
return daemon;
109115
}
110116
private async onDidEnvironmentVariablesChange(affectedResoruce: Resource) {
111117
const workspaceFolderIdentifier = this.workspaceService.getWorkspaceFolderIdentifier(affectedResoruce);
112118
this.daemonPool = this.daemonPool.filter((item) => {
113119
if (item.workspaceFolderIdentifier === workspaceFolderIdentifier) {
114-
item.daemon.then((d) => d.dispose()).catch(noop);
120+
item.daemon
121+
.then((d) => {
122+
if ('dispose' in d) {
123+
d.dispose();
124+
}
125+
})
126+
.catch(noop);
115127
return false;
116128
}
117129
return true;
@@ -132,7 +144,16 @@ export class KernelDaemonPool implements IDisposable {
132144
const daemon = this.createDaemon(resource, interpreter.path);
133145
// Once a daemon is created ensure we pre-warm it (will load ipykernel and start the kernker process waiting to start the actual kernel code).
134146
// I.e. we'll start python process thats the kernel, but will not start the kernel module (`python -m ipykernel`).
135-
daemon.then((d) => d.preWarm()).catch(traceError.bind(`Failed to prewarm kernel daemon ${interpreter.path}`));
147+
daemon
148+
.then((d) => {
149+
// Prewarm if we support prewarming
150+
if ('preWarm' in d) {
151+
d.preWarm().catch(traceError.bind(`Failed to prewarm kernel daemon ${interpreter.path}`));
152+
}
153+
})
154+
.catch((e) => {
155+
traceError(`Failed to load deamon: ${e}`);
156+
});
136157
this.daemonPool.push({
137158
daemon,
138159
interpreterPath: interpreter.path,
@@ -172,7 +193,13 @@ export class KernelDaemonPool implements IDisposable {
172193
this.daemonPool = this.daemonPool.filter((item) => {
173194
const interpreterForWorkspace = currentInterpreterInEachWorksapce.get(item.key);
174195
if (!interpreterForWorkspace || !this.fs.areLocalPathsSame(interpreterForWorkspace, item.interpreterPath)) {
175-
item.daemon.then((d) => d.dispose()).catch(noop);
196+
item.daemon
197+
.then((d) => {
198+
if ('dispose' in d) {
199+
d.dispose();
200+
}
201+
})
202+
.catch(noop);
176203
return false;
177204
}
178205

‎src/client/datascience/kernel-launcher/kernelLauncherDaemon.ts‎

Lines changed: 3 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66
import { ChildProcess } from 'child_process';
77
import { inject, injectable } from 'inversify';
88
import { IDisposable } from 'monaco-editor';
9-
import { IPythonExecutionService, ObservableExecutionResult } from '../../common/process/types';
9+
import { ObservableExecutionResult } from '../../common/process/types';
1010
import { Resource } from '../../common/types';
1111
import { noop } from '../../common/utils/misc';
1212
import { PythonInterpreter } from '../../pythonEnvironments/info';
@@ -47,14 +47,9 @@ export class PythonKernelLauncherDaemon implements IDisposable {
4747

4848
// The daemon pool can return back a non-IPythonKernelDaemon if daemon service is not supported or for Python 2.
4949
// Use a check for the daemon.start function here before we call it.
50-
if (!daemon.start) {
50+
if (!('start' in daemon)) {
5151
// If we don't have a KernelDaemon here then we have an execution service and should use that to launch
52-
// Typing is a bit funk here, as createDaemon can return an execution service instead of the requested
53-
// daemon class
54-
// tslint:disable-next-line:no-any
55-
const executionService = (daemon as any) as IPythonExecutionService;
56-
57-
const observableOutput = executionService.execModuleObservable(moduleName, moduleArgs, {
52+
const observableOutput = daemon.execModuleObservable(moduleName, moduleArgs, {
5853
env,
5954
cwd: workingDirectory
6055
});

‎src/test/datascience/kernel-launcher/kernelDaemonPool.unit.test.ts‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,9 @@ suite('DataScience - Kernel Daemon Pool', () => {
6666
when(daemon1.preWarm()).thenResolve();
6767
when(daemon2.preWarm()).thenResolve();
6868
when(daemon3.preWarm()).thenResolve();
69+
when(daemon1.dispose()).thenResolve();
70+
when(daemon2.dispose()).thenResolve();
71+
when(daemon3.dispose()).thenResolve();
6972

7073
when(envVars.onDidEnvironmentVariablesChange).thenReturn(didEnvVarsChange.event);
7174
when(interpeterService.onDidChangeInterpreter).thenReturn(didChangeInterpreter.event);

‎src/test/datascience/kernel-launcher/kernelLauncherDaemon.unit.test.ts‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -66,8 +66,6 @@ suite('DataScience - Kernel Launcher Daemon', () => {
6666
when(
6767
executionService.execModuleObservable('ipkernel_launcher', deepEqual(['-f', 'file.json']), anything())
6868
).thenReturn(instance(observableOutputForDaemon));
69-
// 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
70-
when((executionService as any).start).thenReturn(false);
7169
// Else ts-mockit doesn't allow us to return an instance of a mock as a return value from an async function.
7270
(instance(executionService) as any).then = undefined;
7371
when(daemonPool.get(anything(), anything(), anything())).thenResolve(instance(executionService) as any);

0 commit comments

Comments
 (0)