update algorithm parameters from env variables #580

kaushikmitr · 2025-03-26T20:53:01Z

This pull request introduces a significant refactor to make scheduler configuration values dynamically configurable through environment variables. The changes include creating a new Config struct to hold configuration values, updating various functions to use these new configuration values, and adding utility functions to load configuration from environment variables.

Key changes include:

Configuration Management:

Added Config struct to hold configuration values (KVCacheThreshold, QueueThresholdCritical, QueueingThresholdLoRA, LoraAffinityThreshold) and initialized it using environment variables (pkg/epp/scheduling/scheduler.go).
Implemented utility functions getEnvFloat and getEnvInt to retrieve configuration values from environment variables with default fallbacks (pkg/epp/scheduling/scheduler.go).

Function Updates:

Updated leastQueuingFilterFunc and loRASoftAffinityFilter to use the new configuration values (pkg/epp/scheduling/filter.go). [1] [2]

Testing Enhancements:

Modified TestLoRASoftAffinityDistribution to set and restore configuration values for testing purposes (pkg/epp/scheduling/filter_test.go). [1] [2] [3]

Scheduler Enhancements:

Added logging of current configuration values in the Schedule method for better debugging (pkg/epp/scheduling/scheduler.go).
Added UpdateLoraAffinityThreshold function to allow dynamic updates of the LoRA affinity threshold (pkg/epp/scheduling/scheduler.go).[Copilot is generating a summary...]

tested with:
- name: KV_CACHE_THRESHOLD
value: "0.9"
- name: QUEUE_THRESHOLD_CRITICAL
value: "10"
- name: QUEUING_THRESHOLD_LORA
value: "23"
- name: LORA_AFFINITY_THRESHOLD
value: "0.7"
logs printed:
2025-03-26T20:44:36Z LEVEL(-3) scheduling/scheduler.go:206 Scheduler configuration {"request": {"Model":"meta-llama/Llama-2-7b-hf","TargetModels":null,"ResolvedTargetModel":"meta-llama/Llama-2-7b-hf","Critical":true}, "KVCacheThreshold": 0.9, "QueueThresholdCritical": 10, "QueueingThresholdLoRA": 23, "LoraAffinityThreshold": 0.7}
2025-03-26T20:44:36Z LEVEL(-3) scheduling/scheduler.go:214 Scheduling a request.

k8s-ci-robot · 2025-03-26T20:53:10Z

Hi @kaushikmitr. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

netlify · 2025-03-26T20:53:16Z

✅ Deploy Preview for gateway-api-inference-extension ready!

Name	Link
🔨 Latest commit	`bae8e28`
🔍 Latest deploy log	https://app.netlify.com/sites/gateway-api-inference-extension/deploys/67e600f3a77d070009784ca7
😎 Deploy Preview	https://deploy-preview-580--gateway-api-inference-extension.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

liu-cong · 2025-03-26T22:10:13Z

pkg/epp/scheduling/scheduler.go

 )

+// getEnvFloat gets a float64 from an environment variable with a default value
+func getEnvFloat(key string, defaultVal float64, logger logr.Logger) float64 {


Can we move these env var parsing helper functions to /util/env/env.go

I agree.

It would likely blow up the size of this PR tho. Global vars are run before the main func. And our filter chain is a global var, which consumes these variables.

Or do you just mean the parsing funcs themselves.

it makes sense to have a separate env pkg within utils which has the env parsers. updated accordingly

kfswain

Left a few comments, I would generally suggest restructuring this.

However, with kubecon so close, and we need this for v0.3.0, and if you are short on time... It may be acceptable to mark TODOs all over this, and add issues the refactor this

kfswain · 2025-03-26T22:37:02Z

pkg/epp/scheduling/scheduler.go

 )

+// getEnvFloat gets a float64 from an environment variable with a default value
+func getEnvFloat(key string, defaultVal float64, logger logr.Logger) float64 {


Or do you just mean the parsing funcs themselves.

kfswain · 2025-03-26T22:40:03Z

pkg/epp/scheduling/scheduler.go

+}
+
+// LoadConfig loads configuration from environment variables
+func LoadConfig() Config {


Typically env variables are processed in the main func. It would be considered surprising to process an env var in a package like this and load it into a global var.

The unification of these config variables allows a reader to be able to reliably find the config, rather than having to hunt for its consumption.

i agree with this, will tackle it in a separate issue as suggested above

We can add a global struct to hold all env vars for discoverability. I am OK with a follow up.

kfswain · 2025-03-26T22:40:46Z

pkg/epp/scheduling/scheduler.go

@@ -108,6 +177,15 @@ var (
 	}
 )

