Skip to content

don't try to prewarm an execution service #13377

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.

Sign up for GitHub

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
IanMatthewHuff merged 3 commits into from
Aug 11, 2020
Merged

don't try to prewarm an execution service #13377

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5680310
don't try to prewarm an execution service
Aug 10, 2020
aa275d9
remove funky old cast
Aug 10, 2020
5b6f2ba
update unit tests
Aug 11, 2020
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/13258.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Prevent daemon from trying to prewarm an execution service.
4 changes: 2 additions & 2 deletions src/client/common/process/pythonExecutionFactory.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ export class PythonExecutionFactory implements IPythonExecutionFactory {

public async createDaemon<T extends IPythonDaemonExecutionService | IDisposable>(
Copy link
Copy Markdown

@rchiodo rchiodo Aug 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make more sense to put this 'or' on the 'extends'? Or does that not work?

Copy link
Copy Markdown
Member Author

@IanMatthewHuff IanMatthewHuff Aug 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking no, since the <> part I see as the "request" for what the user wants. Honestly IPythonDaemonExecutionService is almost the same as IPythonExecutionService so it could go together, but I felt like the distinction was important here between what users are actually request (some type of IPythonDaemonExecutionService) and the fact that if we can't create one then we just return an IPythonExecutionService.

options: DaemonExecutionFactoryCreationOptions
): Promise<T> {
): Promise<T | IPythonExecutionService> {
const pythonPath = options.pythonPath
? options.pythonPath
: this.configService.getSettings(options.resource).pythonPath;
Expand All @@ -93,7 +93,7 @@ export class PythonExecutionFactory implements IPythonExecutionFactory {
});
// No daemon support in Python 2.7 or during shutdown
if (!interpreterService || (interpreter?.version && interpreter.version.major < 3)) {
return (activatedProcPromise! as unknown) as T;
return activatedProcPromise;
}

// Ensure we do not start multiple daemons for the same interpreter.
Expand Down
2 changes: 1 addition & 1 deletion src/client/common/process/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ export interface IPythonExecutionFactory {
*/
createDaemon<T extends IPythonDaemonExecutionService | IDisposable>(
options: DaemonExecutionFactoryCreationOptions
): Promise<T>;
): Promise<T | IPythonExecutionService>;
createActivatedEnvironment(options: ExecutionFactoryCreateWithEnvironmentOptions): Promise<IPythonExecutionService>;
createCondaExecutionService(
pythonPath: string,
Expand Down
41 changes: 34 additions & 7 deletions src/client/datascience/kernel-launcher/kernelDaemonPool.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import { inject, injectable } from 'inversify';
import { IWorkspaceService } from '../../common/application/types';
import { traceError } from '../../common/logger';

import { IPythonExecutionFactory } from '../../common/process/types';
import { IPythonExecutionFactory, IPythonExecutionService } from '../../common/process/types';
import { IDisposable, Resource } from '../../common/types';
import { noop } from '../../common/utils/misc';
import { IEnvironmentVariablesProvider } from '../../common/variables/types';
Expand All @@ -23,7 +23,7 @@ type IKernelDaemonInfo = {
workspaceResource: Resource;
workspaceFolderIdentifier: string;
interpreterPath: string;
daemon: Promise<IPythonKernelDaemon>;
daemon: Promise<IPythonKernelDaemon | IPythonExecutionService>;
};

@injectable()
Expand Down Expand Up @@ -68,7 +68,7 @@ export class KernelDaemonPool implements IDisposable {
resource: Resource,
kernelSpec: IJupyterKernelSpec,
interpreter?: PythonInterpreter
): Promise<IPythonKernelDaemon> {
): Promise<IPythonKernelDaemon | IPythonExecutionService> {
const pythonPath = interpreter?.path || kernelSpec.argv[0];
// If we have environment variables in the kernel.json, then its not we support.
// Cuz there's no way to know before hand what kernelspec can be used, hence no way to know what envs are required.
Expand Down Expand Up @@ -104,14 +104,26 @@ export class KernelDaemonPool implements IDisposable {
dedicated: true,
resource
});
daemon.then((d) => this.disposables.push(d)).catch(noop);
daemon
.then((d) => {
if ('dispose' in d) {
this.disposables.push(d);
}
})
.catch(noop);
return daemon;
}
private async onDidEnvironmentVariablesChange(affectedResoruce: Resource) {
const workspaceFolderIdentifier = this.workspaceService.getWorkspaceFolderIdentifier(affectedResoruce);
this.daemonPool = this.daemonPool.filter((item) => {
if (item.workspaceFolderIdentifier === workspaceFolderIdentifier) {
item.daemon.then((d) => d.dispose()).catch(noop);
item.daemon
.then((d) => {
if ('dispose' in d) {
d.dispose();
}
})
.catch(noop);
return false;
}
return true;
Expand All @@ -132,7 +144,16 @@ export class KernelDaemonPool implements IDisposable {
const daemon = this.createDaemon(resource, interpreter.path);
// 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).
// I.e. we'll start python process thats the kernel, but will not start the kernel module (`python -m ipykernel`).
daemon.then((d) => d.preWarm()).catch(traceError.bind(`Failed to prewarm kernel daemon ${interpreter.path}`));
daemon
.then((d) => {
// Prewarm if we support prewarming
if ('preWarm' in d) {
d.preWarm().catch(traceError.bind(`Failed to prewarm kernel daemon ${interpreter.path}`));
}
})
.catch((e) => {
traceError(`Failed to load deamon: ${e}`);
});
this.daemonPool.push({
daemon,
interpreterPath: interpreter.path,
Expand Down Expand Up @@ -172,7 +193,13 @@ export class KernelDaemonPool implements IDisposable {
this.daemonPool = this.daemonPool.filter((item) => {
const interpreterForWorkspace = currentInterpreterInEachWorksapce.get(item.key);
if (!interpreterForWorkspace || !this.fs.areLocalPathsSame(interpreterForWorkspace, item.interpreterPath)) {
item.daemon.then((d) => d.dispose()).catch(noop);
item.daemon
.then((d) => {
if ('dispose' in d) {
d.dispose();
}
})
.catch(noop);
return false;
}

Expand Down
11 changes: 3 additions & 8 deletions src/client/datascience/kernel-launcher/kernelLauncherDaemon.ts
View file
Open in desktop
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 { IPythonExecutionService, ObservableExecutionResult } from '../../common/process/types';
import { ObservableExecutionResult } from '../../common/process/types';
import { Resource } from '../../common/types';
import { noop } from '../../common/utils/misc';
import { PythonInterpreter } from '../../pythonEnvironments/info';
Expand Down Expand Up @@ -47,14 +47,9 @@ export class PythonKernelLauncherDaemon implements IDisposable {

// 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 (!('start' in daemon)) {
Copy link
Copy Markdown
Member Author

@IanMatthewHuff IanMatthewHuff Aug 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is where I'd made the previous fix to 2.7 non-daemon launching. But that funky type coming back from the daemon creation was still causing issues in other locations (such as with prewarm) so I converted the deamon create to return the actual type union that it creates and then make sure to handle the cases that might be different for a daemon versus an execution service. I should have changed it the first time around 😞 .

// 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, {
const observableOutput = daemon.execModuleObservable(moduleName, moduleArgs, {
env,
cwd: workingDirectory
});
Expand Down
3 changes: 3 additions & 0 deletions src/test/datascience/kernel-launcher/kernelDaemonPool.unit.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,9 @@ suite('DataScience - Kernel Daemon Pool', () => {
when(daemon1.preWarm()).thenResolve();
when(daemon2.preWarm()).thenResolve();
when(daemon3.preWarm()).thenResolve();
when(daemon1.dispose()).thenResolve();
when(daemon2.dispose()).thenResolve();
when(daemon3.dispose()).thenResolve();

when(envVars.onDidEnvironmentVariablesChange).thenReturn(didEnvVarsChange.event);
when(interpeterService.onDidChangeInterpreter).thenReturn(didChangeInterpreter.event);
Expand Down
2 changes: 0 additions & 2 deletions src/test/datascience/kernel-launcher/kernelLauncherDaemon.unit.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,6 @@ suite('DataScience - Kernel Launcher Daemon', () => {
when(
executionService.execModuleObservable('ipkernel_launcher', deepEqual(['-f', 'file.json']), anything())
).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);
Expand Down