feature: rng primitive refactoring #3040

Alexandr-Solovev · 2025-01-10T13:50:35Z

Description

Add a comprehensive description of proposed changes

List associated issue number(s) if exist(s): #6 (for example)

Documentation PR (if needed): #1340 (for example)

Benchmarks PR (if needed): IntelPython/scikit-learn_bench#155 (for example)

PR should start as a draft, then move to ready for review state after CI is passed and all applicable checkboxes are closed.
This approach ensures that reviewers don't spend extra time asking for regular requirements.

You can remove a checkbox as not applicable only if it doesn't relate to this PR in any way.
For example, PR with docs update doesn't require checkboxes for performance while PR with any change in actual code should have checkboxes and justify how this code change is expected to affect performance (or justification should be self-evident).

Checklist to comply with before moving PR from draft:

PR completeness and readability

I have reviewed my changes thoroughly before submitting this pull request.
I have commented my code, particularly in hard-to-understand areas.
I have updated the documentation to reflect the changes or created a separate PR with update and provided its number in the description, if necessary.
Git commit message contains an appropriate signed-off-by string (see CONTRIBUTING.md for details).
I have added a respective label(s) to PR if I have a permission for that.
I have resolved any merge conflicts that might occur with the base branch.

Testing

I have run it locally and tested the changes extensively.
All CI jobs are green or I have provided justification why they aren't.
I have extended testing suite if new functionality was introduced in this PR.

Performance

I have measured performance for affected algorithms using scikit-learn_bench and provided at least summary table with measured data, if performance change is expected.
I have provided justification why performance has changed or why changes are not expected.
I have provided justification why quality metrics have changed or why changes are not expected.
I have extended benchmarking suite and provided corresponding scikit-learn_bench PR if new measurable functionality was introduced in this PR.

Alexandr-Solovev · 2025-01-10T13:52:07Z

/intelci: run

Alexandr-Solovev · 2025-01-13T12:31:46Z

/intelci: run

Alexandr-Solovev · 2025-01-22T09:33:37Z

/intelci: run

Alexandr-Solovev · 2025-01-22T12:25:39Z

/intelci: run

Alexandr-Solovev · 2025-01-22T15:11:03Z

/intelci: run

Alexandr-Solovev · 2025-01-22T20:53:43Z

/intelci: run

Alexandr-Solovev · 2025-01-23T13:11:52Z

/intelci: run

Alexandr-Solovev · 2025-01-23T13:19:57Z

/intelci: run

cpp/daal/src/algorithms/engines/mrg32k3a/mrg32k3a_batch_container.h

cpp/daal/src/algorithms/engines/mrg32k3a/mrg32k3a_batch_impl.h

cpp/daal/src/externals/service_rng_mkl.h

cpp/daal/src/externals/service_rng_openrng.h

cpp/daal/src/externals/service_rng_ref.h

cpp/oneapi/dal/backend/primitives/rng/dpc_engine_collection.hpp

Alexandr-Solovev · 2025-01-23T15:11:42Z

/intelci: run

cpp/oneapi/dal/backend/primitives/rng/dpc_engine.hpp

Vika-F · 2025-01-24T09:16:29Z

cpp/oneapi/dal/backend/primitives/rng/dpc_engine_collection.hpp

