Skip to content

Add a set of configuration defaults #1223

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Jul 25, 2025
Merged

Add a set of configuration defaults #1223

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
41d77dc
If a picker wasn't specified, add one
shmuelk Jul 23, 2025
49e3586
If there is only one profile and a profile handler wasn't specified, …
shmuelk Jul 23, 2025
09ab296
If there aren't any ScehdulingProfiles, add one
shmuelk Jul 23, 2025
c9f2e7a
Updated and fixed tests
shmuelk Jul 23, 2025
3501801
Updated the configuration guide
shmuelk Jul 23, 2025
3483f05
Don't hard code the default scorer weight
shmuelk Jul 23, 2025
94b39bf
If a picker is missing, always add a MaxScorePicker
shmuelk Jul 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions apix/config/v1alpha1/defaults.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,27 @@ package v1alpha1
//
// This naming convension is required by the defalter-gen code.
func SetDefaults_EndpointPickerConfig(cfg *EndpointPickerConfig) {
// If no name was given for the plugin, use it's type as the name
for idx, pluginConfig := range cfg.Plugins {
if pluginConfig.Name == "" {
cfg.Plugins[idx].Name = pluginConfig.Type
}
}

// If No SchedulerProfiles were specified in the confguration,
// create one named default with references to all of the
// plugins mentioned in the Plugins section of the configuration.
if len(cfg.SchedulingProfiles) == 0 {
cfg.SchedulingProfiles = make([]SchedulingProfile, 1)

thePlugins := []SchedulingPlugin{}
for _, pluginConfig := range cfg.Plugins {
thePlugins = append(thePlugins, SchedulingPlugin{PluginRef: pluginConfig.Name})
Copy link
Contributor

@ahg-g ahg-g Jul 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we only add the plugins that are applicable allowed in a scheduling profile? scorer, filter, picker?

Copy link
Contributor Author

@shmuelk shmuelk Jul 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At this point in time in the code, one doesn't know what the plugins are as they have not yet been instantiated.

If you want, I can move this code to the LoadSchedulingConfig function, with appropriate changes to the configuration validation code.

Copy link
Contributor

@ahg-g ahg-g Jul 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recommend consolidating validation in this pkg, this is the practice, validation is invoked after defaulting automatically. We can do that in a followup PR.

Copy link
Contributor Author

@shmuelk shmuelk Jul 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also the code in ScheculingProfile.AddPlugins, ignores any plugins that are of types other than filter, scorer, picker, or postCycle

Copy link
Contributor

@ahg-g ahg-g Jul 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, but when we default and log the config, they will show up, silently ignoring them is not ideal.

}

cfg.SchedulingProfiles[0] = SchedulingProfile{
Name: "default",
Plugins: thePlugins,
}
}
}
38 changes: 30 additions & 8 deletions pkg/epp/common/config/loader/configloader.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ import (
"sigs.k8s.io/gateway-api-inference-extension/pkg/epp/plugins"
"sigs.k8s.io/gateway-api-inference-extension/pkg/epp/scheduling"
"sigs.k8s.io/gateway-api-inference-extension/pkg/epp/scheduling/framework"
"sigs.k8s.io/gateway-api-inference-extension/pkg/epp/scheduling/framework/plugins/picker"
"sigs.k8s.io/gateway-api-inference-extension/pkg/epp/scheduling/framework/plugins/profile"
)

