Add blis package #8925

s-sajid-ali · 2018-08-09T14:22:31Z

First pass at adding the blis linear algebra package.

healther · 2018-08-09T14:44:35Z

var/spack/repos/builtin/packages/blis/package.py

+        multi=False
+    )
+
+    # TODO: add cpu variants. Currently using auto.


do you plan to add this, or is this just a remark for future expansions?

There's some knl specific hbw_malloc options. I'm not sure which is better so I'll let people who use this on knl to add the options in a meaningful way.

adamjstewart · 2018-08-09T15:29:10Z

var/spack/repos/builtin/packages/blis/package.py

+from spack import *
+
+
+class Blis(Package):


Can you convert this to AutotoolsPackage? Everything looks pretty standard, you should only need to override configure_args.

It started life in the spack universe as an Autotools package but realizing that they don't use autotools, I changed it back !

Browsing through the repo it looks like this is not designed as an autotools package so do we want to make it one here ? (see flame/blis#17, flame/blis#197, flame/blis#195, etc )

Ah, so this is a "fake" Autotools package that writes their own script that just happens to be called configure. In that case, you're right, this shouldn't be an Autotools package. Can you add a comment mentioning this so others aren't confused?

adamjstewart · 2018-08-09T18:04:35Z

var/spack/repos/builtin/packages/blis/package.py

+                  "auto")
+        make()
+        make('install')
+        make('check')


Normally make('check') is run after make() and before make('install'). Do their installation docs say otherwise?

Also, this should be optional. If you change it to:

if self.run_tests: make('check')

it will only run the tests if spack install --test is used.

adamjstewart · 2018-08-09T18:05:01Z

var/spack/repos/builtin/packages/blis/package.py

