scheduler redesign continuation #937
Conversation
Signed-off-by: Nir Rozenbaum <[email protected]>
✅ 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
cncf-cla: yes
nirrozenbaum
changed the title
| Pick(ctx context.Context, request *types.LLMRequest, profiles map[string]*SchedulerProfile, executionResults map[string]*types.Result) map[string]*SchedulerProfile | ||
| // Pick selects the SchedulingProfiles to run from a list of candidate profiles, while taking into consideration the request properties | ||
| // and the previously executed SchedluderProfile cycles along with their results. | ||
| Pick(ctx context.Context, request *types.LLMRequest, profiles map[string]*SchedulerProfile, profileResults map[string]*types.ProfileRunResult) map[string]*SchedulerProfile |
There was a problem hiding this comment.
Perhaps we need a different name so we don't confuse it with the endpoint picker plugin. One suggestion is Select.
There was a problem hiding this comment.
Generally speaking, I'd like to be consistent in terminology.
if we use Pick function in the Picker interface, keeping consistency here means having Pick function in the ProfileHandler interface.
I'm ok with changing it to Select but in such a case I'd consider changing in all places to keep it consistent -
e.g., rename Picker to Selector and rename Pick function to Select in both places.
LMKWYT.
There was a problem hiding this comment.
What I was trying to address is this: if I am searching the code for pick, it would be better if most of the results are unambiguous and referred to one thing, which is endpoint picking for example, although I have to admit select is also generic (could mean label selection). I am fine to keep it as pick, just sharing a thought I had.
| ScorerPluginType = "Scorer" | ||
| PickerPluginType = "Picker" | ||
| PostCyclePluginType = "PostCycle" | ||
| ProfilePickerType = "ProfilePicker" |
There was a problem hiding this comment.
Here it's actually the extension point name and not the plugin name -
we want to record a metric for how much time it takes to perform profile picking and how much time to perform ProcessProfilesResults.
This is why the list contains both (please ignore PostCycle which will be removed as soon as possible):
const (
ProfilePickerType = "ProfilePicker"
FilterPluginType = "Filter"
ScorerPluginType = "Scorer"
PickerPluginType = "Picker"
PostCyclePluginType = "PostCycle"
ProcessProfilesResultsType = "ProcessProfilesResults"
)| type Result struct { | ||
| // ProfileRunResult captures the profile run result. | ||
| type ProfileRunResult struct { | ||
| TargetPod Pod |
There was a problem hiding this comment.
this is supposed to be a list, or is that planned in a followup?
There was a problem hiding this comment.
yes. it should be a list but is not addressed in this PR which focused on ProfileHandler changes and adding the ProcessResult extension point.
will be addressed in a follow up.
Signed-off-by: Nir Rozenbaum <[email protected]>
nirrozenbaum
force-pushed
the
profile-handler
branch
from
238b3a7 to
ef498b9
Compare
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ahg-g, nirrozenbaum 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
added
the
approved
* implement agreed points on scheduler redesign Signed-off-by: Nir Rozenbaum <[email protected]> * addressed code review comments Signed-off-by: Nir Rozenbaum <[email protected]> --------- Signed-off-by: Nir Rozenbaum <[email protected]>
3 participants
following agreements made on #905,
this PR implements another step towards the end goal of the scheduler new design. main changes include:
PreRequestplugin).PostCycle. once PostResponse is used in prefix plugin, we should remove PostCycle.