Skip to content

Commit ff13536

Browse files
committed
Copilot comments
Signed-off-by: roytman <roytman@il.ibm.com>
1 parent c0a9ea0 commit ff13536

3 files changed

Lines changed: 54 additions & 9 deletions

File tree

pkg/epp/framework/plugins/scheduling/profilehandler/headerphase/README.md

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,8 +13,9 @@ Reads the configured header from the incoming request and looks up the
1313
`schedulingProfiles` entry with that exact name:
1414

1515
- If a matching profile hasn't run yet, it runs that profile alone.
16-
- If the header is missing or names a profile that isn't configured, no profile runs and
17-
the request fails with an error identifying the header.
16+
- If the header is missing or names a profile that isn't configured, no profile runs; the
17+
request fails with the scheduler's generic "failed to run any scheduler profile" error,
18+
and the EPP logs the specific reason (missing header vs. unconfigured value).
1819

1920
This differs from the [disagg profile handler](../disagg/README.md), which decides which
2021
profiles to run via decider plugins rather than a header.

pkg/epp/framework/plugins/scheduling/profilehandler/headerphase/header_phase_profile_handler.go

Lines changed: 18 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,8 @@ import (
2222
"fmt"
2323
"strings"
2424

25+
"sigs.k8s.io/controller-runtime/pkg/log"
26+
2527
fwkplugin "github.com/llm-d/llm-d-router/pkg/epp/framework/interface/plugin"
2628
fwksched "github.com/llm-d/llm-d-router/pkg/epp/framework/interface/scheduling"
2729
)
@@ -103,10 +105,22 @@ func (h *HeaderPhaseProfileHandler) phaseHeader(request *fwksched.InferenceReque
103105
return strings.TrimSpace(request.Headers[h.headerName])
104106
}
105107

108+
// noMatchError explains why no configured scheduling profile matches phase, the
109+
// already-trimmed value of the phase header.
110+
func (h *HeaderPhaseProfileHandler) noMatchError(phase string) error {
111+
if phase == "" {
112+
return fmt.Errorf("header-phase profile handler: missing %q header", h.headerName)
113+
}
114+
return fmt.Errorf("header-phase profile handler: no scheduling profile configured for %q header value %q", h.headerName, phase)
115+
}
116+
106117
// Pick selects the single SchedulingProfile named by the request's phase header. It
107118
// returns an empty map once that profile has run, or when the header is missing or
108-
// names a profile that isn't configured (ProcessResults then reports the error).
109-
func (h *HeaderPhaseProfileHandler) Pick(_ context.Context, request *fwksched.InferenceRequest, profiles map[string]fwksched.SchedulerProfile,
119+
// names a profile that isn't configured. In the latter case the scheduler's run loop
120+
// (pkg/epp/scheduling.Scheduler.Schedule) stops without ever calling ProcessResults,
121+
// since no profile ever ran, so the specific reason is logged here rather than
122+
// returned from ProcessResults, where it would be unreachable in practice.
123+
func (h *HeaderPhaseProfileHandler) Pick(ctx context.Context, request *fwksched.InferenceRequest, profiles map[string]fwksched.SchedulerProfile,
110124
profileResults map[string]*fwksched.ProfileRunResult) map[string]fwksched.SchedulerProfile {
111125
// TODO(#2135): single profile per request; extend to loop over an ordered phase
112126
// list parsed from the header for non-deferred multi-profile scheduling.
@@ -117,6 +131,7 @@ func (h *HeaderPhaseProfileHandler) Pick(_ context.Context, request *fwksched.In
117131
phase := h.phaseHeader(request)
118132
profile, ok := profiles[phase]
119133
if !ok {
134+
log.FromContext(ctx).Error(h.noMatchError(phase), "no scheduling profile selected for request")
120135
return map[string]fwksched.SchedulerProfile{}
121136
}
122137

@@ -133,11 +148,7 @@ func (h *HeaderPhaseProfileHandler) ProcessResults(_ context.Context, request *f
133148
// multi-profile scheduling.
134149
switch len(profileResults) {
135150
case 0:
136-
phase := h.phaseHeader(request)
137-
if phase == "" {
138-
return nil, fmt.Errorf("header-phase profile handler: missing %q header", h.headerName)
139-
}
140-
return nil, fmt.Errorf("header-phase profile handler: no scheduling profile configured for %q header value %q", h.headerName, phase)
151+
return nil, h.noMatchError(h.phaseHeader(request))
141152
case 1:
142153
// exactly one profile ran, handled below
143154
default:

pkg/epp/framework/plugins/scheduling/profilehandler/headerphase/header_phase_profile_handler_test.go

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -106,6 +106,39 @@ func TestHeaderPhaseProfileHandlerFactory(t *testing.T) {
106106
}
107107
}
108108

109+
func TestHeaderPhaseNoMatchError(t *testing.T) {
110+
handler := NewHeaderPhaseProfileHandler(defaultHeaderName)
111+
112+
tests := []struct {
113+
name string
114+
phase string
115+
wantErrContain string
116+
}{
117+
{
118+
name: "empty phase reports missing header",
119+
phase: "",
120+
wantErrContain: `missing "epp-phase" header`,
121+
},
122+
{
123+
name: "non-empty phase reports the unconfigured value",
124+
phase: "prefill",
125+
wantErrContain: `no scheduling profile configured for "epp-phase" header value "prefill"`,
126+
},
127+
}
128+
129+
for _, tt := range tests {
130+
t.Run(tt.name, func(t *testing.T) {
131+
err := handler.noMatchError(tt.phase)
132+
if err == nil {
133+
t.Fatalf("noMatchError() returned nil, want an error")
134+
}
135+
if !strings.Contains(err.Error(), tt.wantErrContain) {
136+
t.Errorf("noMatchError() = %q, want it to contain %q", err.Error(), tt.wantErrContain)
137+
}
138+
})
139+
}
140+
}
141+
109142
func TestHeaderPhaseWithName(t *testing.T) {
110143
handler := NewHeaderPhaseProfileHandler(defaultHeaderName).WithName("renamed")
111144

0 commit comments

Comments
 (0)