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

Make shap an extra dependency #376

Merged
merged 7 commits into from
Mar 31, 2021
Merged

Conversation

jklaise
Copy link
Contributor

@jklaise jklaise commented Mar 29, 2021

This PR is meant to (temporarily) make shap an optional dependency. The reason is because the build process of shap currently is not optimal and will result in RuntimeError when packages have a different runtime dependency on numpy than the build dependency of numpy used for shap.

Concretely, shap on linux is built with the latest available version of numpy (currently 1.20), whilst tensorflow has a numpy pin of 1.19.5. This means that pip install alibi results in a broken installation as shap is still built against 1.20. As far as I've looked, it is not possible to specify a different version of numpy to build shap against in a "one-pass" install of alibi (e.g. if numpy is not present, shap will be built against 1.20 regardless, if numpy is present it will be built against that version).

As a result, this PR proposes installing alibi with shap support in two steps:

pip install alibi
pip install alibi[shap]

I have added the above instructions to the README as well as at the top of the shap-specific notebooks.

This workaround should be resolved once shap specifies oldest-supported-numpy as a build dependency instead. For more information see here: shap/shap#1802

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@codecov
Copy link

codecov bot commented Mar 29, 2021

Codecov Report

Merging #376 (f07d6a6) into master (d9be65f) will decrease coverage by 0.14%.
The diff coverage is 60.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #376      +/-   ##
==========================================
- Coverage   87.73%   87.59%   -0.15%     
==========================================
  Files          56       56              
  Lines        7372     7376       +4     
==========================================
- Hits         6468     6461       -7     
- Misses        904      915      +11     
Impacted Files Coverage Δ
alibi/explainers/__init__.py 85.71% <60.00%> (-14.29%) ⬇️
alibi/explainers/anchor_base.py 89.24% <0.00%> (-2.85%) ⬇️

Copy link

@RafalSkolasinski RafalSkolasinski left a comment

Choose a reason for hiding this comment

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

Looks okay but what if someone tries to import something from shap-dependent parts of library without shap being installed?
alibi/explainers/shap_wrappers.py ?

I think it will fail in quite an ugly way -> maybe worth to have some logic that do try ... except in shap imports that in case shap is not present raise a valid warning / information?

README.md Outdated Show resolved Hide resolved
@jklaise
Copy link
Contributor Author

jklaise commented Mar 30, 2021

Looks okay but what if someone tries to import something from shap-dependent parts of library without shap being installed?
alibi/explainers/shap_wrappers.py ?

I think it will fail in quite an ugly way -> maybe worth to have some logic that do try ... except in shap imports that in case shap is not present raise a valid warning / information?

Good point, forgot to handle this completely as it would fail immediately on alibi import since it imports the shap specific explainers too... The try ... except logic should be put in alibi.explainers.__init__ so only the relevant explainers are loaded.

@jklaise jklaise merged commit e358950 into SeldonIO:master Mar 31, 2021
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