Make permute deterministic given a seed#3075
Conversation
permute() drew its affine permutation coefficients from the unseeded global rand(), so make_regression produced different output on repeated calls with the same random_state (cuML issue #7871). Derive the coefficients from a local std::mt19937_64 seeded per call, thread the seed through make_regression's row and feature shuffles, and add a determinism test. Public wrappers default the seed so existing callers are unaffected.
|
This is a non-breaking bug fix (the new permute seed parameter is defaulted, so the public API stays backward-compatible). Could a maintainer add the bug and non-breaking labels? Thanks! |
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe random permute APIs now accept optional seeds and use standard RNG engines for deterministic permutation generation. Regression shuffling shares an engine across permutations, and parameterized tests validate reproducibility and seed variation. ChangesSeeded deterministic permutation
Estimated code review effort: 3 (Moderate) | ~20 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.
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)
139-165: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winConvenience
permuteoverload doesn't forwardseed.This overload (used when passing
std::nulloptforpermsOut/out) never accepts or forwards aseed, so any caller through this entry point is silently locked toseed = 0and cannot opt into deterministic seeding, unlike the other two updated overloads.🐛 Proposed fix
template <typename InputOutputValueType, typename IdxType, typename Layout, typename PermsOutType, typename OutType> void permute(raft::resources const& handle, raft::device_matrix_view<const InputOutputValueType, IdxType, Layout> in, PermsOutType&& permsOut, - OutType&& out) + OutType&& out, + uint64_t seed = 0ULL) { ... std::optional<perms_out_view_type> permsOut_arg = std::forward<PermsOutType>(permsOut); std::optional<out_view_type> out_arg = std::forward<OutType>(out); - permute(handle, in, permsOut_arg, out_arg); + permute(handle, in, permsOut_arg, out_arg, seed); }🤖 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 139 - 165, The convenience permute overload in permute.cuh is missing seed support, so callers using the std::nullopt path are forced to the default seed. Update this template overload of permute to accept a seed parameter and forward it into the inner permute call, matching the behavior of the other permute overloads; use the permute function signature and the permsOut/out forwarding logic to locate the change.
🧹 Nitpick comments (2)
cpp/include/raft/random/permute.cuh (1)
57-94: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDoxygen not updated for new
seedparameter.Both public overloads gained a
seedparam (lines 94 and 196) but their doc comments (lines 57-88 and 169-187) were not updated with an@param[in] seedentry describing default/determinism semantics.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: 169-199
🤖 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 57 - 94, Update the Doxygen for the public permute overloads so the new seed argument is documented in both declarations of permute. Add an `@param`[in] seed entry alongside the existing handle/in/permsOut/out docs, and describe its default behavior and how it affects determinism so the public API comment stays in sync with the signature.Source: Path instructions
cpp/tests/random/permute.cu (1)
345-411: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a
test_data_typealias instead of hardcodingfloatin the TEST_P body.
PermSeedTest<T>::SetUp()correctly usesTfor thepermute<T, int, int>(...)calls (lines 371-374), but theTEST_P(PermSeedTestF, SameSeedIsDeterministic)body hardcodespermute<float, int, int>(...)(line 401) instead of following theusing test_data_type = T;pattern already used by sibling fixturesPermTest/PermMdspanTest(lines 37, 85). This works today only becausePermSeedTestis solely instantiated forfloat; extending coverage todouble(matching the existingPermTestD/PermMdspanTestDsymmetry) would require copy-pasting this class and updating the hardcoded type, which is easy to overlook until it fails to compile.♻️ Proposed refactor
template <typename T> class PermSeedTest : public ::testing::TestWithParam<PermInputs<T>> { + public: + using test_data_type = T; + protected: ... }; using PermSeedTestF = PermSeedTest<float>; TEST_P(PermSeedTestF, SameSeedIsDeterministic) { + using test_data_type = PermSeedTestF::test_data_type; auto stream = resource::get_cuda_stream(handle); ... if (N >= 1024) { rmm::device_uvector<int> perms3(N, stream); - permute<float, int, int>( + permute<test_data_type, int, int>( perms3.data(), out.data(), in.data(), params.D, N, params.rowMajor, stream, params.seed + 1);🤖 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 345 - 411, Replace the hardcoded float in the PermSeedTest fixture with a test_data_type alias, matching the pattern used by PermTest and PermMdspanTest, and use that alias in the TEST_P body. Update PermSeedTest<T> to define test_data_type = T, then change the permute<float, int, int>(...) call in SameSeedIsDeterministic to permute<test_data_type, int, int>(...) so the fixture stays generic and can be reused for double without copy-pasting.
🤖 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.
Outside diff comments:
In `@cpp/include/raft/random/permute.cuh`:
- Around line 139-165: The convenience permute overload in permute.cuh is
missing seed support, so callers using the std::nullopt path are forced to the
default seed. Update this template overload of permute to accept a seed
parameter and forward it into the inner permute call, matching the behavior of
the other permute overloads; use the permute function signature and the
permsOut/out forwarding logic to locate the change.
---
Nitpick comments:
In `@cpp/include/raft/random/permute.cuh`:
- Around line 57-94: Update the Doxygen for the public permute overloads so the
new seed argument is documented in both declarations of permute. Add an
`@param`[in] seed entry alongside the existing handle/in/permsOut/out docs, and
describe its default behavior and how it affects determinism so the public API
comment stays in sync with the signature.
In `@cpp/tests/random/permute.cu`:
- Around line 345-411: Replace the hardcoded float in the PermSeedTest fixture
with a test_data_type alias, matching the pattern used by PermTest and
PermMdspanTest, and use that alias in the TEST_P body. Update PermSeedTest<T> to
define test_data_type = T, then change the permute<float, int, int>(...) call in
SameSeedIsDeterministic to permute<test_data_type, int, int>(...) so the fixture
stays generic and can be reused for double without copy-pasting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 870a805f-b734-4504-92c6-79a4fe214946
📒 Files selected for processing (4)
cpp/include/raft/random/detail/make_regression.cuhcpp/include/raft/random/detail/permute.cuhcpp/include/raft/random/permute.cuhcpp/tests/random/permute.cu
|
cc @vinaydes |
achirkin
left a comment
achirkin
left a comment
There was a problem hiding this comment.
Thanks for the PR! It's a clear improvement indeed, yet I have couple minor suggestions.
| n_cols, | ||
| false, | ||
| stream, | ||
| seed + 1); // different derived seed for feature shuffle keeps sample and feature independent |
There was a problem hiding this comment.
Maybe it would be a bit cleaner to pass a single generator by reference to the two functions here in place of the integer seed?
There was a problem hiding this comment.
Thanks for the suggestion! I'll build one std::mt19937_64 in make_regression_caller and pass it by reference to both permute calls instead of deriving seed/seed + 1.
| std::optional<raft::device_vector_view<IntType, IdxType>> permsOut, | ||
| std::optional<raft::device_matrix_view<InputOutputValueType, IdxType, Layout>> out) | ||
| std::optional<raft::device_matrix_view<InputOutputValueType, IdxType, Layout>> out, | ||
| uint64_t seed = 0ULL) |
There was a problem hiding this comment.
I think, this is a "slightly breaking" change: the previous behavior was non-deterministic and the new behavior is deterministic. Shall we pass the seed as std::optional<> instead (uint64_t or an optional generator type by value) and default to rand() producing the seed non-deterministically?
There was a problem hiding this comment.
I'll change it to std::optional<uint64_t> seed = std::nullopt, falling back to rand() when unset so existing callers keep their current non-deterministic behavior. Will push an update soon.
achirkin
added
enhancement
…llback. Corrected function parameters from seed to gen.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cpp/include/raft/random/permute.cuh (1)
90-95: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the new
seedparameter (Doxygen).The public mdspan overload gained
std::optional<uint64_t> seed, but the Doxygen block (Lines 58-89) has no@param[in] seed. Public headers require Doxygen for new API surface. The legacy overload (Lines 173-200) is missing it too.As per path instructions: "require Doxygen on new APIs and deprecation warnings on breaking changes."
🤖 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 90 - 95, The public permute API in permute.cuh now accepts an optional seed, but the Doxygen for the mdspan overload and the legacy overload still omits documentation for this new parameter. Update the comments above the permute overloads to add an `@param`[in] entry for seed, using the same symbol names (permute, seed) so the new API surface is fully documented in the public header.Source: Path instructions
🤖 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/permute.cuh`:
- Around line 125-126: The fallback seed in the permute initialization currently
relies on rand(), which uses shared global state and can race across concurrent
calls. Update the seed generation in the permute path to use a per-call
thread-safe random source instead of rand(), and make sure the same change is
applied consistently in both permute overloads so the unseeded behavior is
stable and thread-safe. Use the permute function and its seed initialization
block as the place to fix this.
---
Nitpick comments:
In `@cpp/include/raft/random/permute.cuh`:
- Around line 90-95: The public permute API in permute.cuh now accepts an
optional seed, but the Doxygen for the mdspan overload and the legacy overload
still omits documentation for this new parameter. Update the comments above the
permute overloads to add an `@param`[in] entry for seed, using the same symbol
names (permute, seed) so the new API surface is fully documented in the public
header.
🪄 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: 941e2100-285a-4a03-807c-84ca8411fd79
📒 Files selected for processing (3)
cpp/include/raft/random/detail/make_regression.cuhcpp/include/raft/random/detail/permute.cuhcpp/include/raft/random/permute.cuh
…d, add Doxygen, use test_data_type alias
|
Pushed an update addressing the review feedback:
|
|
Thanks @viclafargue for tagging me. Thanks @says1117 for this PR. I also have been working on overhauling the RAFT permute. Unfortunately, the affine transformation ( 
I used Feistel network based hashing to get near perfect avalanche. See more in my PR #3079 |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cpp/tests/random/permute.cu (1)
400-411: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winHIGH: Avoid the probabilistic different-seed assertion.
ASSERT_NE(h1, h3)can fail when distinct seeds produce the same affine permutation; forN == 1024, there are onlyN * φ(N) = 524,288possible outputs. Have you considered using a deterministic golden-output check or dropping this assertion while retaining same-seed reproducibility and permutation-validity checks?🤖 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 400 - 411, Replace the probabilistic ASSERT_NE(h1, h3) check in the N >= 1024 branch with a deterministic golden-output assertion, or remove the different-seed comparison entirely. Retain the existing same-seed reproducibility and permutation-validity checks, and remove any now-unused perms3/h3 setup if the assertion is dropped.Source: Path instructions
🤖 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.
Outside diff comments:
In `@cpp/tests/random/permute.cu`:
- Around line 400-411: Replace the probabilistic ASSERT_NE(h1, h3) check in the
N >= 1024 branch with a deterministic golden-output assertion, or remove the
different-seed comparison entirely. Retain the existing same-seed
reproducibility and permutation-validity checks, and remove any now-unused
perms3/h3 setup if the assertion is dropped.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 399ac9b7-212d-4366-8611-0c73c3b52f16
📒 Files selected for processing (4)
cpp/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 (2)
- cpp/include/raft/random/detail/permute.cuh
- cpp/include/raft/random/detail/make_regression.cuh
|
Thanks for the detailed writeup and the heatmap @vinaydes, that's a much deeper fix than what I was going for here. This PR was purely aimed at closing the determinism gap flagged in rapidsai/cuml#7871 (same seed to same shuffle), not at fixing the affine transform's statistical quality, so it looks like #3079 supersedes what I'm doing here once it lands. Happy to either: (a) keep this as a narrow, low-risk fix to unblock cuml now while #3079 is still in progress, or (b) close this in favor of #3079 if you'd rather consolidate. No strong preference either way. Let me know which you'd prefer. |
4 participants
Summary
raft::random::permutedrew the coefficients of its affine permutationfrom the unseeded global C
rand(), so results were not reproducibleacross calls. This surfaced in cuML as rapidsai/cuml#7871:
make_regression(..., shuffle=True)returned different data on repeatedcalls with the same
random_state.Changes
seedparameter topermutethat derives the permutationcoefficients from a local
std::mt19937_64, replacing the globalrand().make_regressionseed through both the sample andfeature shuffles (a distinct derived seed each, so the two shuffles stay
independent).
permuteoverloads defaultseed = 0, so existing callersare unaffected.
PermSeedTestverifying the same seed yields identical permutationsand different seeds yield different ones.
Testing
All 667 tests in
RANDOM_TESTpass locally (RTX 2060, CUDA arch 75).Relates to rapidsai/cuml#7871