[enhancement] add oneDAL finiteness_checker implementation to onedal #2126

icfaust · 2024-10-23T11:08:08Z

Description

A GPU side finiteness checker was added to oneDAL in 2024.7 (oneapi-src/oneDAL#2781), this becomes useful with the merge of #2045. This will expose the first step, which is to include it in the onedal folder. This may replace daal4py's assert_all_finite, but will occur in a later PR. This adds limited testing on onedal side.

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.

icfaust · 2024-10-28T20:51:22Z

/azp run CI

azure-pipelines · 2024-10-28T20:51:31Z

Azure Pipelines successfully started running 1 pipeline(s).

icfaust · 2024-10-29T08:32:00Z

/azp run CI

azure-pipelines · 2024-10-29T08:32:12Z

Azure Pipelines successfully started running 1 pipeline(s).

icfaust · 2024-11-03T13:36:31Z

/intelci: run

icfaust · 2024-11-03T20:58:43Z

Private CI fail unrelated to this implementation.

onedal/utils/tests/test_validation.py

samir-nasibli

@icfaust Thank you for the excellent work! I have a few questions and clarifications, and I’ve left some comments

samir-nasibli · 2024-11-05T08:13:40Z

onedal/datatypes/_data_conversion.py

I like refactoring for to_table/from_table primitives, but since this api used in several other PRs, I think it worth to move this common changes into separate PR, to be used on #2096, #2106, #2100

samir-nasibli · 2024-11-05T08:15:00Z

onedal/datatypes/_data_conversion.py


 def _apply_and_pass(func, *args, **kwargs):
    if len(args) == 1:
        return func(args[0], **kwargs)
    return tuple(map(lambda arg: func(arg, **kwargs), args))


-if _is_dpc_backend:
+def convert_one_to_table(arg):
+    return _backend.to_table(np.atleast_2d(arg) if np.isscalar(arg) else arg)


It worth to add a comment. Will this properly work with dpnp ndarrays and dpctl usm_ndarrays?

samir-nasibli · 2024-11-05T08:22:47Z

onedal/datatypes/_data_conversion.py

+            if xp == dpnp:
+                return dpnp.array(dpnp.dpctl.tensor.asarray(table), copy=False)


I don't know is this dpctl module in dpnp is an official part of API? This may work now, but I am not sure for further versions of DPNP. It worth to add some comments here

samir-nasibli · 2024-11-05T08:24:07Z

onedal/datatypes/_data_conversion.py

-    if dpnp_available:
+if _is_dpc_backend:
+
+    try:


please use dpnp_available from _dpep_helpers instead.

samir-nasibli · 2024-11-05T08:30:08Z

onedal/utils/validation.py

+    if not _backend.finiteness_checker.compute.compute(policy, params, X_t).finite:
+        type_err = "infinity" if allow_nan else "NaN, infinity"
+        padded_input_name = input_name + " " if input_name else ""
+        msg_err = f"Input {padded_input_name}contains {type_err}."


Please cover with tests this message catch

samir-nasibli · 2024-11-05T08:34:50Z

onedal/common/_policy.py

@@ -48,12 +48,8 @@ def __init__(self):


 if _is_dpc_backend:
-    from onedal._device_offload import DummySyclQueue


This is the only place where DummySyclQueue is used. I am ok with this refactoring, but it worth also remove it definition as well. I didn't find the description for these changes, make sense to move into separate branch or add into commit messages on the merge this work as well, to be clear other dev/users why and when this were removed.

samir-nasibli · 2024-11-11T18:12:17Z

@icfaust I believe it would be beneficial to include links to other dependent PRs in the description.

* carryover from #2126 * formatting * Update test_memory_usage.py * Update _data_conversion.py * Update _data_conversion.py * Update _data_conversion.py * Update test_data.py * Update onedal/datatypes/_data_conversion.py Co-authored-by: Victoriya Fedotova <viktoria.nn@gmail.com> * Update _data_conversion.py * Update _data_conversion.py * Update onedal/datatypes/_data_conversion.py Co-authored-by: Samir Nasibli <samir.nasibli@intel.com> * Update _data_conversion.py * Update _data_conversion.py --------- Co-authored-by: Victoriya Fedotova <viktoria.nn@gmail.com> Co-authored-by: Samir Nasibli <samir.nasibli@intel.com>

icfaust and others added 9 commits October 23, 2024 13:02

add finiteness_checker pybind11 bindings

32fe269

added finiteness checker

cdbf1b5

Update finiteness_checker.cpp

62674a2

Update finiteness_checker.cpp

c75c23b

Update finiteness_checker.cpp

6a20938

Update finiteness_checker.cpp

382d7a1

Update finiteness_checker.cpp

c8ffd9c

Update finiteness_checker.cpp

9aa13d5

Rename finiteness_checker.cpp to finiteness_checker.cpp

84e15d5

icfaust marked this pull request as ready for review October 24, 2024 08:52

icfaust requested review from Alexsandruss and samir-nasibli as code owners October 24, 2024 08:52

icfaust marked this pull request as draft October 24, 2024 08:53

icfaust added 2 commits October 24, 2024 10:58

Update finiteness_checker.cpp

63073c6

Merge branch 'intel:main' into dev/new_assert_all_fininte

d915da5

icfaust and others added 11 commits November 1, 2024 00:30

add next step

3dddf2d

follow conventions

1e1213e

make xtable explicit

0531713

remove comment

e831167

Update validation.py

d6eb1d0

Update __init__.py

fb30d6e

Update validation.py

63a18c2

Update __init__.py

76c0856

Update __init__.py

7deb2bb

Update validation.py

ed46b29

Update _data_conversion.py

67d6273

icfaust and others added 20 commits November 2, 2024 13:35

last move

d985053

Update policy_common.cpp

90ec48b

Update policy_common.cpp

8c2c854

Update policy_common.cpp

6fa38d7

Update policy_common.cpp

9c1ca9c

Update validation.py

4b67dbd

add testing

fa59a3c

isort

3330b33

attempt to fix module error

4895940

add fptype

0c6dd5d

fix typo

e2182fa

Update validation.py

982ef2c

remove sua_ifcae from to_table

2fb52a8

isort and black

28dc267

Update test_memory_usage.py

2f85fd4

format

8659248

Update _data_conversion.py

3827d6f

Update _data_conversion.py

55fa7d2

Update test_validation.py

175cd78

remove unnecessary code

7016ad0

icfaust marked this pull request as ready for review November 3, 2024 20:58

Vika-F reviewed Nov 4, 2024

View reviewed changes

onedal/utils/tests/test_validation.py Show resolved Hide resolved

samir-nasibli reviewed Nov 5, 2024

View reviewed changes

icfaust added a commit to icfaust/scikit-learn-intelex that referenced this pull request Nov 5, 2024

carryover from intel#2126

c40c8f4

icfaust mentioned this pull request Nov 5, 2024

[enhancement] refactor onedal to_table #2151

Merged

13 tasks

icfaust added a commit to icfaust/scikit-learn-intelex that referenced this pull request Nov 5, 2024

carryover from intel#2126

a1c9df5

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[enhancement] add oneDAL finiteness_checker implementation to onedal #2126

[enhancement] add oneDAL finiteness_checker implementation to onedal #2126

icfaust commented Oct 23, 2024 •

edited

Loading

icfaust commented Oct 28, 2024

azure-pipelines bot commented Oct 28, 2024

icfaust commented Oct 29, 2024

azure-pipelines bot commented Oct 29, 2024

icfaust commented Nov 3, 2024

icfaust commented Nov 3, 2024

samir-nasibli left a comment

samir-nasibli Nov 5, 2024

samir-nasibli Nov 5, 2024

samir-nasibli Nov 5, 2024

samir-nasibli Nov 5, 2024

samir-nasibli Nov 5, 2024

samir-nasibli Nov 5, 2024

samir-nasibli commented Nov 11, 2024

		if xp == dpnp:
		return dpnp.array(dpnp.dpctl.tensor.asarray(table), copy=False)

		@@ -48,12 +48,8 @@ def __init__(self):


		if _is_dpc_backend:
		from onedal._device_offload import DummySyclQueue

[enhancement] add oneDAL finiteness_checker implementation to onedal #2126

Are you sure you want to change the base?

[enhancement] add oneDAL finiteness_checker implementation to onedal #2126

Conversation

icfaust commented Oct 23, 2024 • edited Loading

Description

icfaust commented Oct 28, 2024

azure-pipelines bot commented Oct 28, 2024

icfaust commented Oct 29, 2024

azure-pipelines bot commented Oct 29, 2024

icfaust commented Nov 3, 2024

icfaust commented Nov 3, 2024

samir-nasibli left a comment

Choose a reason for hiding this comment

samir-nasibli Nov 5, 2024

Choose a reason for hiding this comment

samir-nasibli Nov 5, 2024

Choose a reason for hiding this comment

samir-nasibli Nov 5, 2024

Choose a reason for hiding this comment

samir-nasibli Nov 5, 2024

Choose a reason for hiding this comment

samir-nasibli Nov 5, 2024

Choose a reason for hiding this comment

samir-nasibli Nov 5, 2024

Choose a reason for hiding this comment

samir-nasibli commented Nov 11, 2024

icfaust commented Oct 23, 2024 •

edited

Loading