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

Make _determine_next_host an async function #102

Open
divyansshhh opened this issue Sep 1, 2023 · 6 comments
Open

Make _determine_next_host an async function #102

divyansshhh opened this issue Sep 1, 2023 · 6 comments
Labels
enhancement An improvement to an existing feature

Comments

@divyansshhh
Copy link

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 divyansshhh added the enhancement An improvement to an existing feature label Sep 1, 2023
@welcome
Copy link

welcome bot commented Sep 1, 2023

Thank you for opening your first issue in this project! Engagement like this is essential for open source projects! 🤗

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! 👋

Welcome to the Jupyter community! 🎉

@kevin-bates
Copy link
Member

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.

@divyansshhh
Copy link
Author

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

@kevin-bates
Copy link
Member

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

@divyansshhh
Copy link
Author

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

Apologies for the delay here, raised - #122

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An improvement to an existing feature
Projects
None yet
Development

No branches or pull requests

2 participants