Skip to content
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

Expose kernel_info_timeout configuration #1353

Merged
merged 3 commits into from
Feb 20, 2024

Conversation

lresende
Copy link
Member

No description provided.

@lresende lresende requested a review from kevin-bates December 13, 2023 17:08
@lresende
Copy link
Member Author

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

@lresende lresende force-pushed the kernel-info-timeout branch 5 times, most recently from 1c43746 to 7c63195 Compare December 13, 2023 18:55
Copy link
Member

@kevin-bates kevin-bates left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @lresende. Aside from the comments posted here, I have some other comments/questions:

  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.
  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.

Since the kernel-info response is really handled via the websocket negotiation logic - which EG doesn't provide code for, it seems like this really needs to be part in JupyterServer rather than EG since there's no place for the enforcement. Hmm, and checking JupyterServer, I see kernel_info_timeout is defined there already so I guess I'm confused.

Why does EG need to be concerned about this at all? Seems like this is fully configurable (and usable) from the client entirely.

docs/source/operators/config-add-env.md Outdated Show resolved Hide resolved
docs/source/operators/config-add-env.md Outdated Show resolved Hide resolved
@@ -55,6 +55,9 @@ There are several supported `KERNEL_` variables that the Enterprise Gateway serv
expected to exceed that of the EG_KERNEL_LAUNCH_TIMEOUT set when Enterprise
Gateway starts.

KERNEL_INFO_TIMEOUT=<from user> or EG_KERNEL_INFO_TIMEOUT=60
The time (in seconds) Enterprise Gateway will wait before giving up on a kernel
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps use the same text as the EG_ variable (once clarified by previous comment), but also please add how this is different (e.g., this only applies to the single kernel launch because its conveyed from the client or something).

enterprise_gateway/services/kernels/remotemanager.py Outdated Show resolved Hide resolved
@lresende
Copy link
Member Author

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
Copy link
Member Author

  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
Copy link
Member

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 lresende force-pushed the kernel-info-timeout branch 2 times, most recently from 669cd4d to 2cc074f Compare February 19, 2024 05:31
@lresende
Copy link
Member Author

@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...

Copy link
Member

@kevin-bates kevin-bates left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one doc cleanup then we're good.

docs/source/users/kernel-envs.md Outdated Show resolved Hide resolved
@lresende
Copy link
Member Author

Just one doc cleanup then we're good.

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

Copy link
Member

@kevin-bates kevin-bates left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@lresende lresende merged commit 90961ab into jupyter-server:main Feb 20, 2024
14 checks passed
@lresende lresende deleted the kernel-info-timeout branch February 20, 2024 00:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants