Skip to content

Conversation

david-cortes-intel
Copy link
Contributor

Description

Dependent on #2721 being merged.

This PR adds a doc page with details about serialization of model objects, documenting behaviors that happen with GPU data, array API, compatibility, and similar.


Checklist:

Completeness and readability

  • I have updated the documentation to reflect the changes or created a separate PR with updates 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 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.


.. warning:: Note that, unlike objects from |sklearn|, objects from the |sklearnex| will not necessarily issue a warning when deserializing them with an incompatible library version.

Serialization of GPU models
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Alexsandruss @ethanglaser Feel free to add a paragraph or subsection about how serialization of distributed models works.

Serialization of GPU models
---------------------------

Be aware that if using the :ref:`target offload option <target_offload>` to fit models on GPU or on another SyCL device, upon deserialization of those models, the internal data behind them will be re-created on host (CPU), hence the deserialized models will become CPU/host ones and will not be able to make predictions on GPU data.
Copy link
Contributor

Choose a reason for hiding this comment

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

Its always 'SYCL' https://www.khronos.org/sycl/


__all__ = ["get_config", "set_config", "config_context"]

tab = " " if (sys.version_info.major == 3 and sys.version_info.minor < 13) else ""
Copy link
Contributor

Choose a reason for hiding this comment

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

Whats going on here with the tabs?

Copy link
Contributor Author

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.

__all__ = ["get_config", "set_config", "config_context"]

tab = " " if (sys.version_info.major == 3 and sys.version_info.minor < 13) else ""
_options_docstring = f"""Parameters
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess you are trying to duplicate the documentation between the various aspects? If so add this to the list of things changed in the description

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's trying to unify the docs of two functions that accept the same arguments.

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"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Why add the __all__ attribute?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To avoid exporting other things when doing asterisk imports.

@david-cortes-intel
Copy link
Contributor Author

@icfaust This PR contains the commits from a previous one as a base, as it depends on it but it hasn't been merged yet.

Could you leave the comments in that other PR instead?
#2721

@david-cortes-intel david-cortes-intel marked this pull request as draft October 14, 2025 14:59
@icfaust
Copy link
Contributor

icfaust commented Oct 14, 2025

@icfaust This PR contains the commits from a previous one as a base, as it depends on it but it hasn't been merged yet.

Could you leave the comments in that other PR instead? #2721

Yep, sorry missed that.

Copy link

codecov bot commented Oct 14, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.

Flag Coverage Δ
azure 80.37% <100.00%> (?)
github 73.03% <100.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
sklearnex/_config.py 100.00% <100.00%> (ø)

... and 30 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants