Reworking of `lapack_shared_libs` and similar properties #1682

alalazo · 2016-08-31T18:10:59Z

Modifications

added class LibraryList + unit tests
added convenience functions find_libraries and dedupe
modifed non Intel blas/lapack providers to not use setup_dependent_package and define properties instead
modified packages using blas_shared_libs and similar functions
updated intel-parallel-studio and mkl
updated variants in atlas

alalazo · 2016-08-31T18:13:05Z

lib/spack/spack/test/library_list.py

+from llnl.util.filesystem import LibraryList
+
+
+class LibraryListTest(unittest.TestCase):


@davydden Can you take a look at this test file and tell me if there are other operations you would like to perform on a list of libraries ?

sure, will have a look tomorrow...

as far as i can tell,

get list of names with defined separator, i.e. foo bar baz

get list of directories where libraries are (i.e. /path/to/openblas/lib)

complete link flags -L/path/to/dir -lfoo -lbar -lbaz

list of full paths with separator, i.e. path/to/libfoo.so path/to/libbar.so

Looks like this should be enough.

davydden · 2016-08-31T20:57:25Z

there are two functions in the utilities to_lib_name and to_link_flags, which you could substitute with functionality of the new class.

davydden · 2016-08-31T20:59:38Z

lib/spack/llnl/util/filesystem.py

+        self.libraries = list(libraries)
+
+    @property
+    def directories(self):


donno what's the convention, but maybe one-line doc-string for properties? Especially for things like libnames vs names.

davydden · 2016-08-31T21:43:37Z

Looks good!

alalazo · 2016-09-01T05:53:41Z

@davydden Thanks for the thorough review! I'll start working on the PR + cleaning and documenting the code.I'll come back to you when all this will be ready for a test-drive 😄

alalazo · 2016-09-01T15:23:02Z

var/spack/repos/builtin/packages/arpack-ng/package.py

-            ])
+        options.extend([
+                '--with-blas={0}'.format(
+                    ' '.join(spec['blas'].blas_libs.ld_flags)


Before anybody says anything : since ' '.join(ld_flags) seems a common idiom I'll modify the PR to avoid having to write the join in the package 😄

davydden · 2016-09-03T14:14:10Z

var/spack/repos/builtin/packages/intel-parallel-studio/package.py

+    def blas_libs(self):
+        shared = True if '+shared' in self.spec else False
+        suffix = dso_suffix if '+shared' in self.spec else 'a'
+        mkl_integer = ['libmkl_intel_lp64'] if '+ilp64' in self.spec else ['libmkl_intel_ilp64']  # NOQA: ignore=E501


if ~ilp64

alalazo · 2016-09-03T14:38:48Z

@tgamblin @adamjstewart ready to be reviewed

davydden · 2016-09-03T14:50:07Z

@alalazo is it rebased on top of develop? I can test it on macOS now.

alalazo · 2016-09-03T14:54:08Z

@davydden Yes, it should be

fix : failing unit tests due to missing `self`

alalazo · 2016-09-21T13:51:09Z

Would there be any incentive to add support for include flags in addition to library linker flags?

@adamjstewart I would do it in another PR, as it requires some thinking on the searching logic

…e.py

tgamblin · 2016-09-21T17:31:00Z

@KineticTheory: if this does not clobber your changes from #1539, I think it's ready to merge. Can you take a look?

adamjstewart · 2016-09-21T18:01:41Z

This PR looks pretty good. There's only a couple of things I think it's missing. I would like find_libraries to accept either a list or a string, since most of the time we will be looking for a single library. Right now it only accepts a list or other sequence. I also think we could use quite a bit more documentation on what methods are available and what they return.

davydden · 2016-09-21T18:22:55Z

@adamjstewart let's get it merged and then fix minor issues in follow up PRs.

tgamblin · 2016-09-21T19:25:28Z

NOTE: this is not a request for changes. This is a proposal for how this PR should evolve because I think it's heading in the right direction and it solves a problem that other packages have.

One thing I thought about when looking at this PR: I don't really like it that some dependents still search for their dependencies libraries, though I realize that it can be necessary. Basically there are two guiding principles here:

The dependencies know themselves best, and if they can expose their build information consistently, they should do that and let dependents access it.
Dependents should prefer to ask dependencies for build information (lib locations, etc.) and only search if necessary. Searching complicates build logic and Spack should already know where things are because it already installed the dependencies.

(2) breaks down for external packages, which Spack didn't install and which may even be laid out differently than Spack would lay them out (e.g. a install of boost might have al the weird lib suffixes that boost supports, and I think Spack wouldn't currently know what to do with that). Even better is that find_libraries is hidden behind a property interface so the client code doesn't know how the library names are being assigned.

This PR follows both of these principles for LAPACK and BLAS. Using properties on specs for things like blas_libs and lapack_libs is great. It doesn't follow these for fftw, arpack, superlu, etc. See the install method for armadillo, which does its own find routines.

I think this PR makes it clearer to me what a good convention for passing args among packages in a build should look like. You want to ask the package for a property, and the package needs to know a) the property name and b) the vdep it's being asked as. For non-virtual packages, (b) doesn't matter and is always just the plain package name, but when you're asked to provide the libs/headers for a particular interface, you need to know the interface name.

