-
Notifications
You must be signed in to change notification settings - Fork 133
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
[Issue 29]: Options to enable HTTP access to kernels #35
[Issue 29]: Options to enable HTTP access to kernels #35
Conversation
Refactored with changes from @Lull3rSkat3r and @parente: * Use kernel client library to wait for the kernel to start, draining shell output when executing non-API cells * Added path params for access from API functions. * Fixed encoding issue with args parsing. * Added scotch notebook example. * Add parsing of query parameters * Can access request body from cells * Body assignment and API cell indicator respect notebook kernel spec (c) Copyright IBM Corp. 2015
(c) Copyright IBM Corp. 2015
… the future. Added license headers to test_request_utils.py (c) Copyright IBM Corp. 2015
Does this replace PR #32? |
Yes. |
@@ -152,9 +153,16 @@ def _default_kernel_name_default(self): | |||
def _list_kernels_default(self): | |||
return os.getenv(self.list_kernels_env, 'False') == 'True' | |||
|
|||
api_env = 'KG_API' | |||
api = Unicode(config=True, | |||
help='Designates that the kernel gateway will serve APIs from this notebook (KG_API env var)' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a choices traitlet? If so, this should use it. Either way, the currently supported options should be "jupyter-websocket" and "notebook-http". Anything else should be an error. The help string needs to reflect the available options if you can't define a set of choices in a traitlet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need a dedicated option for that plus another specifying the notebook whose APIs should be exposed, or just renaming and documenting this one for clarity?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The two options, the notebook to use and the API to expose, are separate. The seed notebook makes sense under both API modes (and potentially new modes that will be added.)
Some system tests would also be good. You can keep cramming them into |
* Change the API argument to switch between Jupyter Kernel mode and Notebook API serving mode, and exit with error text on wrong value * Serve APIs from the seed_uri notebook * Merge the two kernel manager classes back into one * Fix the sorted_endpoints iterating * Change extraneous print() call to logging * Delegate per-language handling into methods and dictionaries (c) Copyright IBM Corp. 2015
(c) Copyright IBM Corp. 2015
…raitlet (c) Copyright IBM Corp. 2015
(c) Copyright IBM Corp. 2015
…ls from leaking. (c) Copyright IBM Corp. 2015
(c) Copyright IBM Corp. 2015
…ssages (c) Copyright IBM Corp. 2015
… execution to complete. (c) Copyright IBM Corp. 2015
Also fix a couple of spelling typos (c) Copyright IBM Corp. 2015
(c) Copyright IBM Corp. 2015
(c) Copyright IBM Corp. 2015
(c) Copyright IBM Corp. 2015
OK. I think this is MVP enough to put into master. I've opened some follow on issues. And we'll open more as we find bugs or potential improvements. Nice work @nitind and @Lull3rSkat3r. I think people are going to have some fun with this once we make it easy to convert and deploy. |
[Issue 29]: Options to enable HTTP access to kernels
Refactored with changes from @Lull3rSkat3r and @parente:
draining shell output when executing non-API cells
Ref issue #29