feature: enabling oneDPL and sort primitive refactoring #3046

Alexandr-Solovev · 2025-01-16T15:58:06Z

Description:

Feature: enabling oneDPL and sort primitive refactoring

Summary:

This PR introduces oneDPL enabling and radix sort replacement.

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.

WORKSPACE

david-cortes-intel · 2025-01-20T07:52:04Z

Before merging, please remember to add this new dependency to the installation instructions in INSTALL.md, along with instructions for setting necessary env. variables when using conda:
https://github.com/uxlfoundation/oneDAL/blob/main/INSTALL.md

Alexandr-Solovev · 2025-01-22T20:28:28Z

/intelci: run

.ci/pipeline/ci.yml

cpp/oneapi/dal/algo/decision_forest/backend/gpu/train_kernel_hist_impl_dpc.cpp

Vika-F · 2025-02-12T12:44:56Z

cpp/oneapi/dal/backend/primitives/sort/sort_dpc.cpp

+    auto event = oneapi::dpl::experimental::kt::gpu::esimd::radix_sort_by_key<true, 8>(
+        queue,
+        val_in.get_mutable_data(),
+        val_in.get_mutable_data() + val_in.get_count(),
+        ind_in.get_mutable_data(),
+        dpl::experimental::kt::kernel_param<256, 32>{});


Magic value 8, 256, 32 should be at least explained here.

Also, it might be beneficial not to hardcode them, but let the algorithmic kernel define them. I think different values can be chosen for different algorithms, or for different hardware platforms for better performance.

At least write the comments that the parameters are chosen for PVC.

cpp/oneapi/dal/backend/primitives/sort/sort_dpc.cpp

Vika-F

The code looks good to me, I only have couple of minor comments.

Vika-F · 2025-03-06T10:54:45Z

cpp/oneapi/dal/algo/decision_forest/backend/gpu/train_kernel_hist_impl_dpc.cpp

@@ -19,7 +19,7 @@
 #include "oneapi/dal/table/row_accessor.hpp"
 #include "oneapi/dal/detail/profiler.hpp"
 #include "oneapi/dal/algo/decision_forest/backend/gpu/train_helpers.hpp"
-
+#include <iostream>


Please remove this include.

Vika-F · 2025-03-06T11:00:14Z

cpp/oneapi/dal/algo/decision_forest/backend/gpu/train_kernel_hist_impl_dpc.cpp

+            return ::oneapi::dal::backend::primitives::engine_type::philox4x32x10;
+        case df_engine_types::mt19937:
+            return ::oneapi::dal::backend::primitives::engine_type::mt19937;
+        default: throw std::invalid_argument("Unsupported engine type 2");


Please change the message in the exception. "type 2" sounds strange.

Vika-F · 2025-03-06T11:03:06Z

cpp/oneapi/dal/algo/decision_forest/common.hpp

@@ -133,6 +133,20 @@ enum class splitter_mode {
    random
 };

+/// Available engine methods for building trees
+enum class df_engine_types {
+    /// mt2203 engine


Those comments look excessive. Consider removing.

Vika-F · 2025-03-06T11:03:32Z

cpp/oneapi/dal/algo/decision_forest/common.hpp

@@ -594,6 +611,17 @@ class descriptor : public detail::descriptor_base<Task> {
        return *this;
    }

+    /// Engine method for the random numbers generator used by the algorithm
+    /// @remark default = df_engine_method::philox


Suggested change

/// @remark default = df_engine_method::philox

/// @remark default = df_engine_method::philox4x32x10

Vika-F · 2025-03-06T11:04:32Z

cpp/oneapi/dal/algo/decision_forest/test/spmd.cpp

-    const auto model = train_result.get_model();
-    this->infer_base_checks(desc, data_test, this->get_homogen_table_id(), model, checker_list);
-}
+// DF_SPMD_CLS_TEST("df cls base check with default params and train weights") {


Can you please explain the reason of commenting out the test?

Vika-F · 2025-03-06T11:09:07Z

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

+    else {
+        auto& device_engine =
+            *(static_cast<gen_mt2203*>(engine_.get_device_engine_base_ptr().get()))->get();
+        return oneapi::mkl::rng::generate(distr, device_engine, count, dst, deps);
+    }
+
+    // default: throw std::runtime_error("Unsupported engine type in generate_rng");
+    //}


Please add error handling

Suggested change

else {

auto& device_engine =

*(static_cast<gen_mt2203*>(engine_.get_device_engine_base_ptr().get()))->get();

return oneapi::mkl::rng::generate(distr, device_engine, count, dst, deps);

}

// default: throw std::runtime_error("Unsupported engine type in generate_rng");

//}

else if (engine_type == engine_type::mt2203) {

auto& device_engine =

*(static_cast<gen_mt2203*>(engine_.get_device_engine_base_ptr().get()))->get();

return oneapi::mkl::rng::generate(distr, device_engine, count, dst, deps);

} else {

throw std::runtime_error("Unsupported engine type in generate_rng");

}

Vika-F · 2025-03-06T11:10:42Z

cpp/oneapi/dal/backend/primitives/sort/sort_dpc.cpp

@@ -19,6 +19,15 @@
 #include "oneapi/dal/detail/profiler.hpp"

 #include <sycl/ext/oneapi/experimental/builtins.hpp>
+#include <sycl/ext/oneapi/experimental/builtins.hpp>


Please remove this line.

Suggested change

#include <sycl/ext/oneapi/experimental/builtins.hpp>

Vika-F · 2025-03-06T11:12:05Z