var scheme = runtime.NewScheme()
Expand Down Expand Up @@ -64,18 +66,39 @@ func LoadSchedulerConfig(configProfiles []configapi.SchedulingProfile, handle pl
profiles := map[string]*framework.SchedulerProfile{}
for _, namedProfile := range configProfiles {
profile := framework.NewSchedulerProfile()
pickerAdded := false
scorerAdded := false
for _, plugin := range namedProfile.Plugins {
referencedPlugin := handle.Plugin(plugin.PluginRef)
if scorer, ok := referencedPlugin.(framework.Scorer); ok {
if plugin.Weight == nil {
return nil, fmt.Errorf("scorer '%s' is missing a weight", plugin.PluginRef)
// Set default weight to 1
Copy link
Contributor

@ahg-g ahg-g Jul 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I strongly recommend consolidating all defaulting in the defaults.go for two reasons: 1) it is harder to reason about the default behavior and debug it when it is scattered 2) logging the actual configuration to be applied become easier and the log at https://github.com/kubernetes-sigs/gateway-api-inference-extension/blob/main/cmd/epp/runner/runner.go#L407 would show all the defaults we apply

Copy link
Contributor Author

@shmuelk shmuelk Jul 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have changed the number one to a constant. At the point in time in the code where there is logging, the addition of the profile handler and picker are not yet done, In addition the setting of the default weight for scorers also happens latter on.

I can change the code to modify the structs of the read in configuration and move the logging a bit, if you want.

Copy link
Contributor

@ahg-g ahg-g Jul 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not about the weight only, I am referring to the whole logic we added here, including adding a default picker and default handler. I think all that should be done in the defaulting file.

Copy link
Contributor Author

@shmuelk shmuelk Jul 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As there are two defaults.go files in the repo, I want to make sure we are talking about the same file. I assume you are talking about defaults.go in the package apix/config/v1alpha1.

If I am correct, then:

  1. This code is automatically invoked by the K8S machinery that loads the YAML
  2. In order to do the Scorer weight defaulting one needs to know that the plugin is a scorer, similarly to know that a profile handler or a picker were not specified, one needs to instantiate the plugins to check their types.
  3. Similarly, when creating a default SchedulerProfile, with only Scheduler plugins, one needs to instantiate them to know their types.
  4. This would require duplicating the code that instantiates plugins. The real place for such code is not inside the package apix/config/v1alpha1. If it lives elsewhere, as it should, using it in the package apix/config/v1alpha1, will likely cause an import loop

Copy link
Contributor

@ahg-g ahg-g Jul 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I was referring to the defaulting code there.

ok, I guess we need to have the registry expose interfaces to do the needed checks and somehow plumb the registry in, this is a much bigger change that we need t consider separately.

I am ok with this PR, but can you please remove the random picker and converge on the max score one as suggested in another comment?

Copy link
Contributor Author

@shmuelk shmuelk Jul 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. I updated the code, tests, doc, and description of this PR

weight := 1
if plugin.Weight != nil {
weight = *plugin.Weight
}
referencedPlugin = framework.NewWeightedScorer(scorer, *plugin.Weight)
referencedPlugin = framework.NewWeightedScorer(scorer, weight)
scorerAdded = true
}
if _, ok := referencedPlugin.(framework.Picker); ok {
pickerAdded = true
}
if err := profile.AddPlugins(referencedPlugin); err != nil {
return nil, fmt.Errorf("failed to load scheduler config - %w", err)
}
}
if !pickerAdded {
// There isn't a picker in this profile, add one
var thePicker framework.Picker
if scorerAdded {
thePicker = picker.NewMaxScorePicker(picker.DefaultMaxNumOfEndpoints)
} else {
thePicker = picker.NewRandomPicker(picker.DefaultMaxNumOfEndpoints)
}

if err := profile.AddPlugins(thePicker); err != nil {
return nil, fmt.Errorf("failed to load scheduler config - %w", err)
}
}
profiles[namedProfile.Name] = profile
}

Expand All @@ -89,7 +112,10 @@ func LoadSchedulerConfig(configProfiles []configapi.SchedulingProfile, handle pl
}
}
if profileHandler == nil {
return nil, errors.New("no profile handler was specified")
if len(profiles) != 1 {
return nil, errors.New("no profile handler was specified")
}
profileHandler = profile.NewSingleProfileHandler()
}

return scheduling.NewSchedulerConfig(profileHandler, profiles), nil
Expand Down Expand Up @@ -125,10 +151,6 @@ func instantiatePlugins(configuredPlugins []configapi.PluginSpec, handle plugins
}

func validateSchedulingProfiles(config *configapi.EndpointPickerConfig) error {
if len(config.SchedulingProfiles) == 0 {
return errors.New("there must be at least one scheduling profile in the configuration")
}

profileNames := sets.New[string]()
for _, profile := range config.SchedulingProfiles {
if profile.Name == "" {
Expand Down
Loading