Build py-numpy with MKL by glennpj · Pull Request #2361 · spack/spack

glennpj · 2016-11-18T20:27:48Z

This PR gets a GCC built python to use MKL for blas/lapack. This is not perfect and I
have put comments for TODO items.

The changes to intel-parallel-studio are to get the correct library path for blas. I was
using that to attempt to use it but there were a couple of other issues. The changes to
the mkl package are to get the proper paths in the environment module, similar to what
was done previously for the intel-parallel-studio package.

For py-numpy itself, the section title for the defaults changed from DEFAULTS to ALL with
numpy-1.10 so that is configured accordingly. Also, for MKL to be used there must be a
section called, '[mkl]'. Finally, the mkl_rt library is needed, especially when built
with GCC. As such, that library is made explicit in the [mkl] section of the config file.

This PR gets a GCC built python to use MKL for blas/lapack. This is not perfect and I have put comments for TODO items. The changes to intel-parallel-studio are to get the correct library path for blas. I was using that to attempt to use it but there were a couple of other issues. The changes to the mkl package are to get the proper paths in the environment module, similar to what was done previously for the intel-parallel-studio package. For py-numpy itself, the section title for the defaults changed from DEFAULTS to ALL with numpy-1.10 so that is configured accordingly. Also, for MKL to be used there must be a section called, '[mkl]'. Finally, the mkl_rt library is needed, especially when built with GCC. As such, that library is made explicit in the [mkl] section of the config file.

adamjstewart · 2016-11-18T20:33:37Z

+        env.prepend_path('LD_LIBRARY_PATH',
+                         join_path(self.prefix, 'lib', 'intel64'))
+        env.prepend_path('LIBRARY_PATH',
+                         join_path(self.prefix, 'lib', 'intel64'))


So you remove, and then prepend, and then prepend a second time?

Oops, looks like duplicates sneaked in there. I will remove those.

davydden

Also, for MKL to be used there must be a section called, '[mkl]'.

i think it can, but it does NOT have to.

Finally, the mkl_rt library is needed, especially when built with GCC.

from my experience on Ubuntu16 with GCC@5.4 i never had any needs to use mkl_rt anywhere when building trilinos, petsc, mumps, superlu_dist, hypre and a number of other linear algebra packages.

I must admit i never tried building py-numpy with MKL, but I would be surprised if it does not work.

davydden · 2016-11-18T20:31:55Z


    def setup_environment(self, spack_env, env):
