-
Notifications
You must be signed in to change notification settings - Fork 292
epp: add turn-priority strategy to program-aware fairness #2116
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
Open
ruocco
wants to merge
1
commit into
llm-d:main
Choose a base branch
from
ruocco:fairness-turn-based-priority
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
94 changes: 94 additions & 0 deletions
94
pkg/epp/framework/plugins/flowcontrol/fairness/program-aware/strategy_turnpriority.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,94 @@ | ||
| package programaware | ||
|
|
||
| import ( | ||
| "math" | ||
| "time" | ||
|
|
||
| "github.com/prometheus/client_golang/prometheus" | ||
|
|
||
| "github.com/llm-d/llm-d-router/pkg/epp/framework/interface/flowcontrol" | ||
| fwkrc "github.com/llm-d/llm-d-router/pkg/epp/framework/interface/requestcontrol" | ||
| fwksched "github.com/llm-d/llm-d-router/pkg/epp/framework/interface/scheduling" | ||
| ) | ||
|
|
||
| const turnPriorityStrategyName = "turn-priority" | ||
|
|
||
| var _ Strategy = &turnPriorityStrategy{} | ||
|
|
||
| // turnPriorityStrategy scores each flow by its head request's priority | ||
| // | ||
| // prio = turnNumber + timeWeight * elapsedSeconds | ||
| // | ||
| // and picks the highest. The elapsed term ages waiting flows up so a flow with | ||
| // a lower turn number is not starved indefinitely. The strategy keeps no | ||
| // accumulated per-program state. | ||
| type turnPriorityStrategy struct { | ||
| timeWeight float64 | ||
| } | ||
|
|
||
| func (s *turnPriorityStrategy) Name() string { return turnPriorityStrategyName } | ||
|
|
||
| func (s *turnPriorityStrategy) Pick(_ int, queues map[string]QueueInfo) flowcontrol.FlowQueueAccessor { | ||
| // Collect the flows with pending work. Scoring only orders flows against | ||
| // each other, so it is meaningful only under contention: with no waiting | ||
| // flow there is nothing to dispatch, and with exactly one the choice is | ||
| // forced. Score only when two or more flows are waiting. | ||
| waiting := make([]QueueInfo, 0, len(queues)) | ||
| for _, qi := range queues { | ||
| if qi.Len == 0 || qi.Queue.Peek() == nil { | ||
| continue | ||
| } | ||
| waiting = append(waiting, qi) | ||
| } | ||
|
|
||
| switch len(waiting) { | ||
| case 0: | ||
| return nil | ||
| case 1: | ||
| return waiting[0].Queue | ||
| } | ||
|
|
||
| var best flowcontrol.FlowQueueAccessor | ||
| bestScore := math.Inf(-1) | ||
| now := time.Now() | ||
|
|
||
| for _, qi := range waiting { | ||
| head := qi.Queue.Peek() | ||
| elapsed := now.Sub(head.EnqueueTime()).Seconds() | ||
| if elapsed < 0 { | ||
| elapsed = 0 | ||
| } | ||
| score := float64(turnNumberFor(qi.Metrics)) + s.timeWeight*elapsed | ||
| if score > bestScore { | ||
| bestScore = score | ||
| best = qi.Queue | ||
| } | ||
| } | ||
|
|
||
| return best | ||
| } | ||
|
|
||
| func (s *turnPriorityStrategy) OnPreRequest(_ *ProgramMetrics, _ *fwksched.InferenceRequest) {} | ||
|
|
||
| func (s *turnPriorityStrategy) OnCompleted(_ *ProgramMetrics, _ *fwksched.InferenceRequest, _ *fwkrc.Response) { | ||
| } | ||
|
|
||
| func (s *turnPriorityStrategy) EvictProgram(_ string) {} | ||
|
|
||
| func (s *turnPriorityStrategy) Collectors() []prometheus.Collector { return nil } | ||
|
|
||
| // turnNumberFor returns the turn number of a program's head request: the count | ||
| // of requests already dispatched for that program plus the waiting request | ||
| // itself (dispatchedCount+1). | ||
| // | ||
| // The counter is program-scoped: each program (fairness ID) has its own count, | ||
| // and every dispatch advances it by one. The count resets to zero when idle | ||
| // program state is evicted: a program that goes idle past the eviction TTL has | ||
| // likely lost its KV cache, so on return it restarts from turn one and re-earns | ||
| // priority from a cold state. | ||
| func turnNumberFor(metrics *ProgramMetrics) int64 { | ||
| if metrics == nil { | ||
| return 1 | ||
| } | ||
| return metrics.DispatchedCount() + 1 | ||
| } |
95 changes: 95 additions & 0 deletions
95
pkg/epp/framework/plugins/flowcontrol/fairness/program-aware/strategy_turnpriority_test.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,95 @@ | ||
| package programaware | ||
|
|
||
| import ( | ||
| "testing" | ||
| "time" | ||
|
|
||
| "github.com/stretchr/testify/assert" | ||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| func TestTurnPriority_Name(t *testing.T) { | ||
| assert.Equal(t, "turn-priority", (&turnPriorityStrategy{}).Name()) | ||
| } | ||
|
|
||
| func TestTurnPriority_Pick_PrefersLongerWait(t *testing.T) { | ||
| s := &turnPriorityStrategy{timeWeight: 0.5} | ||
| now := time.Now() | ||
|
|
||
| // Both programs have zero dispatches (equal turn), so the longer-waiting | ||
| // head wins on the elapsed term. | ||
| idA, qA := makeInfo("alpha", now.Add(-5*time.Second)) | ||
| idB, qB := makeInfo("beta", now.Add(-1*time.Second)) | ||
| queues := map[string]QueueInfo{idA: qA, idB: qB} | ||
|
|
||
| got := s.Pick(0, queues) | ||
| require.NotNil(t, got) | ||
| assert.Equal(t, "alpha", got.FlowKey().ID) | ||
| } | ||
|
|
||
| func TestTurnPriority_Pick_PrefersHigherTurn(t *testing.T) { | ||
| s := &turnPriorityStrategy{timeWeight: 0.5} | ||
| now := time.Now() | ||
|
|
||
| // Equal wait, but beta has more dispatched requests, so its turn number is | ||
| // higher and it wins. | ||
| idA, qA := makeInfo("alpha", now.Add(-1*time.Second)) | ||
| idB, qB := makeInfo("beta", now.Add(-1*time.Second)) | ||
| for i := 0; i < 10; i++ { | ||
| qB.Metrics.RecordDispatched(time.Time{}) | ||
| } | ||
| queues := map[string]QueueInfo{idA: qA, idB: qB} | ||
|
|
||
| got := s.Pick(0, queues) | ||
| require.NotNil(t, got) | ||
| assert.Equal(t, "beta", got.FlowKey().ID) | ||
| } | ||
|
|
||
| func TestTurnPriority_TurnNumberFromDispatchedCount(t *testing.T) { | ||
| // Waiting request is the next turn: dispatchedCount + 1. | ||
| assert.Equal(t, int64(1), turnNumberFor(&ProgramMetrics{})) | ||
| assert.Equal(t, int64(1), turnNumberFor(nil)) | ||
|
|
||
| m := &ProgramMetrics{} | ||
| m.RecordDispatched(time.Time{}) | ||
| m.RecordDispatched(time.Time{}) | ||
| assert.Equal(t, int64(3), turnNumberFor(m)) | ||
| } | ||
|
|
||
| func TestTurnPriority_Pick_SkipsEmptyQueues(t *testing.T) { | ||
| s := &turnPriorityStrategy{timeWeight: 0.5} | ||
| queues := map[string]QueueInfo{ | ||
| "empty": {Queue: makeQueue("empty", 0, time.Time{}), Metrics: &ProgramMetrics{}, Len: 0}, | ||
| } | ||
| assert.Nil(t, s.Pick(0, queues)) | ||
| } | ||
|
|
||
| func TestTurnPriority_Pick_SingleWaitingFlowDispatchesDirectly(t *testing.T) { | ||
| s := &turnPriorityStrategy{timeWeight: 0.5} | ||
|
|
||
| // A just-enqueued head scores ~0; with only one waiting flow it must still | ||
| // be picked, proving the single-flow path bypasses scoring. | ||
| id, qi := makeInfo("only", time.Now()) | ||
| queues := map[string]QueueInfo{ | ||
| id: qi, | ||
| "empty": {Queue: makeQueue("empty", 0, time.Time{}), Metrics: &ProgramMetrics{}, Len: 0}, | ||
| } | ||
|
|
||
| got := s.Pick(0, queues) | ||
| require.NotNil(t, got) | ||
| assert.Equal(t, "only", got.FlowKey().ID) | ||
| } | ||
|
|
||
| func TestTurnPriority_Pick_ZeroWeightIgnoresElapsed(t *testing.T) { | ||
| s := &turnPriorityStrategy{timeWeight: 0} | ||
| now := time.Now() | ||
|
|
||
| // With timeWeight 0 and equal turn numbers, all scores tie; a flow is still | ||
| // selected rather than nil. | ||
| idA, qA := makeInfo("alpha", now.Add(-5*time.Second)) | ||
| idB, qB := makeInfo("beta", now.Add(-1*time.Second)) | ||
| queues := map[string]QueueInfo{idA: qA, idB: qB} | ||
|
|
||
| got := s.Pick(0, queues) | ||
| require.NotNil(t, got) | ||
| } |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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'm fairly sure that individual instances of an agentic session will NOT get their own fairnessID. There is a queue in flowcontrol per fairnessID (per priorityBand). In the use case you are describibg there would be a single element in each queue, being very wastefull.
The original fairness policy was, I think, looking at each agentic workload as a whole, that is all of the requests for the same workload, vs requests for a different workload.
This PR if I understand is trying to look at the individual sessions and prioritize them. This sounds more like an in queue prioritization policy, rather than a fairness policy.
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.
Thanks @shmuelk .
You are correct, this PR is about scheduling sessions, and prioritizing some (the more advanced) in place of others.
Is your concern the fact that each session has its own queue, with (most of the times) only one element inside being the active request, and you are suggesting that it would be more efficient to have a single queue, with sessions as elements?
What would you recommend? Moving away from Fairness and into a more generic OrderingPolicy?
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 main point I was trying to raise is that the fairnessID is NOT a sessionID. You need to track by some sort of sessionID (there are other PRs that have dealt with sessions and sessionIDs in the past).
You are tracking by program or agenticc workload level and not what you wanted.
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.
@shmuelk I guess you are referring to this PR #1754
This assigns fairnessID based on the agent's session ID parsed from the header of a request.
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 would claim that an agentID as described in the PR is not a sessionID. It identifies the agent, not the session with the agent.
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.
@shmuelk , maybe I hooked into the wrong handle, but at the moment the fairnessID is tied to each session, and this commit achieves the intended behaviour, at least for the OTEL traces replay via inference-perf.
If there's a better way on getting a session hook, or a better place in llm-d where to implement this, I'm happy to refactor.