jupyter-server / enterprise_gateway

A lightweight, multi-tenant, scalable and secure gateway that enables Jupyter Notebooks to share resources across distributed clusters such as Apache Spark, Kubernetes and others.
https://jupyter-enterprise-gateway.readthedocs.io/en/latest/
Other
620 stars 223 forks source link

Move shutdown kernel code to be within KernelClient #1281

Closed starskyreverie closed 1 year ago

starskyreverie commented 1 year ago

The heart of the code that shuts down the kernel (the API request) should be within KernelClient itself. This way, when the kernel can shut itself done without the help of GatewayClient. This is important in handling the websocket exception, as when we call kernel.shutdown(), we actually want to run the API request to shut down the kernel as well.

Additionally, we should still raise the exception on KernelClient.__init__() if the websocket connection fails, as that way GatewayClient.start_kernel will error out if the connection to the KernelClient isn't working.

kevin-bates commented 1 year ago

Hmm - I guess Python still creates an instance and __init__() isn't like a constructor in Java.

Could we please move the initializing of these 3 attributes to PRIOR to the create_connection call?

starskyreverie commented 1 year ago

Done

starskyreverie commented 1 year ago

Agreed, I was unsure about that too, I'll definitely submit another PR if I notice the thread being an issue

kevin-bates commented 1 year ago

Why were the attribute checks restored? Please see https://github.com/pq43/enterprise_gateway/pull/1 and consider merging it as part of this PR.

starskyreverie commented 1 year ago

Ah I restored them because all tests were failing. I wanted to debug what was going on, I think it has something to do with the response reader thread not being shutdown properly but I'm unsure. The solution in that PR (initializing but only starting the thread after the WS connection) makes a lot of sense though, I didn't realize you could do that as well - thanks!