fix(awsneuron): allocate by device index, not slice position - #2960
fix(awsneuron): allocate by device index, not slice position#2960mesutoezdil wants to merge 2 commits into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mesutoezdil 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 |
|
/kind bug |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughChangesNeuron device selection
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This localized fix makes AWS Neuron allocation use physical device indexes, preventing reordered device lists from selecting an already-busy device. No actionable merge-blocking risk remains after normal checks and review. Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
continuousDeviceAvailable collected slice positions, and Fit matched those values against DeviceUsage.Index. The two agree only while the slice happens to be in index order, and it is not: score.go sorts the device list before calling Fit. On a node whose device 5 is busy, the sorted order starts [5 0 1 2 3 4 6 7 ...]. Asking for four devices returns positions [4 5 6 7], which hold the free indexes 3, 4, 6 and 7, but Fit reads them as indexes 4, 5, 6 and 7 and allocates index 5, which is already in use. Return indexes from continuousDeviceAvailable and order a copy of the slice by index in graphSelect, so adjacency is still read from the physical layout. kunlun's topology selector already does this and says why in its own comment. Sorting first also puts the device carrying the node type back at position zero, where graphSelect looks for it. Signed-off-by: mesutoezdil <mesudozdil@gmail.com>
The first round only guarded the ordering change: the shared test helper builds Index equal to slice position, so reverting the index change alone left every test passing. Add cases built on an index set that does not start at zero, where a result made of positions is distinguishable from one made of indexes, and a case where only the lowest-index device carries the node type, which is what the reordered lookup depends on. Reverting either half of the fix now fails a test. Also switch the comparator to cmp.Compare, since Index is unsigned and subtracting two converted values is a pattern worth avoiding. Signed-off-by: mesutoezdil <mesudozdil@gmail.com>
1983f62 to
06b76d8
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:
|
1 participant
continuousDeviceAvailablecollected slice positions whileFitmatched those values againstDeviceUsage.Index, and the two agree only when the slice is in index order, which it is not because the scheduler sorts devices by score first. On a node whose device 5 is busy the sorted order starts[5 0 1 2 3 4 6 7 ...], so a four device request returns positions[4 5 6 7]andFitreads them as indexes and allocates index 5, which is already in use.