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

Added support for Jinja2 3.1.2 and scikit-learn 1.0.2 #418

Closed
wants to merge 1 commit into from

Conversation

dvorst
Copy link

@dvorst dvorst commented May 1, 2022

When using Jinja2 3.1.2 and scikit-learn 1.0.2, the following errors occurred:

  • AttributeError: module 'jinja2.ext' has no attribute 'with_'
  • ModuleNotFoundError: No module named 'sklearn.metrics.scorer'
  • ModuleNotFoundError: No module named 'sklearn.feature_selection.base'

The changes in 'html.py', 'permutation_importance.py' and 'transform.py' are such that these errors are resolved while maintaining support for older versions of jinja2 and scikit-learn.

'test_import.py' is used to test importing eli5 for both newer and older dependency versions.

Also see: #417

…s occurred:

Jinja2 			3.1.2 		AttributeError: module 'jinja2.ext' has no attribute 'with_'
scikit-learn 	1.0.2		ModuleNotFoundError: No module named 'sklearn.metrics.scorer'
							ModuleNotFoundError: No module named 'sklearn.feature_selection.base'

The changes in 'html.py', 'permutation_importance.py' and 'transform.py' are such that these errors are resolved while maintaining support for older versions of jinja2 and scikit-learn.

'test_import.py' is used to test importing eli5 for both newer and older dependency versions.
@dvorst dvorst marked this pull request as ready for review May 1, 2022 15:53
Copy link
Contributor

@lopuhin lopuhin left a comment

Choose a reason for hiding this comment

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

Hi @dvorst , thanks for the fixes 👍 I didn't run the tests but changes look good to me, left one comment below. However, would you mind submitting the PR against https://github.com/eli5-org/eli5/, if you don't mind?

Comment on lines +7 to +10
if version.parse(jinja2.__version__) <= version.parse('3.0.3'):
raise ImportError('module "Jinja2" must be of a later version than "3.0.3".')
if version.parse(sklearn.__version__) < version.parse('1.0.2'):
raise ImportError('module "Jinja2" must be version "1.0.2" or later.')
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the goal of these checks? From the code above it seems that we do support older versions of these libraries? If we want to work only with versions specified here, it's better to specify this in setup.py.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, the test file is probably not really needed, I omitted it and also slightly changed some other stuff in the PR for https://github.com/eli5-org/eli5/ (the sklearn issue for example was already resolved)

@Hellisotherpeople
Copy link

Can we get a merge ASAP on this? ELI5 is now broken for most use-cases since you know, most people are running this on sklearn models...

Also breaks my HF space: https://huggingface.co/spaces/Hellisotherpeople/Interpretable_Text_Classification_And_Clustering

@lopuhin
Copy link
Contributor

lopuhin commented May 11, 2022

@Hellisotherpeople this was merged in eli5-org/eli5#19 and released in 0.12, thanks for a reminder

@dvorst dvorst closed this Aug 23, 2023
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.

3 participants