Skip to content

BLD: Allow easy use of openblas.#3642

Merged
charris merged 5 commits intonumpy:masterfrom
jseabold:openblas-install
Sep 5, 2013
Merged

BLD: Allow easy use of openblas.#3642
charris merged 5 commits intonumpy:masterfrom
jseabold:openblas-install

Conversation

@jseabold
Copy link
Copy Markdown
Contributor

I'm not the author of this patch, but this has become a major pain point for me. Please offer suggestions for updating the site.cfg or better practices.

Will close #3571.

cc // @akesandgren

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This unconditional import is fine, and better than the conditional import it gets replaced with below...

@jseabold
Copy link
Copy Markdown
Contributor Author

Original commit.

akesandgren@363339d

Let me cherry-pick the changes again and fix the regressions. I will force push to this PR.

@jseabold
Copy link
Copy Markdown
Contributor Author

See if that's better. Removed the regressions and removed the change for the below

blas_info = get_info('blas')

@njsmith
Copy link
Copy Markdown
Member

njsmith commented Aug 20, 2013

Much better, thanks...

I really don't understand the overall logic for how it picks which library to use for things like BLAS -- I'm not sure anyone does -- but there's a special case for OSX at the top of blas_opt_info that checks for the $ATLAS envvar. I think the effect is that on OSX it always uses Accelerate unless $ATLAS is set to something, and ignores all other configuration settings. Whether this makes sense or not is one thing, but whatever we do it seems like openblas and atlas should get the same treatment?

Also just to be clear: I think this changes the defaults so that if openblas and atlas are both available, it uses openblas. Is that correct, and is it desirable?

@pv
Copy link
Copy Markdown
Member

pv commented Aug 20, 2013

The overall logic is quite simple: a setup.py asks for get_info('blas_opt') and it eventually ends up in blas_opt_info.calc_info which tries the BLAS libraries one by one.

@pv
Copy link
Copy Markdown
Member

pv commented Aug 20, 2013

Just a note: someone should spend a bit of time understanding how the INI file is supposed to work and then write the damned thing down in the documentation so that it doesn't seem like ugly magic to future generations...

@njsmith
Copy link
Copy Markdown
Member

njsmith commented Aug 20, 2013

Right, I mean specifically the logic for how it picks which one to use when
multiple might work.

On Tue, Aug 20, 2013 at 6:49 PM, Pauli Virtanen notifications@github.comwrote:

The overall logic is quite simple: a setup.py asks for
get_info('blas_opt') and it eventually ends up in blas_opt_info.calc_infowhich tries the BLAS libraries one by one.


Reply to this email directly or view it on GitHubhttps://github.com//pull/3642#issuecomment-22963654
.

@charris
Copy link
Copy Markdown
Member

charris commented Sep 2, 2013

@pv @njsmith Is this ready?

@pv
Copy link
Copy Markdown
Member

pv commented Sep 2, 2013

Might need to add an and not os.environ.get('OPENBLAS', None) to the Accelerate check, otherwise looks OK.

@charris
Copy link
Copy Markdown
Member

charris commented Sep 2, 2013

@pv Thanks for the update. Looks like this would be a backport candidate for 1.8..0 if the timing works out...

@njsmith
Copy link
Copy Markdown
Member

njsmith commented Sep 2, 2013

I have no real idea, I'm far from an expert on numpy's build system, just
wanted to raise a flag because I couldn't tell what effect this had on
people's ability to select e.g. atlas over openblas.

I'd suggest it go into a prerelease as soon as possible if it goes into 1.8
at all, given that it's the sort of change that will only show problems
when tested in lots of configurations.
On 2 Sep 2013 18:36, "Charles Harris" notifications@github.com wrote:

@pv https://github.com/pv Thanks for the update. Looks like this would
be a backport candidate for 1.8..0 if the timing works out...


Reply to this email directly or view it on GitHubhttps://github.com//pull/3642#issuecomment-23671735
.

@charris
Copy link
Copy Markdown
Member

charris commented Sep 2, 2013

Agree, if it is backported it should be in a beta or maybe an rc1. The idea is that things don't get widely tested until the release process starts up and it sound like this might be helpful.

@jseabold
Copy link
Copy Markdown
Contributor Author

jseabold commented Sep 3, 2013

Where should I add the OSX checks? It's not completely obvious to me.

ogrisel referenced this pull request in akesandgren/numpy Sep 4, 2013
 - openblas is usually a lot faster then ATLAS so make sure it can get
   correctly detected.
