-
Notifications
You must be signed in to change notification settings - Fork 184
DOC, MAINT: Improve GPU docs, remove references to dpctl arrays #2721
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
DOC, MAINT: Improve GPU docs, remove references to dpctl arrays #2721
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests.
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 75 files with indirect coverage changes 🚀 New features to boost your workflow:
|
|
Failing doc CI job appears to be a rate limiting issue with medium.com. |
doc/sources/input-types.rst
Outdated
| will be converted to CSR, which implies more than just data copying. | ||
| - Heterogeneous NumPy array | ||
| - If SYCL queue is provided for device without ``float64`` support but data are ``float64``, data are copied with reduced precision. | ||
| - If SyCL queue is provided for device without ``float64`` support but data are ``float64``, data are copied with reduced precision. |
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.
Its SYCL in all cases not in code.
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.
Fixed.
| from daal4py.sklearn._utils import sklearn_check_version | ||
| from onedal._config import _get_config as onedal_get_config | ||
|
|
||
| __all__ = ["get_config", "set_config", "config_context"] |
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.
why add __all__?
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.
It defines more objects than these 3. Adding __all__ would avoid making them exportable for asterisk imports.
|
|
||
| __all__ = ["get_config", "set_config", "config_context"] | ||
|
|
||
| tab = " " if (sys.version_info.major == 3 and sys.version_info.minor < 13) else "" |
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.
Whats up with the tabs?
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.
Python 3.13 changed how it deals with leading whitespace in docstrings.
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.
Does that mean we need to do it throughout the repo?
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 same workaround is already used in other places throughout the repository.
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.
Pull Request Overview
This PR improves GPU-related documentation and modernizes array handling recommendations by removing references to soon-to-be-deprecated dpctl arrays and replacing them with dpnp arrays. Key changes include:
- Adds comprehensive documentation for sklearnex configuration contexts (
config_context,set_config,get_config) - Restructures and clarifies GPU support documentation with better organization and examples
- Updates code examples across documentation to use dpnp and torch tensors instead of dpctl arrays
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| sklearnex/_config.py | Adds shared docstring for configuration options, updates function docstrings to reference it |
| doc/sources/substitutions.rst | Adds substitution for dpnp_array reference link |
| doc/sources/oneapi-gpu.rst | Major rewrite of GPU docs with clearer sections, updated examples, and removal of dpctl array references |
| doc/sources/input-types.rst | Removes dpctl.tensor references, updates to mention only dpnp.ndarray |
| doc/sources/index.rst | Updates examples to use torch tensors instead of dpnp, fixes syntax error in code block |
| doc/sources/distributed-mode.rst | Updates text to reference dpnp_array instead of dpctl arrays |
| doc/sources/config-contexts.rst | New documentation page for configuration contexts |
| doc/sources/array_api.rst | Updates SyCL/SYCL spelling, removes dpctl references, adds torch tensor examples |
| doc/sources/algorithms.rst | Minor formatting fix to verbose mode reference |
Comments suppressed due to low confidence (1)
sklearnex/_config.py:1
- Corrected spelling of 'accelered' to 'accelerated'.
# ==============================================================================
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| __all__ = ["get_config", "set_config", "config_context"] | ||
|
|
||
| tab = " " if (sys.version_info.major == 3 and sys.version_info.minor < 13) else "" |
Copilot
AI
Nov 6, 2025
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.
The magic number 13 in the Python version check lacks explanation. Consider adding a comment explaining why Python 3.13+ requires different tab handling for docstrings, or define this as a named constant like PYTHON_VERSION_WITH_DOCSTRING_CHANGE = (3, 13).
| tab = " " if (sys.version_info.major == 3 and sys.version_info.minor < 13) else "" | |
| # Python 3.13+ changes tab handling in docstrings, so we adjust indentation accordingly. | |
| PYTHON_VERSION_WITH_DOCSTRING_CHANGE = (3, 13) | |
| tab = " " if (sys.version_info.major == 3 and sys.version_info.minor < PYTHON_VERSION_WITH_DOCSTRING_CHANGE[1]) else "" |
| os.environ["SCIPY_ARRAY_API"] = "1" | ||
| import numpy as np | ||
| import dpnp | ||
| import torch |
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.
Why not keep dpnp here?
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.
It's currently lacking important capabilities like serialization, and more likely to segfault instead of producing errors when doing something wrong with it. I think it's more likely that a user playing with this example would run into issues with dpnp than with torch, and then conclude that sklearnex can't do something before reading the docs.
| Overview | ||
| ======== | ||
|
|
||
| Just like |sklearn|, the |sklearnex| offers configurable options which can be managed |
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.
Minor naming conventions nitpick - I'd say it makes more sense to refer to "|sklearnex|" compared to "the |sklearnex|". The name of the project is Extension for scikit-learn. This could apply to line 24, 29, 32, 36, 48, etc.
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 is a macro that expands to "Extension for Scikit-learn*":
scikit-learn-intelex/doc/sources/conf.py
Line 132 in 186b741
| .. |sklearnex| replace:: Extension for Scikit-learn* |
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.
Yes but I mean to remove "the" in "the |sklearnex|" and just refer to it as |sklearnex|
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.
So in this case "Just like |sklearn|, the Extension for Scikit-learn offers configurable options which can be managed" would be "Just like |sklearn|, Extension for Scikit-learn offers configurable options which can be managed"
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'd say
The extension for scikit-learn offers ...
.. sounds more correct to me than
Extension for scikit-learn offers ...
.. if put within a larger paragraph.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: ethanglaser <[email protected]>
Co-authored-by: ethanglaser <[email protected]>
Co-authored-by: ethanglaser <[email protected]>
Description
This PR:
Checklist:
Completeness and readability
Testing