Skip to content

joblib development version breaks sklearn.show_versions() #22614

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

Closed
lesteve opened this issue Feb 25, 2022 · 4 comments · Fixed by #22621
Closed

joblib development version breaks sklearn.show_versions() #22614

lesteve opened this issue Feb 25, 2022 · 4 comments · Fixed by #22621

Comments

@lesteve
Copy link
Member

lesteve commented Feb 25, 2022

When we release joblib (soonish let's say a matter of weeks) we will break sklearn.show_versions(). We may need to do a bug-fix release of scikit-learn.

To reproduce:

mamba create -n test-env scikit-learn -y
conda activate test-env
pip install git+https://github.com/joblib/joblib
python -c 'import sklearn; sklearn.show_versions()'

This is a mixture of different things:

  1. python -c 'import pip; import setuptools' breaks
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/home/lesteve/miniconda3/envs/test/lib/python3.10/site-packages/setuptools/__init__.py", line 8, in <module>
    import _distutils_hack.override  # noqa: F401
  File "/home/lesteve/miniconda3/envs/test/lib/python3.10/site-packages/_distutils_hack/override.py", line 1, in <module>
    __import__('_distutils_hack').do_override()
  File "/home/lesteve/miniconda3/envs/test/lib/python3.10/site-packages/_distutils_hack/__init__.py", line 72, in do_override
    ensure_local_distutils()
  File "/home/lesteve/miniconda3/envs/test/lib/python3.10/site-packages/_distutils_hack/__init__.py", line 59, in ensure_local_distutils
    assert '_distutils' in core.__file__, core.__file__
AssertionError: /home/lesteve/miniconda3/envs/test/lib/python3.10/distutils/core.py
  1. python -c 'import setuptools; import pip' gives you only a warning about setuptools replacing distutils
  2. importing pip before setuptools breaks and it is considered as a won't fix: [BUG] ensure_local_distutils is failing when pip is imported before setuptools pypa/setuptools#3044. The reason is that you are not supposed to import pip see e.g. https://pip.pypa.io/en/latest/user_guide/#using-pip-from-your-program or Error with import pip in pip 9.0.2 pypa/pip#5081 (comment)
  3. joblib removed some distutils version classes usage in Bump up dependency versions on the CI config joblib/joblib#1272. As a side-effect distutils is not imported by joblib and so when importing sklearn you are anymore importing distutils (hence setuptools) before `pipand you end up in case 1 with an error
@lesteve
Copy link
Member Author

lesteve commented Feb 25, 2022

After chatting a bit with @ogrisel, it may be that the most pragmatic way of tackling this issue would be to reintroduce an unused import distutils in joblib.
Pros:

  • avoid unnecessary disruption (people with weird errors when using sklearn.show_versions(), telling them to update scikit-learn or downgrade joblib, or giving them manual instructions to get the info we need)
  • avoid the work of doing a scikit-learn bug-fix release

Cons:

  • not super satisfactory but oh well 🤷

@thomasjpfan
Copy link
Member

most pragmatic way of tackling this issue would be to reintroduce an unused import distutils in joblib

If joblib devs are okay with this, then I think this is the best path forward.

If joblib does not want to import distuils all the time, joblib can check the version of scikit-learn and then import disutils only if scikit-learn < 1.1.

@thomasjpfan thomasjpfan added Bug Build / CI and removed Needs Triage Issue requires triage labels Feb 25, 2022
@ogrisel
Copy link
Member

ogrisel commented Feb 25, 2022

I think we can just keep on importing distutils for nothing in joblib for one year to ensure a safe overlap.

But we need to switch to importlib.metadata.version-based version introspection in scikit-learn's show_versions() before releasing 1.1.

@ogrisel
Copy link
Member

ogrisel commented Feb 26, 2022

All the work mentioned above has been done in joblib/joblib#1277 and #22621, the latter requires #22617 to be merged first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants