[FEA] permute: replace LCG with keyed Feistel permutation, add deterministic key API#3079
[FEA] permute: replace LCG with keyed Feistel permutation, add deterministic key API#3079vinaydes wants to merge 23 commits into
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. |
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe permutation implementation changes from random a/b indexing to a deterministic keyed Feistel permutation. Explicit keys are added across the API and detail implementation, propagated to regression shuffles, and covered by updated benchmarks and tests. ChangesKeyed permutation implementation and propagation
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
8460ddf to
d77909f
Compare
|
Addressed all the @coderabbitai review comments. |
|
Tip For best results, initiate chat on the files or code changes.
|
vinaydes
changed the title
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 (1)
cpp/include/raft/random/permute.cuh (1)
206-251: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDeprecated overloads now return the same shuffle every call. These wrappers always forward
key=0, so any legacy caller that omitted a key loses per-call randomness and will get the same permutation for a givenNon every invocation. Either keep a temporary varying fallback key or make the deprecation text/docs explicitly warn that the no-key path is now deterministic.🤖 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 206 - 251, The deprecated no-key permute overloads always use key=0, making legacy calls deterministic. Update the three deprecated permute wrappers to generate or reuse a varying fallback key for each invocation, while preserving forwarding behavior and deprecation annotations.
🧹 Nitpick comments (2)
cpp/tests/random/permute.cu (1)
378-407: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNew test calls
detail::functions directly instead of the publicpermute()API.
kperm_seed_diversity_kernel/SeedDiversityreach intodetail::kperm_params,detail::make_kperm_params, anddetail::kperm_indexrather than the exportedraft::random::permuteoverloads used by the rest of this file. As per path instructions, "permutation tests should call the exported raft::random::permute overloads (not detail::permute) to match what users consume." Given the test needs to cheaply compare 16384 distinct keys, going through the full public API would likely be impractical, so this may be a reasonable, deliberate exception — but worth a short comment explaining why, so it isn't mistaken for drift from the guideline on future edits.🤖 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 378 - 407, Add a brief comment in the SeedDiversity test explaining that it intentionally uses the detail-level permutation helpers to compare 16384 distinct seeds efficiently, rather than invoking the public raft::random::permute API for every key. Leave the existing kperm_seed_diversity_kernel and SeedDiversity behavior unchanged.Source: Path instructions
cpp/include/raft/random/permute.cuh (1)
206-251: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo test coverage for the deprecated no-key
permute()overloads. The deprecated overloads inpermute.cuh(which forward withkey=0, silently dropping prior per-call randomness — see separate comment on that range) are never exercised bycpp/tests/random/permute.cu; every call site there was updated to pass an explicit key/seed, so the backward-compat forwarding path has zero regression coverage.
cpp/include/raft/random/permute.cuh#L206-L251: this is the contract that needs guarding — behavior for no-key callers changed from randomized-per-call to fixed key=0.cpp/tests/random/permute.cu#L1-L433: add at least one test (wrapped with a deprecation-warning suppression pragma if-Werroris enforced) that calls the deprecated 4-arg/7-arg overloads and asserts they still produce a valid permutation equivalent to explicitkey=0.🤖 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 206 - 251, cpp/include/raft/random/permute.cuh:206-251 requires no direct change; add regression coverage in cpp/tests/random/permute.cu:1-433 for the deprecated no-key permute overloads, suppressing deprecation warnings if required. Exercise at least the 4-argument/7-argument APIs and verify their outputs are valid permutations matching the corresponding explicit-key calls with key=0.Source: Coding guidelines
🤖 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/tests/random/permute.cu`:
- Around line 416-421: Immediately after the kperm_seed_diversity_kernel launch,
add the standard RAFT CUDA launch-error check using
RAFT_CUDA_TRY(cudaPeekAtLastError()) before copying results or synchronizing the
stream. Keep the existing raft::update_host and resource::sync_stream flow
unchanged.
---
Outside diff comments:
In `@cpp/include/raft/random/permute.cuh`:
- Around line 206-251: The deprecated no-key permute overloads always use key=0,
making legacy calls deterministic. Update the three deprecated permute wrappers
to generate or reuse a varying fallback key for each invocation, while
preserving forwarding behavior and deprecation annotations.
---
Nitpick comments:
In `@cpp/include/raft/random/permute.cuh`:
- Around line 206-251: cpp/include/raft/random/permute.cuh:206-251 requires no
direct change; add regression coverage in cpp/tests/random/permute.cu:1-433 for
the deprecated no-key permute overloads, suppressing deprecation warnings if
required. Exercise at least the 4-argument/7-argument APIs and verify their
outputs are valid permutations matching the corresponding explicit-key calls
with key=0.
In `@cpp/tests/random/permute.cu`:
- Around line 378-407: Add a brief comment in the SeedDiversity test explaining
that it intentionally uses the detail-level permutation helpers to compare 16384
distinct seeds efficiently, rather than invoking the public
raft::random::permute API for every key. Leave the existing
kperm_seed_diversity_kernel and SeedDiversity behavior unchanged.
🪄 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: b0c3dda5-4cf4-462c-ba51-23d522d91aa0
📒 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
🚧 Files skipped from review as they are similar to previous changes (1)
- cpp/include/raft/random/detail/permute.cuh
| @@ -192,7 +274,7 @@ void permute(IntType* perms, | |||
| bool rowMajor, | |||
| cudaStream_t stream) | |||
| { | |||
| detail::permute<Type, IntType, IdxType, TPB>(perms, out, in, D, N, rowMajor, stream); | |||
| detail::permute<Type, IntType, IdxType, TPB>(perms, out, in, D, N, rowMajor, stream, uint64_t{0}); | |||
| } | |||
There was a problem hiding this comment.
Maybe we'd just pass rand() as the explicit key to the new overload that requires it to preserve the expected behavior?
There was a problem hiding this comment.
Okay, are you suggesting something like this?
detail::permute<Type, IntType, IdxType, TPB>(perms, out, in, D, N, rowMajor, stream, uint64_t{rand()});
aamijar
added
non-breaking
3 participants
The previous permute() implementation used a linear-congruential generator (LCG) --
(a * tid + b) % N-- seeded fromrand()to map output indices to input indices. This had two significant problems:rand()is seeded by global state, so results varied between runs and between callers. There was no way to request the same permutation twice.This PR replaces the LCG with a keyed Feistel permutation built from a 4-round network whose round function is a reduced version of MurmurHash3 32-bit finalizer (
fmix32). The domain is extended to the next power of two and cycle walking is used to map results back into[0, N)without any modulo bias.API Changes
Both the legacy pointer API and the mdspan API gain a required uint64_t key parameter. The same (key, N) pair always produces the same permutation, giving full reproducibility.
Callers that only want the permutation index array (no data shuffle) can pass out = nullptr; this now dispatches to a dedicated
permsOnlyKernelwithITEMS_PER_THREAD = 8for higher throughput on large index arrays.Implementation Details
detail/permute.cuhfmix32(h): Reudced version of MurmurHash3 finalizer; used as the Feistel round function.kperm_params: host-precomputed schedule (split widthsa_bits/b_bits, per-round key prefixes). Computed once perpermute()call on the host and passed as a kernel argument, so all threads share it without extra loads.make_kperm_params(N, key): builds the key schedule.kperm_mix_nbits<WordType>(m, p): 4-round unrolled Feistel step.kperm_index<IdxType>(idx, p): cycle-walking wrapperpermsOnlyKernel: new kernel invoked whenout == nullptr; processes 8 indices per thread for better occupancy on pure index-generation workloads.detail/make_regression.cuhDerives two distinct keys from the caller-supplied seed so the sample shuffle and the feature shuffle get independent permutations:
Tests
PermTestandPermMdspanTestcases updated to passparams.seedas the keySeedDiversitytest: 16384 threads each generate a permutation keyed bybase_seed + tid + 1and count index coincidences with the reference permutation. The test asserts that the total match rate is below 5%, verifying that adjacent keys produce statistically independent permutations.Benchmarks
out = nullptr) at sizes up to 1 GiB.Avalanche Property
A permutation of a finite set is by definition a bijection: a function that is both injective (no two inputs map to the same output) and surjective (every output is hit). A keyed hash function that is itself a bijection for every key is therefore directly usable as a permutation generator: apply it to every index in [0, N) and the results are a rearrangement with no duplicates or gaps. The Feistel construction guarantees bijectivity structurally -- each round is an invertible XOR-and-mask, so the full network is invertible regardless of what the round function does internally. The quality of the resulting permutation is measured by the avalanche property: flipping any single input bit should flip each output bit with probability close to 1/2, ensuring that nearby keys produce statistically independent permutations. I evaluated the avalanche behavior of this Feistel construction across all domain widths from 2 to 32 bits and found it satisfactory throughout.