gateway_provisioners icon indicating copy to clipboard operation
gateway_provisioners copied to clipboard

Make `_determine_next_host` an async function

Open divyansshhh opened this issue 2 years ago • 6 comments

Problem

We are creating a custom kernel provisioner and we have an API for finding a host based on certain parameters but this API takes upto 20s to send the host. This 20s wait blocks the server and makes lab unusable during those 20s. We tried making the _determine_next_host function async ourselves by running the host fetching API in a asyncio's run_in_executor and awaiting the _determine_next_host call in the launch_kernel function.

A simple reproducer for this is adding a asyncio.sleep(10) in the DistributedProvisioner._determine_next_host function. The result will be that the kernel never reaches an alive state.

Proposed Solution

We would like a way to asynchronously determine the next host.

Additional context

I'm using gateway_provisioners v0.2.0

divyansshhh avatar Sep 01 '23 06:09 divyansshhh

Thank you for opening your first issue in this project! Engagement like this is essential for open source projects! :hugs:
If you haven't done so already, check out Jupyter's Code of Conduct. Also, please try to follow the issue template as it helps other other community members to contribute more effectively. welcome You can meet the other Jovyans by joining our Discourse forum. There is also an intro thread there where you can stop by and say Hi! :wave:
Welcome to the Jupyter community! :tada:

welcome[bot] avatar Sep 01 '23 06:09 welcome[bot]

Hi @divyansshhh - thank you for using Gateway Provisioners and opening this issue.

I think this makes a lot of sense (and should have probably been async from the start).

Because _determine_next_host is "private", I think we could go ahead and decorate the method as async without issue. However, I'm thinking this seems like a great "extension point" for folks, including those that don't want to implement a new algorithm using the existing approach, and wondering if we just "promote" _determine_next_host() to a full-fledged method that can be "officially" overridden:

async def determine_next_host(self, env_dict: dict) -> str:

Do you see any issues with that?

Would you be willing to create such a pull request?

Thanks, Kevin.

kevin-bates avatar Sep 01 '23 14:09 kevin-bates

I don't see any issues with this, I can raise the PR for this.

divyansshhh avatar Sep 05 '23 05:09 divyansshhh

It just occurred to me that we could preserve backward compatibility by injecting an async method without the underscore that then calls the current sync method. Then await the call to the new method. Folks that want custom behaviors could then override the new method. (This may have been what you were thinking anyway.)

kevin-bates avatar Sep 05 '23 14:09 kevin-bates

I agree with the suggestion and I had something similar in mind.

I'm trying to debug something which may or may not be related to this. Let me do that and then raise the PR.

divyansshhh avatar Sep 06 '23 16:09 divyansshhh

Apologies for the delay here, raised - https://github.com/jupyter-server/gateway_provisioners/pull/122

divyansshhh avatar Nov 23 '23 13:11 divyansshhh