docs: propose composable scheduler policy chain - #2077
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: AnkushUjawane 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 |
github-actions
Bot
added
the
kind/documentation
|
Welcome @AnkushUjawane! It looks like this is your first PR to Project-HAMi/HAMi 🎉 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a documentation proposal for composable device and node scheduler policy chains and scheduler-level exclusion filtering, including compatibility rules, execution points, locking requirements, user stories, open questions, and implementation references. ChangesScheduler policy proposal
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related issues
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request proposes a design document for a composable scheduler policy chain and a mutual-exclusion (mutex) policy in HAMi. The review feedback highlights several critical design and implementation concerns: the proposed NUMA sorting behavior breaks backward compatibility with the default spread policy; an empty policy chain could result in unstable sorting, which can be mitigated with a fallback mechanism; the mechanism for retrieving pod annotations for mutex-group checks needs clarification to avoid performance issues; and node-level mutex filtering should be optimized by executing it earlier in the scheduling loop.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
docs/develop/scheduler-policy-composition.md (1)
88-91: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMake invalid-chain fallback semantics explicit.
The proposal does not state whether an unknown token discards the entire chain or is ignored. The fallback also differs by layer: the supplied GPU comparator defaults to spread behavior, while the node comparator defaults to binpack. Define per-layer parsing and fallback behavior, or reject invalid annotations instead of silently changing placement.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/develop/scheduler-policy-composition.md` around lines 88 - 91, Clarify the scheduler-policy composition proposal’s parsing semantics for unknown tokens: define whether each invalid token is ignored or causes the entire chain to fall back, separately for GPU and node layers. Explicitly document each layer’s fallback comparator and behavior, including GPU spread and node binpack, or specify that invalid annotations are rejected rather than silently changing placement.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/develop/scheduler-policy-composition.md`:
- Around line 43-50: Update the NUMA dimension design around
numaDimension.Compare to preserve each policy’s legacy ordering: binpack must
use descending NUMA with ascending score, while the default spread path must use
ascending NUMA with descending score. Add explicit direction-aware dimension
implementations or configuration, and add legacy-mode tests covering both
policies before claiming backward compatibility.
- Around line 67-91: Document how every policy entry point constructs and
propagates the ordered dimension chain: update scheduler.go’s
GetGPUSchedulerPolicyByPod path and score.go’s configuration/annotation path to
invoke the parser, preserve the legacy Policy field for backward compatibility,
and copy the resulting Chain into DeviceUsageList and NodeScoreList. Specify
bare-value parsing, unknown-dimension fallback with warning logging, and ensure
Chain is populated before Less() comparisons.
- Around line 107-114: Add an atomic reservation or bind-time revalidation to
the mutex-group handling proposed around fitInDevices and
pkg/scheduler/score.go, so concurrent scheduling attempts cannot both pass the
conflict check and bind conflicting pods. Define and implement the required
scheduler serialization or assumed-state mechanism while preserving the
configured device/node mutex-scope behavior and hard no-co-location guarantee.
- Around line 97-114: The proposed mutex mechanism lacks the scheduling context
needed by CustomFilterRule to evaluate pod annotations and node occupancy.
Revise the design around fitInDevices to perform the mutex filter in scheduler
code with explicit access to the scheduling pod, candidate device/node, and all
pods bound to that node, or extend CustomFilterRule’s interface to provide
equivalent context. Ensure both device- and node-scoped exclusions can reliably
compare mutex-group and mutex-scope annotations.
- Around line 67-68: Define the node-level comparator contract before describing
or implementing a symmetric NodeScoreList chain: introduce a NodeDimension
abstraction compatible with *NodeScore, or establish a shared score abstraction
that both PolicyDimension and node comparisons can consume. Reuse shared parsing
and registration where appropriate, and update the NodeScoreList Less() design
to use the chosen contract instead of passing *NodeScore to
PolicyDimension.Compare.
---
Nitpick comments:
In `@docs/develop/scheduler-policy-composition.md`:
- Around line 88-91: Clarify the scheduler-policy composition proposal’s parsing
semantics for unknown tokens: define whether each invalid token is ignored or
causes the entire chain to fall back, separately for GPU and node layers.
Explicitly document each layer’s fallback comparator and behavior, including GPU
spread and node binpack, or specify that invalid annotations are rejected rather
than silently changing placement.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7e95fdba-d8ca-49c0-bf1b-13eaa57655c5
📒 Files selected for processing (1)
docs/develop/scheduler-policy-composition.md
| `DeviceUsageList` (and analogously `NodeScoreList`) gains a `Chain | ||
| []PolicyDimension` field, and `Less()` becomes: | ||
|
|
||
| ```go | ||
| func (l DeviceUsageList) Less(i, j int) bool { | ||
| for _, dim := range l.Chain { | ||
| if c := dim.Compare(l.DeviceLists[i], l.DeviceLists[j]); c != 0 { | ||
| return c < 0 | ||
| } | ||
| } | ||
| return false | ||
| } | ||
| ``` | ||
|
|
||
| **Config/annotation format** changes from a single value to a | ||
| comma-separated ordered list: | ||
|
|
||
| ```yaml | ||
| hami.io/gpu-scheduler-policy: "numa,binpack" | ||
| ``` | ||
|
|
||
| Parsing is backward compatible: a bare `"binpack"` still parses to a | ||
| one-element chain, so existing clusters and Helm defaults keep working | ||
| unmodified. Unknown dimension names fall back to the default single-dimension | ||
| behavior with a warning log, rather than a hard error. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Specify chain construction and propagation at every policy entry point.
pkg/scheduler/scheduler.go currently populates DeviceUsageList.Policy from GetGPUSchedulerPolicyByPod, while pkg/scheduler/score.go populates NodeScoreList.Policy from configuration or pod annotations. If only Less() changes, Chain remains empty and its loop returns false for every comparison, effectively disabling policy ordering. Document parser invocation, legacy-field handling, and chain copying for these paths.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/develop/scheduler-policy-composition.md` around lines 67 - 91, Document
how every policy entry point constructs and propagates the ordered dimension
chain: update scheduler.go’s GetGPUSchedulerPolicyByPod path and score.go’s
configuration/annotation path to invoke the parser, preserve the legacy Policy
field for backward compatibility, and copy the resulting Chain into
DeviceUsageList and NodeScoreList. Specify bare-value parsing, unknown-dimension
fallback with warning logging, and ensure Chain is populated before Less()
comparisons.
d178531 to
08a71d6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/develop/scheduler-policy-composition.md`:
- Around line 88-91: Update the comma-separated policy parser to recognize
“topology-aware” as the existing GPU policy and route it through the dedicated
NVIDIA handling unchanged, rather than treating it as an unknown dimension or
defaulting. Add a compatibility test covering the hami.io/gpu-scheduler-policy
annotation set to “topology-aware” and verify the legacy placement behavior is
preserved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8a75b152-391c-4a27-b62f-6d3bcd2f8164
📒 Files selected for processing (1)
docs/develop/scheduler-policy-composition.md
08a71d6 to
8826cb6
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/develop/scheduler-policy-composition.md`:
- Around line 120-125: Update the parser invariant description to distinguish
recognized legacy policies from empty or invalid input: map `binpack` to
`[numa-desc, binpack]` and `spread` to `[numa-asc, spread]`, while retaining
`[numa-asc, spread]` only as the fallback for empty or unparseable values.
- Around line 197-205: Update the scheduler policy composition proposal to
explicitly define that a missing or empty hami.io/exclusion-group disables
exclusion and never matches another empty value. Also specify the source and
matching contract for hami.io/exclusion-scope—whether it is taken from the
scheduling pod, incumbent pod, or required on both sides—so scope evaluation is
consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3a610187-ef6f-4c0d-8034-d57b9d47daf8
📒 Files selected for processing (1)
docs/develop/scheduler-policy-composition.md
8826cb6 to
912240b
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/develop/scheduler-policy-composition.md`:
- Around line 262-263: Update the scheduler policy documentation sentence to
hyphenate “bin-packed” in the description of GPU placement, while preserving the
rest of the wording unchanged.
- Around line 221-227: Update the scope-source rule in the scheduler policy
documentation so exclusion scope is symmetric between the scheduling pod and
incumbent same-group pods, using the broader scope when they differ. Revise the
corresponding explanation and the section around the follow-up guarantee so the
documented behavior preserves the hard no-co-location guarantee across node and
device scopes.
- Around line 163-179: Update ParseDeviceChain to return a tagged result or
preserve the original policy string alongside []DeviceDimension, allowing the
caller to distinguish the explicit "topology-aware" policy from unknown
dimensions. Route that signal through the existing NVIDIA device-combination
path unchanged, while retaining warning-and-default-chain behavior for other
unknown tokens and adding the required compatibility test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 49b32ad4-0431-47db-97ff-e0f3542ed8d6
📒 Files selected for processing (1)
docs/develop/scheduler-policy-composition.md
Signed-off-by: AnkushUjawane <multiverse1808@gmail.com>
912240b to
2fd49fe
Compare
|
both roadmap items here (mutex policy, policy combination) are already assigned in #1889, and pr 2011 already ships a mutex gpu-scheduler-policy value and pr 2012 already has a pending fix for #1806 might be worth coordinating on #1889 first like other contributors did before designing further with @archlitchi |
|
/hold |
|
/unhold |
…HAMi#1806 sections (superseded by Project-HAMi#2011) Signed-off-by: AnkushUjawane <multiverse1808@gmail.com>
|
Rebased and narrowed to just the composable chain — dropped the mutex and #1806-sort sections entirely since #2011 covers both. This revision proposes zero behavior change: verified the chain construction reproduces #2011's current four resolved paths (mutex, numa-bind×binpack, numa-bind×spread, default) exactly, via an equivalence table in the doc. Also flagged #2220 as a concrete example of the kind of addition this would make easier going forward, since it'd otherwise be a fourth hand-edited branch in Less(). |
Reminder: Answers must be written by human being. You can view the relevant rule here. |
|
title still says composable chain + mutex policy. body says mutex part got dropped. pls update title to match, just composable chain now. |
Codecov Report✅ All modified and coverable lines are covered by tests.
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
AnkushUjawane
changed the title
|
Done, I change title name |
|
#2621 merged today |
2 participants
What type of PR is this?
/kind design
What this PR does / why we need it:
This PR proposes a design for two v2.10 roadmap items: a composable scheduler
policy chain (node-scheduler-policy / gpu-scheduler-policy currently only
support a single policy at a time, e.g. "binpack" OR "spread" OR
"topology-aware", with no way to combine dimensions like NUMA + binpack), and
a new
mutexscheduling policy for hard mutual-exclusion constraints betweenpods/devices.
The composable chain also addresses the root cause of #1806:
gpu_policy.go'sLess()already hardcodes a two-dimension comparison (NUMA, then Score) butit isn't configurable or extensible to other dimensions.
This is a design doc only — no code changes in this PR. I'd like feedback on
the approach before implementing.
Which issue(s) this PR fixes:
Relates to #1889, #1806
Special notes for your reviewer:
(
pkg/scheduler/policy/,pkg/scheduler/score.go,pkg/util/types.go)and draft an initial version of this doc, which I then reviewed and edited
myself. Happy to discuss/defend any part of the design.
needs a parallel
NodeDimensioninterface alongsidePolicyDimension, andwhether
hami.io/mutex-groupis the right annotation name given existingnvidia.com/prioritysemantics (see FAQ HAMi FAQ #646).planning to split the policy-chain refactor and the mutex filter into
separate PRs since they're independent.
Does this PR introduce a user-facing change?:
Summary by CodeRabbit
topology-aware.