-
Notifications
You must be signed in to change notification settings - Fork 180
Multi cycle scheduler #862
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
Multi cycle scheduler #862
Conversation
Signed-off-by: Nir Rozenbaum <[email protected]>
Signed-off-by: Nir Rozenbaum <[email protected]>
Signed-off-by: Nir Rozenbaum <[email protected]>
Signed-off-by: Nir Rozenbaum <[email protected]>
Signed-off-by: Nir Rozenbaum <[email protected]>
Signed-off-by: Nir Rozenbaum <[email protected]>
Signed-off-by: Nir Rozenbaum <[email protected]>
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. |
Signed-off-by: Nir Rozenbaum <[email protected]>
Signed-off-by: Nir Rozenbaum <[email protected]>
/hold I was tempted to close this. But I will hold for now. Scheduler refactors should not move forward until: https://github.com/kubernetes-sigs/gateway-api-inference-extension/pull/845/files is merged |
targetPod := results[0].TargetPod.GetPod() | ||
var targetPod *backend.Pod | ||
// TODO should handle multi cycle results, this should be pluggable logic | ||
for _, result := range results { |
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.
this will just set the final targetPod. Can we instead just index on the key & get the result that way? We can leave the todo to indicate this is a transitory state.
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.
you mean doing this?
targetPod = results[key].TargetPod.GetPod()
instead of:
targetPod = result.TargetPod.GetPod()
not sure I got the intention. we have here a map from profile-name -> result.
this is a transitionary stage where only one profile is used, so only one result.
but since it's a map I must use the range loop.
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.
Yeah, just do targetPod = results[key].TargetPod.GetPod()
and drop the for loop.
Since we only end up with one result as is even if there are multiple profiles, it reads more obvious if we just use the key, I think.
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.
so now the next question, what is the value of key
in this line
targetPod = results[key].TargetPod.GetPod()?
which key
value to use?
if we used default configuration it's just "default", but if we used schedulerv2, it's a different profile.
since we don't know the key, I'm using the loop...
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.
I expect this to be solved as soon as we implement the extension point
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.
The only one we set is schedulerv2
right?
line 215 in the main file of this PR
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.
I agree that this may be a nit, but the for loop would look out of place since the iteration isn't quite being used
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.
if we used the env var to enable "schedulerv2" than the key is "schedulerv2".
if not and we use default configuration, the key is "default" (in NewScheduler func):
https://github.com/kubernetes-sigs/gateway-api-inference-extension/blob/main/pkg/epp/scheduling/scheduler.go#L73
this is why I didn't put here a const.
mostly I think this is fine, a lot of this will be replaced as we most closely align with: #845 small comment to keep the director clean for now. |
yes, agreed. |
/lgtm Holding submission for author to choose to address comments |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kfswain, 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 |
unholding to avoid conflicts with other PRs since this PR touches 30 files. /unhold |
* code review Signed-off-by: Nir Rozenbaum <[email protected]> * minor change Signed-off-by: Nir Rozenbaum <[email protected]> * add support for multi cycle scheduling Signed-off-by: Nir Rozenbaum <[email protected]> * minor change Signed-off-by: Nir Rozenbaum <[email protected]> * moved plugins under plugins dir Signed-off-by: Nir Rozenbaum <[email protected]> * few more changes Signed-off-by: Nir Rozenbaum <[email protected]> * moved RunCycle logic into SchedulerProfile Signed-off-by: Nir Rozenbaum <[email protected]> * minor changes Signed-off-by: Nir Rozenbaum <[email protected]> * linter Signed-off-by: Nir Rozenbaum <[email protected]> * minor change in unit-test Signed-off-by: Nir Rozenbaum <[email protected]> --------- Signed-off-by: Nir Rozenbaum <[email protected]>
* code review Signed-off-by: Nir Rozenbaum <[email protected]> * minor change Signed-off-by: Nir Rozenbaum <[email protected]> * add support for multi cycle scheduling Signed-off-by: Nir Rozenbaum <[email protected]> * minor change Signed-off-by: Nir Rozenbaum <[email protected]> * moved plugins under plugins dir Signed-off-by: Nir Rozenbaum <[email protected]> * few more changes Signed-off-by: Nir Rozenbaum <[email protected]> * moved RunCycle logic into SchedulerProfile Signed-off-by: Nir Rozenbaum <[email protected]> * minor changes Signed-off-by: Nir Rozenbaum <[email protected]> * linter Signed-off-by: Nir Rozenbaum <[email protected]> * minor change in unit-test Signed-off-by: Nir Rozenbaum <[email protected]> --------- Signed-off-by: Nir Rozenbaum <[email protected]>
This is an initial PR that lays the ground for a multi cycle scheduler.
we have an ongoing open discussions in #845 and on google docs and this is not intended to serve as a replacement for the discussions.
This PR is not covering all the planned changes that were discussed, but takes a big step towards the end goal.
in the scope of this PR:
/scheduling/framework
.SchedulerConfig
toSchedulerProfile
. the Profile includes the extensions.SchedulerConfig
that now includes the registered profiles (map from profile name(string) to the profile config) and the profiles-picker.out of scope:
summary:
additional changes may be needed as we continue to shape how we want the scheduler to look like.
as a first step, this implements many of the points we already agree on, enables the removal of P/D scheduler from llm-d, and allows configuring all llm-d scheduler related behavior (including conditional profile picking) through plugins only.