Skip to content

cleanup(metax): decouple MetaxDevices.ScoreNode from scheduler policy - #2635

Closed
Rickydama3 wants to merge 1 commit into
Project-HAMi:masterfrom
Rickydama3:fix-metax-scorenode-decouple
Closed

cleanup(metax): decouple MetaxDevices.ScoreNode from scheduler policy#2635
Rickydama3 wants to merge 1 commit into
Project-HAMi:masterfrom
Rickydama3:fix-metax-scorenode-decouple

Conversation

@Rickydama3

@Rickydama3 Rickydama3 commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

What type of PR is this?
/kind bug

What this PR does / why we need it:
I noticed that the Metax-GPU backend was still handling the binpack and spread policy checks directly inside ScoreNode. This was causing the issue reported in #2566, where nodes could be completely ignored if they only had one of the two topology annotations.

Since we already decoupled the Metax-SGPU backend in #2413, I followed the same approach here for Metax-GPU.

I removed the policy checks from the backend. It now uses the scores annotation when available and falls back to losses when needed, converting it to the standard higher-is-better scale. I also added the PolicyNeutralScorer marker to the device so the shared scheduler's OverrideScore layer can handle the 10000 weighting and spread sign flip, the same way it does for SGPU.

I also added tests in device_test.go to cover the policy-independent scoring and the fallback logic.

Which issue(s) this PR fixes:
Fixes #2566
Fixes #2572

Special notes for your reviewer:

I also added some tests to make sure it doesn't crash if the node annotations are missing or broken.

Does this PR introduce a user facing change?:

NONE

<!-- This is an auto-generated comment: release notes by coderabbit.ai -->
## Summary by CodeRabbit

* **Bug Fixes**
  * Improved device scoring to consistently prioritize valid topology scores.
  * Added fallback handling for topology loss values, including missing, malformed, and large-loss annotations.
  * Invalid or unavailable topology annotations now produce a neutral score instead of unreliable results.
  * Ensured device scores remain independent of scheduler policy.
  * Updated score calculations to preserve accurate weighting for high-value device scores.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

@hami-robot hami-robot Bot added kind/bug Something isn't working dco-signoff: yes labels Aug 13, 2026
@hami-robot

hami-robot Bot commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Rickydama3
Once this PR has been reviewed and has the lgtm label, please assign shouren for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@hami-robot
hami-robot Bot requested review from FouoF and archlitchi August 13, 2026 03:31
@github-actions github-actions Bot removed the kind/bug Something isn't working label Aug 13, 2026
@hami-robot hami-robot Bot added the size/L label Aug 13, 2026
@coderabbitai

coderabbitai Bot commented Aug 13, 2026

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 598d6ca8-e76c-48bd-8f4d-2e4be890b353

📥 Commits

Reviewing files that changed from the base of the PR and between e23aa61 and 4df5635.

📒 Files selected for processing (1)
  • pkg/scheduler/policy/node_policy_test.go

📝 Walkthrough

Walkthrough

Metax-GPU node scoring now ignores scheduler policy, prefers valid topology scores, and falls back to converted topology losses. The implementation adds a shared loss conversion constant and a policy-neutral scorer marker. Tests cover parsing, fallback behavior, policy independence, and scheduler score scaling.

Changes

Metax-GPU topology scoring

Layer / File(s) Summary
Policy-independent topology scoring
pkg/device/metax/device.go
ScoreNode validates annotations, prefers topology scores, converts topology losses with MetaxMaxTopologyLoss, and implements PolicyNeutralScore.
Scoring behavior validation
pkg/device/metax/device_test.go, pkg/scheduler/policy/node_policy_test.go
Tests cover malformed and missing annotations, score precedence, loss fallback, policy independence, marker implementation, fixture typing, and updated scheduler score scaling.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Mergeability Score: ⚪ Minimal · up to 4df56

The PR decouples Metax-GPU scoring from scheduler policy and adds fallback handling for node annotations; no actionable merge-blocking risk remains after normal checks and review.

Possibly related issues

Possibly related PRs

Suggested labels: enhancement

Suggested reviewers: archlitchi

Poem

A rabbit checks each topology sign,
Scores take precedence in the line.
Losses become scores when needed,
Policy branches are now deleted.
Tests hop after Metax-GPU! 🐇

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly states that MetaxDevices.ScoreNode is decoupled from scheduler policy, which is the primary change.
Linked Issues check ✅ Passed The changes implement policy-independent scoring, annotation fallback, the PolicyNeutralScorer marker, and focused tests required by issues [#2566] and [#2572].
Out of Scope Changes check ✅ Passed All modified production and test files directly support the linked issue objectives and contain no unrelated changes.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/device/metax/device.go`:
- Around line 207-211: Update parseMetaxAnnos to return both the parsed loss and
a validity flag, distinguishing malformed JSON or a missing key from a valid
loss value of 0. In the MetaxAnnotationLoss handling block, return a topology
score of 0 when parsing is invalid, while preserving MetaxMaxTopologyLoss - loss
for valid values, including zero; add coverage for malformed loss JSON and a
missing loss key.
🪄 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: 79ef3b83-f7bc-411e-8a61-2442e63363df

📥 Commits

Reviewing files that changed from the base of the PR and between 2008d2b and 1d33c0a.

📒 Files selected for processing (2)
  • pkg/device/metax/device.go
  • pkg/device/metax/device_test.go

Comment thread pkg/device/metax/device.go
@Rickydama3
Rickydama3 force-pushed the fix-metax-scorenode-decouple branch 4 times, most recently from e23aa61 to 4df5635 Compare August 13, 2026 06:42
@codecov

codecov Bot commented Aug 13, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

Flag Coverage Δ
unittests 62.49% <100.00%> (+0.11%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
pkg/device/metax/device.go 79.31% <100.00%> (+2.95%) ⬆️

... and 3 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Fixes Project-HAMi#2572 and fixes Project-HAMi#2566 by reading the available topology annotation independent of the active scheduler policy and deferring weighting to the shared policy layer via PolicyNeutralScorer.

Signed-off-by: Ricky Dama <rickydama2006@gmail.com>
@mesutoezdil

Copy link
Copy Markdown
Contributor

This is being closed because it does not comply with the contribution guidelines.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

2 participants