-
Notifications
You must be signed in to change notification settings - Fork 53
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
ENH Work on supporting scikit-learn intelex #267
ENH Work on supporting scikit-learn intelex #267
Conversation
Partially solves skops-dev#251 This is one part of the work required to solve the mentioned issue. The other part will have to be added on the API inference side, once this PR is finalized. Description Added the option use_intelex to hub_utils.init. It adds a new entry to config.json which can later be used by the inference API to decide whether to run it with intelex or not. On top of that, if use_intelex=True, scikit-learn-intelex will be added as a dependency to the requirements if not already there. Moreover, if a metadata for a model card is loaded, a scikit-learn-intelex tag will be added.
@skops-dev/maintainers Ready for review. |
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.
Thanks @BenjaminBossan
@adrinjalali I addressed your comments, pls review again. |
I just saw this: def patch_sklearn(name=None, verbose=True, global_patch=False):
if not sklearn_check_version('0.22'):
raise NotImplementedError("Intel(R) Extension for Scikit-learn* patches apply "
"for scikit-learn >= 0.22 only ...") Which patching wouldn't be an option for us. That's a VERY OLD sklearn version. I checked their codebase, and they have their own estimators which can be imported and used using explicit imports from their library. If the user explicitly imports from the Which means this task would be reduced to a documentation and an example script on our side, to showcase how one can use |
I think this is the minimum sklearn version, so we should be good. When I tested locally, patching worked (or, at least, didn't raise an error). |
Oh sorry, it's checking if the sklearn version is >0.22, that's a very bad way of writing code lol, but sure, it works. |
Yes, naming is hard :) |
Partially solves #251
This is one part of the work required to solve the mentioned issue. The other part will have to be added on the API inference side, once this PR is finalized.
Description
Added the option
use_intelex
tohub_utils.init
. It adds a new entry toconfig.json
which can later be used by the inference API to decide whether to run it with intelex or not.On top of that, if
use_intelex=True
, scikit-learn-intelex will be added as a dependency to the requirements if not already there. Moreover, if metadata for a model card is loaded, a"scikit-learn-intelex"
tag will be added.