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.

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

Jump to bottom

Make kernel connection function synchronous #4725

Merged
merged 8 commits into from Jun 28, 2018
Merged

Make kernel connection function synchronous #4725

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
f7e3b10
WIP change kernel connectTo function to be synchronous.
jasongrout Jun 14, 2018
adca029
Initial propagation of the connectTo changes.
jasongrout Jun 27, 2018
fe9bb51
Fix tests
jasongrout Jun 28, 2018
a1aed58
Fix client session async rewrite.
jasongrout Jun 28, 2018
c4eb407
Fix docs typo.
jasongrout Jun 28, 2018
ff0ebe9
Fix lint error.
jasongrout Jun 28, 2018
911c6de
Fix function documentation.
jasongrout Jun 28, 2018
5a2cb1c
Remove spurious commas from JSX.
ian-r-rose Jun 28, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
31 changes: 14 additions & 17 deletions packages/apputils/src/clientsession.tsx
View file
Open in desktop
Expand Up @@ -515,30 +515,27 @@ class ClientSession implements IClientSession {
* If a default kernel is available, we connect to it.
* Otherwise we ask the user to select a kernel.
*/
initialize(): Promise<void> {
async initialize(): Promise<void> {
if (this._initializing || this._isReady) {
return this._ready.promise;
}
this._initializing = true;
let manager = this.manager;
return manager.ready.then(() => {
let model = find(manager.running(), item => {
return item.path === this._path;
});
if (!model) {
return;
}
return manager.connectTo(model).then(session => {
await manager.ready;
let model = find(manager.running(), item => {
return item.path === this._path;
});
if (model) {
try {
let session = manager.connectTo(model);
this._handleNewSession(session);
}).catch(err => {
} catch (err) {
this._handleSessionError(err);
});
}).then(() => {
return this._startIfNecessary();
}).then(() => {
this._isReady = true;
this._ready.resolve(void 0);
});
}
}
await this._startIfNecessary();
this._isReady = true;
this._ready.resolve(undefined);
}

/**
Expand Down
163 changes: 81 additions & 82 deletions packages/help-extension/src/index.tsx
View file
Open in desktop
Expand Up @@ -172,96 +172,95 @@ function activate(app: JupyterLab, mainMenu: IMainMenu, palette: ICommandPalette
if (kernelInfoCache.has(sessionModel.kernel.name)) {
return;
}
serviceManager.sessions.connectTo(sessionModel).then(session => {
session.kernel.ready.then(() => {
// Check the cache second time so that, if two callbacks get scheduled,
// they don't try to add the same commands.
if (kernelInfoCache.has(sessionModel.kernel.name)) {
return;
const session = serviceManager.sessions.connectTo(sessionModel);
session.kernel.ready.then(() => {
// Check the cache second time so that, if two callbacks get scheduled,
// they don't try to add the same commands.
if (kernelInfoCache.has(sessionModel.kernel.name)) {
return;
}
// Set the Kernel Info cache.
const name = session.kernel.name;
const kernelInfo = session.kernel.info;
kernelInfoCache.set(name, kernelInfo);

// Utility function to check if the current widget
// has registered itself with the help menu.
const usesKernel = () => {
let result = false;
const widget = app.shell.currentWidget;
if (!widget) {
return result;
}
// Set the Kernel Info cache.
const name = session.kernel.name;
const kernelInfo = session.kernel.info;
kernelInfoCache.set(name, kernelInfo);

// Utility function to check if the current widget
// has registered itself with the help menu.
const usesKernel = () => {
let result = false;
const widget = app.shell.currentWidget;
if (!widget) {
return result;
helpMenu.kernelUsers.forEach(u => {
if (u.tracker.has(widget) &&
u.getKernel(widget) &&
u.getKernel(widget).name === name) {
result = true;
}
helpMenu.kernelUsers.forEach(u => {
if (u.tracker.has(widget) &&
u.getKernel(widget) &&
u.getKernel(widget).name === name) {
result = true;
}
});
return result;
};

// Add the kernel banner to the Help Menu.
const bannerCommand = `help-menu-${name}:banner`;
const spec = serviceManager.specs.kernelspecs[name];
const kernelName = spec.display_name;
let kernelIconUrl = spec.resources['logo-64x64'];
if (kernelIconUrl) {
let index = kernelIconUrl.indexOf('kernelspecs');
kernelIconUrl = baseUrl + kernelIconUrl.slice(index);
}
commands.addCommand(bannerCommand, {
label: `About the ${kernelName} Kernel`,
isVisible: usesKernel,
isEnabled: usesKernel,
execute: () => {
// Create the header of the about dialog
let headerLogo = (<img src={kernelIconUrl} />);
let title = (
<span className='jp-About-header'>,
Copy link
Member

Choose a reason for hiding this comment

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

I think this happened when converting the dialogs to JSX, but I saw it when testing: this JSX has commas in it that it should not have.

Copy link
Member

Choose a reason for hiding this comment

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

I went ahead and fixed the issue, hope you don't mind @jasongrout

{headerLogo},
<div className='jp-About-header-info'>{kernelName}</div>
</span>
);
const banner = (<pre>{kernelInfo.banner}</pre>);
let body = (
<div className='jp-About-body'>
{banner}
</div>
);

showDialog({
title,
body,
buttons: [
Dialog.createButton({
label: 'DISMISS',
className: 'jp-About-button jp-mod-reject jp-mod-styled'
})
]
});
return result;
};

// Add the kernel banner to the Help Menu.
const bannerCommand = `help-menu-${name}:banner`;
const spec = serviceManager.specs.kernelspecs[name];
const kernelName = spec.display_name;
let kernelIconUrl = spec.resources['logo-64x64'];
if (kernelIconUrl) {
let index = kernelIconUrl.indexOf('kernelspecs');
kernelIconUrl = baseUrl + kernelIconUrl.slice(index);
}
commands.addCommand(bannerCommand, {
label: `About the ${kernelName} Kernel`,
});
helpMenu.addGroup([{ command: bannerCommand }], 20);

// Add the kernel info help_links to the Help menu.
const kernelGroup: Menu.IItemOptions[] = [];
(session.kernel.info.help_links || []).forEach((link) => {
const commandId = `help-menu-${name}:${link.text}`;
commands.addCommand(commandId, {
label: link.text,
isVisible: usesKernel,
isEnabled: usesKernel,
execute: () => {
// Create the header of the about dialog
let headerLogo = (<img src={kernelIconUrl} />);
let title = (
<span className='jp-About-header'>,
{headerLogo},
<div className='jp-About-header-info'>{kernelName}</div>
</span>
);
const banner = (<pre>{kernelInfo.banner}</pre>);
let body = (
<div className='jp-About-body'>
{banner}
</div>
);

showDialog({
title,
body,
buttons: [
Dialog.createButton({
label: 'DISMISS',
className: 'jp-About-button jp-mod-reject jp-mod-styled'
})
]
});
}
});
helpMenu.addGroup([{ command: bannerCommand }], 20);

// Add the kernel info help_links to the Help menu.
const kernelGroup: Menu.IItemOptions[] = [];
(session.kernel.info.help_links || []).forEach((link) => {
const commandId = `help-menu-${name}:${link.text}`;
commands.addCommand(commandId, {
label: link.text,
isVisible: usesKernel,
isEnabled: usesKernel,
execute: () => { commands.execute(CommandIDs.open, link); }
});
kernelGroup.push({ command: commandId });
execute: () => { commands.execute(CommandIDs.open, link); }
});
helpMenu.addGroup(kernelGroup, 21);

// Dispose of the session object since we no longer need it.
session.dispose();
kernelGroup.push({ command: commandId });
});
helpMenu.addGroup(kernelGroup, 21);

// Dispose of the session object since we no longer need it.
session.dispose();
});
});

Expand Down
17 changes: 8 additions & 9 deletions packages/services/src/kernel/default.ts
View file
Open in desktop
Expand Up @@ -1142,10 +1142,9 @@ namespace DefaultKernel {
* If the kernel was already started via `startNewKernel`, we return its
* `Kernel.IModel`.
*
* Otherwise, we attempt to find to the existing
* kernel.
* The promise is fulfilled when the kernel is found,
* otherwise the promise is rejected.
* Otherwise, we attempt to find an existing kernel by connecting to the
* server. The promise is fulfilled when the kernel is found, otherwise the
* promise is rejected.
*/
export
function findById(id: string, settings?: ServerConnection.ISettings): Promise<Kernel.IModel> {
Expand Down Expand Up @@ -1212,14 +1211,14 @@ namespace DefaultKernel {
*
* @param settings - The server settings for the request.
*
* @returns A promise that resolves with the kernel object.
* @returns The kernel object.
*
* #### Notes
* If the kernel was already started via `startNewKernel`, the existing
* Kernel object info is used to create another instance.
*/
export
function connectTo(model: Kernel.IModel, settings?: ServerConnection.ISettings): Promise<Kernel.IKernel> {
function connectTo(model: Kernel.IModel, settings?: ServerConnection.ISettings): Kernel.IKernel {
return Private.connectTo(model, settings);
}

Expand Down Expand Up @@ -1269,6 +1268,8 @@ namespace Private {

/**
* Find a kernel by id.
*
* Will reach out to the server if needed to find the kernel.
*/
export
function findById(id: string, settings?: ServerConnection.ISettings): Promise<Kernel.IModel> {
Expand Down Expand Up @@ -1389,11 +1390,9 @@ namespace Private {

/**
* Connect to a running kernel.
*
* TODO: why is this function async?
*/
export
async function connectTo(model: Kernel.IModel, settings?: ServerConnection.ISettings): Promise<Kernel.IKernel> {
function connectTo(model: Kernel.IModel, settings?: ServerConnection.ISettings): Kernel.IKernel {
let serverSettings = settings || ServerConnection.makeSettings();
let kernel = find(runningKernels, value => {
return value.id === model.id;
Expand Down