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

adding options to run notebook with kernel gateway #3006

Closed
wants to merge 1 commit into from

Conversation

shiti-saxena
Copy link
Contributor

resolves #3005

Copy link
Member

@minrk minrk left a comment

Choose a reason for hiding this comment

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

Thanks! I think this mostly makes sense, but special-casing kernel handlers doesn't seem quite right. What about sessions etc.?

Maybe there should be a generic handlers-overrides mechanism that would allow overriding any handlers?

Thanks to the load_handlers mechanism, this could be a list of modules, e.g. extra_services to be loaded that are ensured to be loaded at higher priority to the default services.

What do you think?

help=_('The kernel handlers to use.'),
)

kernelspecs_handlers = Unicode(
Copy link
Member

Choose a reason for hiding this comment

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

nit: singluar 'kernelspec_handlers' to match 'kernel_handlers'

@shiti-saxena
Copy link
Contributor Author

@minrk It is possible to modify the load_handlers method to allow overriding any handler as currently it adds notebook. prefix. But I wasn't sure if we would want to expose that to users. Should I update the PR to do that as well?

parente
parente previously requested changes Dec 9, 2017
Copy link
Member

@parente parente left a comment

Choose a reason for hiding this comment

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

I agree with @minrk here. Overriding the kernel handler and kernel manager handler was the only requirement for the nb2kg demo. Making the overrides general purpose seems more appropriate for a notebook server feature.

@parente parente dismissed their stale review December 9, 2017 02:10

Did not mean to request changes, only comment. Trying again!

@shiti-saxena
Copy link
Contributor Author

@minrk @parente I've created another PR #3158 based on the feedback and comments, what do you think of it?

@takluyver
Copy link
Member

Closing for now in favour of #3158

@takluyver takluyver closed this Jan 5, 2018
@minrk minrk added this to the Reference milestone Jun 15, 2018
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 31, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add option to run Notebook with Kernel Gateway
5 participants