feat: make gangPolicy optional (#448)#959
feat: make gangPolicy optional (#448)#959AyushSriv06 wants to merge 1 commit intovolcano-sh:mainfrom
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Welcome @AyushSriv06! It looks like this is your first PR to volcano-sh/kthena 🎉 |
There was a problem hiding this comment.
Code Review
This pull request introduces a disableGangScheduling field to the ModelBooster CRD, allowing users to opt-out of gang scheduling for generated ModelServing resources. The changes encompass CRD schema updates, documentation, client-go apply configurations, and the controller logic required to handle the new flag. A review comment suggests using an existing local variable in the conversion logic to improve code consistency and readability.
There was a problem hiding this comment.
Pull request overview
This PR adds a disableGangScheduling toggle to the ModelBooster backend API to allow generated ModelServing resources to omit spec.template.gangPolicy, enabling opt-out from gang scheduling behavior where it’s not desired/supported.
Changes:
- Added
spec.backend.disableGangSchedulingto theModelBoosterCRD/API types and client-go applyconfiguration. - Updated
BuildModelServingconversion to nil outserving.Spec.Template.GangPolicywhen the toggle is enabled, plus added unit coverage. - Regenerated/updated CRD reference docs and Helm CRD schema; updated golden expected YAML revisions.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/model-booster-controller/convert/model_serving.go | Clears GangPolicy in generated ModelServing when disableGangScheduling is enabled. |
| pkg/model-booster-controller/convert/model_serving_test.go | Adds tests validating GangPolicy is removed when disabled and preserved by default. |
| pkg/apis/workload/v1alpha1/model_booster_types.go | Introduces the disableGangScheduling field on ModelBackend. |
| client-go/applyconfiguration/workload/v1alpha1/modelbackend.go | Exposes WithDisableGangScheduling for server-side apply usage. |
| charts/kthena/charts/workload/crds/workload.serving.volcano.sh_modelboosters.yaml | Updates Helm-packaged CRD schema to include disableGangScheduling. |
| docs/kthena/docs/reference/crd/workload.serving.volcano.sh.md | Updates generated CRD reference documentation for the new field. |
| pkg/model-booster-controller/convert/testdata/expected/model-serving.yaml | Updates expected revision label output (golden). |
| pkg/model-booster-controller/convert/testdata/expected/disaggregated-model-serving.yaml | Updates expected revision label output (golden). |
| pkg/model-booster-controller/convert/testdata/expected/disaggregated-model-serving-mooncake.yaml | Updates expected revision label output (golden). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
dff6680 to
28b4c8d
Compare
Signed-off-by: Ayush <ayushsrisks@gmail.com>
28b4c8d to
25e464d
Compare
|
In fact, this issue is simply intended to discuss whether this feature is required. |
|
/cc @hzxuzhonghu |
hello thanks for the review this is why i have taken up the issue, |
What type of PR is this?
What this PR does / why we need it:
This PR introduces a
disableGangSchedulingtoggle to the ModelBooster API. Previously, gangPolicy was unconditionally injected into generated ModelServing resources. This change allows users to opt-out of gang scheduling for environments where it is not required or supported.Which issue(s) this PR fixes:
Fixes #448