Fix KNN neighbor buffer layout bugs - #8393
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe changes normalize callable KNN weights to C-contiguous ChangesNeighbors layout fixes
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
viclafargue
left a comment
viclafargue
left a comment
There was a problem hiding this comment.
LGTM for the most part, but I have one 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 `@python/cuml/cuml/neighbors/weights.py`:
- Around line 59-61: Update the callable-weights conversion in weights.py to use
an unconditional allocating CuPy conversion, such as cp.array with copy=True,
while preserving float32 dtype and C order so the resulting pointer is aligned
for kneighbors_regressor.pyx. Add coverage using an unaligned C-contiguous
device-array result from the callable and verify the conversion produces a safe
aligned allocation.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e206c94b-e742-476f-ad69-1d3114bec701
📒 Files selected for processing (2)
python/cuml/cuml/neighbors/weights.pypython/cuml/tests/test_kneighbors_regressor.py
💤 Files with no reviewable changes (1)
- python/cuml/tests/test_kneighbors_regressor.py
|
Pushed the simplification and merged latest upstream main. The whole alignment-handling block is replaced by the one-liner as suggested, and the regression tests pass unchanged since they only assert behavior. |
There was a problem hiding this comment.
Thanks! Please update the PR description to remove the claim that unaligned weights are handled. Otherwise, this looks good to me.
It looks like cuPy can successfully convert and reorder unaligned memory. However, if the type and orders are correct the unaligned pointer is preserved throughout.
|
Will do & Happy to help :) |
|
/ok to test d1c8ef1 |
|
/ok to test 6607a3e |
|
/ok to test 565dcf3 |
|
@nethum529 Please update the PR description to reflect the current changes. |
I believe the request was addressed.
I've edited the description. |
565dcf3 to
d7f7065
Compare
|
I accidentally pushed the wrong commit which closed this PR. I'll create a replacement, very sorry about that. |
## Summary - Converts callable weights to C-contiguous `float32` CuPy arrays before the KNN kernel reads them. - Converts sparse self-edge indices to contiguous `int64` before the raw kernel while keeping explicit sparse query results as `int32`. - Adds regressions for non-C-contiguous callable weights and duplicate sparse rows. ## Testing - `14 passed` for the KNN weights test selection. - Changed-file pre-commit checks pass. - Full CI passes. Supersedes #8393. Fixes #8362 Authors: - Simon Adorf (https://github.com/csadorf) - Nethum Weerasinghe (https://github.com/nethum529) Approvers: - Victor Lafargue (https://github.com/viclafargue) URL: #8619
6 participants
Summary
float32CuPy arrays before the KNN kernel reads them.int64before the raw kernel while keeping explicit sparse query results asint32.Testing
14 passedfor the KNN weights test selection.Fixes #8362