Skip to content

Added the option to wait for model & tokenizer in lmql.model.serve #15

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

Merged
merged 1 commit into from
Apr 16, 2023

Conversation

chrispan68
Copy link

Currently the model starts up and runs the server while the model and tokenizer are loading.

The client cannot tell when the model & tokenizer are ready, so they could send requests during this period. However, since models often take a long time to load, if the client sends a request during this period, they get a timeout, and an exception is raised.

A simple solution to this is to add an optional flag that tells the model server not to run until after the model and tokenizer are loaded. That way the client can know not to send requests while the model is loading (since the port is not open)

In some use cases where a python script wants to host a model and query inference, having this optional flag would be really convenient.

Since this is an optional flag, it doesn't change the current behavior at all, but just provides the option to people who want to use it.

@lbeurerkellner
Copy link
Collaborator

Thanks, looks good. I would test and then merge this PR.

Just a quick heads up, the core team has decided to relicense LMQL under the Apache 2.0 license, do you agree with merging your changes under this updated license. Let me know.

@chrispan68
Copy link
Author

I've tested this locally, and it works well.

I have a python script that hosts the model using POpen and queries the model once the server is running. Without the option, the script errors out on Timeout, after turning the option on, the script properly waits until both the model and tokenizer are running and no longer errors out.

I also agree to merging the changes under the updated license. Once this is approved I can merge it.

@lbeurerkellner
Copy link
Collaborator

Thanks, I could just test the fix. Works great. Merging this, thanks a lot.

@lbeurerkellner lbeurerkellner merged commit 847ca08 into eth-sri:main Apr 16, 2023
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