Permute overhaul#3079
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe permutation implementation in raft::random::permute is changed from a random a/b-based index shuffle to a deterministic keyed Feistel-network permutation. A uint64_t key parameter is added throughout the public and detail API overloads, propagated into make_regression's shuffle logic (using two derived keys), and updated in benchmarks and tests to pass explicit seeds. ChangesKeyed permutation implementation and propagation
Estimated code review effort: 3 (Moderate) | ~30 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
cpp/include/raft/random/permute.cuh (1)
91-96: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftHIGH: Preserve source compatibility or add deprecation coverage for the new key parameter.
Issue: Adding required
uint64_t keyparameters to public overloads breaks existing callers that use the previous signatures.
Why: This is a public API change incpp/include/raft/random/permute.cuh; consider a deprecated compatibility overload or a defaulted key while callers migrate.Suggested compatibility pattern
void permute(raft::resources const& handle, raft::device_matrix_view<const InputOutputValueType, IdxType, Layout> in, std::optional<raft::device_vector_view<IntType, IdxType>> permsOut, std::optional<raft::device_matrix_view<InputOutputValueType, IdxType, Layout>> out, uint64_t key)// Add a deprecated overload with the old signature that forwards to a documented default key.As per path instructions, “For public headers under cpp/include/raft, also require Doxygen on new APIs and deprecation warnings on breaking changes.”
Also applies to: 193-203
🤖 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 `@cpp/include/raft/random/permute.cuh` around lines 91 - 96, The new required key parameter on the public permute overloads breaks existing callers, so preserve source compatibility in permute by adding a deprecated compatibility overload with the old signature that forwards to a documented default key, or otherwise default the new parameter where appropriate. Update the affected permute declarations/definitions in raft::random::permute to include Doxygen for the new API and emit deprecation warnings on the old overloads so downstream code can migrate without a hard break.Source: Path instructions
cpp/include/raft/random/detail/permute.cuh (1)
243-265: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winCRITICAL: Avoid launching permute kernels with zero blocks.
Issue:
N == 0makesnblks == 0, but the row-major and column-major paths still callpermuteImpl, which launches<<<0, TPB, 0, stream>>>.
Why: CUDA treats zero-block launches as invalid configuration, so empty inputs fail instead of behaving as an empty permutation.Suggested fix
{ + if (N == 0) { return; } + auto nblks = raft::ceildiv(N, (IntType)TPB);As per coding guidelines, “Do not launch kernels with zero blocks/threads or invalid grid/block dimensions.”
🤖 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 `@cpp/include/raft/random/detail/permute.cuh` around lines 243 - 265, Guard the permute launch path in permute.cuh so empty inputs do not reach permute_impl_t::permuteImpl with a zero grid size. In the host-side setup where nblks is computed and the rowMajor/column-major branches call permuteImpl, add an early return or equivalent no-op when N == 0 (or nblks == 0) before building/using feistel_permute_params fp and launching the kernel. Keep the existing permuteImpl specializations unchanged; the fix should only prevent <<<0, TPB, 0, stream>>> launches from the permute wrapper.Source: Coding guidelines
cpp/tests/random/permute.cu (1)
340-341: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd small-N permutation cases back to the test matrix. Every current
PermInputsentry starts atN=32, so theN <= 1identity path and the small non-power-of-two cycle-walk cases in the keyed Feistel permute aren’t exercised.🤖 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 `@cpp/tests/random/permute.cu` around lines 340 - 341, The PermMdspanTestD test matrix currently only instantiates cases from inputsd starting at N=32, so it misses the N<=1 identity path and the small non-power-of-two cycle-walk behavior in the keyed Feistel permute. Update the test setup around INSTANTIATE_TEST_CASE_P and the PermInputs/inputsd definitions to add back small-N permutation cases, including N=0/1 and a few small non-power-of-two sizes, so the existing permute coverage exercises those code paths.
🤖 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 `@cpp/include/raft/random/detail/permute.cuh`:
- Around line 78-83: Guard the Feistel width selection loop in the permutation
setup so it cannot overflow `uint64_t` when computing the next power of two for
large N. In the logic that updates pow2 and n, add a termination/saturation
check before shifting so the loop exits or clamps once pow2 would exceed the
representable range, and make sure any invalid or negative N values are rejected
or handled before entering this path. Keep the bijection/cycle-walking behavior
intact for supported N by preserving the current width computation semantics in
the permutation routine.
---
Outside diff comments:
In `@cpp/include/raft/random/detail/permute.cuh`:
- Around line 243-265: Guard the permute launch path in permute.cuh so empty
inputs do not reach permute_impl_t::permuteImpl with a zero grid size. In the
host-side setup where nblks is computed and the rowMajor/column-major branches
call permuteImpl, add an early return or equivalent no-op when N == 0 (or nblks
== 0) before building/using feistel_permute_params fp and launching the kernel.
Keep the existing permuteImpl specializations unchanged; the fix should only
prevent <<<0, TPB, 0, stream>>> launches from the permute wrapper.
In `@cpp/include/raft/random/permute.cuh`:
- Around line 91-96: The new required key parameter on the public permute
overloads breaks existing callers, so preserve source compatibility in permute
by adding a deprecated compatibility overload with the old signature that
forwards to a documented default key, or otherwise default the new parameter
where appropriate. Update the affected permute declarations/definitions in
raft::random::permute to include Doxygen for the new API and emit deprecation
warnings on the old overloads so downstream code can migrate without a hard
break.
In `@cpp/tests/random/permute.cu`:
- Around line 340-341: The PermMdspanTestD test matrix currently only
instantiates cases from inputsd starting at N=32, so it misses the N<=1 identity
path and the small non-power-of-two cycle-walk behavior in the keyed Feistel
permute. Update the test setup around INSTANTIATE_TEST_CASE_P and the
PermInputs/inputsd definitions to add back small-N permutation cases, including
N=0/1 and a few small non-power-of-two sizes, so the existing permute coverage
exercises those code paths.
🪄 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: 94b3576c-a4cd-4edd-8180-45105614b8bc
📒 Files selected for processing (5)
cpp/bench/prims/random/permute.cucpp/include/raft/random/detail/make_regression.cuhcpp/include/raft/random/detail/permute.cuhcpp/include/raft/random/permute.cuhcpp/tests/random/permute.cu
| int n = 0; | ||
| uint64_t pow2 = 1; | ||
| while (pow2 < N) { | ||
| pow2 <<= 1; | ||
| ++n; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
CRITICAL: Guard the Feistel width loop against uint64_t overflow.
Issue: For N > 2^63, pow2 <<= 1 overflows to 0, so while (pow2 < N) never terminates.
Why: A 64-bit IdxType or a negative signed N cast at Line 247 can hang the host before any CUDA error is reported.
Suggested fix
int n = 0;
uint64_t pow2 = 1;
while (pow2 < N) {
+ if (n == 63) {
+ n = 64;
+ break;
+ }
pow2 <<= 1;
++n;
}As per path instructions, “Algorithm edge cases: keyed Feistel permutation + cycle-walking must still form a bijection over [0, N) for all supported N.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| int n = 0; | |
| uint64_t pow2 = 1; | |
| while (pow2 < N) { | |
| pow2 <<= 1; | |
| ++n; | |
| } | |
| int n = 0; | |
| uint64_t pow2 = 1; | |
| while (pow2 < N) { | |
| if (n == 63) { | |
| n = 64; | |
| break; | |
| } | |
| pow2 <<= 1; | |
| +n; | |
| } |
🤖 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 `@cpp/include/raft/random/detail/permute.cuh` around lines 78 - 83, Guard the
Feistel width selection loop in the permutation setup so it cannot overflow
`uint64_t` when computing the next power of two for large N. In the logic that
updates pow2 and n, add a termination/saturation check before shifting so the
loop exits or clamps once pow2 would exceed the representable range, and make
sure any invalid or negative N values are rejected or handled before entering
this path. Keep the bijection/cycle-walking behavior intact for supported N by
preserving the current width computation semantics in the permutation routine.
Source: Path instructions
1 participant
To be updated.