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

Handling error during selecting a kernel #7094

Merged
merged 3 commits into from Aug 27, 2019
Merged

Conversation

@IMAM9AIS
Copy link

@IMAM9AIS IMAM9AIS commented Aug 26, 2019

This PR tries to solve the issue created here:- #7093

@jupyterlab-dev-mode
Copy link

@jupyterlab-dev-mode jupyterlab-dev-mode bot commented Aug 26, 2019

Thanks for making a pull request to JupyterLab!

To try out this branch on binder, follow this link: Binder

@@ -623,7 +623,11 @@ export class ClientSession implements IClientSession {
});
}
if (model) {
return this._changeKernel(model).then(() => undefined);
return this._changeKernel(model).then(() => undefined)
Copy link
Contributor

@jasongrout jasongrout Aug 26, 2019

Choose a reason for hiding this comment

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

_changeKernel devolves down to two cases:

return session.changeKernel(options);

In the above case, no error message is shown, and presumably is the case you are hitting, and

return this._startSession(options);

In the above case, the error message is shown if there is a problem.

If we trap errors from _changeKernel, we end up showing the error twice if we hit the second case above. How about we change _changeKernel to also show an error in the first case above instead?

Copy link
Author

@IMAM9AIS IMAM9AIS Aug 27, 2019

Choose a reason for hiding this comment

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

@jasongrout Nice catch. I have made the required changes. Do let me know your comments.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Aug 26, 2019

Thanks! I've put in a few comments inline.

Copy link
Member

@blink1073 blink1073 left a comment

LGTM!

@blink1073 blink1073 merged commit 3e4351b into jupyterlab:master Aug 27, 2019
9 checks passed
@blink1073
Copy link
Member

@blink1073 blink1073 commented Aug 27, 2019

Thanks and congratulations on your first contribution @IMAM9AIS!

@IMAM9AIS
Copy link
Author

@IMAM9AIS IMAM9AIS commented Sep 9, 2019

Hi @jasongrout ,

Thanks a lot for this merge.

We are currently using 0.33.x version of jupyter lab in our internal systems, while this PR was merged into the master.We wanted to understand what steps will be needed if we wanted to backport this fix in 0.33.x version.

Do let me know.

@lock lock bot locked as resolved and limited conversation to collaborators Oct 9, 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