Skip to content
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

Set session name for the remote sessions created #438

Merged
merged 1 commit into from
Nov 11, 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
12 changes: 7 additions & 5 deletions src/client/datascience/baseJupyterSession.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@ export abstract class BaseJupyterSession implements IJupyterSession {
protected get session(): ISessionWithSocket | undefined {
return this._session;
}
protected kernelConnectionMetadata?: KernelConnectionMetadata;
public get kernelSocket(): Observable<KernelSocketInformation | undefined> {
return this._kernelSocket;
}
Expand All @@ -68,6 +67,7 @@ export abstract class BaseJupyterSession implements IJupyterSession {
public get isConnected(): boolean {
return this.connected;
}
protected kernelConnectionMetadata?: KernelConnectionMetadata;
protected onStatusChangedEvent: EventEmitter<ServerStatus> = new EventEmitter<ServerStatus>();
protected statusHandler: Slot<ISessionWithSocket, Kernel.Status>;
protected connected: boolean = false;
Expand All @@ -77,7 +77,6 @@ export abstract class BaseJupyterSession implements IJupyterSession {
private _jupyterLab?: typeof import('@jupyterlab/services');
private ioPubEventEmitter = new EventEmitter<KernelMessage.IIOPubMessage>();
private ioPubHandler: Slot<ISessionWithSocket, KernelMessage.IIOPubMessage>;

constructor(private restartSessionUsed: (id: Kernel.IKernelConnection) => void, public workingDirectory: string) {
this.statusHandler = this.onStatusChanged.bind(this);
this.ioPubHandler = (_s, m) => this.ioPubEventEmitter.fire(m);
Expand All @@ -86,7 +85,7 @@ export abstract class BaseJupyterSession implements IJupyterSession {
return this.shutdown();
}
// Abstracts for each Session type to implement
public abstract async waitForIdle(timeout: number): Promise<void>;
public abstract waitForIdle(timeout: number): Promise<void>;

public async shutdown(): Promise<void> {
if (this.session) {
Expand Down Expand Up @@ -191,7 +190,7 @@ export abstract class BaseJupyterSession implements IJupyterSession {
if (!this.session) {
throw new Error(localize.DataScience.sessionDisposed());
}
this.restartSessionUsed(this.session.kernel);
this.onRestartSessionUsed(this.session.kernel);
traceInfo(`Got new session ${this.session.kernel.id}`);

// Rewire our status changed event.
Expand Down Expand Up @@ -320,10 +319,13 @@ export abstract class BaseJupyterSession implements IJupyterSession {
throw new Error(localize.DataScience.sessionDisposed());
}
}
protected onRestartSessionUsed(id: Kernel.IKernelConnection) {
this.restartSessionUsed(id);
}

// Sub classes need to implement their own restarting specific code
protected abstract startRestartSession(): void;
protected abstract async createRestartSession(
protected abstract createRestartSession(
kernelConnection: KernelConnectionMetadata | undefined,
session: ISessionWithSocket,
cancelToken?: CancellationToken
Expand Down
1 change: 1 addition & 0 deletions src/client/datascience/jupyter/jupyterServer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,7 @@ export class JupyterServerBase implements INotebookServer {
session = await this.sessionManager.startNew(
launchInfo.kernelConnectionMetadata,
launchInfo.connectionInfo.rootDirectory,
launchInfo.uri,
cancelToken
);
const idleTimeout = this.configService.getSettings().jupyterLaunchTimeout;
Expand Down
59 changes: 52 additions & 7 deletions src/client/datascience/jupyter/jupyterSession.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,28 @@ import { getNameOfKernelConnection } from './kernels/helpers';
import { KernelConnectionMetadata } from './kernels/types';

export class JupyterSession extends BaseJupyterSession {
/**
* Ensure session name is the name of the current file.
* If its a notebook, session name = name of ipynb file with extension.
* If its an interactive window, session name = name of file with extension (xyz.py or xyz.cs).
*/
private get sessionName(): string {
if (this._sessionName) {
return this._sessionName;
}
if (!this.uri || !this.sessionType) {
this._sessionName = uuid();
return this._sessionName;
}
return (this._sessionName = path.basename(this.uri));
}
private get sessionType(): 'notebook' | 'file' | undefined {
// If we have a uri and its a notebook, the session type is `notebook`, else its `file`
// Remember uri could be a python or csharp file (for interactive window).
// If no uri provided, then default to `undefined`.
return this.uri?.toLocaleLowerCase().endsWith('.ipynb') ? 'notebook' : this.uri ? 'file' : undefined;
}
private _sessionName?: string;
constructor(
private connInfo: IJupyterConnection,
private serverSettings: ServerConnection.ISettings,
Expand All @@ -39,12 +61,12 @@ export class JupyterSession extends BaseJupyterSession {
private readonly restartSessionCreated: (id: Kernel.IKernelConnection) => void,
restartSessionUsed: (id: Kernel.IKernelConnection) => void,
readonly workingDirectory: string,
private readonly idleTimeout: number
private readonly idleTimeout: number,
private readonly uri?: string
) {
super(restartSessionUsed, workingDirectory);
this.kernelConnectionMetadata = kernelSpec;
}

@reportAction(ReportableAction.JupyterSessionWaitForIdleSession)
@captureTelemetry(Telemetry.WaitForIdleJupyter, undefined, true)
public waitForIdle(timeout: number): Promise<void> {
Expand Down Expand Up @@ -85,7 +107,12 @@ export class JupyterSession extends BaseJupyterSession {
newSession = this.sessionManager.connectTo(kernelConnection.kernelModel.session);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like we're already connecting to an existing session.
However with the new remote experience, we'll do this ONLY if the paths match.
I.e. if opening a notebook b.ipynb and trying to connect to session a.ipynb, then we will start a new session (but use the same kernel connection) - this is what Jupyter does.

Will only do that for new remote experience (where files and jupyter are on the same server)
This is because the paths for old remote experience is always unique (guid).

newSession.isRemoteSession = true;
} else {
newSession = await this.createSession(this.serverSettings, kernelConnection, cancelToken);
newSession = await this.createSession(
this.sessionName,
this.serverSettings,
kernelConnection,
cancelToken
);
}

// Make sure it is idle before we return
Expand All @@ -102,6 +129,17 @@ export class JupyterSession extends BaseJupyterSession {

return newSession;
}
protected onRestartSessionUsed(id: Kernel.IKernelConnection) {
super.onRestartSessionUsed(id);
const newSession = this.session;
if (newSession) {
traceInfo(`Updating name of restart session from ${newSession.name} to ${this.sessionName}`);
newSession
.setName(this.sessionName)
.then(() => traceInfo('Name of restart session updated'))
.catch((ex) => traceError('Failed to update name of restart session', ex));
}
}

protected async createRestartSession(
kernelConnection: KernelConnectionMetadata | undefined,
Expand All @@ -118,7 +156,12 @@ export class JupyterSession extends BaseJupyterSession {
let exception: any;
while (tryCount < 3) {
try {
result = await this.createSession(session.serverSettings, kernelConnection, cancelToken);
result = await this.createSession(
`${this.sessionName} (restart)`,
session.serverSettings,
kernelConnection,
cancelToken
);
await this.waitForIdleOnSession(result, this.idleTimeout);
this.restartSessionCreated(result.kernel);
return result;
Expand Down Expand Up @@ -186,19 +229,21 @@ export class JupyterSession extends BaseJupyterSession {
}

private async createSession(
sessionName: string,
serverSettings: ServerConnection.ISettings,
kernelConnection: KernelConnectionMetadata | undefined,
cancelToken?: CancellationToken
): Promise<ISessionWithSocket> {
// Create our backing file for the notebook
const backingFile = await this.createBackingFile();

const type = this.sessionType;
// Create our session options using this temporary notebook and our connection info
const options: Session.IOptions = {
path: backingFile.path,
kernelName: getNameOfKernelConnection(kernelConnection) || '',
name: uuid(), // This is crucial to distinguish this session from any other.
serverSettings: serverSettings
name: sessionName,
serverSettings: serverSettings,
type
};

return Cancellation.race(
Expand Down
4 changes: 3 additions & 1 deletion src/client/datascience/jupyter/jupyterSessionManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ export class JupyterSessionManager implements IJupyterSessionManager {
public async startNew(
kernelConnection: KernelConnectionMetadata | undefined,
workingDirectory: string,
uri?: string,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using a string, as that's whats being passed in other places, a string.

cancelToken?: CancellationToken
): Promise<IJupyterSession> {
if (!this.connInfo || !this._sessionManager || !this._contentsManager || !this.serverSettings) {
Expand All @@ -85,7 +86,8 @@ export class JupyterSessionManager implements IJupyterSessionManager {
this.restartSessionCreatedEvent.fire.bind(this.restartSessionCreatedEvent),
this.restartSessionUsedEvent.fire.bind(this.restartSessionUsedEvent),
workingDirectory,
this.configService.getSettings().jupyterLaunchTimeout
this.configService.getSettings().jupyterLaunchTimeout,
uri
);
try {
await session.connect(this.configService.getSettings().jupyterLaunchTimeout, cancelToken);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,10 @@ export class GuestJupyterSessionManager implements IJupyterSessionManager {
public startNew(
kernelConnection: KernelConnectionMetadata | undefined,
workingDirectory: string,
uri: string,
cancelToken?: CancellationToken
): Promise<IJupyterSession> {
return this.realSessionManager.startNew(kernelConnection, workingDirectory, cancelToken);
return this.realSessionManager.startNew(kernelConnection, workingDirectory, uri, cancelToken);
}

public async getKernelSpecs(): Promise<IJupyterKernelSpec[]> {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,12 @@ export class HostJupyterServer extends LiveShareParticipantHost(JupyterServerBas
const session =
possibleSession && this.fs.areLocalPathsSame(possibleSession.workingDirectory, workingDirectory)
? possibleSession
: await sessionManager.startNew(info.kernelConnectionMetadata, workingDirectory, cancelToken);
: await sessionManager.startNew(
info.kernelConnectionMetadata,
workingDirectory,
resource?.toString(),
cancelToken
);
traceInfo(`Started session ${this.id}`);
return { info, session };
};
Expand Down
1 change: 1 addition & 0 deletions src/client/datascience/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -378,6 +378,7 @@ export interface IJupyterSessionManager extends IAsyncDisposable {
startNew(
kernelConnection: KernelConnectionMetadata | undefined,
workingDirectory: string,
uri?: string,
cancelToken?: CancellationToken
): Promise<IJupyterSession>;
getKernelSpecs(): Promise<IJupyterKernelSpec[]>;
Expand Down
1 change: 1 addition & 0 deletions src/test/datascience/mockJupyterManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -435,6 +435,7 @@ export class MockJupyterManager implements IJupyterSessionManager {
public startNew(
_kernelConnection: KernelConnectionMetadata | undefined,
_workingDirectory: string,
_uri?: string,
cancelToken?: CancellationToken
): Promise<IJupyterSession> {
if (this.sessionTimeout && cancelToken) {
Expand Down