+              seed_(seed) {
+        engines_.reserve(count_);
+        for (Size i = 0; i < count_; ++i) {
+            engines_.push_back(dpc_engine<EngineType>(queue, seed_));


This initialization looks meaningless for mt2203. Because different engine_idx should also be provided to have independent sequences of random numbers:
https://www.intel.com/content/www/us/en/docs/onemkl/developer-reference-dpcpp/2023-0/oneapi-mkl-rng-mt2203.html

cpp/oneapi/dal/backend/primitives/rng/host_engine.hpp

Vika-F · 2025-01-24T09:26:46Z

cpp/oneapi/dal/backend/primitives/rng/host_engine.hpp

+///
+/// @tparam EngineType The RNG engine type to be used. Defaults to `engine_method::mt2203`.
+///
+/// @param[in] seed  The initial seed for the random number generator. Defaults to `777`.


The came comment as for dpc_engine apply here: need to add descriptions to the methods and functions; need to fix rule of five.

cpp/oneapi/dal/backend/primitives/rng/test/rng_dpc.cpp

Alexandr-Solovev · 2025-01-28T10:31:56Z

/intelci: run

Alexandr-Solovev · 2025-01-30T14:43:33Z

/intelci: run

Alexandr-Solovev · 2025-01-30T14:43:47Z

/intelci: run

Alexandr-Solovev · 2025-01-30T17:47:40Z

/intelci: run

icfaust

This is a review just for the DAAL side of things, will do a second review for the DAL side. DAAL looks rather straightforward, and looks as though oneDAL requires the most attention.

icfaust · 2025-01-31T05:45:23Z

cpp/daal/include/algorithms/engines/mrg32k3a/mrg32k3a.h

@@ -0,0 +1,183 @@
+/* file: mrg32k3a.h */
+/*******************************************************************************
+* Copyright contributors to the oneDAL project


The files in cpp/daal/include/* seems to follow exactly the layout of the other RNG engines, so I have glanced through to make sure that they are the same, but didn't look super close. @Alexandr-Solovev let me know if there is anything specific to mrg32k3a or philox4x32x10 in these folders that are different. This implies that there isn't any architectural change to the RNG engines on the DAAL side of things outside of the inclusion of the two new ones. Is that so? (on second glance, it looks that we are doing a better standardization of the error codes, but thats it?)

in daal there are no critical changes

icfaust · 2025-01-31T05:50:01Z

cpp/daal/src/algorithms/engines/mrg32k3a/mrg32k3a.cpp

@@ -0,0 +1,60 @@
+/* file: mrg32k3a.cpp */


Overall structure in the daal/src/* also looks to match. I will focus on reviewing the batch_impl.h files.

Thanks for the review!

icfaust · 2025-01-31T05:55:31Z

cpp/daal/src/algorithms/engines/mrg32k3a/mrg32k3a_batch_impl.h

+    {
+        switch (technique)
+        {
+        case engines::internal::family: return false;


Not anything you need to change in your PR, but I find it a bit odd that all of the RNGs in DAAL define or try unused skipping methods entirely even if the technique isn't known to be implemented (i.e. false) rather than just setting the errorcode directly. Am I misunderstanding things?

icfaust · 2025-01-31T05:58:08Z

cpp/daal/src/algorithms/engines/mt2203/mt2203_kernel.h

@@ -19,8 +19,8 @@
 //  Declaration of template function that calculate mt2203s.
 //--

-#ifndef __MCG59_KERNEL_H__


This is crazy, especially for something that is so widely used in the codebase. Good catch.

cpp/oneapi/dal/backend/primitives/rng/device_engine_collection.hpp

Vika-F · 2025-01-31T09:55:00Z

cpp/oneapi/dal/backend/primitives/rng/device_engine.hpp

+    /// Constructor that initializes the mt2203 generator for use on the GPU.
+    /// @param[in] queue The SYCL queue to manage device operations.
+    /// @param[in] seed The initial seed for the generator.
+    gen_mt2203(sycl::queue queue, std::int64_t seed) : _gen(queue, seed, 0) {}


One more parameter should be added to the constructor.
Because mt2203 is a set of 6024 Mersenne Twister RNGs.

Suggested change

gen_mt2203(sycl::queue queue, std::int64_t seed) : _gen(queue, seed, 0) {}

gen_mt2203(sycl::queue queue, std::int64_t seed, std::int64_t engine_idx = 0) : _gen(queue, seed, engine_idx) {}

Vika-F · 2025-01-31T10:02:46Z

cpp/oneapi/dal/backend/primitives/rng/device_engine_collection.hpp

+        for (std::int64_t i = 0; i < count_; ++i) {
+            engines_.push_back(device_engine<EngineType>(queue, base_seed_ + i));
+        }


More correctly will be to initialize the collection with the different RNGs for mt2203:

Suggested change

for (std::int64_t i = 0; i < count_; ++i) {

engines_.push_back(device_engine<EngineType>(queue, base_seed_ + i));

}

if (EngineType == engine_type::mt2203)

{

for (std::int64_t i = 0; i < count_; ++i) {

engines_.push_back(device_engine<EngineType>(queue, base_seed_, i));

}

}

else

{

for (std::int64_t i = 0; i < count_; ++i) {

engines_.push_back(device_engine<EngineType>(queue, base_seed_ + i));

}

}

cpp/oneapi/dal/backend/primitives/rng/device_engine_dpc.cpp

Vika-F · 2025-01-31T10:07:36Z

cpp/oneapi/dal/backend/primitives/rng/host_engine.hpp

+        }
+    }
+
+    explicit host_engine(const daal::algorithms::engines::EnginePtr& eng) : host_engine_(eng) {


I do not think that this is a proper fix to rule of three. Because a copy constructor should accept the object of the same type host_engine.

cpp/oneapi/dal/backend/primitives/rng/device_engine_dpc.cpp

Alexandr-Solovev · 2025-01-31T10:41:45Z

/intelci: run

Alexandr-Solovev · 2025-02-03T09:38:42Z

/intelci: run

Alexandr-Solovev · 2025-02-03T11:01:19Z

/intelci: run

Alexandr-Solovev · 2025-02-03T14:18:46Z

/intelci: run

Alexandr-Solovev · 2025-02-03T14:19:27Z

/intelci: run

Alexandr-Solovev · 2025-02-03T15:18:15Z

correct last commit ci job
https://intel-ci.intel.com/efe23a65-55b1-f183-ad29-a4bf010d0e2d

ethanglaser

Mostly reviewed, looking good! Please add PR description. Docs are not added, so this checkbox should not be checked. But none of the rst files for other engines are >70 lines so this should be easily added in this PR.

ethanglaser · 2025-02-03T19:44:19Z

cpp/daal/src/algorithms/engines/mt2203/mt2203_kernel.h

@@ -19,8 +19,8 @@
 //  Declaration of template function that calculate mt2203s.
 //--

-#ifndef __MCG59_KERNEL_H__


ethanglaser · 2025-02-03T20:24:15Z

cpp/oneapi/dal/algo/decision_forest/backend/gpu/train_splitter_impl.hpp

@@ -19,7 +19,7 @@
 #include "oneapi/dal/table/common.hpp"
 #include "oneapi/dal/backend/primitives/ndarray.hpp"
 #include "oneapi/dal/backend/primitives/utils.hpp"
-#include "oneapi/dal/backend/primitives/rng/rng_engine_collection.hpp"
+#include "oneapi/dal/backend/primitives/rng/host_engine_collection.hpp"


is the include here even utilized?

ethanglaser · 2025-02-03T20:35:33Z

cpp/oneapi/dal/backend/primitives/rng/host_engine.hpp

+/// @param[in, out] dst Pointer to the array to be shuffled.
+/// @param[in] engine_ Reference to the device engine.
+template <typename Type>
+void partial_fisher_yates_shuffle(ndview<Type, 1>& result_array,


Just wondering, why was this combined into the host_engine file? Previously it was in its own

ethanglaser · 2025-02-03T20:46:01Z

cpp/oneapi/dal/backend/primitives/rng/device_engine_dpc.cpp

+/// @param[in] method The rng engine type. Defaults to `mt19937`.
+/// @param[in] deps Dependencies for the SYCL event.
+template <typename Type>
+sycl::event partial_fisher_yates_shuffle(sycl::queue& queue_,


Would it be possible to somehow wrap the host function to avoid code duplication? because it looks like everything aside from the engine and event logic are the exact same

ethanglaser · 2025-02-03T21:00:59Z

cpp/daal/src/algorithms/engines/mrg32k3a/mrg32k3a_batch_impl.h

+namespace internal
+{
+template <CpuType cpu, typename algorithmFPType = DAAL_ALGORITHM_FP_TYPE, Method method = defaultDense>
+class BatchImpl : public algorithms::engines::mrg32k3a::interface1::Batch<algorithmFPType, method>,


Not needed in this PR, but in general there seems to be a lot of overlap in functionality between these impls, could create a base class to define it and reduce duplication

ethanglaser · 2025-02-03T21:10:03Z

cpp/oneapi/dal/backend/primitives/rng/test/rng_dpc.cpp

+        const DataType* val_arr_2_host_ptr = arr_2_host.get_data();
+
+        for (std::int64_t el = 0; el < arr_2_host.get_count(); el++) {
+            // Due to MKL inside generates floats on GPU and doubles on CPU, it makes sense to add minor eps.


maybe eps should be a function arg? also what is the range of values for results here - 0.01 seems a bit large

ethanglaser · 2025-02-03T21:14:50Z

cpp/oneapi/dal/backend/primitives/rng/test/rng_dpc.cpp

First test (host vs device) makes sense - what is goal of next two? and maybe they could be combined?

But overall host vs device test looks good, not sure if more test scope is required.

inc 1

f9c8265

minor fixes

0ce01a6

minor fix

b6e5135

minor fix for macros

91929f3

Alexandr-Solovev added 2 commits January 22, 2025 04:42

add dpc files

50b34e6

minor fix

86d1a8e

Alexandr-Solovev changed the title ~~inc 1~~ feature: rng primitive refactoring Jan 22, 2025

Alexandr-Solovev added the dpc++ Issue/PR related to DPC++ functionality label Jan 22, 2025

fixes

7a31362

Alexandr-Solovev marked this pull request as ready for review January 22, 2025 20:53

Alexandr-Solovev requested review from Alexsandruss and samir-nasibli as code owners January 22, 2025 20:53

Merge branch 'uxlfoundation:main' into dev/asolovev_test_rng_incremental

e850e09

fix for bazel tests

418e741

Vika-F reviewed Jan 23, 2025

View reviewed changes

fix comments

3d44f9e