-
Notifications
You must be signed in to change notification settings - Fork 221
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feature(rf optimizations): enabling oneDPL and sort primitive refactoring #3046
feature(rf optimizations): enabling oneDPL and sort primitive refactoring #3046
Conversation
Before merging, please remember to add this new dependency to the installation instructions in |
/intelci: run |
/intelci: run |
Please resolve docbuild fail before merge: https://dev.azure.com/daal/DAAL/_build/results?buildId=44462&view=logs&j=12f1170f-54f2-53f3-20dd-22fc7dff55f9&t=5caf77c8-9b10-50ef-b5c7-ca89c63e1c86&l=950 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, CI is green, and I have ran this enough times on cluster to know it works :) but would wait for others on feedback for specific implementation details
cpp/oneapi/dal/algo/decision_forest/backend/gpu/train_feature_type_dpc.cpp
Show resolved
Hide resolved
/azp run CI |
Azure Pipelines successfully started running 1 pipeline(s). |
/intelci: run |
/intelci: run |
/azp run CI |
Azure Pipelines successfully started running 1 pipeline(s). |
function install_mkl { | ||
sudo apt-get install -y intel-oneapi-mkl-devel-2025.0 | ||
install_tbb | ||
install_dpl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is dpl a dependency of MKL? I thought tbb was integrated here to install_mkl for that reason
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess mkl and tbb have no deps on each other, but my understanding its a step for install all necessary deps for onedal
@@ -129,6 +134,9 @@ elif [ "${component}" == "tbb" ]; then | |||
elif [ "${component}" == "mkl" ]; then | |||
add_repo | |||
install_mkl | |||
elif [ "${component}" == "dpl" ]; then | |||
add_repo | |||
install_dpl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add to the help list at the end of this file "dpl"
name = "dpl", | ||
root_env_var = "DPL_ROOT", | ||
urls = [ | ||
"https://files.pythonhosted.org/packages/95/f6/18f78cb933e01ecd9e99d37a10da4971a795fcfdd1d24640799b4050fdbb/onedpl_devel-2022.7.1-py2.py3-none-manylinux_2_28_x86_64.whl", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dumb question, but how do we find these values/maintain them? It looks painful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we do the same thing for all other packages like tbb and mkl. Find it on pypi and copy links)
auto src_ind = pr::ndarray<Index, 1>::empty(queue_, { src.get_count() }); | ||
return pr::radix_sort_indices_inplace<Float, Index>{ queue_ }(src, src_ind, deps); | ||
if (device_name.find("Data Center GPU Max") != std::string::npos) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels dangerous somehow. Definitely add some comments. Ideally device checking should exist as a primitive rather than in an algo because this is a bit of a nasty surprise to anyone not well-versed in this algo when trying to debug on various hardware.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Vika-F planned to add this feature in future
@@ -61,6 +61,10 @@ class descriptor_impl : public base { | |||
error_metric_mode error_metric_mode_value = error_metric_mode::none; | |||
infer_mode infer_mode_value = infer_mode::class_responses; | |||
|
|||
// The default engine has been switched from mt2203 to philox for GPU, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very good, I would love to see what this does to overall performance.
/intelci: run |
Description:
RF optimizations: enabling oneDPL and sort primitive refactoring and several functions optimization
Summary:
This PR introduces oneDPL enabling and radix sort replacement. Also the engine_type support has been added for RF GPU. A lot of CPU functions have been replaced with GPU analogues.
PR completeness and readability
Testing
Performance