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

ENH Work on supporting scikit-learn intelex #267

Merged

Conversation

BenjaminBossan
Copy link
Collaborator

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 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 metadata for a model card is loaded, a "scikit-learn-intelex" tag will be added.

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.
@BenjaminBossan
Copy link
Collaborator Author

@skops-dev/maintainers Ready for review.

Copy link
Member

@adrinjalali adrinjalali left a comment

Choose a reason for hiding this comment

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

skops/card/_model_card.py Show resolved Hide resolved
skops/hub_utils/_hf_hub.py Outdated Show resolved Hide resolved
skops/hub_utils/_hf_hub.py Outdated Show resolved Hide resolved
skops/hub_utils/_hf_hub.py Show resolved Hide resolved
skops/hub_utils/tests/test_hf_hub.py Show resolved Hide resolved
@BenjaminBossan
Copy link
Collaborator Author

@adrinjalali I addressed your comments, pls review again.

@adrinjalali
Copy link
Member

I just saw this:

https://github.com/intel/scikit-learn-intelex/blob/d71f63c12cba7ee99191e3013182b0650ac9ec71/sklearnex/dispatcher.py#L104-L107

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 scikit-learn-intelex library, then everything works as expected. They can have it as an extra dependency, and the backend would install it and load the model successfully.

Which means this task would be reduced to a documentation and an example script on our side, to showcase how one can use scikit-learn-intelex on HF hub. WDYT @BenjaminBossan @echarlaix

@BenjaminBossan
Copy link
Collaborator Author

Which patching wouldn't be an option for us. That's a VERY OLD sklearn version.

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).

@adrinjalali
Copy link
Member

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.

@adrinjalali adrinjalali changed the title Work on supporting scikit-learn intelex ENH Work on supporting scikit-learn intelex Jan 23, 2023
@adrinjalali adrinjalali merged commit 311f524 into skops-dev:main Jan 23, 2023
@BenjaminBossan
Copy link
Collaborator Author

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 :)

@BenjaminBossan BenjaminBossan deleted the FEAT-support-for-sklearn-intelex branch January 23, 2023 13:02
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.

2 participants