Skip to content

Fix multiprocessing problems with setting __file__ #14376

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
merged 3 commits into from
Oct 12, 2020
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/12530.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Make sure not to set ```__file__``` unless necessary as this can mess up some modules (like multiprocessing)
17 changes: 6 additions & 11 deletions src/client/datascience/interactive-common/interactiveBase.ts
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ export abstract class InteractiveBase extends WebviewPanelHost<IInteractiveWindo
protected abstract get notebookMetadata(): INotebookMetadataLive | undefined;

protected abstract get notebookIdentity(): INotebookIdentity;

protected fileInKernel: string | undefined;
private unfinishedCells: ICell[] = [];
private restartingKernel: boolean = false;
private perceivedJupyterStartupTelemetryCaptured: boolean = false;
Expand Down Expand Up @@ -538,6 +538,8 @@ export abstract class InteractiveBase extends WebviewPanelHost<IInteractiveWindo

protected abstract updateNotebookOptions(kernelConnection: KernelConnectionMetadata): Promise<void>;

protected abstract setFileInKernel(file: string, cancelToken: CancellationToken | undefined): Promise<void>;

protected async clearResult(id: string): Promise<void> {
await this.ensureConnectionAndNotebook();
if (this._notebook) {
Expand Down Expand Up @@ -631,16 +633,9 @@ export abstract class InteractiveBase extends WebviewPanelHost<IInteractiveWindo
}

// If the file isn't unknown, set the active kernel's __file__ variable to point to that same file.
if (file !== Identifiers.EmptyFileName) {
await this._notebook.execute(
`__file__ = '${file.replace(/\\/g, '\\\\')}'`,
file,
line,
uuid(),
cancelToken,
true
);
}
await this.setFileInKernel(file, cancelToken);

// Setup telemetry
if (stopWatch && !this.perceivedJupyterStartupTelemetryCaptured) {
this.perceivedJupyterStartupTelemetryCaptured = true;
sendTelemetryEvent(Telemetry.PerceivedJupyterStartupNotebook, stopWatch?.elapsedTime);
Expand Down
4 changes: 4 additions & 0 deletions src/client/datascience/interactive-ipynb/nativeEditor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -579,6 +579,10 @@ export class NativeEditor extends InteractiveBase implements INotebookEditor {
// Actually don't close, just let the error bubble out
}

protected async setFileInKernel(_file: string, _cancelToken: CancellationToken | undefined): Promise<void> {
// Native editor doesn't set this as the ipython file should be set for a notebook.
}

protected async close(): Promise<void> {
// Fire our event
this.closedEvent.fire(this);
Expand Down
35 changes: 34 additions & 1 deletion src/client/datascience/interactive-window/interactiveWindow.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@
// Licensed under the MIT License.
import type { nbformat } from '@jupyterlab/coreutils';
import * as path from 'path';
import { Event, EventEmitter, Memento, Uri, ViewColumn } from 'vscode';
import * as uuid from 'uuid';
import { CancellationToken, Event, EventEmitter, Memento, Uri, ViewColumn } from 'vscode';
import {
IApplicationShell,
ICommandManager,
Expand Down Expand Up @@ -420,6 +421,38 @@ export class InteractiveWindow extends InteractiveBase implements IInteractiveWi
protected async closeBecauseOfFailure(_exc: Error): Promise<void> {
this.dispose();
}

protected async setFileInKernel(file: string, cancelToken: CancellationToken | undefined): Promise<void> {
Copy link
Member

Choose a reason for hiding this comment

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

You're right, even aside from fixing the bug seems better from a perf standpoint.

// If in perFile mode, set only once
if (this.mode === 'perFile' && !this.fileInKernel && this.notebook && file !== Identifiers.EmptyFileName) {
this.fileInKernel = file;
await this.notebook.execute(
`__file__ = '${file.replace(/\\/g, '\\\\')}'`,
file,
0,
uuid(),
cancelToken,
true
);
} else if (
(!this.fileInKernel || !this.fs.areLocalPathsSame(this.fileInKernel, file)) &&
this.mode !== 'perFile' &&
this.notebook &&
file !== Identifiers.EmptyFileName
) {
// Otherwise we need to reset it every time
this.fileInKernel = file;
await this.notebook.execute(
`__file__ = '${file.replace(/\\/g, '\\\\')}'`,
file,
0,
uuid(),
cancelToken,
true
);
}
}

protected ensureConnectionAndNotebook(): Promise<void> {
// Keep track of users who have used interactive window in a worksapce folder.
// To be used if/when changing workflows related to startup of jupyter.
Expand Down
11 changes: 8 additions & 3 deletions src/client/datascience/jupyter/jupyterSession.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import { reportAction } from '../progress/decorator';
import { ReportableAction } from '../progress/types';
import { IJupyterConnection, ISessionWithSocket } from '../types';
import { JupyterInvalidKernelError } from './jupyterInvalidKernelError';
import { JupyterWaitForIdleError } from './jupyterWaitForIdleError';
import { JupyterWebSockets } from './jupyterWebSocket';
import { getNameOfKernelConnection } from './kernels/helpers';
import { KernelConnectionMetadata } from './kernels/types';
Expand Down Expand Up @@ -90,9 +91,13 @@ export class JupyterSession extends BaseJupyterSession {
// Make sure it is idle before we return
await this.waitForIdleOnSession(newSession, timeoutMS);
} catch (exc) {
traceError('Failed to change kernel', exc);
// Throw a new exception indicating we cannot change.
throw new JupyterInvalidKernelError(kernelConnection);
if (exc instanceof JupyterWaitForIdleError) {
Copy link
Member

Choose a reason for hiding this comment

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

Related fix? Just something you noticed at the same time?

throw exc;
} else {
traceError('Failed to change kernel', exc);
// Throw a new exception indicating we cannot change.
throw new JupyterInvalidKernelError(kernelConnection);
}
}

return newSession;
Expand Down