blas_opt and lapack_opt sections are not actually read by the
numpy.distutils.system_info machinery. They are just meta entry points for the
`get_info` public function that in turns introspec the atlas, openblas and mkl
sections.
@ogrisel
Copy link
Copy Markdown
Contributor

ogrisel commented Sep 4, 2013

I have opened a new pull request jseabold#1 against this branch to solve the OSX issue: the ATLAS env variable is no longer expected. Regular atlas_info / atlas_threads_info introspection is used instead. If there is no mkl, openblas or atlas section in the site.cfg file, then the OSX build will automatically fallback to vecLib or Accelerate (depending on the version of OSX).

I also took the liberty to remove the mistaken examples to use [blas_opt] and [lapack_opt] sections from the site.cfg.example file as I think there are misleading: the code in numpy.distutils.system_info will never look for those sections and instead fallback to mkl, openblas or atlas info instead (or OSX system blas when available).

@charris
Copy link
Copy Markdown
Member

charris commented Sep 4, 2013

I'd like to make a 1.8.0b2 release this weekend, so it would be good if this can be settled by then.

@ogrisel
Copy link
Copy Markdown
Contributor

ogrisel commented Sep 4, 2013

I did some build test with the current state of my branch jseabold#1: I was able to build numpy successfully against:

  • OSX framework without any site.cfg nor specific env variable
  • Manually installed Atlas 3.8.3 in /opt/atlas using the following site.cfg:
[atlas]
library_dirs = /opt/atlas/lib
include_dirs = /opt/atlas/include
  • Manually installed OpenBLAS (master from github) in /opt/OpenBLAS using the following site.cfg:
[openblas]
library_dirs = /opt/OpenBLAS/lib
include_dirs = /opt/OpenBLAS/include

Under Ubuntu 13.014, I installed libatlas-base and libatlas-base-dev (and python-dev) and then manually created the following symlinks to *.so.3 files:

$ ls -l /usr/lib/atlas-base/
total 6852
drwxr-xr-x 2 root root    4096 Sep  4 15:05 atlas
lrwxrwxrwx 1 root root      35 Sep  4 15:02 libatlas.so -> /usr/lib/atlas-base/libatlas.so.3.0
lrwxrwxrwx 1 root root      15 Nov  6  2012 libatlas.so.3 -> libatlas.so.3.0
-rw-r--r-- 1 root root 6644744 Nov  6  2012 libatlas.so.3.0
lrwxrwxrwx 1 root root      33 Sep  4 15:02 libcblas.so -> /usr/lib/atlas-base/libcblas.so.3
lrwxrwxrwx 1 root root      15 Nov  6  2012 libcblas.so.3 -> libcblas.so.3.0
-rw-r--r-- 1 root root  118936 Nov  6  2012 libcblas.so.3.0
lrwxrwxrwx 1 root root      35 Sep  4 15:03 libf77blas.so -> /usr/lib/atlas-base/libf77blas.so.3
lrwxrwxrwx 1 root root      17 Nov  6  2012 libf77blas.so.3 -> libf77blas.so.3.0
-rw-r--r-- 1 root root  118616 Nov  6  2012 libf77blas.so.3.0
lrwxrwxrwx 1 root root      42 Sep  4 15:03 liblapack_atlas.so -> /usr/lib/atlas-base/liblapack_atlas.so.3.0
lrwxrwxrwx 1 root root      22 Nov  6  2012 liblapack_atlas.so.3 -> liblapack_atlas.so.3.0
-rw-r--r-- 1 root root  122224 Nov  6  2012 liblapack_atlas.so.3.0
$ ls -l /usr/lib/atlas-base/atlas/
total 16872
lrwxrwxrwx 1 root root       14 Nov  6  2012 libblas.so.3 -> libblas.so.3.0
-rw-r--r-- 1 root root  6856360 Nov  6  2012 libblas.so.3.0
lrwxrwxrwx 1 root root       40 Sep  4 15:05 liblapack.so -> /usr/lib/atlas-base/atlas/liblapack.so.3
lrwxrwxrwx 1 root root       16 Nov  6  2012 liblapack.so.3 -> liblapack.so.3.0
-rw-r--r-- 1 root root 10418480 Nov  6  2012 liblapack.so.3.0

Then I was able to build against the system atlas without the need for any site.cfg or env variable.

Finally I have also build numpy against a fresh build of OpenBLAS in /opt/OpenBLAS under Linux with the same site.cfg file as I used under OSX.