I guess the question is whether this should be a convention or a more formal interface. The convention currently looks like:

    spec['blas'].blas_libs
    spec['lapack'].lapack_libs

Slightly annoying things about it:

You have to write the interface twice even though you know what you asked for.
The interface name is written once as a string and once as a property name prefix.

So, think we could have a convention like this:

def provider_property(function):
    """Decorator for provider properties that get info from dependencies."""
    def __init__(self, name):
        self.name = name

    def __call__(self, provider):
        """Get provider-specific property if available, or default to generic property."""
        specific = provider.replace('-', '_') + '_' + self.name
        dep = spec[provider]
        if hasattr(dep, specific):
            return getattr(dep, specific)
        else:
            return getattr(dep, self.name)  # default to generic attribute


class Spec(object):
    ...
    libs = provider_property('libs')
    includes = provider_property('includes')

Now the interface looks like this:

    spec.libs('blas')
    spec.libs('lapack')

And you've got a convention for properties to add to packages (libs, <provider>_libs, etc.). Package could provide a default implementation:

class Package(object):
    ...
    def libs(self):
        # requiring installation allows us to be more generic b/c we can search,
        # but it does restrict when you can call the method.
        assert(self.installed)    

        return find_libraries(... something that returns all the libs in `lib` or `lib64`...)

And packages that need to can add things like blas_libs, lapack_libs, superlu_dist_libs, etc. as they do in this PR. Packages like arpack and superlu can do nothing b/c we find their libs automatically and they're available through, e.g. spec.libs('arpack').

I guess the question is whether the interface above is so much better than the simple convention we've got that it's worthwhile. The current convention is not so bad. I think it boils down which is simpler to document and teach to package authors.

The other question is whether this convention is worthwhile for non-virtuals. Sometimes the client needs to be specific about which lib names it needs, in which case find_libraries seems like the right thing to do.

What do you all think?

davydden · 2016-09-21T19:47:11Z

You have to write the interface twice even though you know what you asked for.

one have to do that because a single package can provide different virtual packages (e.g. mkl provides blas and lapack and scalapack). Would the convention you suggested work here as well?

The other question is whether this convention is worthwhile for non-virtuals. Sometimes the client needs to be specific about which lib names it needs, in which case find_libraries seems like the right thing to do.

i would say for most packages the library names are simple (libarpack, libsuperlu_dist, etc) and for simple configure systems -L%s -larpack' % spec['arpac'].prefix.lib will do. For complicated cases like trilinos and oce, which ave tens (!) of libraries, dependents (i.e. dealii) know which libs to pick up from a prefix. So using this class is an overkill and hard to maintain as the list of libraries may change between versions.

So i would not adopt this approach to every package, but only to a few virtuals (fftw is probably the only one missing).

