Fix py-scipy build with intel compilers by s-sajid-ali · Pull Request #8817 · spack/spack

s-sajid-ali · 2018-07-27T20:23:10Z

Closes #8616, closes #3204
(Feel free to reopen the second issue if the goal is to not link to mkl_rt)

Need to resolve the F90 env variable setting first. I was able to install with --dirty and this build recipe.

Refer to 1.
Numpy parallel builds now work. Refer to 2.

Test matrix :

(Note : Known issues with intel-mkl while testing.)

CentOS/Py 2.7.14	Intel-MKL	OpenBLAS
Intel18	[ ]	[ ]
GCC 7.3.0	Pass	Pass
Clang 6.0.1	[ ]	[ ]

CentOS/Py 3.6.6	Intel-MKL	OpenBLAS
Intel18	Pass	[ ]
GCC 7.3.0	Pass	Pass
Clang 6.0.1	[ ]	[ ]

MacOS/Py 2.7.15	Intel-MKL	OpenBLAS
Intel18	[ ]	[ ]
GCC 7.3.0	[ ]	[ ]
Clang 6.0.1	[ ]	[ ]

MacOS/Py 3.6.6	Intel-MKL	OpenBLAS
Intel18	[ ]	[ ]
GCC 7.3.0	[ ]	[ ]
Clang 6.0.1	[ ]	[ ]

Bash script to automate building & testing :

Note : There's an issue with intel compilers and py-scipy that requires one to explicitly set F90.
PS : Edit the location where spack is present and git checkout statement appropriately

pyver="3.6.6"

for compiler in gcc@7.3.0 intel@18.0.3
do
        if [ "$compiler" = "intel@18.0.3" ]
        then
                export F90="/home/sajid/packages/spack/lib/spack/env/intel/ifort"
        fi

        for mathlib in openblas@0.3.2
        do
                cd ~/packages/spack
                git checkout intel_scipy_test
                cd ~
                spack install py-scipy@1.1.0 ^py-numpy@1.15.0+blas+lapack ^$mathlib ^python@$pyver %$compiler
                spack activate py-scipy@1.1.0 ^$mathlib ^python@$pyver %$compiler
                if [ "$mathlib" = "intel-mkl" ]
                then
                KMP_INIT_AT_FORK=FALSE python test_numpy_scipy.py &> py$pyver.$mathlib.$compiler.txt
                else
                python test_numpy_scipy.py &> py$pyver.$mathlib.$compiler.txt
                fi
                spack load python@$pyver %$compiler
                spack deactivate py-scipy@1.1.0 ^$mathlib ^python@$pyver %$compiler
                spack deactivate py-numpy@1.15.0 ^$mathlib ^python@$pyver %$compiler

        done
done

Test script :

#Show python version, OS version, test numpy and scipy.                 
import sys,platform                                                     
import numpy                                                            
import scipy                                                            
                                                                        
if sys.version[0]>2:                                                    
    print('\n'+'Python version'+'\n')                                   
    print(sys.version)                                                  
    print('\n'+'OS info'+'\n')                                          
    print(platform.platform())                                          
    print('\n'+'Numpy BLAS/Lapack linkage'+'\n')                        
    numpy.__config__.show()                                             
    print('\n'+'Scipy BLAS/Lapack linkage'+'\n')                        
    scipy.__config__.show()                                             
    print('\n\n')                                                       
    numpy.test('full')                                                  
    scipy.test('full')                                                  
else :                                                                  
    print '\n'+'Python version'+'\n'                                    
    print sys.version                                                   
    print '\n'+'OS info'+'\n'                                           
    print platform.platform()                                           
    print '\n'+'Numpy BLAS/Lapack linkage'+'\n'                         
    numpy.__config__.show()                                             
    print'\n'+'Scipy BLAS/Lapack linkage'+'\n'                          
    scipy.__config__.show()                                             
    print '\n\n'                                                        
    numpy.test('full')                                                  
    scipy.test('full')

…cipy_test

modified: var/spack/repos/builtin/packages/py-scipy/package.py

…cipy_test

modified: var/spack/repos/builtin/packages/py-scipy/package.py

adamjstewart

Thanks for trying to fix Numpy/Scipy + Intel! I've been having problems with them for the longest time. I've also tried my own fixes in #3207 and #4545 but failed miserably. For this to get merged, it will require testing by several people on multiple OSes with multiple compilers and BLAS/LAPACK libraries. It would be good to set up a testing table like in #3207 (comment) to see what still needs to be tested.

adamjstewart · 2018-08-05T21:09:58Z

var/spack/repos/builtin/packages/py-numpy/package.py


-    depends_on('py-nose@1.0.0:', type='test')
+    depends_on('py-nose@1.0.0:', type='test', when="@:1.14.5")
+    depends_on('py-pytest', type='test', when="@1.15.0:")


I would do:

depends_on('py-nose@1.0.0:', type='test', when='@:1.14') depends_on('py-pytest', type='test', when='@1.15:')

just in case they release a 1.14.6 bugfix someday.

adamjstewart · 2018-08-05T21:12:40Z

var/spack/repos/builtin/packages/py-numpy/package.py

-            if spec['python'].version < Version('3.5'):
-                args = ['-j', str(make_jobs)]
-
-        return args


In your commit message, you mention that numpy fixed its parallel builds. So why remove the parallel flag? Is it done internally now? It looks like the -j flag still exists:

$ python setup.py build --help ... --parallel (-j) number of parallel build jobs ...

adamjstewart · 2018-08-05T21:14:04Z

var/spack/repos/builtin/packages/py-numpy/package.py

        run_env.prepend_path('CPATH', include_path)

+    # Do the usual with gcc
+    def get_phases(self):


What is get_phases? It seems like you're trying to set the phases to run based on the compiler, but I don't see how this works.

P.S. No other package in Spack changes its phases dynamically, they are all static as far as I know.

I wrote this function because I wanted to build and install in one step only for intel compilers. If using GCC/Clang, do the usual and if using Intel, build and install in one step. Is there a better way to do this ?

I don't know of any package that does this. If it's possible, it would look something like this:

@property def phases(self): if self.spec.satisfies('%intel'): return [...] else: return [...]

@alalazo might know if this is kosher or not. Do you have to use different phases? Could you use the same phases for both?

I'm just doing what the docs ask me to with no modifications whatsover so yes. If that has to be avoided then someone needs to check it by breaking it into multiple steps and checking the libraries being linked at each stage. That would have the same multimethods issue where the build and install args would be different depending on the compiler used.

