[maintenance] lazy load dpnp.tensor/dpnp and prepare for array_api lazy importing #2509
Conversation
…rn-intelex into dev/lazy_load
sklearnex/utils/validation.py
Outdated
| try: | ||
| too_small = X.size < 32768 | ||
| except TypeError: | ||
| too_small = math.prod(X.shape) < 32768 |
There was a problem hiding this comment.
Could also use np.prod, since numpy is already imported throughout the codebase.
There was a problem hiding this comment.
https://github.com/scikit-learn/scikit-learn/blob/73a8a656b8df6d02cf88ef8f9cf98373a3f42051/sklearn/utils/_array_api.py#L215 Not entirely sure how numpy would interact with pytorch in that case. Could check that if you want, but its following the precedent set by sklearn itself
|
|
||
|
|
||
| @functools.lru_cache(100) | ||
| def _is_subclass_fast(cls: type, modname: str, clsname: str) -> bool: |
There was a problem hiding this comment.
Would this work if one of those array classes is subsetted by the user?
There was a problem hiding this comment.
Nope, but neither would array_api_compat, meaning that steps before in sklearnex are likely to have thrown an error: https://github.com/data-apis/array-api-compat/blob/main/array_api_compat/common/_helpers.py#L63
There was a problem hiding this comment.
actually let me check this, i may be wrong
| return array | ||
|
|
||
|
|
||
| @lazy_import("dpctl.memory") |
There was a problem hiding this comment.
Wouldn't importing the module inside the function have the same effect?
There was a problem hiding this comment.
Trying to avoid adding an unnecessary slowdown via the dictionary search of sys.modules. I don't think it impacts the readability as it is, and follows precedent set by other codebases like sqlite3: https://stackoverflow.com/a/61647085
There was a problem hiding this comment.
I don't follow. Their idea is to use the module multiple times, but here it gets only used inside a single function. Why would that lazy loader decorator be more efficient than importing the module inside of the function?
|
/intelci: run |
icfaust
requested review from
Alexsandruss,
yuejiaointel,
ahuber21,
ethanglaser,
razdoburdin,
avolkov-intel and
Vika-F
as code owners
| # limitations under the License. | ||
| # ============================================================================== | ||
|
|
||
| """Utilities for accessing third party pacakges such as DPNP, DPCtl. |
There was a problem hiding this comment.
| """Utilities for accessing third party pacakges such as DPNP, DPCtl. | |
| """Utilities for accessing third party packages such as DPNP, DPCtl. |
There was a problem hiding this comment.
I'm not sure if third_party is the most correct term for these frameworks. Is frameworks_support or frameworks_compat better?
There was a problem hiding this comment.
I'd agree with that except for the fact that we centralize the import of SyclQueue for use in a number of locations there (which isn't part of a framework) and that we already have an equivalent 'datatypes' onedal module.
| self.classes_ = xp.unique(y) | ||
| except AttributeError: |
There was a problem hiding this comment.
A comment why this error type might be expected is needed.
Description
Dpctl and dpnp are quasi-dependencies which will silently error out if not installed. This is done at import time throughout the codebase, meaning that it is mixed into the codebase in a difficult manner. As the number of supported data frameworks are increased, such a strategy is unsustainable. Lazy loading of the necessary packages must be done, as the load time of follow-on frameworks like PyTorch are non-negligible (>1s). If we were to follow the same strategy, load times of sklearnex would be even longer even if pytorch isn't used but is available. This will compound as we would add framework support. Cleanly separating and isolating their use is necessary.
Therefore we need to first move dpnp and dpctl.tensor support to a lazy loading approach which will then be extended by follow-on frameworks. The next step will be pytorch queue extraction, which will require this infrastructure.
The strategy will follow that of
array_api_compatwhich can check for namespaces without importing the actual modules, and for the direct use of the frameworks, a depedency injection + monkeypatching scheme is used with decoratorlazy_import.NOTE TO REVIEWERS: Let me know if I should do a performance benchmarks for this.
PR should start as a draft, then move to ready for review state after CI is passed and all applicable checkboxes are closed.
This approach ensures that reviewers don't spend extra time asking for regular requirements.
You can remove a checkbox as not applicable only if it doesn't relate to this PR in any way.
For example, PR with docs update doesn't require checkboxes for performance while PR with any change in actual code should have checkboxes and justify how this code change is expected to affect performance (or justification should be self-evident).
Checklist to comply with before moving PR from draft:
PR completeness and readability
Testing
Performance