scheduler proposal continuation #905
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,8 +28,7 @@ type Endpoint struct { | |
|
|
||
| type EndpointState struct { | ||
| // storage is per Scheduling Cycle, and so has no thread-safe concerns. | ||
| storage map[string]any //nolint:unused | ||
| } | ||
|
|
||
| // Request is a structured representation of the fields we parse out of the Request body. | ||
|
|
@@ -58,49 +57,57 @@ type Scheduler struct { | |
| } | ||
|
|
||
| // SchedulerConfig is the struct that maps to the configuration file that should be further discussed. | ||
| // the configuration file should include the ProfileHandler plugin as well as the profiles with their plugins. | ||
| type SchedulerConfig struct { | ||
| // exactly one ProfileHandler instance is required. | ||
| profileHandler ProfileHandler //nolint:unused | ||
| // map from profile name to its set of plugins. | ||
| profiles map[string]*SchedulerProfile //nolint:unused | ||
| } | ||
|
|
||
| // SchedulerProfile is used to describe a profile that will | ||
| // run for a given scheduling cycle. | ||
| type SchedulerProfile struct { | ||
| // Filters lists all Filter plugins associated with this Profile. | ||
| // Filters are optional. | ||
| filters []Filter //nolint:unused | ||
| // Scorers lists all Score plugins associated with this Profile. | ||
| // Scorers are optional. | ||
| scorers []*WeightedScorer //nolint:unused | ||
| // Picker returns the function that picks the endpoint(s). Picker is required. | ||
| picker Picker //nolint:unused | ||
| } | ||
|
|
||
| type SchedulingResult struct { | ||
| ProfileResults map[string][]*Endpoint // a map from profile name to its scheduling result | ||
| PrimaryProfileName string // key of the primary profile, its selected endpoints will be used by default as the destination | ||
| } | ||
|
|
||
| // Plugin is the parent type for all the scheduling framework plugins. | ||
| type Plugin interface { | ||
| Name() string | ||
| } | ||
|
|
||
| // ProfileHandler defines the interface for handling multi SchedulerProfile instances. | ||
| // More specifically, this interfaction defines two extension points, 'PickProfiles' | ||
| // which runs iteratively, and 'ProcessProfilesResults' which runs after all profiles runs complete | ||
| // and process the results of all profiles. | ||
| type ProfileHandler interface { | ||
| Plugin | ||
| // Pick picks the SchedulingProfile objects to run from a list of candidate profiles, | ||
| // while taking into consideration the request properties | ||
| // and the previously executed SchedluderProfile runs along with their results. | ||
| // returns: | ||
| // - profiles - A subset of the registered scheduling profiles to be ran in next iteration | ||
| Pick(request *Request, profiles map[string]*SchedulerProfile, executionResults map[string][]*ScoredEndpoint) map[string]*SchedulerProfile | ||
|
|
||
| // ProcessResults handles the outcome of each profile run. | ||
| // It may aggregate results, log test profile outputs, or apply custom logic. It specifies in the SchedulingResult the | ||
| // key of the primary profile that should be used to get the request selected destination. | ||
| // Example: suppose you have 2 profiles ShadowBoxing Profile & Production Profile. | ||
| // ProcessProfileResults would know to simply log the result of ShadowBoxing | ||
| // profile, and do nothing else with it. | ||
nirrozenbaum marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| ProcessResults(request *Request, profileResults map[string][]*ScoredEndpoint) *SchedulingResult | ||
| } | ||
|
Contributor
Author
There was a problem hiding this comment. a question I was thinking about- in llm-d for example - since scheduler is aware of http, we can set the prefill header at this point and return only decode selection.
Contributor
There was a problem hiding this comment. I was thinking that this will be done by the
This maintains the separation of concern and composes with the non p/d case nicely.
Contributor
Author
There was a problem hiding this comment. I followed everything except for the CycleState part, in which I probably didn't understand your intention. ProcessProfileResults(request *Request, profileResults map[string][]*ScoredEndpoint) map[string][]*Endpointlooking on the return value here - we get back a map from profile-name -> it's results (set of endpoints). let's assume we have can you explain what you meant in the CycleState part?
CycleState scope is within the scheduler, so it's not returning back to requestcontrol layer.
Contributor
There was a problem hiding this comment.
Oh, I thought we return a single final list that in the normal case a PreRequest plugin uses to set the destination endpoints.
I assumed CycleState is scoped across all extension points executed for a request. If we do that, then what I was proposing is to use CycleState as the communication channel between the P/D specific ProcessResults and PreRequest plugins to communicate the list of prefill servers to be set in the headers. Again, this was assuming that ProcessResults returned a list of endpoints, not a map. Another approach is to continue to have ProcessResult return a map, but the decode endpoints should have a key that the default PreRequest plugin understands for both the default and p/d case (in the p/d case, ProcessResult can set the key for the decode profile endpoints to whatever value expected by the default PreRequest plugin). And the addition PreRequest plugin operates on the prefill endpoints to set them as a header. I am trying to avoid having the scheduling layer implement parts of the epp <-> proxy protocol. In summary, we will have two PreRequest plugins: The first ships with IGE and implements the destination endpoint protocol, from the
Contributor
There was a problem hiding this comment. I prefer the second approach, but I still think we should scope CycleState to be across all extension points executed on a request. We can define CycleState in the common pkg.
Contributor
Author
There was a problem hiding this comment. I’m also ok with cycle state per request, although I’d prefer avoiding it. I just think we need to pay special attention that the field is not abused, cause it becomes extremely easy to communicate information in a generic cycle state rather than well defined interfaces. I would have a cycle state per layer during the request lifecycle to verify the interfaces between the layers are well defined.
Contributor
There was a problem hiding this comment.
Yes, we have a few followups as well related to converting this into a generic extensibility proposal with subsections for various layers, defining a common pkg for types and interfaces, grouping the plugin implementations under one directory.
Collaborator
There was a problem hiding this comment.
Just calling this out. But if we do this. We are stating that the EPP architecture is no longer made up of independent subsystems that could be broken out into their own lib, and instead tying them all together. I don't particularly love that. The hope was that the director/request control would be the mortar where we would handle the grey area. But not willing to die on this hill. More just calling it out.
Contributor
There was a problem hiding this comment. Discussed offline with Kellen, I think we need to strike a balance between our goal of maintaining a reusable scheduling library and a design that works well across the layers we define in EPP. My recommendation is to define common types, like CycleState and Request that all layers depend on. This doesn't prevent the scheduling library from being reusable in a different context. I don't think we want to prevent plugins from implementing extensions across subsystems if it makes sense to do so. Each plugin should declare its dependencies, and if someone wants to use the scheduling library in a different context, then they can disable the plugins that does that if they are incompatible with their system, or implement in their system the behavior expected by those other extensions (e.g, adding state to CycleState that is expected by the scheduling parts of the plugin)
Contributor
Author
There was a problem hiding this comment. I agree with Kellen (at least with his initial comment) that it would be good NOT to have one CycleState per request, as it may cause some issues of bypassing interfaces that we define between the layers.
I agree that we need to balance as @ahg-g mentioned. |
||
|
|
||
| // Filter runs before any scoring, and remove endpoints that are not fit for selection. | ||
|
|
@@ -124,7 +131,7 @@ type Scorer interface { | |
| // Using a map is much less convenient for this purpose. | ||
| type WeightedScorer struct { | ||
| Scorer | ||
| weight int //nolint:unused | ||
| } | ||
|
|
||
| // Picker selects the endpoint(s) from the provided list of scored endpoints. | ||
|
|
@@ -133,9 +140,3 @@ type Picker interface { | |
| Plugin | ||
| Pick(ctx context.Context, state *scheduling.CycleState, endpoints []*ScoredEndpoint) []*ScoredEndpoint | ||
| } | ||
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 thought we wanted to return a boolean
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.
ideally it would be good.
the point is that ProfilePick next iteration depends on the current profile execution result, so that's not always possible. for example in P/D (let's assume it's enabled):
this means we cannot decide upfront on the boolean of whether additional iteration should be called or not.
this is the original reason I haven't added bool to this part.
see code example from llm-d here.
this code is going to change to use SchedulerProfiles instead of two schedulers, so this check should be embedded in the profile selection logic.
Uh oh!
There was an error while loading. Please reload this page.
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.
Without a boolean, you always need an extra call to ProfilePick. With boolean, in some cases you will get to execute the right number of iterations, in others you will need an extra call that returns false with nil profile to execute.
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.
Are we allowing multiple ProfileSelect functions to be defined per scheduler?
Additionally, the boolean makes it much more clear to the implementer how multiple selection is handled. Otherwise a user may skim the documentation and miss the line that specifies how multiple runs should be handled
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.
IIRC, we wanted a boolean "call again" as an explicit return, whereas an empty Alice is more of an implicit convention.
Both options are equally expressive:
call again == (len(profiles)==0)
The ProfilePick always gets called a second time in the example. The difference is only between the second call returning an empty slice or a boolean to indicate a third call is not needed.
At least that's the way I understand it...
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.
They provide the same functionality, completely agreed there.
I'm just suggesting that a return type of
...) map[string]*SchedulerProfilevs...) (selectedProfiles map[string]*SchedulerProfile, rerun bool)allows an implementer to more easily intuit the functionality of the system, or ask themselves the question 'what does thererunbool do?' and then go to the documentation and learn. Vs the implicit method, it requires a user to be aware of a somewhat 'secret' functionality.Sure we will document it, but I don't think all users read documentation exhaustively, and with the added benefit of no forced extra call to ProfilePick it seems slightly better to have a bool imo.
Uh oh!
There was an error while loading. Please reload this page.
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 personally think bool return value here has an inconsistent behavior.
in some scenarios, we know that no extra call is needed.
in other scenarios like the one I described above, we will do an extra call but get back empty profiles map.
IMO it's better to define a behavior that is consistent with its definition in all cases, rather than having an inconsistent bool that may confuse more than help.
btw, I do expect whoever wants to implement a plugin to read its documentation.
the function godoc is only two lines, not an exhaustive description.
only one ProfileHandler plugin is allowed, means only one ProfileSelect is allowed.
this is still not validated in code but is documented (should be also validated in next PRs).