Note: under Linux I had to explicitly:

export LD_LIBRARY_PATH=$LD_LIBRARY_PATH:/opt/OpenBLAS/lib

to be able to import numpy. Otherwise I would have got a:

ImportError: libopenblas.so.0: cannot open shared object file: No such file or directory

This seems expected but apparently this step is not necessary under OSX for some reason.

To make sure that OpenBLAS was properly used by numpy I changed the value of the OPENBLAS_NUM_THREADS env var from 1 to 8 to see the impact on the CPU usage when doing a big np.dot(x, x.T) with x = np.random.norma(size=(2000, 5000)).

I have also built scipy and scikit-learn against the OpenBLAS numpy under OSX without any problem.

@njsmith
Copy link
Copy Markdown
Member

njsmith commented Sep 4, 2013

@jseabold: Have you had a chance to look at Oliver's patch? Any opinion?

On Wed, Sep 4, 2013 at 4:38 PM, Olivier Grisel notifications@github.comwrote:

I did some build test with the current state of my branch: I was able to
build numpy successfully against:

  • OSX framework without any site.cfg nor specific env variable
  • Manually installed Atlas 3.8.3 in /opt/atlas using the following
    site.cfg:

[atlas]
library_dirs = /opt/atlas/lib
include_dirs = /opt/atlas/include

  • Manually installed OpenBLAS (master from github) in /opt/OpenBLAS
    using the following site.cfg:

[openblas]
library_dirs = /opt/OpenBLAS/lib
include_dirs = /opt/OpenBLAS/include

Under Ubuntu 13.014, I installed libatlas-base and libatlas-base-dev (and
python-dev) and then manually created the following symlinks to *.so.3files:

$ ls -l /usr/lib/atlas-base/
total 6852
drwxr-xr-x 2 root root 4096 Sep 4 15:05 atlas
lrwxrwxrwx 1 root root 35 Sep 4 15:02 libatlas.so -> /usr/lib/atlas-base/libatlas.so.3.0
lrwxrwxrwx 1 root root 15 Nov 6 2012 libatlas.so.3 -> libatlas.so.3.0
-rw-r--r-- 1 root root 6644744 Nov 6 2012 libatlas.so.3.0
lrwxrwxrwx 1 root root 33 Sep 4 15:02 libcblas.so -> /usr/lib/atlas-base/libcblas.so.3
lrwxrwxrwx 1 root root 15 Nov 6 2012 libcblas.so.3 -> libcblas.so.3.0
-rw-r--r-- 1 root root 118936 Nov 6 2012 libcblas.so.3.0
lrwxrwxrwx 1 root root 35 Sep 4 15:03 libf77blas.so -> /usr/lib/atlas-base/libf77blas.so.3
lrwxrwxrwx 1 root root 17 Nov 6 2012 libf77blas.so.3 -> libf77blas.so.3.0
-rw-r--r-- 1 root root 118616 Nov 6 2012 libf77blas.so.3.0
lrwxrwxrwx 1 root root 42 Sep 4 15:03 liblapack_atlas.so -> /usr/lib/atlas-base/liblapack_atlas.so.3.0
lrwxrwxrwx 1 root root 22 Nov 6 2012 liblapack_atlas.so.3 -> liblapack_atlas.so.3.0
-rw-r--r-- 1 root root 122224 Nov 6 2012 liblapack_atlas.so.3.0
$ ls -l /usr/lib/atlas-base/atlas/
total 16872
lrwxrwxrwx 1 root root 14 Nov 6 2012 libblas.so.3 -> libblas.so.3.0
-rw-r--r-- 1 root root 6856360 Nov 6 2012 libblas.so.3.0
lrwxrwxrwx 1 root root 40 Sep 4 15:05 liblapack.so -> /usr/lib/atlas-base/atlas/liblapack.so.3
lrwxrwxrwx 1 root root 16 Nov 6 2012 liblapack.so.3 -> liblapack.so.3.0
-rw-r--r-- 1 root root 10418480 Nov 6 2012 liblapack.so.3.0

Then I was able to build against the system atlas without the need for any
site.cfg or env variable.

Finally I have also build numpy against a fresh build of OpenBLAS in
/opt/OpenBLAS under Linux with the same site.cfg file as I used under OSX.

Note: under Linux I had to explicitly:

export LD_LIBRARY_PATH=$LD_LIBRARY_PATH:/opt/OpenBLAS/lib

to be able to import numpy. Otherwise I would have got a:

