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! |
|
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 random permute APIs now accept an optional seed and use it to drive deterministic permutation generation. Callers forward the seed through the detail implementation, regression shuffling uses explicit engines, and tests cover same-seed determinism. ChangesSeeded deterministic permutation
Estimated code review effort: 2 (Simple) | ~12 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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
| std::mt19937_64 gen(seed.has_value() ? *seed | ||
| : rand()); // use the given seed, else a random one |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Is C rand() guaranteed thread-safe by the C++ standard?
💡 Result:
No, the C++ standard does not guarantee that rand is thread-safe [1][2]. It is implementation-defined whether a call to rand introduces data races [2]. Because rand typically relies on hidden global state (a seed) to generate its sequence, concurrent calls from multiple threads can result in data races if the implementation does not provide internal synchronization [3][4]. Developers should not assume thread safety and are generally advised to use the C++11 library facilities instead [1][2]. If is used, PRNG engines and distributions are not inherently thread-safe and should be managed (for example, by using thread_local storage) to avoid race conditions [5][6].
Citations:
- 1: https://en.cppreference.com/cpp/numeric/random/rand
- 2: https://eel.is/c++draft/c.math.rand
- 3: https://pubs.opengroup.org/onlinepubs/9799919799/functions/rand_r.html
- 4: https://pubs.opengroup.org/onlinepubs/9690949599/functions/rand_r.html
- 5: https://stackoverflow.com/questions/8813592/c11-thread-safety-of-random-number-generators
- 6: https://stackoverflow.com/questions/60450514/safe-random-number-generation-using-rand-or-stdrandom-device-in-c
🏁 Script executed:
ast-grep outline cpp/include/raft/random/permute.cuh --view expanded
sed -n '100,230p' cpp/include/raft/random/permute.cuh | cat -nRepository: NVIDIA/raft
Length of output: 5648
Avoid rand() for the fallback seed. The unseeded path still uses shared global state, so concurrent permute calls can race and produce cross-call variability. Use a per-call thread-safe source instead, and apply the same fix to both overloads.
🤖 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 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.
Source: Path instructions
…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 |
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