What should be systematically addressed is lib folder. I don't have a good overview, but i guess libraires may end up both in prefix/lib and prefix/lib64 on 64bit systems. In some packages %s/lib is hardcoded, in others it is used as spec[package].prefix.lib, but I am not sure that each package somehow checks whether or not things are installed in lib or lib64. If that's the case already, then spec[package].prefix.lib should be used throughout.

davydden · 2016-09-21T19:47:48Z

@tgamblin p.s. maybe you copy-paste your comment into a separate issue to continue the discussion?

adamjstewart · 2016-09-21T20:58:30Z

@alalazo @tgamblin I just updated to the latest develop and now I'm seeing infinite error messages:

Exception RuntimeError: 'maximum recursion depth exceeded while calling a Python object' in <type 'exceptions.AttributeError'> ignored

If I checkout the commit before this PR was merged, everything works fine. There must be some infinite recursion going on here.

adamjstewart · 2016-09-21T21:03:39Z

To reproduce the bug, just try installing any package. Even ones that don't depend on blas/lapack/etc. Maybe a getattribute problem?

adamjstewart · 2016-09-21T21:05:16Z

lib/spack/spack/spec.py

+        # This line is to avoid infinite recursion in case package is
+        # not present among self attributes
+        package = super(Spec, self).__getattribute__('package')
+        return getattr(package, item)


I've confirmed that these changes caused the bug I'm seeing. When I comment this function out, Spack works again. What was this added for?

Today I succeeded installing cp2k before pushing changes and right now I correctly installed hdf5 on a fresh develop checkout... sorry I can't reproduce the problem...

The two lines are needed to forward requests for non-existing attributes from Spec to Package. It's what makes :

blas = spec['blas'].blas_libs

possible instead of :

blas = spec['blas'].package.blas_libs

Ok, this is weird. Spack works on my laptop but not on our cluster. Going to investigate further.

Ouch... could reproduce with python@2.6. I wonder why Travis never fired up a warning about that

EDIT : Travis seems to be running with 2.6.9 and no issues. Python 2.6.6 instead shows the problem.

On my cluster, Spack works with Python 2.7 but crashes with the system Python 2.6 Can you try 2.6 on your end to see if you can reproduce the problem?

Yeah, I'm running Python 2.6.6 too.

@alalazo: if you're working on a fix I'll merge it asap, otherwise I'll work on it.

EDIT : Travis seems to be running with 2.6.9 and no issues. Python 2.6.6 instead shows the problem.

If only we could just drop 2.6. Sigh.

@tgamblin Sorry for the trouble. ~~I couldn't find a solution that works for both python versions right now. If you couldn't solve it right away, feel free to revert the merge and I'll work it out without impacting everyone...~~

@tgamblin I think #1823 will solve this issue.

pramodk · 2016-09-24T22:44:02Z

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

+        suffix = dso_suffix if '+shared' in self.spec else 'a'
+        mkl_integer = ['libmkl_intel_ilp64'] if '+ilp64' in self.spec else ['libmkl_intel_lp64']  # NOQA: ignore=E501
+        mkl_threading = ['libmkl_sequential']
+        if '+openmp' in spec:


@alalazo : this should be self.spec ?

if '+openmp' in spec:

@pramodk True! Feel free to submit a PR to fix this. Otherwise, if you prefer, I'll take care of that.

I am testing now Atlas and MKL and i will create a PR today, need to fix a few extra minor things in MKL.

citibeth · 2016-10-07T14:50:59Z

lib/spack/docs/packaging_guide.rst

 choice of ``Blas`` implementation, each package which provides it
-sets up ``self.spec.blas_shared_lib`` and ``self.spec.blas_static_lib`` to
-point to the shared and static ``Blas`` libraries, respectively. The same


Blas-->BLAS

citibeth · 2016-10-07T14:51:20Z

lib/spack/docs/packaging_guide.rst

 choice of ``Blas`` implementation, each package which provides it
-sets up ``self.spec.blas_shared_lib`` and ``self.spec.blas_static_lib`` to
-point to the shared and static ``Blas`` libraries, respectively. The same
-applies to packages which provide ``Lapack``. Package developers are advised to


Lapack-->LAPACK

alalazo reviewed Aug 31, 2016
View reviewed changes

