-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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
Update gateway support with recent changes #4431
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -91,6 +91,16 @@ def _kernels_endpoint_default(self): | |
def _kernelspecs_endpoint_default(self): | ||
return os.environ.get(self.kernelspecs_endpoint_env, self.kernelspecs_endpoint_default_value) | ||
|
||
kernelspecs_resource_endpoint_default_value = '/kernelspecs' | ||
kernelspecs_resource_endpoint_env = 'JUPYTER_GATEWAY_KERNELSPECS_RESOURCE_ENDPOINT' | ||
kernelspecs_resource_endpoint = Unicode(default_value=kernelspecs_resource_endpoint_default_value, config=True, | ||
help="""The gateway endpoint for accessing kernelspecs resources | ||
(JUPYTER_GATEWAY_KERNELSPECS_RESOURCE_ENDPOINT env var)""") | ||
|
||
@default('kernelspecs_resource_endpoint') | ||
def _kernelspecs_resource_endpoint_default(self): | ||
return os.environ.get(self.kernelspecs_resource_endpoint_env, self.kernelspecs_resource_endpoint_default_value) | ||
|
||
connect_timeout_default_value = 20.0 | ||
connect_timeout_env = 'JUPYTER_GATEWAY_CONNECT_TIMEOUT' | ||
connect_timeout = Float(default_value=connect_timeout_default_value, config=True, | ||
|
@@ -333,6 +343,11 @@ def start_kernel(self, kernel_id=None, path=None, **kwargs): | |
|
||
kernel_env = {k: v for (k, v) in dict(os.environ).items() if k.startswith('KERNEL_') | ||
or k in GatewayClient.instance().env_whitelist.split(",")} | ||
|
||
# Convey the full path to where this notebook file is located. | ||
if path is not None and kernel_env.get('KERNEL_WORKING_DIR') is None: | ||
kernel_env['KERNEL_WORKING_DIR'] = kwargs['cwd'] | ||
|
||
json_body = json_encode({'name': kernel_name, 'env': kernel_env}) | ||
|
||
response = yield gateway_request(kernel_url, method='POST', body=json_body) | ||
|
@@ -467,7 +482,10 @@ class GatewayKernelSpecManager(KernelSpecManager): | |
|
||
def __init__(self, **kwargs): | ||
super(GatewayKernelSpecManager, self).__init__(**kwargs) | ||
self.base_endpoint = url_path_join(GatewayClient.instance().url, GatewayClient.instance().kernelspecs_endpoint) | ||
self.base_endpoint = url_path_join(GatewayClient.instance().url, | ||
GatewayClient.instance().kernelspecs_endpoint) | ||
self.base_resource_endpoint = url_path_join(GatewayClient.instance().url, | ||
GatewayClient.instance().kernelspecs_resource_endpoint) | ||
|
||
def _get_kernelspecs_endpoint_url(self, kernel_name=None): | ||
"""Builds a url for the kernels endpoint | ||
|
@@ -498,14 +516,7 @@ def get_all_specs(self): | |
notebook_default=km.default_kernel_name)) | ||
km.default_kernel_name = remote_default_kernel_name | ||
|
||
# gateway doesn't support resources (requires transfer for use by NB client) | ||
# so add `resource_dir` to each kernelspec and value of 'not supported in gateway mode' | ||
remote_kspecs = fetched_kspecs.get('kernelspecs') | ||
for kernel_name, kspec_info in remote_kspecs.items(): | ||
if not kspec_info.get('resource_dir'): | ||
kspec_info['resource_dir'] = 'not supported in gateway mode' | ||
remote_kspecs[kernel_name].update(kspec_info) | ||
|
||
raise gen.Return(remote_kspecs) | ||
|
||
@gen.coroutine | ||
|
@@ -540,9 +551,32 @@ def get_kernel_spec(self, kernel_name, **kwargs): | |
raise | ||
else: | ||
kernel_spec = json_decode(response.body) | ||
# Convert to instance of Kernelspec | ||
kspec_instance = self.kernel_spec_class(resource_dir=u'', **kernel_spec['spec']) | ||
raise gen.Return(kspec_instance) | ||
|
||
raise gen.Return(kernel_spec) | ||
|
||
@gen.coroutine | ||
def get_kernel_spec_resource(self, kernel_name, path): | ||
"""Get kernel spec for kernel_name. | ||
|
||
Parameters | ||
---------- | ||
kernel_name : str | ||
The name of the kernel. | ||
path : str | ||
The name of the desired resource | ||
""" | ||
kernel_spec_resource_url = url_path_join(self.base_resource_endpoint, str(kernel_name), str(path)) | ||
self.log.debug("Request kernel spec resource '{}' at: {}".format(path, kernel_spec_resource_url)) | ||
try: | ||
response = yield gateway_request(kernel_spec_resource_url, method='GET') | ||
except HTTPError as error: | ||
if error.code == 404: | ||
kernel_spec_resource = None | ||
else: | ||
raise | ||
else: | ||
kernel_spec_resource = response.body | ||
raise gen.Return(kernel_spec_resource) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FWIW, since we require Python 3, we can use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok - thanks. Since this is one of many gen.Returns, I think it makes sense to change these whenever we "make the sweep" to move to async/await - if that's okay with you. |
||
|
||
|
||
class GatewaySessionManager(SessionManager): | ||
|
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.
Does every endpoint need to be separately configurable? It seems to me like a single base path should be everything that's needed.
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.
I believe these are configurable (in NB2KG is done via just envs) for cases when the gateway server (JKG or JEG) is behind a reverse proxy. This should be the last of them. (yeah, famous last words, I know. 😄)
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.
ok, let's switch this over to a single configurable prefix before adding any more after this one.