+
+    def install(self, spec, prefix):
+        configure("--prefix=" + self.spec.prefix,
+                  "--enable-threading=" + self.spec.variants['threads'].value,


You don't have to use self here since you already have access to spec and prefix.

adamjstewart · 2018-08-09T20:31:12Z

var/spack/repos/builtin/packages/blis/package.py

+    version('0.3.0', sha256='d34d17df7bdc2be8771fe0b7f867109fd10437ac91e2a29000a4a23164c7f0da')
+    version('0.2.2', sha256='4a7ecb56034fb20e9d1d8b16e2ef587abbc3d30cb728e70629ca7e795a7998e8')
+
+    depends_on('python@2.7:2.8,3.4:')


type=('build', 'run')?

adamjstewart · 2018-08-09T20:32:31Z

var/spack/repos/builtin/packages/blis/package.py

+    vairant(
+        'blas', default='false',
+        description='BLAS compatibility',
+        values=('true', 'false'),


Typo: vairant

Also, none of these variants (aside from threads) should be multi-valued variants. They can be simple traditional boolean variants:

variant('blas', default=False, description='BLAS compatibility')

Also, what exactly does this variant do? Does it still provide blas when blis~blas? None of these variants seem to be passed to the build instructions.

I'll do it soon, but here is the explanation
While BLIS exports a new BLAS-like API, it also includes a BLAS compatibility layer which gives application developers access to BLIS implementations via traditional BLAS routine calls. An object-based API unique to BLIS is also available.

adamjstewart · 2018-08-10T04:06:20Z

var/spack/repos/builtin/packages/blis/package.py

+    provides('blas', when="+blas")
+    provides('blas', when="+cblas")
+
+    phases = ['configure', 'install']


I would also add a phase for build. See http://spack.readthedocs.io/en/latest/build_systems/custompackage.html for examples of how each phase works and how to add testing.

adamjstewart · 2018-08-10T04:07:37Z

var/spack/repos/builtin/packages/blis/package.py

+        if spec.variants['cblas']:
+            cblas = "--enable-cblas"
+        else:
+            cblas = "--disable-cblas"


Instead of saving these as variables, I would append them to an args list. Then, when you call configure, use:

configure(*args)

That's how most other packages work, and then you don't need to keep track of any additional variables.

*args unpacks a list into a set of strings ?

Correct, see https://docs.python.org/3.7/tutorial/controlflow.html#unpacking-argument-lists

adamjstewart · 2018-08-10T04:21:51Z

var/spack/repos/builtin/packages/blis/package.py

+    def build(self, spec, prefix):
+        make()
+        if self.run_tests:
+            make('check')


This can be made into a separate function like so: http://spack.readthedocs.io/en/latest/build_systems/custompackage.html#testing

Just replace test with check.

adamjstewart · 2018-08-10T04:23:12Z

var/spack/repos/builtin/packages/blis/package.py

+    phases = ['configure', 'build', 'install']
+
+    def configure(self, spec, prefix):
+        congfig_args = []


typo: congfig -> config

adamjstewart · 2018-08-10T04:27:51Z

var/spack/repos/builtin/packages/blis/package.py

+        else:
+            config_args.append("--disable-static")
+
+        configure("--prefix=" + spec.prefix,


You can just use prefix here, no need for spec.prefix.

s-sajid-ali · 2018-08-10T04:49:03Z

Everything works with gcc but with intel it takes the compiler to be gcc despite passing env['CC']. What am I doing wrong ?

[sajid@xrmlite ~]$ spack install -v --test root blis ^python@3.6.6 %intel@18.0.3
.......
==> Installing blis
==> Using cached archive: /home/sajid/packages/spack/var/spack/cache/blis/blis-0.4.0.tar.gz
==> Staging archive: /home/sajid/packages/spack/var/spack/stage/blis-0.4.0-ol33ytxgje4qww32ehx4rsdepcyhdlrd/0.4.0.tar.gz
==> Created stage in /home/sajid/packages/spack/var/spack/stage/blis-0.4.0-ol33ytxgje4qww32ehx4rsdepcyhdlrd
==> No patches needed for blis
==> Building blis [Package]
==> Executing phase: 'configure'
==> './configure' '--prefix=/home/sajid/packages/spack/opt/spack/linux-centos7-x86_64/intel-18.0.3/blis-0.4.0-ol33ytxgje4qww32ehx4rsdepcyhdlrd' '--enable-threading=none' '--enable-cblas' '--enable-blas' '--enable-shared' '--enable-static' 'CC=/home/sajid/packages/spack/lib/spack/env/intel/icc' 'auto'
configure: detected Linux kernel version 3.10.0-862.9.1.el7.x86_64.
configure: C compiler search list is: gcc clang cc.
configure: using 'gcc' C compiler.
configure: C++ compiler search list is: g++ clang++ c++.
configure: using 'g++' C++ compiler.
configure: found gcc version 7.3.0 (maj: 7, min: 3, rev: 0).

scheibelp · 2018-08-13T18:22:11Z

Everything works with gcc but with intel it takes the compiler to be gcc despite passing env['CC']. What am I doing wrong ?

The configure script for blis doesn't appear to allow for icc - see get_cc_search_list in https://github.com/flame/blis/blob/master/configure

EDIT: to be clear, the search function doesn't acknowledge the possibility of a compiler binary called icc, although there are other functions which acknowledge that the string "icc" may appear when you call cc --version.

Spack should automatically set CC to be it's compiler wrapper, which will be icc when you set that in the Spec. That is all to say: this should work if you remove the explicit CC= option in the configure function of the blis package.

s-sajid-ali · 2018-08-14T14:17:13Z

I removed CC and I still get the same error:

[sajid@xrmlite ~]$ spack install -v blis^python@3.6.6%intel@18.0.3
...
==> Installing blis
==> Fetching https://github.com/flame/blis/archive/0.4.0.tar.gz
######################################################################## 100.0%
==> Staging archive: /home/sajid/packages/spack/var/spack/stage/blis-0.4.0-ol33ytxgje4qww32ehx4rsdepcyhdlrd/0.4.0.tar.gz
==> Created stage in /home/sajid/packages/spack/var/spack/stage/blis-0.4.0-ol33ytxgje4qww32ehx4rsdepcyhdlrd
==> No patches needed for blis
==> Building blis [Package]
==> Executing phase: 'configure'
==> './configure' '--prefix=/home/sajid/packages/spack/opt/spack/linux-centos7-x86_64/intel-18.0.3/blis-0.4.0-ol33ytxgje4qww32ehx4rsdepcyhdlrd' '--enable-threading=none' '--enable-cblas' '--enable-blas' '--enable-shared' '--enable-static' 'auto'
configure: detected Linux kernel version 3.10.0-693.el7.x86_64.
configure: C compiler search list is: gcc clang cc.
configure: using 'gcc' C compiler.
configure: C++ compiler search list is: g++ clang++ c++.
configure: using 'g++' C++ compiler.
configure: found gcc version 7.3.0 (maj: 7, min: 3, rev: 0).
configure: checking for blacklisted configurations due to gcc 7.3.0.
configure: found assembler ('as') version 2.27 (maj: 2, min: 27, rev: ).

adamjstewart · 2018-08-14T14:21:38Z

var/spack/repos/builtin/packages/blis/package.py

+        else:
+            config_args.append("--disable-blas")
+
+        if self.variants['shared']:


In some places you use spec.variants, and in others you use self.variants. I would use the same syntax that every other package uses:

if '+shared' in spec:

adamjstewart · 2018-08-14T14:24:30Z

var/spack/repos/builtin/packages/blis/package.py

+            config_args.append("--disable-shared")
+
+        if '+static' in spec:
+        config_args.append("--enable-static")


Indentation

Fixed it already.

adamjstewart · 2018-08-14T14:30:52Z

var/spack/repos/builtin/packages/blis/package.py:118: [W293] blank line contains whitespace
var/spack/repos/builtin/packages/blis/package.py:120: [E265] block comment should start with '# '

…add_blis

s-sajid-ali · 2018-08-15T21:19:08Z

@adamjstewart : Fixed the build and test issues. Can you merge this ?

adamjstewart · 2018-08-15T21:29:32Z

Yep, just ping me when the Travis tests finish.

scheibelp · 2018-08-15T22:06:01Z

Did I miss a resolution for #8925 (comment) or did you want to handle that later (EDIT: not necessarily you but I mean are you fine with that being handled later)?

s-sajid-ali · 2018-08-16T16:19:59Z

I don't remember how I fixed it but it's working now.

[sajid@xrmlite spack]$ spack install -v  blis@develop+cblas^python@3.6.6%intel@18.0.3
...
==> Installing blis
==> Cloning git repository: https://github.com/flame/blis.git on branch master
==> No checksum needed when fetching with git
==> Already staged blis-develop-o63fbaqwdapvkovy67ptgpnzz5kzettw in /home/sajid/packages/spack/var/spack/stage/blis-develop-o63fbaqwdapvkovy67ptgpnzz5kzettw
==> No patches needed for blis
==> Building blis [Package]
==> Executing phase: 'configure'
==> './configure' '--prefix=/home/sajid/packages/spack/opt/spack/linux-centos7-x86_64/intel-18.0.3/blis-develop-o63fbaqwdapvkovy67ptgpnzz5kzettw' '--enable-threading=none' '--enable-cblas' '--enable-blas' '--enable-shared' '--enable-static' 'auto'
configure: detected Linux kernel version 3.10.0-693.el7.x86_64.
configure: C compiler search list is: gcc clang cc.
configure: using '/home/sajid/packages/spack/lib/spack/env/intel/icc' C compiler.
configure: C++ compiler search list is: g++ clang++ c++.
configure: using '/home/sajid/packages/spack/lib/spack/env/intel/icpc' C++ compiler.
configure: found icc version 18.0.3 (maj: 18, min: 0, rev: 3).
configure: checking for blacklisted configurations due to /home/sajid/packages/spack/lib/spack/env/intel/icc 18.0.3.
configure: found assembler ('as') version 2.27 (maj: 2, min: 27, rev: ).

s-sajid-ali and others added 5 commits August 6, 2018 17:36

new file: var/spack/repos/builtin/packages/blis/package.py

7a9da5c

modified: var/spack/repos/builtin/packages/blis/package.py

18ef7fe

clean style

59c97ff

Update package.py

f13da2d

Update package.py

e255b7d

healther reviewed Aug 9, 2018

View reviewed changes

adamjstewart added new-package blas-lapack-scalapack labels Aug 9, 2018

adamjstewart requested changes Aug 9, 2018

View reviewed changes

s-sajid-ali added 2 commits August 9, 2018 12:15

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

162b4f0

modified: var/spack/repos/builtin/packages/blis/package.py

cf5a5b3

adamjstewart reviewed Aug 9, 2018

View reviewed changes

modified: var/spack/repos/builtin/packages/blis/package.py

cc37242

s-sajid-ali force-pushed the add_blis branch from b2b1518 to cc37242 Compare August 9, 2018 20:13

s-sajid-ali added 2 commits August 9, 2018 15:14

Update package.py

69b7a3b

Update package.py

2ae0f8d

adamjstewart reviewed Aug 9, 2018

View reviewed changes

modified: var/spack/repos/builtin/packages/blis/package.py

6e01c9c

s-sajid-ali force-pushed the add_blis branch from 33ddf3c to 6e01c9c Compare August 10, 2018 03:58

s-sajid-ali added 2 commits August 9, 2018 23:01

Update package.py

443948c

Update package.py

7c8c9c2

adamjstewart requested changes Aug 10, 2018

View reviewed changes

s-sajid-ali added 4 commits August 9, 2018 23:08

Update package.py

5b42e97

Update package.py

daddb16

Update package.py

c510c28

Update package.py

bc7b113

adamjstewart reviewed Aug 10, 2018

View reviewed changes

s-sajid-ali added 4 commits August 9, 2018 23:29

Update package.py

5f9dc0a

Update package.py

81dbde5

Update package.py

0758dcf

Update package.py

a13c42f

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

b640cdc

modified: var/spack/repos/builtin/packages/blis/package.py

f4a60cd

adamjstewart reviewed Aug 14, 2018

View reviewed changes

s-sajid-ali added 2 commits August 14, 2018 09:23

Update package.py

7353c3e

Update package.py

8215153

adamjstewart reviewed Aug 14, 2018

View reviewed changes

adamjstewart approved these changes Aug 14, 2018

View reviewed changes

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

Build with intel compiler flame/blis#238

Closed

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

Update package.py

5f75f34

Update package.py

096fad0

modified: var/spack/repos/builtin/packages/blis/package.py

5462a80

Update package.py

dc5d4a2

modified: var/spack/repos/builtin/packages/blis/package.py

b0f54a5

Merge branch 'add_blis' of https://github.com/s-sajid-ali/spack into …

bea08cc

…add_blis

adamjstewart merged commit cde5116 into spack:develop Aug 16, 2018

anderson2981 pushed a commit to anderson2981/spack that referenced this pull request Sep 7, 2018

Add blis package (spack#8925)

680ed37

s-sajid-ali deleted the add_blis branch March 27, 2019 20:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add blis package #8925

Add blis package #8925

s-sajid-ali commented Aug 9, 2018

healther Aug 9, 2018

s-sajid-ali Aug 9, 2018

adamjstewart Aug 9, 2018

s-sajid-ali Aug 9, 2018

adamjstewart Aug 9, 2018

adamjstewart Aug 9, 2018

adamjstewart Aug 9, 2018

adamjstewart Aug 9, 2018

adamjstewart Aug 9, 2018

adamjstewart Aug 9, 2018

s-sajid-ali Aug 9, 2018

adamjstewart Aug 10, 2018

adamjstewart Aug 10, 2018

s-sajid-ali Aug 10, 2018

adamjstewart Aug 10, 2018

adamjstewart Aug 10, 2018

adamjstewart Aug 10, 2018

adamjstewart Aug 10, 2018

s-sajid-ali commented Aug 10, 2018 •

edited

Loading

scheibelp commented Aug 13, 2018 •

edited

Loading

s-sajid-ali commented Aug 14, 2018 •

edited

Loading

adamjstewart Aug 14, 2018

s-sajid-ali Aug 14, 2018

adamjstewart Aug 14, 2018

s-sajid-ali Aug 14, 2018

adamjstewart commented Aug 14, 2018

s-sajid-ali commented Aug 15, 2018

adamjstewart commented Aug 15, 2018

scheibelp commented Aug 15, 2018 •

edited

Loading

s-sajid-ali commented Aug 16, 2018

Add blis package #8925

Add blis package #8925

Conversation

s-sajid-ali commented Aug 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s-sajid-ali commented Aug 10, 2018 • edited Loading

scheibelp commented Aug 13, 2018 • edited Loading

s-sajid-ali commented Aug 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamjstewart commented Aug 14, 2018

s-sajid-ali commented Aug 15, 2018

adamjstewart commented Aug 15, 2018

scheibelp commented Aug 15, 2018 • edited Loading

s-sajid-ali commented Aug 16, 2018

s-sajid-ali commented Aug 10, 2018 •

edited

Loading

scheibelp commented Aug 13, 2018 •

edited

Loading

s-sajid-ali commented Aug 14, 2018 •

edited

Loading

scheibelp commented Aug 15, 2018 •

edited

Loading