feat: Add factory functions for all plugins #208

shmuelk · 2025-06-29T11:16:47Z

This PR:

Adds factory functions for all plugins in the repository
Registers the above plugin factory functions
Some minor changes to some plugin contructors to eliminate use of environment variables in constructor

This PR completes steps three and four of issue #201

nirrozenbaum · 2025-06-29T11:42:48Z

verifying - was this tested e2e?

nirrozenbaum · 2025-06-29T11:47:03Z

pkg/plugins/scorer/load_aware_scorer.go

 )

+type loadAwareScorerParameters struct {
+	Threshold float64 `json:"threshold"`


should be int

The threshold here is a float64. It was read from an environment variable and converted to a float64

sorry. saw in the old code GetEnvInt and confused. but in old code in was also converted into float64.
does it make sense that queue length is float and not int? shouldn't we treat queue length as int?

You're probably right. It may be stored as a float64 just to avoid constantly casting the value in calculations.

I made the field an int.

nirrozenbaum · 2025-06-29T11:49:43Z

pkg/plugins/scorer/kvcache_aware.go

+	RedisAddres string `json:"redisAddress"`
+	HfToken     string `json:"hfToken"`


don't you think this is problematic?
putting personal access token or connection string that includes password, in a configuration file?

I revered the code here. These two fields are again fetched from environment variables

thanks.
I recommend also adding a comment about sensitive information in GIE Config API documentation. e.g., something like:

it's not recommended to store sensitive information in the plugin configuration and this kind of information should be stored differently (e.g., using env vars).

*not related to this PR.

nirrozenbaum · 2025-06-29T11:50:53Z

pkg/plugins/register.go

+
+// RegisterAllPlugins registers the factory functions of all plugins in this repository.
+func RegisterAllPlugins() {
+	plugins.Register(filter.ByLabelsFilterType, filter.ByLabelFactory)


why do we register an example filter only for documentation purpose?

I registered all of the plugins

nirrozenbaum · 2025-06-29T11:54:52Z

pkg/plugins/profile/pd-profile-handler.go

+	rawPlugin := handle.Plugins().Plugin(parameters.PrefixScorerRef)
+	if rawPlugin == nil {
+		return nil, fmt.Errorf("there was no plugin with the name '%s' defined", parameters.PrefixScorerRef)
+	}


this is why I suggested returning in GIE (rawPlugin, bool). this check shouldn't be in the responsibility of a config api consumer, this error should return from GIE.
not a big deal and certainly not a blocker, but I think this should be changed in GIE. otherwise, every plugin that may have ref to another plugin would need to write the error (and potentially different developers might write the error differently, making it harder to debug due to inconsistencies in the log).

nirrozenbaum · 2025-06-29T11:57:06Z

pkg/plugins/profile/pd-profile-handler.go

+	rawPlugin := handle.Plugins().Plugin(parameters.PrefixScorerRef)
+	if rawPlugin == nil {
+		return nil, fmt.Errorf("there was no plugin with the name '%s' defined", parameters.PrefixScorerRef)
+	}
+	prefixScorer, ok := rawPlugin.(*scorer.PrefixAwareScorer)
+	if !ok {
+		return nil, fmt.Errorf("the plugin with the name '%s' was not an instance of the PrefixAwareScorer", parameters.PrefixScorerRef)
+	}


all this logic can be implemented as generic function inside GIE, reducing the necessity to do the same code every time a plugin is referencing a different plugin.

see an example here:
https://github.com/kubernetes-sigs/gateway-api-inference-extension/blob/main/pkg/epp/scheduling/types/cycle_state.go#L97-L111

See GIE PR kubernetes-sigs/gateway-api-inference-extension#1091

Updated code to use GIE helper

shmuelk · 2025-06-29T12:32:50Z

verifying - was this tested e2e?

Not yet. This PR only adds the factory functions and registers them.

The idea was to break things up into smaller chunks to make it easier to review.

Signed-off-by: Shmuel Kallner <[email protected]>

nirrozenbaum · 2025-07-01T11:27:20Z

pkg/plugins/filter/by_labels.go

-		return nil, errors.New("ByLabels: missing filter name")
+		return nil, errors.New("ByLabelSelector: missing filter name")


why is it mandatory to specify name?
it's optional in all other plugins..

cc @mayabar

nirrozenbaum · 2025-07-01T11:35:16Z

pkg/plugins/profile/pd_profile_handler.go

+	parameters := pdProfileHandlerParameters{
+		Config: prefix.Config{
+			HashBlockSize:          prefix.DefaultHashBlockSize,
+			MaxPrefixBlocksToMatch: prefix.DefaultMaxPrefixBlocks,
+			LRUCapacityPerServer:   prefix.DefaultLRUCapacityPerServer,
+		},


does it make sense that prefix config appear on profile handler plugin?
shouldn't it appear on prefix plugin only?
something with this configuration looks not natural..

This configuration is from the previous change to eliminate our own Prefix Scorer

nirrozenbaum · 2025-07-01T11:38:11Z

pkg/plugins/profile/pd_profile_handler.go

+	}
+	return NewPdProfileHandler(cfg).WithName(name), nil
+}
+


comment on line 114 (I can't put the comment there cause it wasn't change in this PR):

prefixState, err := types.ReadCycleStateKey[*prefix.SchedulingContextState](cycleState, prefix.PrefixCachePluginType) if err != nil { log.FromContext(ctx).Error(err, "unable to read prefix state") return map[string]*framework.SchedulerProfile{} }

if this the expected behavior? if the prefix scorer failed to write the prefix we don't do PD?
cc @kfirtoledo

This code is from the previous change to eliminate our own Prefix Scorer

nirrozenbaum · 2025-07-01T11:39:27Z

pkg/plugins/profile/pd_profile_handler.go

+			MaxPrefixBlocksToMatch: prefix.DefaultMaxPrefixBlocks,
+			LRUCapacityPerServer:   prefix.DefaultLRUCapacityPerServer,


is profile handler using these values?

nirrozenbaum

/lgtm
/approve

PR looks great, thanks!

I left two non-blocking minor comments/questions, one for @mayabar and one for @kfirtoledo that could be addressed (if needed?) in follow ups.

Signed-off-by: konflux-internal-p02 <170854209+konflux-internal-p02[bot]@users.noreply.github.com> Co-authored-by: konflux-internal-p02[bot] <170854209+konflux-internal-p02[bot]@users.noreply.github.com>

shmuelk requested review from elevran and mayabar June 29, 2025 11:16

nirrozenbaum reviewed Jun 29, 2025

View reviewed changes

shmuelk force-pushed the register-plugins branch from 0caf38d to 640b3f1 Compare June 30, 2025 15:00

shmuelk added 4 commits July 1, 2025 12:52

Upgrade to the latest GIE

7a86fd7

Signed-off-by: Shmuel Kallner <[email protected]>

Added factory functions

ea5af3a

Signed-off-by: Shmuel Kallner <[email protected]>

Updates due to API changes

b8f0ce5

Signed-off-by: Shmuel Kallner <[email protected]>

Register factory functions

eb8552e

Signed-off-by: Shmuel Kallner <[email protected]>

shmuelk force-pushed the register-plugins branch from af178e3 to eb8552e Compare July 1, 2025 09:58

shmuelk requested a review from kfirtoledo July 1, 2025 09:59

nirrozenbaum reviewed Jul 1, 2025

View reviewed changes

mayabar approved these changes Jul 1, 2025

View reviewed changes

nirrozenbaum reviewed Jul 1, 2025

View reviewed changes

nirrozenbaum approved these changes Jul 1, 2025

View reviewed changes

shmuelk merged commit 9063d2e into llm-d:main Jul 1, 2025
2 checks passed

shmuelk deleted the register-plugins branch July 1, 2025 11:54

nirrozenbaum mentioned this pull request Jul 12, 2025

Configure the llm-d-inference-scheduler using the new GIE configuration support #201

Closed

		RedisAddres string `json:"redisAddress"`
		HfToken string `json:"hfToken"`

		return nil, errors.New("ByLabels: missing filter name")
		return nil, errors.New("ByLabelSelector: missing filter name")

		MaxPrefixBlocksToMatch: prefix.DefaultMaxPrefixBlocks,
		LRUCapacityPerServer: prefix.DefaultLRUCapacityPerServer,

feat: Add factory functions for all plugins #208

feat: Add factory functions for all plugins #208

Uh oh!

Conversation

shmuelk commented Jun 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nirrozenbaum commented Jun 29, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shmuelk commented Jun 29, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nirrozenbaum left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

shmuelk commented Jun 29, 2025 •

edited

Loading