Skip to content

Connect to raw kernel with command line args instead of connection file #11894

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
merged 5 commits into from
May 19, 2020
Merged

Connect to raw kernel with command line args instead of connection file #11894

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7d86119
working on mac with basic test
May 19, 2020
369c848
connection file removed
May 19, 2020
2abbfbd
tests fixed up
May 19, 2020
61f1a8b
pre-review cleanup
May 19, 2020
58391f2
add news
May 19, 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
2 changes: 1 addition & 1 deletion .vscode/launch.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,7 +271,7 @@
],
"env": {
// Remove `X` prefix to test with real browser to host DS ui (for DS functional tests).
"xVSC_PYTHON_DS_UI_BROWSER": "1",
"XVSC_PYTHON_DS_UI_BROWSER": "1",
Copy link
Member Author

Choose a reason for hiding this comment

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

That little x was bugging me 😆

// Remove `X` prefix to test with real python (for DS functional tests).
"XVSCODE_PYTHON_ROLLING": "1",
// Remove 'X' to turn on all logging in the debug output
Expand Down
1 change: 1 addition & 0 deletions news/2 Fixes/11883.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Use command line arguments to launch our raw kernels as opposed to a connection file. The connection file seems to be causing issues in particular on windows CI machines with permissions.
3 changes: 0 additions & 3 deletions src/client/datascience/kernel-launcher/kernelLauncher.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import { inject, injectable } from 'inversify';
import * as portfinder from 'portfinder';
import { promisify } from 'util';
import * as uuid from 'uuid/v4';
import { IFileSystem } from '../../common/platform/types';
import { IProcessServiceFactory } from '../../common/process/types';
import { Resource } from '../../common/types';
import { PythonInterpreter } from '../../interpreter/contracts';
Expand All @@ -27,7 +26,6 @@ export class KernelLauncher implements IKernelLauncher {
private static nextFreePortToTryAndUse = PortToStartFrom;
constructor(
@inject(IProcessServiceFactory) private processExecutionFactory: IProcessServiceFactory,
@inject(IFileSystem) private file: IFileSystem,
@inject(KernelDaemonPool) private readonly daemonPool: KernelDaemonPool
) {}

Expand All @@ -40,7 +38,6 @@ export class KernelLauncher implements IKernelLauncher {
const connection = await this.getKernelConnection();
const kernelProcess = new KernelProcess(
this.processExecutionFactory,
this.file,
this.daemonPool,
connection,
kernelSpec,
Expand Down
52 changes: 35 additions & 17 deletions src/client/datascience/kernel-launcher/kernelProcess.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import * as tcpPortUsed from 'tcp-port-used';
import { Event, EventEmitter } from 'vscode';
import { PYTHON_LANGUAGE } from '../../common/constants';
import { traceError, traceInfo, traceWarning } from '../../common/logger';
import { IFileSystem, TemporaryFile } from '../../common/platform/types';
import { IProcessServiceFactory, ObservableExecutionResult } from '../../common/process/types';
import { Resource } from '../../common/types';
import { noop, swallowExceptions } from '../../common/utils/misc';
Expand Down Expand Up @@ -39,7 +38,6 @@ export class KernelProcess implements IKernelProcess {
return this.kernelSpec.language.toLowerCase() === PYTHON_LANGUAGE.toLowerCase();
}
private _process?: ChildProcess;
private connectionFile?: TemporaryFile;
private exitEvent = new EventEmitter<{ exitCode?: number; reason?: string }>();
private pythonKernelLauncher?: PythonKernelLauncherDaemon;
private launchedOnce?: boolean;
Expand All @@ -49,7 +47,6 @@ export class KernelProcess implements IKernelProcess {
private readonly originalKernelSpec: IJupyterKernelSpec;
constructor(
private readonly processExecutionFactory: IProcessServiceFactory,
private readonly file: IFileSystem,
private readonly daemonPool: KernelDaemonPool,
private readonly _connection: IKernelConnection,
kernelSpec: IJupyterKernelSpec,
Expand All @@ -72,7 +69,8 @@ export class KernelProcess implements IKernelProcess {
}
this.launchedOnce = true;

await this.createAndUpdateConnectionFile();
// Update our connection arguments in the kernel spec
this.updateConnectionArgs();

const exeObs = await this.launchAsObservable();

Expand Down Expand Up @@ -127,7 +125,6 @@ export class KernelProcess implements IKernelProcess {
this.exitEvent.fire();
});
swallowExceptions(() => this.pythonKernelLauncher?.dispose());
swallowExceptions(() => this.connectionFile?.dispose());
}

// Make sure that the heartbeat channel is open for connections
Expand All @@ -144,21 +141,42 @@ export class KernelProcess implements IKernelProcess {
}
}

private async createAndUpdateConnectionFile() {
// Create the connection file so that any user can destroy it. The kernel will
// actually read it, delete it, and recreate it with more restricted permissions
this.connectionFile = await this.file.createTemporaryFile('.json', 0o777);
await this.file.writeFile(this.connectionFile.filePath, JSON.stringify(this._connection), {
encoding: 'utf-8',
flag: 'w'
});

// Update the args in the kernelspec to include the conenction file.
// Instead of having to use a connection file update our local copy of the kernelspec to launch
// directly with command line arguments
private updateConnectionArgs() {
Copy link
Member Author

Choose a reason for hiding this comment

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

I thought briefly about where to handle this and I believe this is the right spot.

  1. We already make a deep copy of the kernelspec in KernelProcess so that we can mess with it, but not change the original spec
  2. Same location we were previously creating and inserting the connection file info.

Copy link
Member Author

Choose a reason for hiding this comment

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

I tested on windows as well. Not fully sure if this fixes the nightly test runs (as those issues don't seem to repro doing real python tests locally) so I'll queue up a run for that. I would expect this to fix the nightly test issues but even if there was another issue there this new method seems like a solid improvement, no messing around with creating and removing files.

// First check to see if we have a kernelspec that expects a connection file,
// Error if we don't have one. We expect '-f', '{connectionfile}' in our launch args
const indexOfConnectionFile = findIndexOfConnectionFile(this._kernelSpec);
if (indexOfConnectionFile === -1) {
if (
indexOfConnectionFile === -1 ||
indexOfConnectionFile === 0 ||
this._kernelSpec.argv[indexOfConnectionFile - 1] !== '-f'
) {
throw new Error(`Connection file not found in kernelspec json args, ${this._kernelSpec.argv.join(' ')}`);
}
this._kernelSpec.argv[indexOfConnectionFile] = this.connectionFile.filePath;

// Slice out -f and the connection file from the args
this._kernelSpec.argv.splice(indexOfConnectionFile - 1, 2);

// Add in our connection command line args
this._kernelSpec.argv.push(...this.addConnectionArgs());
}

// Add the command line arguments
private addConnectionArgs(): string[] {
const newConnectionArgs: string[] = [];

newConnectionArgs.push(`--ip=${this._connection.ip}`);
newConnectionArgs.push(`--stdin=${this._connection.stdin_port}`);
newConnectionArgs.push(`--control=${this._connection.control_port}`);
newConnectionArgs.push(`--hb=${this._connection.hb_port}`);
newConnectionArgs.push(`--Session.signature_scheme="${this._connection.signature_scheme}"`);
newConnectionArgs.push(`--Session.key=b"${this._connection.key}"`); // Note we need the 'b here at the start for a byte string
newConnectionArgs.push(`--shell=${this._connection.shell_port}`);
newConnectionArgs.push(`--transport="${this._connection.transport}"`);
newConnectionArgs.push(`--iopub=${this._connection.iopub_port}`);

return newConnectionArgs;
}

private async launchAsObservable() {
Expand Down
4 changes: 1 addition & 3 deletions src/test/datascience/kernelLauncher.functional.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import { assert } from 'chai';

import { KernelMessage } from '@jupyterlab/services';
import * as uuid from 'uuid/v4';
import { IFileSystem } from '../../client/common/platform/types';
import { IProcessServiceFactory } from '../../client/common/process/types';
import { createDeferred } from '../../client/common/utils/async';
import { JupyterZMQBinariesNotFoundError } from '../../client/datascience/jupyter/jupyterZMQBinariesNotFoundError';
Expand Down Expand Up @@ -39,10 +38,9 @@ suite('DataScience - Kernel Launcher', () => {
ioc = new DataScienceIocContainer();
ioc.registerDataScienceTypes();
kernelFinder = new MockKernelFinder(ioc.serviceContainer.get<IKernelFinder>(IKernelFinder));
const file = ioc.serviceContainer.get<IFileSystem>(IFileSystem);
const processServiceFactory = ioc.serviceContainer.get<IProcessServiceFactory>(IProcessServiceFactory);
const daemonPool = ioc.serviceContainer.get<KernelDaemonPool>(KernelDaemonPool);
kernelLauncher = new KernelLauncher(processServiceFactory, file, daemonPool);
kernelLauncher = new KernelLauncher(processServiceFactory, daemonPool);
await ioc.activate();
if (!ioc.mockJupyter) {
pythonInterpreter = await ioc.getJupyterCapableInterpreter();
Expand Down
13 changes: 0 additions & 13 deletions src/test/datascience/raw-kernel/rawKernel.functional.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,8 @@
// Licensed under the MIT License.
'use strict';
import { assert } from 'chai';
import * as fs from 'fs-extra';
import { noop } from 'jquery';
import * as os from 'os';
import * as path from 'path';
import * as uuid from 'uuid/v4';
import { IFileSystem } from '../../../client/common/platform/types';
import { IProcessServiceFactory } from '../../../client/common/process/types';
import { createDeferred, sleep } from '../../../client/common/utils/async';
import { KernelDaemonPool } from '../../../client/datascience/kernel-launcher/kernelDaemonPool';
Expand All @@ -21,7 +17,6 @@ import { requestExecute, requestInspect } from './rawKernelTestHelpers';
// tslint:disable:no-any no-multiline-string max-func-body-length no-console max-classes-per-file trailing-comma
suite('DataScience raw kernel tests', () => {
let ioc: DataScienceIocContainer;
let connectionFile: string;
let rawKernel: RawKernel;
const connectionInfo = {
shell_port: 57718,
Expand Down Expand Up @@ -67,8 +62,6 @@ suite('DataScience raw kernel tests', () => {
.getInterpreterDetails(ioc.getSettings().pythonPath);
assert.ok(interpreter, 'No jupyter interpreter found');
// Start our kernel
connectionFile = path.join(os.tmpdir(), `tmp_${Date.now()}_k.json`);
await fs.writeFile(connectionFile, JSON.stringify(connectionInfo), { encoding: 'utf-8', flag: 'w' });
const kernelSpec: IJupyterKernelSpec = {
argv: [interpreter!.path, '-m', 'ipykernel_launcher', '-f', '{connection_file}'],
metadata: {
Expand All @@ -83,7 +76,6 @@ suite('DataScience raw kernel tests', () => {
};
kernelProcess = new KernelProcess(
ioc.get<IProcessServiceFactory>(IProcessServiceFactory),
ioc.get<IFileSystem>(IFileSystem),
ioc.get<KernelDaemonPool>(KernelDaemonPool),
connectionInfo as any,
kernelSpec,
Expand All @@ -97,11 +89,6 @@ suite('DataScience raw kernel tests', () => {
async function disconnectFromKernel() {
if (kernelProcess) {
await kernelProcess.dispose().catch(noop);
try {
await fs.remove(connectionFile);
} catch {
noop();
}
}
}

Expand Down