cpp/oneapi/dal/backend/primitives/sort/sort_dpc.cpp

+    auto event = oneapi::dpl::experimental::kt::gpu::esimd::radix_sort_by_key<true, 8>(
+        queue,
+        val_in.get_mutable_data(),
+        val_in.get_mutable_data() + val_in.get_count(),
+        ind_in.get_mutable_data(),
+        dpl::experimental::kt::kernel_param<256, 32>{});


At least write the comments that the parameters are chosen for PVC.

Vika-F · 2025-03-06T11:17:38Z

cpp/oneapi/dal/backend/primitives/sort/sort_dpc.cpp

+INSTANTIATE_RADIX_SORT(std::int32_t)
+INSTANTIATE_RADIX_SORT(std::uint32_t)
+INSTANTIATE_RADIX_SORT(std::int64_t)
+INSTANTIATE_RADIX_SORT(std::uint64_t)


Do we really need all those instantiations?

We have these types for non dpl(for no pvc devices) radix sort, so its aligned

init adding dpl

65d9322

david-cortes-intel reviewed Jan 20, 2025

View reviewed changes

WORKSPACE Show resolved Hide resolved

Alexandr-Solovev added 11 commits January 20, 2025 06:51

fixes for dpl

f8028b7

minor fix

0b553e8

minor fix

2a91928

minor fix for dpl from toolkit

ab367c0

minor fix for script

e053cdf

minor fixes

3f1a6fe

minor fix

700cd10

minor fix

809760f

minor fix for dpl

d01ea31

fix correct link

064bb12

minor fixes

6e3587d

Alexandr-Solovev added dpc++ dependencies labels Jan 22, 2025

Alexandr-Solovev changed the title ~~init adding dpl~~ feature: enabling oneDPL and sorting primitive refactoring Jan 22, 2025

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

Alexandr-Solovev requested review from Alexsandruss, samir-nasibli, napetrov, homksei, ahuber21 and ethanglaser as code owners January 22, 2025 20:28

Alexandr-Solovev changed the title ~~feature: enabling oneDPL and sorting primitive refactoring~~ feature: enabling oneDPL and sort primitive refactoring Jan 22, 2025

napetrov reviewed Jan 22, 2025

View reviewed changes

.ci/pipeline/ci.yml Outdated Show resolved Hide resolved

Alexandr-Solovev added 2 commits January 23, 2025 09:24

Merge branch 'uxlfoundation:main' into dev/asolovev_radix_sort_opt

0d9edd6

minor fix

a60eb07

Alexandr-Solovev requested a review from Vika-F as a code owner January 23, 2025 09:18

Alexandr-Solovev added 2 commits February 10, 2025 08:12

initial internal dispatcher

0ecfd35

fixes

94b260b

Vika-F reviewed Feb 12, 2025

View reviewed changes

cpp/oneapi/dal/algo/decision_forest/backend/gpu/train_kernel_hist_impl_dpc.cpp Outdated Show resolved Hide resolved

Vika-F reviewed Feb 12, 2025

View reviewed changes

cpp/oneapi/dal/backend/primitives/sort/sort_dpc.cpp Show resolved Hide resolved

Alexandr-Solovev added 20 commits February 14, 2025 00:51

minor fixes

5bdd54f

Merge branch 'main' into dev/asolovev_radix_sort_opt

e7f4066

fixes

aaa4546

add fraction checker

a7cfed8

minor fixes

b450d82

fixes

24cf250

fixes

2f397cf

fixes for dpl install

caf932e

minor fixes

cb66c3f

fixes for memory usage

6f64de2

minor fix for docs and dpl for non data center devices

530a00d

fixes

9238928

minor fixes

85bd20f

fix for docs

8e14b6b

fixes

5a14e25

minor fixes

8a593ca

minor fixes

fe2b79e

minor fix for naming

5108a3e

Merge branch 'uxlfoundation:main' into dev/asolovev_radix_sort_opt

319ff4b

fixes

e7fa4e8

Vika-F reviewed Mar 6, 2025

View reviewed changes

Alexandr-Solovev added 4 commits March 6, 2025 06:22

bazel test fix

fc9eba4

fixes for rng test

9392c7c

minor fix

11d043c

minor fixes

cc967f9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: enabling oneDPL and sort primitive refactoring #3046

feature: enabling oneDPL and sort primitive refactoring #3046

Alexandr-Solovev commented Jan 16, 2025 •

edited

Loading

david-cortes-intel commented Jan 20, 2025

Alexandr-Solovev commented Jan 22, 2025

Vika-F Feb 12, 2025

Vika-F Mar 6, 2025

Vika-F left a comment

Vika-F Mar 6, 2025

Vika-F Mar 6, 2025

Vika-F Mar 6, 2025

Vika-F Mar 6, 2025

Vika-F Mar 6, 2025

Vika-F Mar 6, 2025

Vika-F Mar 6, 2025

Vika-F Mar 6, 2025

Vika-F Mar 6, 2025

Alexandr-Solovev Mar 6, 2025

	/// @remark default = df_engine_method::philox
	/// @remark default = df_engine_method::philox4x32x10

feature: enabling oneDPL and sort primitive refactoring #3046

Are you sure you want to change the base?

feature: enabling oneDPL and sort primitive refactoring #3046

Conversation

Alexandr-Solovev commented Jan 16, 2025 • edited Loading

Description:

Summary:

david-cortes-intel commented Jan 20, 2025

Alexandr-Solovev commented Jan 22, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vika-F left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Alexandr-Solovev commented Jan 16, 2025 •

edited

Loading