refactor: Make base client concrete and usable#246
refactor: Make base client concrete and usable#246LukeAVanDrie wants to merge 1 commit intokubernetes-sigs:mainfrom
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: LukeAVanDrie The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Welcome @LukeAVanDrie! |
k8s-ci-robot
added
the
size/S
|
I am looking into making some perf improvements in the client code. As I haven't contributed to this project yet, I wanted to start with a smaller change to familiarize myself with the PR and CI/CD processes. If it's intended that openAIModelServerClient is a class that must be subclassed, I'll drop this change, but I would suggest renaming the class itself to indicate it is intended as a base class only. |
LukeAVanDrie
force-pushed
the
refactor/client-architecture
branch
from
56714ad to
052d06c
Compare
The openAIModelServerClient could not be instantiated directly as it declared no supported APIs. While this may have been intended to enforce it as a base class, making it concrete provides more flexibility. This change allows the client to be used with any generic OpenAI-compatible endpoint. It also centralizes the API list so redundant overrides can be removed from the vLLM, TGI, and SGLang subclasses, improving maintainability.
LukeAVanDrie
force-pushed
the
refactor/client-architecture
branch
from
052d06c to
8bbbd52
Compare
achandrasekar
left a comment
achandrasekar
left a comment
There was a problem hiding this comment.
Thanks for sending this out! This can simplify things quite a bit.
Can you also include how you tested the change with an example config and a successful run - one for the new openai type and another for vllm?
|
|
||
| def get_supported_apis(self) -> List[APIType]: | ||
| return [] | ||
| return [APIType.Completion, APIType.Chat] |
There was a problem hiding this comment.
Can you add a type openai here? This could be the generic type that someone can run with if it is one of the model servers where we don't have specific metrics support?
There was a problem hiding this comment.
Please update the documentation as well along with the change.
|
@LukeAVanDrie friendly ping on this |
SachinVarghese
added
the
kind/failing-test
|
@LukeAVanDrie any updates on this PR? |
5 participants
The openAIModelServerClient could not be instantiated directly as it declared no supported APIs. While this may have been intended to enforce it as a base class, making it concrete provides more flexibility.
This change allows the client to be used with any generic OpenAI-compatible endpoint. It also centralizes the API list so redundant overrides can be removed from the vLLM, TGI, and SGLang subclasses, improving maintainability.