davydden reviewed Aug 31, 2016
View reviewed changes

alalazo reviewed Sep 1, 2016
View reviewed changes

alalazo force-pushed the features/share_lib_as_iterables branch from fce956b to b27a904 Compare September 3, 2016 12:13

alalazo mentioned this pull request Sep 3, 2016

[Discussion] : turn self.<provider>_shared_lib into an iterable #1670

Closed

davydden reviewed Sep 3, 2016
View reviewed changes

alalazo changed the title ~~[WIP] Reworking of lapack_shared_libs and similar properties~~ Reworking of lapack_shared_libs and similar properties Sep 3, 2016

alalazo added 3 commits September 21, 2016 14:47

PR review : fixed last issues

ff7aa4d

LibraryList : added test on __add__ return type

12a0be3

LibraryList : added __radd__ fixed unit tests

ff8fb7b

fix : failing unit tests due to missing `self`

cp2k and dependecies : fixed blas-lapack related statements in packag…

be14686

…e.py

alalazo force-pushed the features/share_lib_as_iterables branch from 39df558 to be14686 Compare September 21, 2016 14:07

tgamblin approved these changes Sep 21, 2016

View reviewed changes

tgamblin merged commit d848559 into spack:develop Sep 21, 2016

adamjstewart reviewed Sep 21, 2016

View reviewed changes

tgamblin mentioned this pull request Sep 21, 2016

Spec interface for build information (like lib names, includes, etc.) #1821

Closed

alalazo mentioned this pull request Sep 22, 2016

fix : stops infinite recursion for python 2.6 #1823

Merged

alalazo deleted the features/share_lib_as_iterables branch September 22, 2016 11:31

This was referenced Sep 22, 2016

py-scipy: Make it work with static BLAS and LAPACK. #1541

Closed

hypre not handling '--with-lapack-libs=['"'"'openblas'"'"']' correctly #1840

Closed

pramodk reviewed Sep 24, 2016

View reviewed changes

davydden mentioned this pull request Sep 26, 2016

some fixes to blas/lapack usage in packages #1852

Merged

8 tasks

alalazo mentioned this pull request Sep 29, 2016

spec : interface for build information #1875

Merged

6 tasks

davydden mentioned this pull request Oct 6, 2016

trilinos recipe: spec['blas'].prefix.lib is wrong for mkl #1923

Closed

citibeth reviewed Oct 7, 2016

View reviewed changes

tgamblin mentioned this pull request Oct 21, 2016

How to get spack to use system blas/lapack #2067

Closed

alalazo mentioned this pull request Nov 29, 2016

py-numpy: rework blas/lapack #2365

Merged

4 tasks

		from llnl.util.filesystem import LibraryList


		class LibraryListTest(unittest.TestCase):

Reworking of lapack_shared_libs and similar properties #1682

Reworking of lapack_shared_libs and similar properties #1682

Conversation

alalazo commented Aug 31, 2016 • edited

Modifications

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davydden commented Aug 31, 2016

Choose a reason for hiding this comment

davydden commented Aug 31, 2016

alalazo commented Sep 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alalazo commented Sep 3, 2016

davydden commented Sep 3, 2016

alalazo commented Sep 3, 2016

alalazo commented Sep 21, 2016

tgamblin commented Sep 21, 2016

adamjstewart commented Sep 21, 2016

davydden commented Sep 21, 2016

tgamblin commented Sep 21, 2016 • edited

davydden commented Sep 21, 2016

davydden commented Sep 21, 2016

adamjstewart commented Sep 21, 2016

adamjstewart commented Sep 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alalazo Sep 21, 2016 • edited

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

tgamblin Sep 21, 2016 • edited

Choose a reason for hiding this comment

alalazo Sep 21, 2016 • edited

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

Reworking of `lapack_shared_libs` and similar properties #1682

Reworking of `lapack_shared_libs` and similar properties #1682

alalazo commented Aug 31, 2016 •

edited

tgamblin commented Sep 21, 2016 •

edited

alalazo Sep 21, 2016 •

edited

tgamblin Sep 21, 2016 •

edited

alalazo Sep 21, 2016 •

edited