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

Make kernel connection function synchronous #4725

Merged
merged 8 commits into from Jun 28, 2018

Conversation

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jun 14, 2018

Related to work in and built on top of #4697.

@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Jun 27, 2018

Rebased on master...

@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Jun 27, 2018

@blink1073 - do you remember why the kernel connectTo function returns a promise, i.e., is asynchronous?

function connectTo(model: Kernel.IModel, settings?: ServerConnection.ISettings): Promise<Kernel.IKernel> {
let serverSettings = settings || ServerConnection.makeSettings();
let kernel = find(runningKernels, value => {
return value.id === model.id;
});
if (kernel) {
return Promise.resolve(kernel.clone());
}
return Promise.resolve(new DefaultKernel(
{ name: model.name, serverSettings }, model.id
));
}

Just checking to make sure I'm not messing things up by not remembering why it is the way it is.

@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Jun 27, 2018

Tracing it back further, https://github.com/jupyterlab/jupyterlab/blame/09884af82886c630d0f2fa662d6f8a1525613100/packages/services/src/kernel/default.ts#L1343 indicates that we used to get the kernel model from the server if we didn't have it available locally. This was changed in the rework of services for 1.0, specifically in https://github.com/jupyterlab/jupyterlab/pull/3294/files#diff-6ce6e61bad53706cd4a7885e25ecc551R1324

It seems that now the arguments include the model, and that is all we need to create a DefaultKernel. So it does seem that the finalization of the services library was where we basically decided that connectTo did not need to be asynchronous (i.e., didn't need to query the server).

@jasongrout jasongrout removed this from the Beta 3 milestone Jun 27, 2018
@jasongrout jasongrout added this to the Beta 4 milestone Jun 27, 2018
@blink1073
Copy link
Member

@blink1073 blink1073 commented Jun 27, 2018

That sounds right to me. I'd say leave it, not worth the major version bump.

@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Jun 27, 2018

Thanks. We're already doing a major version bump (to make the kernel message processing async and in order), so I was seeing if it was possible to fix this while we're at it.

@ian-r-rose - I think we may be able to get this into beta 3....I'll spend just a little bit more time chasing down the loose ends.

@jasongrout jasongrout removed this from the Beta 4 milestone Jun 28, 2018
@jasongrout jasongrout added this to the Beta 3 milestone Jun 28, 2018
@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Jun 28, 2018

I think this may actually be good to go for beta 3.

@jasongrout jasongrout changed the title WIP make kernel connection function synchronous Make kernel connection function synchronous Jun 28, 2018
@jasongrout jasongrout requested a review from ian-r-rose Jun 28, 2018
@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Jun 28, 2018

@ian-r-rose - could you review this?

Copy link
Member

@blink1073 blink1073 left a comment

I took a read-through, LGTM!

@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Jun 28, 2018

Thanks Steve!

Copy link
Member

@ian-r-rose ian-r-rose left a comment

I saw one problem here that was not your fault, otherwise, looks good to me!

// Create the header of the about dialog
let headerLogo = (<img src={kernelIconUrl} />);
let title = (
<span className='jp-About-header'>,
Copy link
Member

@ian-r-rose ian-r-rose Jun 28, 2018

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

@ian-r-rose ian-r-rose Jun 28, 2018

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

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Jun 28, 2018

Looks good, thanks @jasongrout!

@ian-r-rose ian-r-rose merged commit c4cdb84 into jupyterlab:master Jun 28, 2018
2 checks passed
@lock lock bot locked as resolved and limited conversation to collaborators Aug 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants