search

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
615 stars 220 forks source link

Expose kernel_info_timeout configuration #1353

Closed lresende closed 4 months ago

lresende commented 6 months ago

@kevin-bates could you take a look and see if I forget any places related to exposing this new property

lresende commented 6 months ago

2. What enforces the info timeout? In process proxy we enforce the launch timeout if we have not received the connection info after that period of time. I don't see similar for kernel-info.

By exposing the property here and passing it to kernel manager, won't it be managed/enforced on the parent class? or are you saying I need to add additional support for enforcing on our side? this seemed to have worked on tests...

--RemoteMappingKernelManager.kernel_info_timeout=${EG_KERNEL_INFO_TIMEOUT}
lresende commented 6 months ago
  1. Could you please update the PR's description with what this is, and why it is necessary? I'm assuming its something about the time to receive the kernel_info_reply and is why its default spans that of KERNEL_LAUNCH_TIMEOUT.

Yes

kevin-bates commented 6 months ago

By exposing the property here and passing it to kernel manager, won't it be managed/enforced on the parent class? or are you saying I need to add additional support for enforcing on our side? this seemed to have worked on tests...

--RemoteMappingKernelManager.kernel_info_timeout=${EG_KERNEL_INFO_TIMEOUT}

Ah, sorry, you're right, EG does inherit most all of the plumbing for starting the kernel (not close enough to this anymore). So you shouldn't need any of the changes in the RemoteKernelManager since kernel_info_timeout pertains only to the MappingKernelManager (and its subclasses). As a result, from an EG config perspective, I think all that you need is EG_KERNEL_INFO_TIMEOUT. Calling the command line with --RemoteMappingKernelManager.kernel_info_timeout=${EG_KERNEL_INFO_TIMEOUT} should just work (since RMKM is subclass or MappingKernelManager), no need for any of the changes in remotemanager.py or processproxy.py or need to document KERNEL_INFO_TIMEOUT. You do need the shell and yaml changes, provided you want your value set from the env or command line.

((Of course, I probably something wrong here, but if my memory serves me correctly (fat chance) I believe this can be trimmed down to essentially documenting the EG variable and CLI.))

lresende commented 4 months ago

@kevin-bates sorry it took a little while to get back to this, but I believe I did the cleanups necessary, let me know what else needs to be done here...

lresende commented 4 months ago

Just one doc cleanup then we're good.

@kevin-bates done, also rebased so the doc builds should be passing now