ImportError: libopenblas.so.0: cannot open shared object file: No such file or directory

This seems expected but apparently this step is not necessary under OSX
for some reason.

To make sure that OpenBLAS was properly used by numpy I changed the value
of the OPENBLAS_NUM_THREADS env var from 1 to 8 to see the impact on the
CPU usage when doing a big np.dot(x, x.T) with x =
np.random.norma(size=(2000, 5000)).

I have also build scipy and scikit-learn against the OpenBLAS numpy under
OSX without any problem.


Reply to this email directly or view it on GitHubhttps://github.com//pull/3642#issuecomment-23799486
.

@jseabold
Copy link
Copy Markdown
Contributor Author

jseabold commented Sep 4, 2013

I glanced over it, but, no, not in any great detail. I don't have any strong opinion here, as it wasn't really my patch. As long as it works and is documented clearly, I'm happy. Note I'm also adding the path to LD_LIBRARY_PATH. I also had to specify BLAS and LAPACK env variables to point to the openblas shared library for building scipy, though it sounds like this is no longer necessary with this patch?

@jseabold
Copy link
Copy Markdown
Contributor Author

jseabold commented Sep 4, 2013

If it's fine with y'all I can merge it and push. Just let me know.

@charris
Copy link
Copy Markdown
Member

charris commented Sep 4, 2013

@jseabold Go ahead.

@jseabold
Copy link
Copy Markdown
Contributor Author

jseabold commented Sep 4, 2013

Done.

@ogrisel
Copy link
Copy Markdown
Contributor

ogrisel commented Sep 4, 2013

BLAS and LAPACK env variables to point to the openblas shared library for building scipy, though it sounds like this is no longer necessary with this patch?

I installed numpy against with the "site.cfg" mentioned above and then could just pip install scipy without any BLAS or LAPACK environment variable. The site.cfg of numpy was installed in the venv/lib/python2.7/site-packages/numpy/distutils/site.cfg and it was enough for scipy to discover the location of openblas (by asking numpy using the get_info('blas_opt') and get_info('lapack_opt') idioms presumably).

@charris
Copy link
Copy Markdown
Member

charris commented Sep 4, 2013

@jseabold @ogrisel I'll let you folks decide if this should be merged as long as the usual builds aren't affected.

@charris
Copy link
Copy Markdown
Member

charris commented Sep 4, 2013

@cournape @rkern And let's get a third opinion if possible.

@ogrisel
Copy link
Copy Markdown
Contributor

ogrisel commented Sep 4, 2013

FYI: I reported the fork + OpenBLAS GEMM crash here: OpenMathLib/OpenBLAS#294 (it's based on previous debugging done in collaboration with @cournape for a similar bug with Apple Accelerate).

@ogrisel
Copy link
Copy Markdown
Contributor

ogrisel commented Sep 4, 2013

@jseabold @ogrisel I'll let you folks decide if this should be merged as long as the usual builds aren't affected.

Well I reported about the manual builds I did to test. Is there an automated build infrastructure to check whether this introduces any build, test or performance regression on other platforms?

@charris
Copy link
Copy Markdown
Member

charris commented Sep 4, 2013

I'm very much not a build guy. Maybe @pv or @rgommers has something to say about that.

@charris
Copy link
Copy Markdown
Member

charris commented Sep 5, 2013

BTW, this should also be mentioned in the 1.8.0 release notes as I'm inclined to put this in unless something bad turns up in the next few days.

@rgommers
Copy link
Copy Markdown
Member

rgommers commented Sep 5, 2013

Looks fine to me. Given all the testing @ogrisel did, should be fine to merge. @ogrisel no automated build infrastructure other than Travis unfortunately.

@charris
Copy link
Copy Markdown
Member

charris commented Sep 5, 2013

Thank Ralf. In it goes. @jseabold Can you make another PR to update the release notes?

charris added a commit that referenced this pull request Sep 5, 2013
BLD: Allow easy use of openblas.
@charris charris merged commit b5dab6d into numpy:master Sep 5, 2013
@jseabold
Copy link
Copy Markdown
Contributor Author

jseabold commented Sep 5, 2013

Would've been easier to do here. Where should I make the changes?

@jseabold jseabold deleted the openblas-install branch September 5, 2013 20:38
@charris
Copy link
Copy Markdown
Member

charris commented Sep 5, 2013

Just make another PR.

charris added a commit to charris/numpy that referenced this pull request Sep 5, 2013
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.

include this patch for building with openblas?

6 participants