+// UpdateLoraAffinityThreshold updates the LoRA affinity threshold value
+// This is useful for testing or dynamic reconfiguration
+func UpdateLoraAffinityThreshold(newValue float64, logger logr.Logger) {


This looks like it's only being consumed in tests, could we make this a test util func for now?

yes, moved it to test

kfswain · 2025-03-26T22:41:22Z

pkg/epp/scheduling/scheduler.go


+	// Log current configuration values for debugging purposes.
+	logger.V(logutil.VERBOSE).Info("Scheduler configuration",


TRACE is probably preferred since this would run on every request, and its unlikely to change frequently.

yes agreed.

liu-cong · 2025-03-27T04:08:26Z

pkg/epp/util/env/env.go

Pls add unit tests

added tests

liu-cong · 2025-03-27T04:09:28Z

pkg/epp/scheduling/scheduler.go

+	LoraAffinityThreshold  float64
+}
+
+var (


nit: these can be const

liu-cong · 2025-03-27T04:11:28Z

pkg/epp/scheduling/scheduler.go


+	// Log current configuration values for debugging purposes.


Since the config is only loaded once, why do we log it in every request? it's already logged in LoadConfig.

its logutil.TRACE, so should be logged in unless tracing is on

Still, I would expect config parameters to be logged once on startup, you can move this log line to NewScheduler for now, and later we should move it to main

There is no need for this since this is already logged on startup. Also logging such environment configurable should be DEFAULT level.

liu-cong · 2025-03-27T04:14:28Z

pkg/epp/scheduling/scheduler.go

+		LoraAffinityThreshold:  envutil.GetEnvFloat("LORA_AFFINITY_THRESHOLD", defaultLoraAffinityThreshold, baseLogger),
+	}
+
+	baseLogger.V(logutil.DEFAULT).Info("Scheduler configuration loaded",


nit: You can simply log the config object itself, logger.Info("Scheduler configuration loaded", "config", config)

pkg/epp/scheduling/filter_test.go

liu-cong · 2025-03-27T04:17:59Z

pkg/epp/scheduling/scheduler.go

+}
+
+// LoadConfig loads configuration from environment variables
+func LoadConfig() Config {


We can add a global struct to hold all env vars for discoverability. I am OK with a follow up.

liu-cong

Agree with @kfswain that there are followups on this, but given the short kubecon timeline I am fine.

The change looks good to me except for a couple nits, and adding unit tests for the env parsing. Will let others approve.

liu-cong · 2025-03-27T15:47:20Z

pkg/epp/scheduling/filter_test.go

+	// Set a specific test value for this test
+	testThreshold := 0.75 // 75%
+	// Inline update of threshold value
+	func(newValue float64, logger logr.Logger) {


Sorry perhaps I wasn't super clear by inline the function. Let's just do config.LoraAffinityThreshold = newValue. I am fine without the logging since this is just in tests. If you want logging, simply add t.Logf().

pkg/epp/scheduling/filter_test.go

liu-cong · 2025-03-27T15:49:38Z

pkg/epp/scheduling/scheduler.go


+	// Log current configuration values for debugging purposes.


There is no need for this since this is already logged on startup. Also logging such environment configurable should be DEFAULT level.

ahg-g · 2025-03-27T15:55:47Z

/ok-to-test

kaushikmitr · 2025-03-27T23:48:00Z

added unit test for env parser

ahg-g · 2025-03-28T01:22:38Z

The PR has some lint errors, run make verify to see them locally or click the failed test link: https://prow.k8s.io/view/gs/kubernetes-ci-logs/pr-logs/pull/kubernetes-sigs_gateway-api-inference-extension/580/pull-gateway-api-inference-extension-verify-main/1905364589357305856

ahg-g · 2025-03-28T02:12:08Z

/lgtm
/approve

k8s-ci-robot · 2025-03-28T02:12:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahg-g, kaushikmitr

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [ahg-g]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

update algorithm parameters form env variables

a21567b

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 26, 2025

k8s-ci-robot requested review from Jeffwan and liu-cong March 26, 2025 20:53

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Mar 26, 2025

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 26, 2025

kaushikmitr changed the title ~~update algorithm parameters form env variables~~ [WIP] update algorithm parameters form env variables Mar 26, 2025

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 26, 2025

liu-cong reviewed Mar 26, 2025

View reviewed changes

kfswain reviewed Mar 26, 2025

View reviewed changes

move env parsers to a new pkg in utils

c6e2d56

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 27, 2025

kaushikmitr changed the title ~~[WIP] update algorithm parameters form env variables~~ update algorithm parameters form env variables Mar 27, 2025

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 27, 2025

Merge branch 'main' into filtering

fbded1c

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 27, 2025

liu-cong reviewed Mar 27, 2025

View reviewed changes

add unit test for env parser

6dec8b4

liu-cong reviewed Mar 27, 2025

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 27, 2025

remove logging env variables during scheduling

1ccb26d

kaushikmitr changed the title ~~update algorithm parameters form env variables~~ update algorithm parameters from env variables Mar 27, 2025

kaushikmitr force-pushed the filtering branch from ee3989c to 571f606 Compare March 28, 2025 01:35

add test for env parser

bae8e28

kaushikmitr force-pushed the filtering branch from 571f606 to bae8e28 Compare March 28, 2025 01:52

k8s-ci-robot assigned ahg-g Mar 28, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 28, 2025

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 28, 2025

k8s-ci-robot merged commit 16ded66 into kubernetes-sigs:main Mar 28, 2025
8 checks passed

ahg-g mentioned this pull request Apr 1, 2025

Create env vars for the algorithm's scheduling parameters #447

Closed


		// Log current configuration values for debugging purposes.
		logger.V(logutil.VERBOSE).Info("Scheduler configuration",

update algorithm parameters from env variables #580

update algorithm parameters from env variables #580

Conversation

kaushikmitr commented Mar 26, 2025

k8s-ci-robot commented Mar 26, 2025

netlify bot commented Mar 26, 2025 • edited Loading

✅ Deploy Preview for gateway-api-inference-extension ready!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kfswain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liu-cong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahg-g commented Mar 27, 2025

kaushikmitr commented Mar 27, 2025

ahg-g commented Mar 28, 2025

ahg-g commented Mar 28, 2025

k8s-ci-robot commented Mar 28, 2025

netlify bot commented Mar 26, 2025 •

edited

Loading