cleanup(metax): decouple MetaxSDevices.ScoreNode from the scheduler policy string - #2413
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesMetaX policy-neutral scoring
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
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
🤖 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/scheduler/policy/node_policy_test.go`:
- Around line 324-326: Replace the topology-aware annotation key at
pkg/scheduler/policy/node_policy_test.go:324-326 and
pkg/scheduler/policy/node_policy_test.go:374-376 with the required hami.io/
prefix, and migrate MetaxSGPUTopologyAware plus every producer and consumer in
the same change to use that hami.io/ annotation contract.
🪄 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: 6849ddaf-e8c9-4f0b-82bd-219280fe3b3c
📒 Files selected for processing (5)
pkg/device/devices.gopkg/device/metax/sdevice.gopkg/device/metax/sdevice_test.gopkg/scheduler/policy/node_policy.gopkg/scheduler/policy/node_policy_test.go
github-actions
Bot
added
kind/bug
6cb9323 to
84ccc66
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
The title of this PR seems unrelated to the content |
asadjan4611
changed the title
|
@archlitchi thanks for noticing i have update the title can you please check it |
Shouren
left a comment
Shouren
left a comment
There was a problem hiding this comment.
@asadjan4611 Please rebase the master to fix the CI Workflow action.
84ccc66 to
24bddce
Compare
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:
|
|
@Shouren can you please review it now |
…string MetaxSDevices.ScoreNode previously changed its result based on the scheduler policy string and hard-coded a ±10000 weight in the device layer. That weight existed only to flip the score's sign so the best node would win under both the Binpack and Spread sort directions, and a TODO noted that scoring should not depend on the policy. ScoreNode now returns a policy-independent, "higher is a better node" score. A new optional device.PolicyNeutralScorer marker interface lets a backend declare this, and the shared scheduler policy layer (OverrideScore) applies the weight and the Spread-policy sign inversion in one place, only for backends that opt in. Only MetaxSDevices implements the marker, so Metax-GPU and the other backends are unaffected. Node ranking is unchanged under both Binpack and Spread. Fixes Project-HAMi#2404 Signed-off-by: asadjan4611 <asadjan4611@gmail.com>
…consumer The PolicyNeutralScorer interface was exported from pkg/device even though it has a single consumer (OverrideScore) and a single implementer. Move it into pkg/scheduler/policy as an unexported policyNeutralScorer interface, next to the code that uses it. Because Go satisfies interfaces structurally, MetaxSDevices keeps its exported PolicyNeutralScore() marker method and still satisfies the private interface, so the capability is unchanged. The device package no longer advertises a public abstraction with only one internal user. Signed-off-by: asadjan4611 <asadjan4611@gmail.com>
24bddce to
ab456b2
Compare
|
@mesutoezdil can you please review it now. |
|
/lgtm |
|
@mesutoezdil sorry for disturbance |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: asadjan4611, Shouren The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
4 participants
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
MetaxSDevices.ScoreNodepreviously changed its result based on the schedulerpolicy string and carried a
TODO: score should not depend on policy. It alsohard-coded a
±10000magic number in the device layer whose only purpose was toflip the score's sign so the best node would win under both the Binpack and
Spread sort directions. This coupled a device backend to scheduler-policy
concerns that belong in the shared policy layer.
This PR decouples them:
ScoreNodenow returns a policy-independent score following a"higher score is a better node" convention, and the
TODOis removed.device.PolicyNeutralScorerlets a backenddeclare that its
ScoreNoderesult is policy-independent.policy.OverrideScore) applies the weight (10000)and the Spread-policy sign inversion (
-10000) in one place, only forbackends that opt in via the marker. Policy handling now lives in a single
location, so future policies no longer require backend-specific changes.
The final weighted scores are numerically identical to the previous behavior
under both policies, so node ranking is unchanged. Backends that genuinely need
the policy string (e.g.
Metax-GPU, which reads different annotations perpolicy) do not implement the marker and are completely unaffected.
Which issue(s) this PR fixes:
Fixes #2404
Special notes for your reviewer:
+10000andSpread stays
-10000, applied inOverrideScoreinstead of insideScoreNode. For a raw device scores, the node's contribution is identicalto before:
+10000·s(binpack) and-10000·s(spread).MetaxSDevicesimplementsPolicyNeutralScorer. The other 12 device backends and the siblingMetax-GPUbackend are untouched (verified by grep + build).TestScoreNodePolicyIndependence— assertsScoreNodereturns the samevalue for
binpack,spread, and""across the online, topology-aware,and no-hint paths.
TestMetaxSDevicesImplementsPolicyNeutralScorer— asserts the marker is wired.TestOverrideScorecases — binpack (+600000) and spread(
-600000) for the same input, proving ranking preservation.go build ./...,go vet, race-tested modified packages,gofmt/goimports, license headers, andgolangci-lint v2.12.2(0 issues)all pass.
Does this PR introduce a user-facing change?:
Summary by CodeRabbit
Bug Fixes
Tests