[MRG] Remove BLAS infos from show_versions and build log by jeremiedbb · Pull Request #14205 · scikit-learn/scikit-learn

jeremiedbb · 2019-06-27T14:19:17Z

remove BLAS infos from show_versions()
remove BLAS from the dependencies in advanced installations doc
remove BLAS infos displayed in the build log

jeremiedbb · 2019-06-27T14:22:38Z

doc/developers/advanced_installation.rst

-Under Debian-based operating systems, which include Ubuntu::
+Installing from source without conda or pip requires you to have installed the
+scikit-learn runtime dependencies, Python development headers and a working
+C/C++ compiler. Under Debian-based operating systems, which include Ubuntu::


I'm not sure to get the purpose of this section. It seems to give the dependencies if you want to build without pip or conda. But everywhere else we only explain how to install everything with pip.

This is to build from source. pip can't install python headers or compilers. If you want to build from source without conda you have to install compiler and headers.
Listing the python packages here is a bit strange, though. I think that is to avoid having to build them, these instructions predate binary wheels.

So I think part of this can be removed, but we should have instructions on how to build without conda.

Ok I see. I updated a bit. Let me know what you think

jnothman · 2019-07-02T04:55:45Z

Will we still want BLAS details from the scipy installation in order to diagnose efficiency issues??

jeremiedbb · 2019-07-02T12:29:33Z

Will we still want BLAS details from the scipy installation in order to diagnose efficiency issues??

We can get some infos about BLAS from numpy and scipy installations with numpy/scipy.get_config(). I added them to show_versions.

However we need to be careful with this as it may not always display the right info. I don't know in which situation it can happen... For instance, if I pip install scipy in a conda env, the path in library_dirs points to usr/lib (where I have an openblas) instead of to the lib folder of site_packages/scipy in my conda env (ldd points to that folder).

jnothman · 2019-07-02T12:39:15Z

Does that imply that get_config is buggy?

rth · 2019-07-02T12:53:29Z

Does that imply that get_config is buggy?

I think get_config correctly solves the problem of which BLAS to link against if you were to build from sources, but it won't tell you if numpy uses the MKL BLAS with conda and scipy uses OpenBlas as it was installed with pip.

For instance, https://stackoverflow.com/a/37190672/1791279 illustrates that there is no easy solution.

Maybe this is something threadpoolctl could detect I'm not sure, but for now I think it's OK to merge this as the currently displayed information can be misleading anyway.

jeremiedbb · 2019-07-02T12:57:21Z

(sorry I wrote it wrong, it's show_config)

jeremiedbb · 2019-07-02T13:00:04Z

thank's for the explanation.

threadpoolctl do detect it. It acts like ldd, so it finds the exact lib<whatever blas>.so that is loaded when you import numpy or scipy

rth · 2019-07-02T12:55:45Z

doc/developers/advanced_installation.rst

-        /usr/lib/atlas-base/atlas/libblas.so.3
-    sudo update-alternatives --set liblapack.so.3 \
-        /usr/lib/atlas-base/atlas/liblapack.so.3
+    pip3 install numpy scipy cython


Why not installing python3-numpy etc? e.g. on ARM you don't have wheel I think while there are distributions, in which case using the system numpy/scipy makes the most sense.

Currently this is under installing build dependencies. I don't think installing system numpy scipy is the usual on ubuntu (which is the target of this command). Maybe we can add how to install system numpy scipy cython as an alternative when binary wheels aren't available ?

The remark of @rth is relevant regarding ARM is relevant. The last time I played with an ARM, I needed to install numpy from python3-numpy to be able to compile scikit-learn from source.

I added a note to install system dependencies when wheels aren't available

rth · 2019-07-02T13:00:58Z

sklearn/utils/_show_versions.py

+    numpy.show_config()
+
+    print('\nScipy BLAS/LAPACK:')
+    scipy.show_config()


This prints half a page of text, we don't really want it in issue reports I think? Also, wouldn't it suffer from the same limitations as the previous _get_blas_info ?

I think show_config is more accurate in general. It's based on infos gathered when building numpy, whereas the previous _get_blas_info takes its infos from numpy.distutils.system_info.get_info which gather infos at run time.

But I'm really not sure how both work so I might be completely wrong...

amueller · 2019-07-02T14:59:54Z

doc/developers/advanced_installation.rst

-            /usr/lib/openblas-base/libopenblas.so.0
-        sudo update-alternatives --set liblapack.so.3 \
-            /usr/lib/lapack/liblapack.so.3
+        sudo apt-get install python3-matplotlib


why system matplotlib? Seems inconsistent with numpy/scipy above.

amueller · 2019-07-02T15:00:24Z

doc/developers/advanced_installation.rst

 On Red Hat and clones (e.g. CentOS), install the dependencies using::

-    sudo yum -y install gcc gcc-c++ numpy python-devel scipy
+    sudo yum -y install gcc gcc-c++ python-devel numpy scipy


again, inconsistent with above.
I don't care that much which way we go but we should be consistent. I slightly prefer the pip way.

jeremiedbb · 2019-07-05T13:49:00Z

on ARM you don't have wheel I think

@rth maybe soon: see :)

