Skip to content
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

MAINT: utility module for Intel data parallel libs; import checks in one place #1936

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

samir-nasibli
Copy link
Contributor

@samir-nasibli samir-nasibli commented Jul 12, 2024

Description

These changes centralized data parallel libs checks, such as DPCtl and DPNP availability. (Also discussed on #1861 (comment))
Proposed changes:

  • doing the check of the data parallel libs availability in one place, reusing related variable through the lib.
  • added is_dpctl_available/is_dpnp_availble tools for checking the specific dpctl, dpnp versions
  • renamed is_dpctl_available from onedal.tests.utils._device_selection module to is_dpctl_device_available. This tool is used in sklearnex/onedal tests where it is needed to check required device(s) availability via dpctl.
  • Comments and docstring is added.

Non prior. Should be integrated after main is unfreeze.

  • 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, if necessary (updated in # - add PR number)
  • The unit tests pass successfully.
  • I have run it locally and tested the changes extensively.
  • I have resolved any merge conflicts that might occur with the base branch.
  • 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.

@samir-nasibli samir-nasibli marked this pull request as ready for review September 11, 2024 10:00
@samir-nasibli samir-nasibli marked this pull request as draft September 11, 2024 10:41
@samir-nasibli
Copy link
Contributor Author

/intelci: run

@samir-nasibli samir-nasibli marked this pull request as ready for review September 11, 2024 15:45
Copy link
Contributor

@icfaust icfaust left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like a change of the name of "dppy", which I can only see in reference to dpctl's official/unofficial anaconda channel. Something which references data parallel intel or SYCL would be preferred.

def is_dpctl_available(targets=None):
try:
def is_dpctl_device_available(targets):
if dpctl_available:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if dpctl_available check can be made an import-time check, rather than runtime.

onedal/utils/_dppy_available.py Outdated Show resolved Hide resolved
onedal/utils/_dppy_available.py Outdated Show resolved Hide resolved
rename the module for dp utilities

update dpnp/dpctl checkers

reusing daal4py sklearn_check_version
@samir-nasibli
Copy link
Contributor Author

/intelci: run

@samir-nasibli samir-nasibli changed the title MAINT: do DPPY libs imports check in one place MAINT: utility module for Intel data parallel libs; import checks in one place Sep 19, 2024
@samir-nasibli
Copy link
Contributor Author

/intelci: run

@samir-nasibli
Copy link
Contributor Author

/intelci: run

from daal4py.sklearn._utils import _package_check_version


def is_dpctl_available(version=None):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These functions need caching just in case they will be used more extensively in future.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does dpep mean? I believe it is confusing name for helper file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/IntelPython/DPEP
https://intelpython.github.io/DPEP/main/
Do you want me provide links in the docs of the module or just rename?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have provided small description in the top of the module

Copy link
Contributor

@Alexsandruss Alexsandruss Sep 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

data_parallel_extensions_helper / data_parallel_ext_helper would be better.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants