cleanup(metax): decouple Metax-GPU ScoreNode from the scheduler policy string - #2445
Conversation
|
[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 branches on scheduler policy. It prefers score annotations, converts loss annotations to higher-is-better scores, handles unusable data as zero, and implements ChangesPolicy-neutral scoring
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant MetaxDevices
participant OverrideScore
participant SchedulerPolicy
MetaxDevices->>OverrideScore: return policy-independent node score
OverrideScore->>SchedulerPolicy: apply Binpack or Spread weighting
SchedulerPolicy-->>OverrideScore: return weighted score
OverrideScore-->>MetaxDevices: preserve node ordering
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 (6)
pkg/scheduler/policy/node_policy_test.go (1)
369-371: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the exported annotation constant instead of the literal.
pkg/device/metaxexportsMetaxSGPUTopologyAwarefor"metax-tech.com/sgpu-topology-aware".pkg/device/metax/sdevice_test.goLine 2931 uses the constant. Import and reuse it here so a rename cannot silently break these four cases.Also applies to: 379-381, 419-421, 429-431
🤖 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 369 - 371, Replace the repeated literal annotation key in the Pod.Annotations setup for all four cases with the exported metax.MetaxSGPUTopologyAware constant. Add the required pkg/device/metax import and keep the existing annotation values and test behavior unchanged.pkg/device/metax/sdevice_test.go (1)
2985-2995: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the expected score value, not only policy equality.
The test passes if
ScoreNodereturns the same value for every policy. A regression that returns0for all inputs still passes. Add awantfield to the table and assert the concrete score for each case.♻️ Proposed change
for _, ts := range []struct { name string podDevices device.PodSingleDevice previous []*device.DeviceUsage + want float32 }{if binpack != spread || binpack != empty { t.Errorf("ScoreNode is policy-dependent: binpack=%v, spread=%v, empty=%v", binpack, spread, empty) } + if binpack != ts.want { + t.Errorf("ScoreNode() = %v, want %v", binpack, ts.want) + }🤖 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/sdevice_test.go` around lines 2985 - 2995, Extend the test-case table used by the ScoreNode subtests with a want score, then update the assertions in the ts.name test block to verify the returned score equals that expected value while retaining the policy-equality check. Populate want for every case from the intended ScoreNode results.pkg/device/metax/device_test.go (2)
482-490: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer a compile-time assertion for the marker interface.
A package-level
var _ device.PolicyNeutralScorer = &MetaxDevices{}fails at build time instead of test time. Also declaredevasdevice.Devicesto matchpkg/device/metax/sdevice_test.goLine 3002, which proves both interfaces at once.🤖 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 482 - 490, Replace the runtime assertion in TestMetaxDevicesImplementsPolicyNeutralScorer with a package-level compile-time assertion assigning &MetaxDevices{} to device.PolicyNeutralScorer. Update the test’s dev declaration to device.Devices, matching the established interface-check pattern in the surrounding tests so both interfaces are validated together.
411-460: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a case for a losses annotation without the requested index.
The table does not cover a
lossesmap that omits the requested device count. That path returns2000today. See the comment onpkg/device/metax/device.goLines 208-214 for the root cause. Add the case once the conversion is fixed, so the behavior is locked in.🤖 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 411 - 460, Add a table-driven test case to the existing scoring test covering a losses annotation whose map omits the requested device-count index, with the expected result of 2000; add it alongside the other losses annotation cases in the test table and ensure it uses the existing twoDevices fixture.pkg/scheduler/policy/node_policy.go (1)
67-73: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the weight into a named constant.
10000and-10000are repeated literals that encode a policy contract. Define one exported or package-level constant and negate it for Spread. This also removes the risk of the two literals drifting apart.♻️ Proposed change
+// policyNeutralScoreWeight makes policy-neutral device scores dominate the +// base node score. +const policyNeutralScoreWeight float32 = 10000 + func (ns *NodeScore) OverrideScore(previous []*device.DeviceUsage, policy string) {if _, ok := device.GetDevices()[idx].(device.PolicyNeutralScorer); ok { - weight := float32(10000) + weight := policyNeutralScoreWeight if policy == util.NodeSchedulerPolicySpread.String() { - weight = -10000 + weight = -policyNeutralScoreWeight } score = weight * score }🤖 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.go` around lines 67 - 73, Define a package-level named constant for the neutral scorer weight in the scoring logic, then use that constant for the default value and negate it when policy equals util.NodeSchedulerPolicySpread.String(). Remove both inline 10000 literals while preserving the existing score calculation in the device.PolicyNeutralScorer branch.pkg/device/metax/device.go (1)
191-194: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the
2000constant and the cross-node scale.The scores branch returns the raw annotation value. The losses branch returns
2000 - loss. The two branches produce values on different scales. If some nodes publishscoresand other nodes publishlosses,OverrideScorecompares the two scales directly. Confirm that Metax always publishes the same annotation on every node of a cluster. Also extract2000into a named constant with a comment about its origin.🤖 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 191 - 194, Update OverrideScore to use a named constant for 2000, with a comment documenting its Metax origin and the losses-to-scores conversion. Confirm and document the Metax contract that every node in a cluster publishes the same annotation type, preserving cross-node comparability when OverrideScore compares values.
🤖 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 208-214: Update parseMetaxAnnos and its caller in the node scoring
logic to distinguish a found loss value from malformed or missing topology data.
When the requested index is unavailable, return the same neutral score used by
the no-annotation path instead of converting zero into the maximum score;
preserve the existing 2000-loss conversion only for known values.
---
Nitpick comments:
In `@pkg/device/metax/device_test.go`:
- Around line 482-490: Replace the runtime assertion in
TestMetaxDevicesImplementsPolicyNeutralScorer with a package-level compile-time
assertion assigning &MetaxDevices{} to device.PolicyNeutralScorer. Update the
test’s dev declaration to device.Devices, matching the established
interface-check pattern in the surrounding tests so both interfaces are
validated together.
- Around line 411-460: Add a table-driven test case to the existing scoring test
covering a losses annotation whose map omits the requested device-count index,
with the expected result of 2000; add it alongside the other losses annotation
cases in the test table and ensure it uses the existing twoDevices fixture.
In `@pkg/device/metax/device.go`:
- Around line 191-194: Update OverrideScore to use a named constant for 2000,
with a comment documenting its Metax origin and the losses-to-scores conversion.
Confirm and document the Metax contract that every node in a cluster publishes
the same annotation type, preserving cross-node comparability when OverrideScore
compares values.
In `@pkg/device/metax/sdevice_test.go`:
- Around line 2985-2995: Extend the test-case table used by the ScoreNode
subtests with a want score, then update the assertions in the ts.name test block
to verify the returned score equals that expected value while retaining the
policy-equality check. Populate want for every case from the intended ScoreNode
results.
In `@pkg/scheduler/policy/node_policy_test.go`:
- Around line 369-371: Replace the repeated literal annotation key in the
Pod.Annotations setup for all four cases with the exported
metax.MetaxSGPUTopologyAware constant. Add the required pkg/device/metax import
and keep the existing annotation values and test behavior unchanged.
In `@pkg/scheduler/policy/node_policy.go`:
- Around line 67-73: Define a package-level named constant for the neutral
scorer weight in the scoring logic, then use that constant for the default value
and negate it when policy equals util.NodeSchedulerPolicySpread.String(). Remove
both inline 10000 literals while preserving the existing score calculation in
the device.PolicyNeutralScorer branch.
🪄 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: 40c5983e-8b05-48cc-9aa8-2bbd3db05596
📒 Files selected for processing (7)
pkg/device/devices.gopkg/device/metax/device.gopkg/device/metax/device_test.gopkg/device/metax/sdevice.gopkg/device/metax/sdevice_test.gopkg/scheduler/policy/node_policy.gopkg/scheduler/policy/node_policy_test.go
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
|
Thanks for working on the MetaX scoring code. This PR is stacked on #2413 and includes that PR's full diff, so it cannot be reviewed as an independent cleanup. More importantly, it changes the MetaX-GPU binpack signal from losses to scores, while the stated acceptance condition is that node ordering remain unchanged. That is a behavioral change rather than a decoupling-only cleanup. We are closing this PR. Please first finish or replace #2413, then propose any independent cleanup from the current master with tests that prove ordering equivalence. |
|
@FouoF @archlitchi can you please review my PR |
Signed-off-by: asadjan4611 <asadjan4611@gmail.com>
9323e6c to
dcd120a
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (3)
pkg/device/metax/device_test.go (2)
104-183: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSeparate the annotation input from the subtest name.
The
namefield carries the annotation JSON and the subtest label. Eight cases share the same JSON, sot.Runproduces#01-style suffixed names. A failing subtest does not show the index under test. Add anannosfield and give each case a descriptive name.♻️ Proposed refactor
tests := []struct { name string + annos string index int value float32 wantFound bool }{ { - name: "{\"1\":0,\"2\":110,\"3\":270,\"4\":540,\"5\":580,\"6\":730,\"7\":930,\"8\":1240}", + name: "index 1", + annos: "{\"1\":0,\"2\":110,\"3\":270,\"4\":540,\"5\":580,\"6\":730,\"7\":930,\"8\":1240}", index: 1, value: 0, wantFound: true, },Then update the call site:
- value, found := parseMetaxAnnos(tt.name, tt.index) + value, found := parseMetaxAnnos(tt.annos, tt.index)🤖 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 104 - 183, Update the parseMetaxAnnos table-driven test to add a separate annos field for the annotation JSON, use descriptive unique values in name for each case (including the tested index), and pass tt.annos to parseMetaxAnnos instead of tt.name. Preserve the existing expected values and found-status assertions.
437-548: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive loss expectations from
metaxTopologyLossBaseand include the policy in assertion failures.
metaxTopologyLossBaseis2000, so replace1800and1950withmetaxTopologyLossBase - float32(200)andmetaxTopologyLossBase - float32(50). Add"policy %q", policytoassert.Equalso failures identify the policy.🤖 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 437 - 548, Update the expected loss-based scores in the tests around the test table to derive them from metaxTopologyLossBase, using subtraction of the corresponding loss values instead of hardcoded 1800 and 1950. In the policy loop’s assertion, include the current policy with the assertion message so failures identify which policy produced the mismatch.pkg/scheduler/policy/node_policy_test.go (1)
563-579: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case where the two annotations disagree.
Both fixtures set
lossesandscoresso that the same node wins. The new backend prefersscoresand falls back tolosses. The old Binpack rule readlosses. A behavior change is therefore only observable when the two annotations rank nodes differently, and no case covers that. Add a fixture with divergent values and assert the intended winner explicitly. This also addresses the ordering-equivalence concern raised in the PR discussion.The premise assertion on Line 588 also derives both expectations from the fixture, so a wrong helper and a wrong implementation could agree. Consider asserting a literal node name per case.
🧪 Suggested additional fixture
tests := []struct { name string nodes []metaxNode + want string }{ { name: "best node has both lowest loss and highest score", nodes: []metaxNode{ {name: "node-a", loss: 300, score: 100}, {name: "node-b", loss: 100, score: 300}, {name: "node-c", loss: 200, score: 200}, }, + want: "node-b", },Then add a divergent case and assert
tt.wantdirectly instead of comparing the two derived winners.🤖 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 563 - 579, Add a test fixture in the node-policy test cases where loss and score rankings disagree, with an explicit intended node name reflecting score preference and loss fallback behavior. Update the table-driven assertions to compare the selected result directly against each case’s literal expected winner rather than deriving expectations from both helpers, preserving the existing cases while making divergent ordering observable.
🤖 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.
Nitpick comments:
In `@pkg/device/metax/device_test.go`:
- Around line 104-183: Update the parseMetaxAnnos table-driven test to add a
separate annos field for the annotation JSON, use descriptive unique values in
name for each case (including the tested index), and pass tt.annos to
parseMetaxAnnos instead of tt.name. Preserve the existing expected values and
found-status assertions.
- Around line 437-548: Update the expected loss-based scores in the tests around
the test table to derive them from metaxTopologyLossBase, using subtraction of
the corresponding loss values instead of hardcoded 1800 and 1950. In the policy
loop’s assertion, include the current policy with the assertion message so
failures identify which policy produced the mismatch.
In `@pkg/scheduler/policy/node_policy_test.go`:
- Around line 563-579: Add a test fixture in the node-policy test cases where
loss and score rankings disagree, with an explicit intended node name reflecting
score preference and loss fallback behavior. Update the table-driven assertions
to compare the selected result directly against each case’s literal expected
winner rather than deriving expectations from both helpers, preserving the
existing cases while making divergent ordering observable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a1c455ea-575c-4b12-b3b1-febee534d7a5
📒 Files selected for processing (3)
pkg/device/metax/device.gopkg/device/metax/device_test.gopkg/scheduler/policy/node_policy_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/device/metax/device.go
|
This is being closed because it does not comply with the contribution guidelines. Pls read the rules. |
4 participants
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
MetaxDevices.ScoreNode(theMetax-GPUbackend) chose its result based on the scheduler policy string: it read themetax-tech.com/gpu.topology.lossesannotation under Binpack andmetax-tech.com/gpu.topology.scoresunder Spread,encoding the policy's sort direction directly in the device layer. This is the same anti-pattern #2404 removed fromMetaxSDevices(Metax-SGPU).This PR makes
ScoreNodereturn a single policy-independent, "higher is a better node" score, and hasMetaxDevicesimplement the existingdevice.PolicyNeutralScorermarker. The shared scheduler policy layer(
OverrideScore) then applies the weight and the Spread sign inversion in one place.OverrideScoreitself is unchanged, and no other backend is affected.Which issue(s) this PR fixes:
Fixes #2439
Special notes for your reviewer:
PolicyNeutralScorermarker andOverrideScoreweighting introduced there. Please merge cleanup(metax): decouple MetaxSDevices.ScoreNode from the scheduler policy string #2413 first; this PR will then narrow to asingle commit.
losseswhile Spread readscores. Both policies now preferscoresand fall back tolosses. Spread behavior is preserved exactly; Binpack now uses the consistentscoressignal when anode publishes both annotations.
pkg/device/metaxandpkg/scheduler/policy,go vet,goimports(local prefix), and license headers all pass.I used AI Assistance for verification of test cases and for the PR description and also verified for the solution implementations.
Does this PR introduce a user-facing change?:
NONESummary by CodeRabbit