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 permute functions in RAFT's random module now accept an explicit seed parameter, replacing rand()-based coefficient generation with deterministic std::mt19937_64. The public mdspan and legacy overloads forward this seed with a default value. make_regression passes distinct seeds for sample and feature shuffles. A new test validates determinism. ChangesDeterministic seeded permutation
Estimated code review effort: 2 (Simple) | ~12 minutes Related PRs: None identified from the provided context. Suggested labels: bug, non-breaking Suggested reviewers: cjnolet, tfeher Poem: 🚥 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 |
2 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