-
Notifications
You must be signed in to change notification settings - Fork 236
[Feature] Support for configure magic on Spark Python Kubernetes Kernels (WIP) #1105
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
base: main
Are you sure you want to change the base?
Changes from 2 commits
2c1cfe2
6780c5f
513ccc4
fadb724
994fc08
a49308d
34672a4
46eef00
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 | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -3,13 +3,20 @@ | |||||||||
"""Tornado handlers for kernel CRUD and communication.""" | ||||||||||
import json | ||||||||||
import os | ||||||||||
from datetime import datetime, timezone | ||||||||||
from functools import partial | ||||||||||
|
||||||||||
import jupyter_server.services.kernels.handlers as jupyter_server_handlers | ||||||||||
import tornado | ||||||||||
from jupyter_client.jsonutil import date_default | ||||||||||
from jupyter_server.base.handlers import APIHandler | ||||||||||
from tornado import web | ||||||||||
|
||||||||||
try: | ||||||||||
from jupyter_client.jsonutil import json_default | ||||||||||
except ImportError: | ||||||||||
from jupyter_client.jsonutil import date_default as json_default | ||||||||||
|
||||||||||
from ...mixins import CORSMixin, JSONErrorsMixin, TokenAuthorizationMixin | ||||||||||
|
||||||||||
|
||||||||||
|
@@ -146,11 +153,178 @@ def get(self, kernel_id): | |||||||||
self.finish(json.dumps(model, default=date_default)) | ||||||||||
|
||||||||||
|
||||||||||
default_handlers = [] | ||||||||||
class ConfigureMagicHandler(CORSMixin, JSONErrorsMixin, APIHandler): | ||||||||||
@web.authenticated | ||||||||||
async def post(self, kernel_id): | ||||||||||
self.log.info(f"Update request received for kernel: {kernel_id}") | ||||||||||
km = self.kernel_manager | ||||||||||
km.check_kernel_id(kernel_id) | ||||||||||
payload = self.get_json_body() | ||||||||||
self.log.debug(f"Request payload: {payload}") | ||||||||||
if payload is None: | ||||||||||
self.log.info("Empty payload in the request body.") | ||||||||||
self.finish( | ||||||||||
json.dumps( | ||||||||||
{"message": "Empty payload received. No operation performed on the Kernel."}, | ||||||||||
|
{"message": "Empty payload received. No operation performed on the Kernel."}, | |
{"message": f"Empty payload received. No operation performed on kernel: {kernel_id}"}, |
Outdated
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.
{"message": "Empty payload received. No operation performed on the Kernel."}, | |
{"message": f"Empty payload received. No operation performed on kernel: {kernel_id}"}, |
Outdated
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.
Same goes for these info
messages that precede exceptions. I believe EG will be logging these exceptions.
Could you please append something like f" for kernel: {kernel_id}"
to the end? As EG becomes more multi-tenant, its important we try to include some unique identifier in messages wherever possible.
Outdated
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.
Same goes for these info
messages that precede exceptions. I believe EG will be logging these exceptions.
Could you please append something like f" for kernel: {kernel_id}"
to the end? As EG becomes more multi-tenant, its important we try to include some unique identifier in messages wherever possible.
Outdated
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.
400, "Duplicate Kernel update request received for Id: %s." % kernel_id | |
400, "Duplicate configure kernel request received for kernel: %s." % kernel_id |
Outdated
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.
400, "Duplicate Kernel update request received for Id: %s." % kernel_id | |
400, "Duplicate configure kernel request received for kernel: %s." % kernel_id |
Outdated
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.
self.log.exception(f"An exception occurred in updating kernel : {kernel_id}: {e}") | |
self.log.exception(f"An exception occurred in re-configuring kernel: {kernel_id}: {e}") |
Outdated
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.
self.log.exception(f"An exception occurred in updating kernel : {kernel_id}: {e}") | |
self.log.exception(f"An exception occurred in re-configuring kernel: {kernel_id}: {e}") |
Outdated
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.
"Error occurred while refreshing Kernel: %s." % kernel_id, | |
"Error occurred while refreshing kernel: %s." % kernel_id, |
Outdated
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.
"Error occurred while refreshing Kernel: %s." % kernel_id, | |
"Error occurred while refreshing kernel: %s." % kernel_id, |
Outdated
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.
info -> debug
Outdated
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.
info -> debug
Outdated
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.
This is throughout. Please lowercase kernel
unless part of a name (e.g., KernelManager
).
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.
my bad. will change is all places.
Outdated
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.
This is throughout. Please lowercase kernel
unless part of a name (e.g., KernelManager
).
Outdated
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.
This feels like it should be r"/api/kernels/configure/%s"
. Is there an issue with placing it there? I don't see that conflicting with the patterns of the default handlers.
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.
sure..I can make the change.
I am also having a second thought to call this kernel Refresh API:
r"/api/kernels/refresh/%s"
?
Outdated
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.
This feels like it should be r"/api/kernels/configure/%s"
. Is there an issue with placing it there? I don't see that conflicting with the patterns of the default handlers.
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.
Isn't this meant to replace ZMQChannelsHandler
? I guess I don't understand why ZMQChannelsHandler
isn't satisfied by the first condition - but I'm not that familiar with globals()
(sorry).
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.
we need to discuss this further.
what I am trying do here is replace the ZMQChannelsHandler
with RemoteZMQChannelsHandler
for handling the channels
requests.
I tried to re-use the same class name on EG but was facing some issue where websocket connection was failing.
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.
These
info
messages aren't necessary since the message returned to the client will indicate where it came from.