API Shift/Refactor #93
Conversation
k8s-ci-robot
added
cncf-cla: yes
k8s-ci-robot
removed
the
needs-rebase
kfswain
force-pushed
the
api-updates
branch
2 times, most recently
from
49760cd to
4cfac64
Compare
|
This is part of the effort for #68 |
|
And #69 (nice) |
|
/label tide/merge-method-squash |
k8s-ci-robot
added
the
tide/merge-method-squash
api/v1alpha1/inferencemodel_types.go
Outdated
| // LLMService represents a set of LLM services that are multiplexed onto one | ||
| // or more LLMServerPools. This resource is managed by the "LLM Service Owner" | ||
| // persona. The Service Owner persona is: a team that trains, verifies, and | ||
| // InferenceModelSpec represents a set of Models/Adapters that are multiplexed onto one |
There was a problem hiding this comment.
I'm a bit confused by the model vs. adapter terminology here. Would it be more accurate to say "models or adapters"?
Secondly, I think this shift includes a move to a singular model/adapter, so recommend changing the terminology here.
There was a problem hiding this comment.
Updated wording and broke formatting out into loose paragraphs to show separate ideas. LMKWYT
|
/hold just so it doesn't get merged by an accidental lgtm |
k8s-ci-robot
added
the
do-not-merge/hold
| // TargetModel represents a deployed model or a LoRA adapter. The | ||
| // Name field is expected to match the name of the LoRA adapter | ||
| // (or base model) as it is registered within the model server. Inference |
There was a problem hiding this comment.
I wonder if TargetModel is a bit misleading if it also represents an adapter.
There was a problem hiding this comment.
It does, but this mirrors the Open AI API spec, which just expects a modelName. So we chose not to deviate from that pattern.
LMKWYT
There was a problem hiding this comment.
it may not be necessarily be an adapter too
| Critical Criticality = "Critical" | ||
| // More important than Sheddable, less important than Critical. | ||
| // Requests in this band will be shed before critical traffic. | ||
| Default Criticality = "Default" |
There was a problem hiding this comment.
What about representing these in numbers like priorities instead of abstract names? Everything is relative to each other.
No worries! I was suprised it lasted as long as it did |
|
/hold I am good with this current incarnation of the api |
k8s-ci-robot
added
the
lgtm
| Critical Criticality = "Critical" | ||
| // More important than Sheddable, less important than Critical. | ||
| // Requests in this band will be shed before critical traffic. | ||
| Default Criticality = "Default" |
There was a problem hiding this comment.
I can get on board with starting with 3 simple values here, but likely worth revisiting in the future to see if additional granularity would be helpful. @terrytangyuan did you have any specific use cases in mind that would benefit from having additional levels here?
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kfswain, robscott The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
removed
the
lgtm
|
/lgtm |
k8s-ci-robot
added
the
lgtm
k8s-ci-robot
removed
the
lgtm
|
/unhold |
k8s-ci-robot
removed
the
do-not-merge/hold
|
/lgtm |
k8s-ci-robot
added
the
lgtm
* Big dawg api change * Feedback revisions * more feedback revisions * more feedback updates * (hopefully) final cleanup * adding required decorator, some more validation and defaulting * ObjRef name update * grammatical tunes * Feedback updates * Comment fix
This PR modfies the CRD name(s) and some of the CRD structure as discussed in the Dec 5th, 2024 inference-gateway contributors meeting.
Highlights being: