feat: add inference request and received time in FlowControlRequest#2475
Conversation
k8s-ci-robot
added
do-not-merge/work-in-progress
✅ Deploy Preview for gateway-api-inference-extension ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
needs-ok-to-test
|
Hi @loicmarchal. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
k8s-ci-robot
added
the
size/S
|
@LukeAVanDrie this is the changes I mentioned to prepare for a SLO-based ordering policy. |
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
| fairnessID: reqCtx.FairnessID, | ||
| priority: priority, | ||
| requestByteSize: uint64(reqCtx.RequestSize), | ||
| reqHeaders: reqCtx.Request.Headers, |
There was a problem hiding this comment.
@LukeAVanDrie I am wondering if we should unify the request related state on LLMRequest (and its future successor InferenceRequest?
There was a problem hiding this comment.
Sure, that will also work and may be ultimately cleaner for the Flow Control layer. Do we want to do that here or in a follow-up? For example this means we can roll the priority field into LLMRequest.RequestObjectives too. I don't want to push that cleanup/simplification work onto this PR though. I can take that on as a no-op refactoring effort later.
There was a problem hiding this comment.
As an aside, one thing I did try to do in the Flow Control layer is ensure that any plugin author only has read-only access to request / flow / priority band state. If we embed this LLMRequest we leak a mutable pointer reference to the policies, but I also understand if you are not concerned about being overly defensive here in the pursuit of streamlining our types.
There was a problem hiding this comment.
@ahg-g @LukeAVanDrie I replaced the headers by the LLMRequest that also contain the headers. Does that work as a preliminary step to include a more generic InferenceRequest suggested in #2447 later?
I left some of the accessor functions such as ID() and TargetModelName(), although we could access those info from the request itself, to not change the contract too much. Is this ok or should it be refactored?
There was a problem hiding this comment.
I'm okay with this for now. I am happy to do that in a follow-up to not muddy this PR with a ton of mock / test changes.
There was a problem hiding this comment.
Thank you @LukeAVanDrie !
If you think it should be done, I don't mind cleaning up the code and the mock / test as part of this PR. Let me know what you think is best.
k8s-ci-robot
added
size/M
|
@ahg-g @LukeAVanDrie are we ok with these changes? Can we merge this or should we do the clean up mentioned in #2475 (comment) first? |
loicmarchal
changed the title
|
/ok-to-test leaving final stamp for @ahg-g. |
k8s-ci-robot
added
ok-to-test
k8s-ci-robot
added
the
lgtm
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ahg-g, loicmarchal, LukeAVanDrie The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
…ubernetes-sigs#2475) * feat: add request headers and received time in FlowControlRequest * replace headers by LLMRequest from scheduling layer
…ubernetes-sigs#2475) * feat: add request headers and received time in FlowControlRequest * replace headers by LLMRequest from scheduling layer
…ubernetes-sigs#2475) * feat: add request headers and received time in FlowControlRequest * replace headers by LLMRequest from scheduling layer
…ubernetes-sigs#2475) * feat: add request headers and received time in FlowControlRequest * replace headers by LLMRequest from scheduling layer
…ubernetes-sigs/gateway-api-inference-extension#2475) * feat: add request headers and received time in FlowControlRequest * replace headers by LLMRequest from scheduling layer
5 participants
What type of PR is this?
/kind feature
What this PR does / why we need it:
Extends the information available in the FlowControlRequest. These information will be useful for specific ordering policies, such as a SLO-based policy
Does this PR introduce a user-facing change?: