Skip to content

Commit

Permalink
Fixes to IPyKernel detection with tests (#15561)
Browse files Browse the repository at this point in the history
  • Loading branch information
DonJayamanne authored Apr 18, 2024
1 parent a4b5eec commit defb5b9
Show file tree
Hide file tree
Showing 9 changed files with 749 additions and 16 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,6 @@ Morbi molestie lacinia sapien nec porttitor. Nam at vestibulum nisi.
assert.equal(cells.length, 1, 'markdown cell multline failed');
assert.equal(cells[0].languageId, 'markdown', 'markdown cell not generated');
assert.equal(splitMarkdown(cells[0].value).length, 39, 'Lines for cell not emitted');
console.error(`"${cells[0].value}"`);
assert.equal(splitMarkdown(cells[0].value)[34], ' - Item 1-a-3-c', 'Lines for markdown not emitted');

// eslint-disable-next-line no-multi-str
Expand Down
20 changes: 10 additions & 10 deletions src/kernels/raw/launcher/kernelProcess.node.ts
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ export class KernelProcess extends ObservableDisposable implements IKernelProces
public get pid() {
return this._pid;
}
public get exited(): Event<{ exitCode?: number; reason?: string }> {
public get exited(): Event<{ exitCode?: number; reason?: string; stderr: string }> {
return this.exitEvent.event;
}
public get kernelConnectionMetadata(): Readonly<
Expand All @@ -110,11 +110,12 @@ export class KernelProcess extends ObservableDisposable implements IKernelProces
return true;
}
private _process?: ChildProcess;
private exitEvent = new EventEmitter<{ exitCode?: number; reason?: string }>();
private exitEvent = new EventEmitter<{ exitCode?: number; reason?: string; stderr: string }>();
private launchedOnce?: boolean;
private connectionFile?: Uri;
private _launchKernelSpec?: IJupyterKernelSpec;
private interrupter?: Interrupter;
private exitEventFired = false;
private readonly _kernelConnectionMetadata: Readonly<
LocalKernelSpecConnectionMetadata | PythonKernelConnectionMetadata
>;
Expand Down Expand Up @@ -182,7 +183,6 @@ export class KernelProcess extends ObservableDisposable implements IKernelProces
}
traceVerbose(`Kernel process ${proc?.pid}.`);
let stderr = '';
let exitEventFired = false;
let providedExitCode: number | null;
const deferred = createDeferred();
deferred.promise.catch(noop);
Expand All @@ -196,12 +196,13 @@ export class KernelProcess extends ObservableDisposable implements IKernelProces
return;
}
traceVerbose(`KernelProcess Exited ${pid}, Exit Code - ${exitCode}`, stderr);
if (!exitEventFired) {
if (!this.exitEventFired) {
this.exitEvent.fire({
exitCode: exitCode || undefined,
reason: getTelemetrySafeErrorMessageFromPythonTraceback(stderr) || stderr
reason: getTelemetrySafeErrorMessageFromPythonTraceback(stderr) || stderr,
stderr
});
exitEventFired = true;
this.exitEventFired = true;
}
if (!cancelToken.isCancellationRequested) {
traceInfoIfCI(`KernelProcessExitedError raised`, stderr);
Expand Down Expand Up @@ -289,8 +290,6 @@ export class KernelProcess extends ObservableDisposable implements IKernelProces
if (cancelToken.isCancellationRequested || deferred.rejected) {
return;
}
console.error('ex');
console.error(ex);
// Do not throw an error, ignore this.
// In the case of VPNs the port does not seem to get used.
// Possible we're blocking it.
Expand Down Expand Up @@ -326,7 +325,6 @@ export class KernelProcess extends ObservableDisposable implements IKernelProces
// If we have the python error message in std outputs, display that.
const errorMessage = getErrorMessageFromPythonTraceback(stdErrToLog) || stdErrToLog.substring(0, 100);
traceInfoIfCI(`KernelDiedError raised`, errorMessage, stderr + '\n' + stderr + '\n');
console.error(`KernelDiedError raised`, e);
throw new KernelDiedError(
DataScience.kernelDied(errorMessage),
// Include what ever we have as the stderr.
Expand Down Expand Up @@ -358,7 +356,9 @@ export class KernelProcess extends ObservableDisposable implements IKernelProces
try {
this.interrupter?.dispose().catch(noop);
this._process?.kill(); // NOSONAR
this.exitEvent.fire({});
if (!this.exitEventFired) {
this.exitEvent.fire({ stderr: '' });
}
} catch (ex) {
traceError(`Error disposing kernel process ${pid}`, ex);
}
Expand Down
24 changes: 23 additions & 1 deletion src/kernels/raw/session/rawKernelConnection.node.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@ import { StopWatch } from '../../../platform/common/utils/stopWatch';
import { dispose } from '../../../platform/common/utils/lifecycle';
import { KernelSocketMap } from '../../kernelSocket';
import { getNotebookTelemetryTracker } from '../../telemetry/notebookTelemetry';
import { KernelProcessExitedError } from '../../errors/kernelProcessExitedError';
import { once } from '../../../platform/common/utils/functional';

let nonSerializingKernel: typeof import('@jupyterlab/services/lib/kernel/default');

Expand Down Expand Up @@ -130,6 +132,7 @@ export class RawKernelConnection implements Kernel.IKernelConnection {
const disposables: IDisposable[] = [];
const postStartToken = wrapCancellationTokens(token);
disposables.push(postStartToken);
let kernelExitedError: KernelProcessExitedError | undefined = undefined;
try {
const oldKernelProcess = this.kernelProcess;
this.kernelProcess = undefined;
Expand Down Expand Up @@ -162,7 +165,18 @@ export class RawKernelConnection implements Kernel.IKernelConnection {
(info) => this.infoDeferred.resolve(info),
(ex) => this.infoDeferred.reject(ex)
);

once(this.kernelProcess.exited)(
(e) => {
kernelExitedError = new KernelProcessExitedError(
e.exitCode || -1,
e.stderr,
this.kernelConnectionMetadata
);
postStartToken.cancel();
},
this,
disposables
);
const timeout = setTimeout(() => postStartToken.cancel(), this.launchTimeout);
disposables.push({ dispose: () => clearTimeout(timeout) });
await KernelProgressReporter.wrapAndReportProgress(
Expand All @@ -179,6 +193,9 @@ export class RawKernelConnection implements Kernel.IKernelConnection {
).finally(() => tracker?.stop());
}
);
if (kernelExitedError) {
throw kernelExitedError;
}
if (token.isCancellationRequested) {
throw new CancellationError();
}
Expand All @@ -193,6 +210,9 @@ export class RawKernelConnection implements Kernel.IKernelConnection {
?.shutdown()
.catch((ex) => traceWarning(`Failed to shutdown kernel, ${this.kernelConnectionMetadata.id}`, ex))
]);
if (kernelExitedError) {
throw kernelExitedError;
}
if (
isCancellationError(error) &&
postStartToken.token.isCancellationRequested &&
Expand Down Expand Up @@ -488,6 +508,8 @@ async function postStartKernel(
const gotIoPubMessage = createDeferred<boolean>();
const kernelInfoRequestHandled = createDeferred<boolean>();
const iopubHandler = () => gotIoPubMessage.resolve(true);
gotIoPubMessage.promise.catch(noop);
kernelInfoRequestHandled.promise.catch(noop);
kernel.iopubMessage.connect(iopubHandler);
const sendKernelInfoRequestOnControlChannel = () => {
const jupyterLab = require('@jupyterlab/services') as typeof import('@jupyterlab/services');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ suite('Raw Session & Raw Kernel Connection', () => {
let exitedEvent: EventEmitter<{
exitCode?: number | undefined;
reason?: string | undefined;
stderr: string;
}>;
const launchTimeout = 1_000;
let disposables: IDisposable[] = [];
Expand Down Expand Up @@ -178,6 +179,7 @@ suite('Raw Session & Raw Kernel Connection', () => {
exitedEvent = new EventEmitter<{
exitCode?: number | undefined;
reason?: string | undefined;
stderr: string;
}>();
nonSerializingKernel.KernelConnection = OldKernelConnectionClass;
const workspaceConfig = mock<WorkspaceConfiguration>();
Expand Down Expand Up @@ -275,7 +277,7 @@ suite('Raw Session & Raw Kernel Connection', () => {
session.kernel!.statusChanged.connect((_, s) => statuses.push(s));
session.kernel!.disposed.connect(() => (disposed = true));

exitedEvent.fire({ exitCode: 1, reason: 'Killed' });
exitedEvent.fire({ exitCode: 1, reason: 'Killed', stderr: '' });

await waitForCondition(
() => !disposed && statuses.join(',') === 'dead',
Expand Down
2 changes: 1 addition & 1 deletion src/kernels/raw/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ export interface IKernelProcess extends IAsyncDisposable {
/**
* This event is triggered if the process is exited
*/
readonly exited: Event<{ exitCode?: number; reason?: string }>;
readonly exited: Event<{ exitCode?: number; reason?: string; stderr: string }>;
/**
* Whether we can interrupt this kernel process.
* If not possible, send a shell message to the underlying kernel.
Expand Down
4 changes: 2 additions & 2 deletions src/platform/common/utils/localize.ts
Original file line number Diff line number Diff line change
Expand Up @@ -281,11 +281,11 @@ export namespace DataScience {
'Do not translate the text "command:jupyter.viewOutput", that is a command Id that will be used by VS Code to open the output panel'
]
});
export const kernelDied = (kernelName: string) =>
export const kernelDied = (message: string) =>
l10n.t({
message:
'The kernel died. Error: {0}... View Jupyter [log](command:jupyter.viewOutput) for further details.',
args: [kernelName],
args: [message],
comment: [
'Do not translate the text "command:jupyter.viewOutput", that is a command Id that will be used by VS Code to open the output panel'
]
Expand Down
Loading

0 comments on commit defb5b9

Please sign in to comment.