+        # Remove paths that were guessed but are incorrect for this package.
+        env.remove_path('LIBRARY_PATH',


AFAIK Spack does not use LD_LIBRARY_PATH or alike anywhere, everything is RPATH'ed, why do you need all those changes?

in other words, if for any packages in Spack you need to set LD_LIBRARY_PATH to have something working, then something is wrong.

davydden · 2016-11-18T20:32:24Z

        mkl_libs = find_libraries(
            mkl_integer + ['libmkl_core'] + mkl_threading,
-            root=join_path(self.prefix.lib, 'intel64'),
+            root=join_path(self.prefix, 'mkl', 'lib', 'intel64'),


that looked like a bug, it's probably better have it in a separate commit.

maybe in a separate PR which can be quickly merged. Then you will rebase this one to get rid of this change.

I'm fine with this change being in this PR. You can't really test this PR without it I imagine.

you surely can't. I am just saying to make things fast one could submit this change as a stand-alone PR, but still keep it here. Once a stand alone PR is merged, one can re-base this one on develop.

But i don't really insist on this, just explain my rationale.

davydden · 2016-11-18T20:36:14Z

+                    # mkl that package will not work here with a non-Intel
+                    # compiler due to header collision.
+                    f.write('[mkl]\n')
+                    f.write('libraries=mkl_rt\n')


i am not in favour of this change. join(lapackblas.names) depends on whole lot of variants (32/64 bits, threaded/serial, and alike). Once you use mkl_rt here it is now up to environment variable. In other words you introduce a way to use different BLAS/LAPACK in DAG.

What was wrong exactly with 'join(lapackblas.names)? At the end of the day we give the complete library lists there and it is bound to work. At least it does in a number of other packages without a need to use mkl_rt anywhere.

can you please try keeping f.write('libraries=%s\n' % ','.join(lapackblas.names)) unconditionally while using f.write('[mkl]\n') for MKL if py-numpy really want it this way?

davydden · 2016-11-18T20:41:46Z

p.s. if py-numpy config system is crazy, so be it, but please try avoid using mkl_rt for consistency with other libraries and stick with lapackblas.names.

glennpj · 2016-11-18T21:17:11Z

Okay; I guess I should have split this in multiple PRs. Regarding LD_LIBRARY_PATH, that is needed for the environment module not for the build.

Regarding the other comments, yes, the [mkl] section is needed in the numpy site.cfg, unless the config is removed it would fall back to MKLROOT. It seemed better to have the section in the config rather than rely on the environment variable.

Using mkl_rt is needed for numpy as far as I can tell. Intel states this as well here: https://software.intel.com/en-us/articles/numpyscipy-with-intel-mkl

Building with GNU Compiler chain:

Make modifications to MKL section in the site.cfg as mentioned above. To build numpy and scipy with Gnu compilers, in the site.cfg file, you must link with mkl_rt only and any other linking method will not work.

Using mkl_rt can be directed by environment variables at run time but it does not depend on them.

I think that addresses the bulk of the comments. I am not really sure how to proceed here given the comments.

davydden · 2016-11-18T21:24:16Z

Regarding LD_LIBRARY_PATH, that is needed for the environment module not for the build.

still don't understand why. Everything is built with RPATH's in Spack, so LD_LIBRARY_PATH shall not be used at all.

Using mkl_rt is needed for numpy as far as I can tell.

I am not saying it does not build with mkl_rt, I am saying it is NOT the only way, actually it is even mentioned in SciPy's website:
https://www.scipy.org/scipylib/building/linux.html#intel-mkl-11-0-updated-dec-2012

So please try using lapackblas.names.

Using mkl_rt can be directed by environment variables at run time but it does not depend on them.

it's not about that. The whole point of NOT using it is to provide consistent BLAS/LAPACK everwhere in the DAG. That is done through the extra class which holds libraries and have methods like names() and alike.

I am not really sure how to proceed here given the comments.

Please try ~~(i) not to set LD_LIBRARY_PATH, it shall not be needed;~~ (ii) use lapackblas.names, the link above suggests that it shall work.

p.s. edited based on @adamjstewart answer below.

davydden · 2016-11-18T21:25:53Z

FYI, from SciPy org (emphasize is mine):

Instead of the layered linking approach for the Intel® MKL as shown above, you MAY also use the dynamic interface lib mkl_rt.lib.

"shown above" is

[mkl]
library_dirs = /opt/intel/composer_xe_2013/mkl/lib/intel64
include_dirs = /opt/intel/composer_xe_2013/mkl/include
mkl_libs = mkl_intel_lp64,mkl_intel_thread,mkl_core

adamjstewart · 2016-11-18T21:29:24Z

Regarding LD_LIBRARY_PATH, that is needed for the environment module not for the build.

still don't understand why. Everything is built with RPATH's in Spack, so LD_LIBRARY_PATH shall not be used at all.

Python packages (like py-scipy) don't currently use RPATH. Also, anyone loading the MKL module outside of Spack for building their own code would surely want to set LD_LIBRARY_PATH. We do for all packages, it just so happens that the directory is wrong for MKL.

davydden · 2016-11-18T21:31:34Z

@adamjstewart :

Python packages (like py-scipy) don't currently use RPATH.

that explains a lot, thanks

glennpj · 2016-11-18T21:35:53Z

@davydden Note the caveat that GCC requires mkl_rt, not the intel compiler, which is what your link refers to. Also, I did try to make it work with the standard blas_libs before agreeing with Intel that GCC requires mkl_rt.

Anyway, I will close this PR as clearly I have missed the mark.

davydden · 2016-11-18T21:36:46Z

Note the caveat that GCC requires mkl_rt

but why? As I said above i had no problems building with MKL (without mkl_rt) and GCC. Can you elaborate?

Also, I did try to make it work with the standard blas_libs

what was the problem?

davydden · 2016-11-18T21:40:58Z

for the record, Intel's link advisor with Select compiler: Gnu C/C++ says

-L${MKLROOT}/lib -Wl,-rpath,${MKLROOT}/lib -lmkl_intel_lp64 -lmkl_sequential -lmkl_core -lpthread -lm -ldl

so i see no reason why GCC would need mkl_rt for py-numpy.

adamjstewart reviewed Nov 18, 2016

View reviewed changes

davydden suggested changes Nov 18, 2016

View reviewed changes

glennpj closed this Nov 18, 2016

This was referenced Nov 18, 2016

intel-parallel-studio: fix location of MKL libs #2364

Merged

py-numpy: rework blas/lapack #2365

Merged

glennpj deleted the numpy_blas branch November 19, 2016 01:43

krafczyk requested review from krafczyk and removed request for krafczyk February 3, 2017 21:01

Conversation

glennpj commented Nov 18, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davydden left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

davydden Nov 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davydden commented Nov 18, 2016

Uh oh!

glennpj commented Nov 18, 2016

Uh oh!

davydden commented Nov 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davydden commented Nov 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adamjstewart commented Nov 18, 2016

Uh oh!

davydden commented Nov 18, 2016

Uh oh!

glennpj commented Nov 18, 2016

Uh oh!

davydden commented Nov 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davydden commented Nov 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

davydden left a comment •

edited

Loading

davydden Nov 18, 2016 •

edited

Loading

davydden commented Nov 18, 2016 •

edited

Loading

davydden commented Nov 18, 2016 •

edited

Loading

davydden commented Nov 18, 2016 •

edited

Loading

davydden commented Nov 18, 2016 •

edited

Loading