-
Notifications
You must be signed in to change notification settings - Fork 14.5k
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
feat(0.5): update support for OpenLLM 0.5 #22442
Conversation
Signed-off-by: paperspace <29749331+aarnphm@users.noreply.github.com>
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Signed-off-by: paperspace <29749331+aarnphm@users.noreply.github.com>
Signed-off-by: paperspace <29749331+aarnphm@users.noreply.github.com>
Signed-off-by: paperspace <29749331+aarnphm@users.noreply.github.com>
model_name: Optional[str] = None, | ||
*, | ||
model_id: Optional[str] = None, | ||
server_url: Optional[str] = None, | ||
server_url: str, | ||
timeout: int = 30, | ||
server_type: Literal["grpc", "http"] = "http", | ||
embedded: bool = True, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shoudl we keep around the old args and raise a warning if they're passed in, so that this isn't a breaking change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure I think we can do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, this is for the new class OpenLLMAPI, but for OpenLLM we can keep backward compat.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah missed the rename :+1
@baskaryan since the OpenLLM implementation here won't support |
bump @baskaryan when you have bandwidth. sorry for the ping :) |
This PR updates the internal wrapper for OpenLLM
Since OpenLLM v0.5 is considered breaking, I have also taken this opportunity to separate the API and local inference component into two separate class:
OpenLLMAPI
will be responsible for handing remote server going forward (async, sync, streaming supported)OpenLLM
will now only for local inference, and only support batching and synchronous generations.Hopefully this would make it a bit more future-proof. In terms of testing, I believe I have tested through some manual workflow and will be include the tests on our end.
updates from #19894, cc @baskaryan