jeremiedbb · 2019-07-05T13:54:01Z

On second thought, I removed numpy/scipy show_config from show_versions. As Roman pointed out, the output is very large and can display a wrong information. Besides, it's not as before when we had a bundled BLAS and we needed to show infos about that. Most issues are not BLAS related, and when we suspect it might be we can ask users to report numpy/scipy.show_config().

jeremiedbb · 2019-07-05T13:55:25Z

I tried to come up to more consistent instructions to install the build dependencies, but I don't know how to update it for centOS. Does anyone can help me with that ?

glemaitre

Oh I see I did not see the change regarding both system and pip are mentioned.
So LGTM then.

rth

Minor comment, otherwise LGTM.

rth · 2019-07-18T08:29:49Z

doc/developers/advanced_installation.rst

-            /usr/lib/lapack/liblapack.so.3
+    sudo apt-get install cython3 python3-numpy python3-scipy python3-matplotlib

 On Red Hat and clones (e.g. CentOS), install the dependencies using::


Maybe add

To use a high performance BLAS library (e.g. OpenBlas) see `scipy installation instructions <https://docs.scipy.org/doc/scipy/reference/building/linux.html>`_.

rth · 2019-07-18T10:09:54Z

doc/developers/advanced_installation.rst

+.. note::

+    To use a high performance BLAS library (e.g. OpenBlas) see 
+    `scipy installation instructions <https://docs.scipy.org/doc/scipy/reference/building/linux.html>`_.


Added a line break, to keep below 80 char length.

rth · 2019-07-18T10:55:37Z

Merging. Thanks @jeremiedbb !

…n#14205)

jeremiedbb added 4 commits June 27, 2019 16:14

remove blas infos from show_versions

8069e68

remove BLAS from dependencies doc

b022e46

remove blas infos from build log

9f32056

remove blas considerations from dependencies & stuff

41436d1

jeremiedbb commented Jun 27, 2019

View reviewed changes

jeremiedbb added 2 commits June 27, 2019 16:53

remove testing blas in show versions

71127c5

no conda build dependencies

ea2d7ea

jeremiedbb added 2 commits July 2, 2019 14:17

show numpy scipy blas config

3558f8b

revert tests modif

22dcbb0

rth reviewed Jul 2, 2019

View reviewed changes

amueller reviewed Jul 2, 2019

View reviewed changes

jeremiedbb added 3 commits July 5, 2019 15:21

remove blas from show versions

2f00bad

same

0dbc9fc

more consistent doc to install build deps

82ab2e3

glemaitre added this to the 0.21.3 milestone Jul 18, 2019

glemaitre approved these changes Jul 18, 2019

View reviewed changes

rth reviewed Jul 18, 2019

View reviewed changes

jeremiedbb and others added 2 commits July 18, 2019 11:43

mention scipy instructions

28851d5

Add line-break

5598754

rth approved these changes Jul 18, 2019

View reviewed changes

rth merged commit 01a140a into scikit-learn:master Jul 18, 2019

jnothman pushed a commit to jnothman/scikit-learn that referenced this pull request Jul 24, 2019

MAINT Remove BLAS infos from show_versions and build log (scikit-lear…

0b3ea8a

…n#14205)

jacobmontiel mentioned this pull request Aug 1, 2019

[FEATURE] SST-HT for Multi-target regression scikit-multiflow/scikit-multiflow#123

Merged

6 tasks

hayesall mentioned this pull request Aug 9, 2019

[MRG] 👽 Maintenance for imblearn.show_versions(), fix Travis build scikit-learn-contrib/imbalanced-learn#591

Merged

Uh oh!

Conversation

jeremiedbb commented Jun 27, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnothman commented Jul 2, 2019

Uh oh!

jeremiedbb commented Jul 2, 2019

Uh oh!

jnothman commented Jul 2, 2019 via email

Uh oh!

rth commented Jul 2, 2019

Uh oh!

jeremiedbb commented Jul 2, 2019

Uh oh!

jeremiedbb commented Jul 2, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeremiedbb commented Jul 5, 2019

Uh oh!

jeremiedbb commented Jul 5, 2019

Uh oh!

jeremiedbb commented Jul 5, 2019

Uh oh!

glemaitre left a comment

Choose a reason for hiding this comment

Uh oh!

rth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rth commented Jul 18, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants