fix(ascend): validate module-pair count and prevent under-allocation for Ascend 910C - #2270
fix(ascend): validate module-pair count and prevent under-allocation for Ascend 910C#2270princexpoddar wants to merge 3 commits into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: princexpoddar 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 |
|
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)
📝 WalkthroughWalkthroughAscend910C allocation now validates complete physical card pairs before success. It rejects under-allocation and exact-count candidates that lack valid pairs. It separates general topology selection, prioritizes full cards, and adds five regression tests. ChangesAscend 910C allocation
Estimated code review effort: 2 (Simple) | ~15 minutes 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: 4
🧹 Nitpick comments (1)
pkg/device/ascend/device_910c_pairing_test.go (1)
39-44: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the required insufficient-allocation reason.
These tests pass for any rejection. Assert that
reasoncontainscommon.AllocatedCardsInsufficientRequest. This verifies the scheduler-facing failure contract from the PR objective.Also applies to: 72-77
🤖 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/ascend/device_910c_pairing_test.go` around lines 39 - 44, Update the assertions in the Ascend 910C pairing test around dev.Fit to require that reason contains common.AllocatedCardsInsufficientRequest, while preserving the existing fit=false and allocation checks. Apply the same assertion to the additional case noted in the review.
🤖 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/ascend/device_910c_pairing_test.go`:
- Line 1: Add the repository-standard Apache 2.0 license header at the beginning
of device_910c_pairing_test.go, before the package ascend declaration, matching
the format used by other Go files.
- Around line 3-9: Reorder the imports in the device_910c_pairing_test.go import
block so the standard-library testing import remains first, the Kubernetes
imports follow as external dependencies, and the
github.com/Project-HAMi/HAMi/pkg/device import appears last.
In `@pkg/device/ascend/device.go`:
- Around line 579-581: Update the insufficient-allocation branch containing
reason[common.AllocatedCardsInsufficientRequest] to store len(combination)
instead of len(tmpDevs), so the reason reports the selected NPU count. Pass
originReq to common.GenReason for this allocation-specific failure path while
preserving the existing log and return behavior.
- Around line 569-575: Update the Ascend 910C allocation flow around
computeBestCombination910C so pair validation runs for eligible multi-device
requests regardless of whether NetworkID is present or needTopology is enabled.
Ensure requests such as indices 0 and 2 cannot bypass full-pair validation,
while preserving existing behavior for other device types and request sizes. Add
a regression case with CustomInfo["NetworkID"] absent.
---
Nitpick comments:
In `@pkg/device/ascend/device_910c_pairing_test.go`:
- Around line 39-44: Update the assertions in the Ascend 910C pairing test
around dev.Fit to require that reason contains
common.AllocatedCardsInsufficientRequest, while preserving the existing
fit=false and allocation checks. Apply the same assertion to the additional case
noted in the review.
🪄 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: 2939ae09-b104-44fb-9680-5052ed6f1691
📒 Files selected for processing (2)
pkg/device/ascend/device.gopkg/device/ascend/device_910c_pairing_test.go
…for Ascend 910C Signed-off-by: Prabal Poddar <prabalpoddar73@gmail.com>
… fix reason reporting Always run computeBestCombination910C for multi-device Ascend 910C requests even when NetworkID is absent or needTopology is false. Update AllocatedCardsInsufficientRequest reason to report the selected NPU count and pass originReq to GenReason. Signed-off-by: Prabal Poddar <prabalpoddar73@gmail.com>
6ac01b8 to
7516c82
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/device/ascend/device_910c_pairing_test.go (1)
56-61: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the required rejection reason.
These tests only assert
fit == false. They can pass ifFitrejects for an unrelated reason. Assert thatreasonreportscommon.AllocatedCardsInsufficientRequestin each incomplete-pair case.Also applies to: 89-94, 149-154
🤖 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/ascend/device_910c_pairing_test.go` around lines 56 - 61, Update the rejection assertions in the incomplete-pair cases around dev.Fit, including the sections at the current and referenced locations, to verify that reason equals common.AllocatedCardsInsufficientRequest in addition to asserting fit is false. Preserve the existing allocation-count checks and apply the same expected reason to every incomplete-pair scenario.
🤖 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/ascend/device_910c_pairing_test.go`:
- Around line 56-61: Update the rejection assertions in the incomplete-pair
cases around dev.Fit, including the sections at the current and referenced
locations, to verify that reason equals common.AllocatedCardsInsufficientRequest
in addition to asserting fit is false. Preserve the existing allocation-count
checks and apply the same expected reason to every incomplete-pair scenario.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1be9858e-3bf2-4e2a-b576-4937b3d34beb
📒 Files selected for processing (2)
pkg/device/ascend/device.gopkg/device/ascend/device_910c_pairing_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/device/ascend/device.go
There was a problem hiding this comment.
heads up, #2035 refactors the same Fit block and its author owns the ascend area
| // NetworkID is absent or needTopology is false, because | ||
| // candidates satisfying the count alone may still be spread | ||
| // across incomplete/partial modules rather than full pairs. | ||
| combination := npu.computeBestCombination910C(nodeInfo, int(originReq), tmpDevs[k.Type]) |
There was a problem hiding this comment.
forcing the pairing on every 910c multi request changes behavior for nodes w/o module info in CustomInfo, does computeBestCombination910C return empty there and make those pods unschedulable? pls cover that case w/ a test
There was a problem hiding this comment.
Added a test for exactly this - TestComputeBestCombination910C_NoFullPairsReturnsEmpty. Checks both computeBestCombination910C directly (returns an empty slice, no panic, for 4 singletons on 4 separate modules with no NetworkID) and the end-to-end Fit() path (fit=false, clean rejection).
from what I could see in the current device plugin source NetworkID is always set to 0 for 910C, so this exact no-NetworkID scenario may be rare in practice but wanted the test in regardless, since Fit() shouldn't depend on that.
There was a problem hiding this comment.
Added a test for exactly this - TestComputeBestCombination910C_NoFullPairsReturnsEmpty. Checks both computeBestCombination910C directly (returns an empty slice, no panic, for 4 singletons on 4 separate modules with no NetworkID) and the end-to-end Fit() path (fit=false, clean rejection).
from what I could see in the current device plugin source NetworkID is always set to 0 for 910C, so this exact no-NetworkID scenario may be rare in practice but wanted the test in regardless, since Fit() shouldn't depend on that.
You should already know the rule from your previous PRs
a reminder for the future:
Reminder: Answers must be written by human being. You can view the relevant rule here.
https://github.com/Project-HAMi/HAMi/blob/master/CONTRIBUTING.md#contribution-gates
"4. Review replies. The reply you post must be written by you and must address the specific point raised. Verbatim or canned AI replies, or replies that do not engage the comment, lead to the PR being closed."
There was a problem hiding this comment.
apologies for not addressing it directly by me...pls I would appreciate if you could reopen the PR..Also thanks for the heads up on #2035..as i checked it doesn't touch the needTopology branch or 910C pairing block this pr modifies..they can merge with no conflicts..will rebase if it changes
Verify that computeBestCombination910C returns empty slice and Fit returns fit=false when no candidate NPUs share a full module and NetworkID is absent. Signed-off-by: Prabal Poddar <prabalpoddar73@gmail.com>
2 participants
What type of PR is this?
/kind bug
What this PR does / why we need it:
Fixes Ascend 910C NPU device allocation in pkg/device/ascend/device.go.
In Fit(), verify that len(combination) == int(originReq) after calling computeBestCombination910C(). If enough complete module pairs cannot be formed, return false with AllocatedCardsInsufficientRequest instead of reporting success with a partial allocation.
Run computeBestCombination910C() for all Ascend910CType multi-device requests so the module-pair validation is not skipped when the raw candidate count already matches the requested count.
Change the sorting in computeBestCombination910C() to descending order so full cards are considered first, matching the intended behavior.
Add regression tests in pkg/device/ascend/device_910c_pairing_test.go covering partial allocation, the exact-count bypass, and a valid full-pair allocation.
Which issue(s) this PR fixes: Fixes #2268
Special notes for your reviewer: Tested with go test -v ./pkg/device/ascend/....
Does this PR introduce a user-facing change? No
AI assistance: Used Claude during development. Verified and tested the final changes.
Summary by CodeRabbit
Bug Fixes
Tests