fix(device): use len(devices) for GenReason in kunlun and awsneuron - #2292
Conversation
Align Fit failure reason denominators with other backends so reports reflect candidate card count rather than the number of reason keys. Signed-off-by: blackdragoon26 <sankalp.jha9643@gmail.com>
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAWS Neuron and Kunlun allocation failure reasons now use the total device count as the denominator. Tests cover NUMA-incompatible allocation and exact conflict reasons. ChangesDevice allocation failure reasons
Estimated code review effort: 2 (Simple) | ~10 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 |
Codecov Report✅ All modified and coverable lines are covered by tests.
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 8 files with indirect coverage changes 🚀 New features to boost your workflow:
|
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/device/awsneuron/device_test.go`:
- Around line 863-889: Update the “fit fail: NumaNotFit with multiple devices”
fixture so its devices have distinct DeviceUsage.Numa values rather than relying
on Index, and adjust the node-type/custom info and request fields to select a
graphSelect path that reaches NUMA-aware allocation for Nums == 2. Preserve the
expected 1/2 NumaNotFit result while ensuring the test genuinely exercises NUMA
incompatibility.
🪄 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: 33b81add-9c2c-432b-8d6c-621bd2e18e02
📒 Files selected for processing (5)
pkg/device/awsneuron/device.gopkg/device/awsneuron/device_test.gopkg/device/kunlun/device.gopkg/device/kunlun/device_test.gopkg/device/kunlun/vdevice.go
Signed-off-by: blackdragoon26 <sankalp.jha9643@gmail.com>
|
@CodeRabbit review |
✅ Action performedReview finished.
|
There is a limited number of tokens available for coderrabbit every day. |
|
Tip For best results, initiate chat on the files or code changes.
✏️ Learnings added
You are interacting with an AI system. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: archlitchi, blackdragoon26 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 |
Resolve conflict in pkg/device/awsneuron/device_test.go by keeping both the CardNotHealth test case (ours) and the NumaNotFit with multiple devices test case (theirs from Project-HAMi#2292). Apply makeAWSDeviceUsage helper to the merged NumaNotFit case for consistency with the refactor. Signed-off-by: Eshiv Pandey <eshivpandey18@gmail.com>
What type of PR is this?
/kind bug
What this PR does / why we need it:
Kunlun XPU, Kunlun VXPU, and one AWS Neuron Fit failure path called
common.GenReason(reason, len(reason)), so the denominator was the number of reason keys (usually1) instead of candidate devices. That produced strings like1/1 NumaNotFitinstead of1/8 NumaNotFit.Other backends already use
len(devices)after #2238; the AWS Neuron single-device fail path in the same file already did too. This PR updates the three remaining call sites and adds unit coverage.Which issue(s) this PR fixes:
Fixes #2291
Special notes for your reviewer:
go test ./pkg/device/kunlun/ ./pkg/device/awsneuron/ -count=1 -shortandmake verify.Does this PR introduce a user-facing change?:
No
AI Assistance Disclosure: Used Cursor to help locate matching call sites and draft the initial test cases. I reviewed the change against the issue, ran the unit tests and
make verifymyself, and confirmed the reason-string behaviour before opening this PR.Summary by CodeRabbit
Bug Fixes
Tests