Does having dynamic phases break anything ? If it doesn't then this approach is preferable because it's better not to mess with whatever assumptions the people who develop numpy.disutils might have taken rather than force static phases only to discover that some assumption was violated (which might not be the case but I wonder if it's worth the effort).

adamjstewart · 2018-08-05T21:15:51Z

var/spack/repos/builtin/packages/py-numpy/package.py

+
+    def install(self, spec, prefix):
+        install_args = self.install_args(self, prefix)
+        self.setup_py(*install_args)


This shouldn't be necessary. If it is, it is a regression of an old bug that required both the default and @when functions to be present in a package.

//required both the default and @when functions to be present in a package// I just followed the docs which say The default version of decorated methods must always come first. Should I just remove the default cases completely then ?

Yep, they should already be present in the base class, no need to override them

adamjstewart · 2018-08-05T21:20:16Z

var/spack/repos/builtin/packages/py-numpy/package.py

+        install_args = self.install_args(spec, prefix)
+        self.setup_py('config', '--compiler=intelem', 'build_clib',
+                      '--compiler=intelem', 'build_ext',
+                      '--compiler=intelem', 'install', *install_args)


Do they have to be run all at the same time? I actually tried something like this back in #4545 but haven't had time to work on that since starting grad school. I vaguely recall having some problems with numpy. I'm wondering if for some reason they have to be run at the same time.

I also tried using intelem back in #3207 but I ran into problems: #3207 (comment). I feel like I may have had a conversation with @wscullin about this...

As I mentioned in #3204 (comment), this might have been happening because loading the intel compiler via module file sets MKL_ROOT. Also, in my case it was only setting the F77 variable and not the F90 variable. This caused all sorts of issues that I resolved by manually editing the module file for intel compiler. Spack needs to eventually be able to work better with the intel compilers but that's something that should succeed #7469.

…cipy_test

modified: var/spack/repos/builtin/packages/py-scipy/package.py

adamjstewart · 2018-08-06T02:08:08Z

var/spack/repos/builtin/packages/py-numpy/package.py

-            # https://github.com/spack/spack/issues/7927
-            # https://github.com/scipy/scipy/issues/7112
-            if spec['python'].version < Version('3.5'):
-                args = ['-j', str(make_jobs)]


I wonder if we should still keep this. What version of Numpy fixed the problem?

It was tagged for the 1.13.0 release as per https://github.com/numpy/numpy/pull/9050#issuecomment-300209782.

So, should I switch the statement to if self.version >= Version('1.13.0'): instead ?

adamjstewart · 2018-08-06T02:09:17Z

var/spack/repos/builtin/packages/py-scipy/package.py

    def build_args(self, spec, prefix):
        args = []
+        # From NumPy 1.10.0 on it's possible to do a parallel build.
+        if self.version >= Version('1.10.0'):


This isn't needed, as far as I know, Scipy has always supported a parallel build. We might want to keep the stuff below though, at least for older versions of scipy. Depends on when the bug was fixed.

The bug was in numpy.disutils so one can't infer from the scipy version when it was fixed. Is there a way to check what numpy version was used in the concretization ?

if self.spec.satifies('^py-numpy@1.2.3:'):

might work.

s-sajid-ali · 2018-08-06T18:55:24Z

I'm unable to build openblas@0.3.2%intel@18.0.3 at the moment. Should the test matrix be changed to BLIS or ATLAS instead ?

For Clang, is it recommended that one installs flang and sets cc to the llvm that comes with it ?

adamjstewart · 2018-08-06T19:01:19Z

OpenBLAS is the default BLAS provider. If that doesn't work, we've got a problem. Does it work for you on develop?

s-sajid-ali · 2018-08-06T19:02:48Z

It fails on develop. Build fail info.

The issue is that libopenblas.so somehow picked up a gfortran@7.3.0 library despite me setting F90. I should probably open a new issue for this sometime but that's got nothing to do with numpy.

adamjstewart · 2018-08-06T19:09:40Z

Let's let others test OpenBLAS then. It has to be working for someone 😆

s-sajid-ali · 2018-08-06T19:23:45Z

Sure, I've posted a bash script for the same to make it easier.

Also, it would be great to have BLIS in spack.

…cipy_test

adamjstewart · 2018-08-16T19:13:18Z

var/spack/repos/builtin/packages/py-numpy/package.py

                if '^openblas' in spec:
                    f.write('[openblas]\n')
                    f.write('libraries=%s\n'    % names)
+                if '^blis' in spec:


modified: var/spack/repos/builtin/packages/py-scipy/package.py

s-sajid-ali · 2018-08-21T16:47:38Z

@adamjstewart : Can this PR be tested/reviewed on macOS ?

PS : all python 2.7 ^intel tests fail due to #9037. I was able to run the tests for ^gcc@7.3.0 by installing py-pytest from pip but that's all I can do at the moment until openblas^intel and py-pytest^python@2.7.14 is fixed.

PPS: Do you have knowledge of flang/clang compilers for python packages ?

adamjstewart · 2019-08-03T18:16:54Z

I think there's a good chance #12170 will fix your problems as numpy's build system has gotten a lot smarter in the latest release. If not, you'll need to rebase this PR once #12170 is merged.

s-sajid-ali · 2019-08-06T17:58:34Z

Once #12170 is merged, I can check whether py-numpy/py-scipy build with intel and see what to make of this PR.

modified: var/spack/repos/builtin/packages/py-scipy/package.py

23c58d6

s-sajid-ali changed the title ~~[WIP] Fix py-scipy build~~ [WIP] Fix py-scipy build with intel compilers Jul 27, 2018

s-sajid-ali added 9 commits July 29, 2018 21:17

Merge branch 'develop' of https://github.com/spack/spack into intel_s…

dc2e849

…cipy_test

modified: var/spack/repos/builtin/packages/py-numpy/package.py

93d18a2

modified: var/spack/repos/builtin/packages/py-scipy/package.py

modified: var/spack/repos/builtin/packages/py-numpy/package.py

37f3f9e

modified: var/spack/repos/builtin/packages/py-numpy/package.py

d7154be

modified: var/spack/repos/builtin/packages/py-numpy/package.py

f4e987c

modified: var/spack/repos/builtin/packages/py-numpy/package.py

a8e5b2f

modified: var/spack/repos/builtin/packages/py-numpy/package.py

7b822cd

Merge branch 'develop' of https://github.com/spack/spack into intel_s…

df43093

…cipy_test

modified: var/spack/repos/builtin/packages/py-numpy/package.py

f861403

s-sajid-ali changed the title ~~[WIP] Fix py-scipy build with intel compilers~~ [help needed] Fix py-scipy build with intel compilers Jul 31, 2018

modified: var/spack/repos/builtin/packages/py-numpy/package.py

fb585f2

modified: var/spack/repos/builtin/packages/py-scipy/package.py

s-sajid-ali force-pushed the intel_scipy_test branch from 7ba5942 to fb585f2 Compare August 5, 2018 07:08

Update package.py

668f51c

s-sajid-ali changed the title ~~[help needed] Fix py-scipy build with intel compilers~~ Fix py-scipy build with intel compilers Aug 5, 2018

adamjstewart requested changes Aug 5, 2018

View reviewed changes

adamjstewart added python blas-lapack-scalapack intel labels Aug 5, 2018

s-sajid-ali and others added 7 commits August 5, 2018 16:40

Update package.py

821cac3

Update package.py

a9f5369

Update package.py

195e1b4

Update package.py

83c8faa

Merge branch 'develop' of https://github.com/spack/spack into intel_s…

829010c

…cipy_test

modified: var/spack/repos/builtin/packages/py-numpy/package.py

289de81

modified: var/spack/repos/builtin/packages/py-scipy/package.py

Update package.py

2ffd553

adamjstewart reviewed Aug 6, 2018

View reviewed changes

Update package.py

2ba0358

s-sajid-ali added 5 commits August 6, 2018 14:28

Update package.py

750f3a8

Update package.py

59bb82b

Update package.py

68cbeb3

Update package.py

4011f85

Update package.py

cc3d0c2

s-sajid-ali mentioned this pull request Aug 13, 2018

openblas@0.3.2 on develop builds incorrectly with intel@18.0.3 #8901

Closed

s-sajid-ali and others added 2 commits August 16, 2018 13:38

Update package.py

d8a8b93

Merge branch 'develop' of https://github.com/spack/spack into intel_s…

5d36dbc

…cipy_test

adamjstewart reviewed Aug 16, 2018

View reviewed changes

s-sajid-ali and others added 6 commits August 16, 2018 14:16

Update package.py

dea33ce

Update package.py

88918a6

Update package.py

662ae28

modified: var/spack/repos/builtin/packages/py-numpy/package.py

fd4c683

modified: var/spack/repos/builtin/packages/py-numpy/package.py

f1cf9b7

modified: var/spack/repos/builtin/packages/py-scipy/package.py

modified: var/spack/repos/builtin/packages/py-scipy/package.py

b3ec14e

Update package.py

ccc77f4

s-sajid-ali mentioned this pull request Aug 2, 2019

Overhaul numpy package #12170

Merged

26 tasks

adamjstewart closed this in #12170 Aug 6, 2019

Conversation

s-sajid-ali commented Jul 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test matrix :

Bash script to automate building & testing :

Test script :

Uh oh!

adamjstewart 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamjstewart Aug 5, 2018 • 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!

s-sajid-ali Aug 5, 2018 • 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!

s-sajid-ali commented Aug 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adamjstewart commented Aug 6, 2018

Uh oh!

s-sajid-ali commented Aug 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adamjstewart commented Aug 6, 2018

Uh oh!

s-sajid-ali commented Aug 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

s-sajid-ali commented Aug 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adamjstewart commented Aug 3, 2019

Uh oh!

s-sajid-ali commented Aug 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

s-sajid-ali commented Jul 27, 2018 •

edited

Loading

adamjstewart Aug 5, 2018 •

edited

Loading

s-sajid-ali Aug 5, 2018 •

edited

Loading

s-sajid-ali commented Aug 6, 2018 •

edited

Loading

s-sajid-ali commented Aug 6, 2018 •

edited

Loading

s-sajid-ali commented Aug 21, 2018 •

edited

Loading

s-sajid-ali commented Aug 6, 2019 •

edited

Loading