cleanup(metax): decouple MetaxDevices.ScoreNode (Metax-GPU) from the scheduler policy string - #2579
Conversation
…licy string MetaxDevices.ScoreNode (Metax-GPU) branched on the scheduler policy string: under Binpack it scored 2000 - losses[n] and under Spread it scored 2000 - scores[n]. The device layer should not know scheduler policy names, and the 2000 - score subtraction only existed to flip the sign so the best node wins under both sort directions. The sibling Metax-SGPU backend was already decoupled this way in Project-HAMi#2413. ScoreNode now returns a single policy-independent, "higher is a better node" score: it prefers the scores annotation and falls back to the losses annotation (converted onto the same scale via 2000 - loss) when scores is absent. MetaxDevices implements the policy-neutral scorer marker, so the shared OverrideScore layer owns the ±10000 weighting and the Spread-policy sign inversion, mirroring Metax-SGPU. OverrideScore itself is unchanged. When a node advertises both annotations they describe the same topology preference, so the lowest-loss node is also the highest-score node and the winning node is unchanged under both Binpack and Spread. A new ordering-equivalence test asserts this by weighting and sorting nodes exactly as the scheduler does. Tests: ScoreNode is asserted identical across binpack/spread/"" policies; a marker-interface test is added; the OverrideScore cases cover the weighted Binpack and inverted Spread results; and the ordering- equivalence test covers node selection. Fixes Project-HAMi#2572 Signed-off-by: asadjan4611 <asadjan4611@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: asadjan4611 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 |
📝 WalkthroughWalkthroughMetax-GPU scoring no longer depends on the scheduler policy. Score annotations take precedence, loss annotations provide fallback values, and the shared scheduler layer applies policy weighting and ordering. Tests cover scoring behavior, marker implementation, weighted scores, and node-ordering stability. ChangesMetax scoring
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels: 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
pkg/scheduler/policy/node_policy_test.go (2)
581-591: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe premise assertion at Line 588 hides which case failed.
assert.Equal(t, wantBinpack, wantSpread)guards the test data, not the production code. If it fails, the message does not say that the fixture is inconsistent. Add a message so a future edit totestsis diagnosed quickly.♻️ Proposed message
- assert.Equal(t, wantBinpack, wantSpread) + assert.Equal(t, wantBinpack, wantSpread, + "test fixture is inconsistent: the lowest-loss node must also be the highest-score node")🤖 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 `@pkg/scheduler/policy/node_policy_test.go` around lines 581 - 591, Add a clear assertion message to the wantBinpack-versus-wantSpread check in the test loop, explicitly identifying inconsistent test fixture annotations or rankings; leave the production winner assertions unchanged.
477-486: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueShare the
config.Configfixture withTestOverrideScore.
TestOverrideScoreat Line 202 builds an almost identicalconfig.Config. Extract one helper so both tests register the same device set.🤖 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 `@pkg/scheduler/policy/node_policy_test.go` around lines 477 - 486, Extract the shared config.Config fixture from TestOverrideScore into a helper that registers the device configuration, then reuse that helper in TestOverrideScore and TestOverrideScoreMetaxGPUOrderingUnchanged. Ensure both tests use the same resource names and DefaultGPUNum values.pkg/device/metax/device.go (2)
205-205: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winLower the scoring log level.
klog.InfoSwrites at verbosity 0.ScoreNoderuns for every candidate node on every scheduling attempt, so this produces one log line per node per pod. Other backends useklog.V(4)orklog.V(3)for the same purpose, for examplepkg/device/ascend/device.go:408andpkg/device/kunlun/device.go:174.♻️ Proposed log level change
- klog.InfoS("Detected annotations", "key", MetaxAnnotationScore, "value", scoreAnno, "requesting", sum, "extract", score) + klog.V(4).InfoS("Detected annotations", "key", MetaxAnnotationScore, "value", scoreAnno, "requesting", sum, "extract", score)Also applies to: 213-213
🤖 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 `@pkg/device/metax/device.go` at line 205, Lower the verbosity of the “Detected annotations” scoring logs in ScoreNode from unconditional klog.InfoS to a guarded verbosity level consistent with the other device backends, applying the same change to both occurrences.
209-215: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the
2000loss offset into a named constant and confirm the two annotation scales are comparable.Two points on the loss fallback:
2000is a magic number. Give it a name so the intent of the conversion is visible.- The two branches return values on different scales. A node that publishes only
scoresreturns the raw score (for example200). A node that publishes onlylossesreturns2000 - loss(for example1800). If a cluster contains nodes of both kinds, the loss-only nodes almost always outrank the score-only nodes. Confirm that Metax publishes the same annotation set on every node in a cluster, or document that assumption here.♻️ Proposed constant extraction
+// MetaxMaxTopologyLoss is the upper bound of the values published in the +// "gpu.topology.losses" annotation. It converts a loss (lower is better) onto +// the same "higher is better" scale used by "gpu.topology.scores". +const MetaxMaxTopologyLoss = 2000 + if lossAnno, ok := node.Annotations[MetaxAnnotationLoss]; ok { // it's preferred to select the node with lower loss, so convert the // loss onto a "higher is better" scale. loss := parseMetaxAnnos(lossAnno, sum) klog.InfoS("Detected annotations", "key", MetaxAnnotationLoss, "value", lossAnno, "requesting", sum, "extract", loss) - return 2000 - loss + return MetaxMaxTopologyLoss - loss }🤖 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 `@pkg/device/metax/device.go` around lines 209 - 215, Extract the loss conversion offset currently hard-coded as 2000 in the annotation-selection logic into a descriptive named constant, and use that constant when returning the converted loss. In the same logic around parseMetaxAnnos and the score/loss branches, confirm or document the assumption that Metax nodes in a cluster publish the same annotation set so both return scales remain comparable.pkg/device/metax/device_test.go (1)
402-460: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider two more table cases for the loss conversion.
The four cases cover the main paths. Two boundary behaviours of the new
2000 - lossconversion are untested:
- A loss value above
2000, which makesScoreNodereturn a negative score.- A malformed or index-missing annotation, where
parseMetaxAnnosreturns0.Both are cheap to add and pin the conversion contract.
💚 Proposed additional cases
{ name: "no topology annotation scores zero", node: &corev1.Node{ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{}}}, podDevices: twoDevices, want: float32(0), }, + { + name: "loss above the conversion offset yields a negative score", + node: &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + MetaxAnnotationLoss: "{\"2\":2500}", + }, + }, + }, + podDevices: twoDevices, + want: float32(-500), + }, + { + name: "malformed scores annotation scores zero", + node: &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + MetaxAnnotationScore: "not-json", + }, + }, + }, + podDevices: twoDevices, + want: float32(0), + }, }🤖 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 `@pkg/device/metax/device_test.go` around lines 402 - 460, Add table-driven cases to the tests around ScoreNode for the loss conversion: verify a loss above 2000 produces the corresponding negative score, and verify malformed or missing-index loss annotations cause parseMetaxAnnos to return 0 and ScoreNode to use that result. Keep the existing score-precedence and zero-score cases unchanged.
🤖 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 `@pkg/device/metax/device.go`:
- Around line 202-207: Update parseMetaxAnnos and ScoreNode so an invalid scores
annotation or one missing the requested sum is distinguishable from a valid
score of 0. In ScoreNode, only return the scores result when the lookup
succeeds; otherwise continue to the MetaxAnnotationLoss fallback while
preserving safe zero scoring when neither annotation provides a usable value.
---
Nitpick comments:
In `@pkg/device/metax/device_test.go`:
- Around line 402-460: Add table-driven cases to the tests around ScoreNode for
the loss conversion: verify a loss above 2000 produces the corresponding
negative score, and verify malformed or missing-index loss annotations cause
parseMetaxAnnos to return 0 and ScoreNode to use that result. Keep the existing
score-precedence and zero-score cases unchanged.
In `@pkg/device/metax/device.go`:
- Line 205: Lower the verbosity of the “Detected annotations” scoring logs in
ScoreNode from unconditional klog.InfoS to a guarded verbosity level consistent
with the other device backends, applying the same change to both occurrences.
- Around line 209-215: Extract the loss conversion offset currently hard-coded
as 2000 in the annotation-selection logic into a descriptive named constant, and
use that constant when returning the converted loss. In the same logic around
parseMetaxAnnos and the score/loss branches, confirm or document the assumption
that Metax nodes in a cluster publish the same annotation set so both return
scales remain comparable.
In `@pkg/scheduler/policy/node_policy_test.go`:
- Around line 581-591: Add a clear assertion message to the
wantBinpack-versus-wantSpread check in the test loop, explicitly identifying
inconsistent test fixture annotations or rankings; leave the production winner
assertions unchanged.
- Around line 477-486: Extract the shared config.Config fixture from
TestOverrideScore into a helper that registers the device configuration, then
reuse that helper in TestOverrideScore and
TestOverrideScoreMetaxGPUOrderingUnchanged. Ensure both tests use the same
resource names and DefaultGPUNum values.
🪄 Autofix
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: 2994854f-4658-4c07-9561-724e9e9731cd
📒 Files selected for processing (3)
pkg/device/metax/device.gopkg/device/metax/device_test.gopkg/scheduler/policy/node_policy_test.go
| if scoreAnno, ok := node.Annotations[MetaxAnnotationScore]; ok { | ||
| // it's preferred to select the node with higher score | ||
| score := parseMetaxAnnos(scoreAnno, sum) | ||
| klog.InfoS("Detected annotations", "key", MetaxAnnotationScore, "value", scoreAnno, "requesting", sum, "extract", score) | ||
| return score | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
A malformed or incomplete scores annotation now suppresses the losses fallback.
parseMetaxAnnos returns 0 when the JSON fails to unmarshal or when the map has no entry for sum. In that case ScoreNode returns 0 and never reads MetaxAnnotationLoss. A node that publishes both annotations, but whose scores map omits the requested device count, therefore loses its topology preference.
The scoring behaviour is safe (0), so this is not a blocker. If you want the fallback to stay effective, let parseMetaxAnnos report whether the lookup succeeded.
🤖 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 `@pkg/device/metax/device.go` around lines 202 - 207, Update parseMetaxAnnos
and ScoreNode so an invalid scores annotation or one missing the requested sum
is distinguishable from a valid score of 0. In ScoreNode, only return the scores
result when the lookup succeeds; otherwise continue to the MetaxAnnotationLoss
fallback while preserving safe zero scoring when neither annotation provides a
usable value.
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
This is being closed because it does not comply with the contribution guidelines. |
|
@mesutoezdil |
2 participants
What this PR does
MetaxDevices.ScoreNode(the Metax-GPU backend) branched on the schedulerpolicystring, reading a different topology annotation for each policy:2000 - losses[n]2000 - scores[n]This is the same coupling that was removed for the sibling Metax-SGPU backend in #2404 / #2413. The device layer should not know scheduler policy names, and the
2000 - scoresubtraction only existed to flip the sign so thebest node wins under both sort directions.
This PR makes
ScoreNodereturn a single policy-independent, "higher is a better node" score:metax-tech.com/gpu.topology.scoresannotation.metax-tech.com/gpu.topology.losses(converted onto the same"higher is better" scale via
2000 - loss) whenscoresis absent.MetaxDevicesnow implements the policy-neutral scorer marker, so the sharedOverrideScorelayer owns the±10000weighting and the Spread-policy sign inversion — exactly as it already does for Metax-SGPU.OverrideScoreitselfis unchanged.
Fixes #2572
Why the node ranking is unchanged
When a node advertises both annotations, they describe the same underlyingtopology preference, so the lowest-loss node is also the highest-score node. Because
OverrideScoreinverts the sign under Spread (which selects the lowestscore) and keeps it under Binpack (which selects the highest), the winning node is the same under both policies as before.
A new test,
TestOverrideScoreMetaxGPUOrderingUnchanged, asserts this directly:it builds nodes with consistent annotations, computes the winner each original
per-policy rule would have chosen, then weights and sorts the nodes exactly as the scheduler does (
OverrideScore→sort.Sort→NodeList[len-1]) and checks the same node wins under both Binpack and Spread.Scope
Exactly three files, no changes to the shared scheduler infrastructure that landed with #2413:
pkg/device/metax/device.go— the fixpkg/device/metax/device_test.go— policy-independence + marker-interface testspkg/scheduler/policy/node_policy_test.go— weighted Binpack/Spread cases + ordering-equivalence testAcceptance criteria (from #2572)
MetaxDevices.ScoreNodeno longer reads thepolicyparameterMetaxDevicesimplements the policy-neutral scorer markerTesting
go test ./pkg/device/metax/... ./pkg/scheduler/policy/... -short --race -count=1— passgo vetclean,go build ./...cleangoimports(local prefix) clean, import aliases correct, license headers presentgolangci-lint runon the changed packages — 0 issuesSummary by CodeRabbit