spec : interface for build information by alalazo · Pull Request #1875 · spack/spack

alalazo · 2016-09-29T09:38:19Z

TLDR

This PR provides a general mechanism to forward queries from Spec to Packages using the descriptor ForwardQueryToPackage. It implements most of the ideas discussed in #1821 (see details below), including a default behavior for the methods spec.libs and spec.cppflags.

To showcase an example some of the packages in the dag:

$ spack spec -Il pexsi
Input spec
--------------------------------
     fse2tj5  pexsi

Normalized
--------------------------------
     ge7amsh  pexsi
     4av5qrp      ^parmetis
     wji7giq          ^[email protected]:
     qfnm56h          ^metis@5:
     2xnea7r          ^mpi

Concretized
--------------------------------
[+]  m6mbmik  [email protected]%[email protected] arch=linux-ubuntu14-x86_64
[+]  ua5fwf7      ^[email protected]%[email protected]~debug~gdb+shared arch=linux-ubuntu14-x86_64
[+]  t5oqkdp          ^[email protected]%[email protected]~doc+ncurses+openssl+ownlibs~qt arch=linux-ubuntu14-x86_64
[+]  qwcc7xb              ^[email protected]%[email protected] arch=linux-ubuntu14-x86_64
[-]  23ldpkv              ^openssl@system%[email protected] arch=linux-ubuntu14-x86_64
[+]  sj5ayy3          ^[email protected]%[email protected]~debug~gdb~idx64~real64+shared arch=linux-ubuntu14-x86_64
[+]  u4ku7al          ^[email protected]%[email protected]~mxm~pmi~psm~psm2~slurm~sqlite3~thread_multiple~tm~verbs+vt arch=linux-ubuntu14-x86_64
[+]  cbteeox              ^[email protected]%[email protected] arch=linux-ubuntu14-x86_64
[+]  6ryt5eq                  ^[email protected]%[email protected] arch=linux-ubuntu14-x86_64
[+]  2ffyrzb                      ^[email protected]%[email protected] arch=linux-ubuntu14-x86_64
[+]  zfagq4d                          ^[email protected]%[email protected]+sigsegv arch=linux-ubuntu14-x86_64
[+]  m3xfbqd                              ^[email protected]%[email protected] arch=linux-ubuntu14-x86_64
[+]  4swdmfk                      ^[email protected]%[email protected]+internal_glib arch=linux-ubuntu14-x86_64
[+]  bunu7o5                      ^[email protected]%[email protected] arch=linux-ubuntu14-x86_64
[+]  yhtbrhi      ^[email protected]%[email protected] arch=linux-ubuntu14-x86_64
[+]  idsxmtv          ^[email protected]%[email protected]+fpic~openmp+shared arch=linux-ubuntu14-x86_64

have been reworked to make use of the new features.

Analysis and requirements

There are a number of things that emerged from #1682 and #1821 concerning build information, that I'll try to recap below for ease of reference :

packages know themselves better than anybody else and thus they should handle requests that are based on their state
the syntax in Reworking of lapack_shared_libs and similar properties #1682 is needlessly redundant (e.g. spec['blas'].blas_libs mentions blas twice)
only blas and lapack providers are taken care of properly, while normal packages are not
use of __getattr__ to intercept calls to non-existing attributes in Spec should be avoided
if we add a mechanism to query for build information, then it should be possible to opt-out of it (see case for dealii and trilinos)

On top of that I would add a couple more points :

sometimes packages provide more than one service (API, functionality, etc.) and the few needed are only known from the calling scope (e.g. openmpi installs cxx, c and fortran APIs and a particular package may need to link with cxx)
the actions required to compute build properties are the same for many simple and well-behaved packages (e.g. spec['name'].libs will require very often to search for lib{name}.{suffix} somewhere in prefix)

Design

I grabbed the basic idea that @tgamblin proposed in #1821 and expanded it further. What I didn't like there was just the syntax it would have imposed :

blas = spec['blas']  # to retrieve the spec providing blas we use __getitem__
blas_libs = spec.libs('blas') # ... but to ask for properties of 'blas' we use function calls
...

so I tried to achieve something like :

blas_libs = spec['blas'].libs # ask the blas provider for list of libraries
mpi_cxx_libs = spec['mpi:cxx'].libs # ask the mpi provider just for the libraries needed for the `cxx` API
mpi_cxx_and_fortran_libs = spec['mpi:cxx,fortran'].libs # ask the mpi provider `cxx` and fortran libs
metis_libs = spec['metis'].libs # I want to be able to do the same thing with a non-virtual package
...
# To opt-out of default behavor
class Trilinos(Package):
    libs = None

And have readable tracebacks in case of failures :

# Here I made openblas fail on purpose
Traceback (most recent call last):
  File "/home/mculpo/PycharmProjects/spack/lib/spack/spack/build_environment.py", line 511, in fork
    function()
  File "/home/mculpo/PycharmProjects/spack/lib/spack/spack/package.py", line 983, in build_process
    self.install(self.spec, self.prefix)
  File "/home/mculpo/PycharmProjects/spack/var/spack/repos/builtin/packages/pexsi/package.py", line 70, in install
    '@LAPACK_LIBS': self.spec['lapack'].libs.joined(),
  File "/home/mculpo/PycharmProjects/spack/lib/spack/spack/spec.py", line 650, in __get__
    raise AttributeError(message)
AttributeError: 'openblas' package has no relevant attribute 'libs'
    spec : '[email protected]%[email protected]+fpic~openmp+shared arch=linux-Ubuntu14-x86_64'
    queried as : 'lapack'
    extra parameters : '[]'

The whole design is based on a few of simple concepts :

a Spec instance has now an internal state to keep track of 'queries' from client code, and a few attributes to retrieve, set or clear the query state
a descriptor takes care of forwarding queries from Spec to Package and uses a chain of responsibility (documented in the code) to serve them
Spec.__getitem__ automatically set a new query state in the required spec. If the spec being queried is virtual __getitem__ returns a copy instead of a reference to deal with the case of packages that provide more than one service.

Modifications

calls forwarded from Spec to Package are now explicit
added descriptor within Spec to manage forwarding
added state in Spec to maintain query information
modified a few packages (the one involved in spack install pexsi) to showcase changes
added unit tests for the new logic in __getitem__
added documentation on cppflags and libs

alalazo · 2016-09-29T09:43:26Z

@tgamblin @davydden @adamjstewart @scheibelp @becker33 If you have time I would be interested in knowing what you think about this PR

davydden

Regarding the opt-out, probably depends on how many packages actually can implement this. Probably keeping it on by default is ok.

One more thing you may consider is extra link flags. blas/lapack compiled with openmp need to add extra flags self.compiler.openmp_flag. So everyone who use blas/lapack should be able to use those, see https://github.com/LLNL/spack/blob/develop/var/spack/repos/builtin/packages/openblas/package.py#L128-L129 . I don't think you want to mix them with cppflags.

Also we need to agree on naming, cppflags return a string, whereas libs return an array/list or alike. This is not clear from names. One way would be to return a string for singulars (cppflag or ld_flag) and arrays/lists for plurals (libs, dirs).

davydden · 2016-09-29T10:04:10Z

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

        )

-    @property
-    def lapack_libs(self):


your current design appears to be limited in case of a single provider supplying several things, like mkl does for blas, lapack and scalapack. In this case, the libs for the last one (scalapack) are certainly different from those used in blas and lapack. In other words
spec['blas'].libs, spec['lapack'].libs and spec['scalapack'].libs could all be provided by the same package and potentially could all be different. Note that it's different from what you do with a query parameter.

@davydden Limited in which sense ? Suppose mkl is used, then if you code :

l = spec['scalapack'].libs

this will in order try to grab / call:

Mkl.scalapack_libs Mkl.libs _libs_default_handler

If you need specialization you can code :

class Mkl(Package): @property def scalapack_libs(self): ... @property def lapack_libs(self): ...

and take appropriate measures based on what you were asked.

There's a little caveat in that I didn't take care so far to return copied specs on __getitem__ call, so that things like :

a = spec['blas'] b = spec['lapack'] ...

don't interfere with each other if they are provided by the same package. That is a detail I'll work on later if this syntax / machinery looks good enough...

@davydden To add more on that : openblas is fairly well-behaved, so with this implementation you can even remove Openblas.libs and have the code still working. The default handler will do just the right thing...

oh, i see, thanks for clarifying. Then this

Mkl.scalapack_libs Mkl.libs _libs_default_handler

should be perfectly fine indeed.

alalazo · 2016-09-29T10:38:12Z

Also we need to agree on naming, cppflags return a string, whereas libs return an array/list or alike. This is not clear from names.

This is definitely one of the thing we should discuss carefully iff the general idea is good enough. I already don't agree with singulars vs. plurals but let's save this fight for later in case 😄

davydden · 2016-09-29T11:27:34Z

lib/spack/spack/spec.py



+def _libs_default_handler(descriptor, spec, cls):
+    """Default handler when looking for 'libs' attribute


i see now what you mean about openblas being simple. 👍

davydden · 2016-09-29T11:31:00Z

For partial opt-out one could do

 @property
 def libs(self):
    // throw an error here

This would prevent anyone from using [package].libs, but would still allow to use cppflags, for example.

davydden · 2016-09-29T11:33:09Z

var/spack/repos/builtin/packages/superlu-dist/package.py


    def install(self, spec, prefix):
-        lapack_blas = spec['lapack'].lapack_libs + spec['blas'].blas_libs
+        lapack_blas = spec['lapack'].libs + spec['blas'].libs


alalazo · 2016-09-29T11:35:14Z

@davydden

class MyPackage(Package):
    libs = None
    ...

will have the same effect : retrieving MyPackage.libs will return a value (None). Being false-ish this will raise the AttributeError in the description

davydden · 2016-11-08T12:40:50Z

Came across another reason why this PR is good to have: on OpenSuse tcl installs libraries into lib64, whereas dependents like environment-modules expect it in lib. So if each packages would know everything about itself, we could remove hardcoded prefix/lib paths to make Spack more robust.

alalazo · 2016-11-25T15:00:14Z

@tgamblin I've noticed this entered the project v0.10 in the Nice to have column. Do you want me to address this next ? I don't think there's much work to be done here to make it ready for a first review...

davydden · 2016-11-25T18:20:55Z

@tgamblin @alalazo : it was me who added it to Nice to have, feel free to remove it if you find it more appropriate.

alalazo · 2016-11-30T18:37:05Z

@tgamblin @adamjstewart @becker33 @scheibelp I think this is ready for review. I edited the description above to have all the information I can remember right now.

I think the method you would like to pay the most attention when reviewing is Spec.__getitem__, which sets the query state for the package. Well, fire questions / comments at will 😄

adamjstewart · 2016-11-30T19:12:54Z

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

+            '@LAPACK_LIBS': self.spec['lapack'].libs.joined(),
+            '@BLAS_LIBS': self.spec['blas'].libs.joined(),
+            # FIXME : what to do with compiler provided libraries ?
            '@STDCXX_LIB': ' '.join(self.compiler.stdcxx_libs)


What to do with compiler provided libraries?

Not sure, but it definitely needs some work. For example, we just added self.compiler.pic_flag because compilers like NAG use -PIC instead of -fPIC. The problem is that if I mix compilers (GCC + NAG), the flag isn't right for each compiler.

I agree and the FIXME is there because I think compiler flags deserve a PR on their own. Especially if the effort to turn them into some sort of dependencies already started.

scheibelp · 2016-11-30T22:54:30Z

lib/spack/spack/spec.py

+            msg += ' At most one is admitted.'
+            raise KeyError(msg)
+
+        name, query_parameters = query_parameters[0], query_parameters[1:]


It looks like query_parameters[1:] could just be query_parameters[1]

And actually since this must be of length two, you can just do:

name, query_parameters = query_parameters

@scheibelp The most frequent case is the one with zero query parameters:

>>> l = [1] >>> a, b = l[0], l[1:] >>> print(a, b) (1, [])

Got it - thanks!

scheibelp · 2016-11-30T23:57:07Z

At a high level, I'm curious why this approach is preferable to e.g. keeping the getattr syntax and then for example changing Openmpi.libs to be a function that accepts arguments like 'cxx'. I suppose that forces users to say something like spec['openmpi'].libs() instead of spec['openmpi'].libs although IMO that's not a big problem (and perhaps providing an object which overwrites iter and call would get around that).

I'm curious because in my opinion this approach adds complexity relative to sticking with an override of getattr. For example stuff like:

if is_virtual and self.concrete:
    # deepcopy above doesn't treat this correctly
    value.package.spec = value

makes me cautious. Perhaps I'm overlooking other major benefits that the getattr approach lacks.

alalazo · 2017-03-02T08:45:32Z

ping

tgamblin · 2017-03-02T18:01:35Z

@alalazo: I think there is still some refactoring that can be done here w.r.t. consolidating the handlers, ForwardQueryToPackage, and SpecWrapper, but we can do that in another PR. Right now it's a little messy how they're all related.

I want to get the feature in, though.

Can you add docs for users in another PR? The docs should probably just talk about the conventions (add a method to your package so you can get at it via spec) and not the mechanism -- I think the mechanism is going to confuse packagers.

alalazo · 2017-03-02T18:08:50Z

@tgamblin Will do. Do you have any preference where to put the docs?

tgamblin · 2017-03-02T21:07:26Z

@alalazo: probably should be a section in the packaging guide; maybe it could be explained along with setup_dependent_package and friends.

introduced by PR spack#1875

…3382) introduced by PR #1875

- Added a new interface for Specs to pass build information - Calls forwarded from Spec to Package are now explicit - Added descriptor within Spec to manage forwarding - Added state in Spec to maintain query information - Modified a few packages (the one involved in spack install pexsi) to showcase changes - This uses an object wrapper to `spec` to implement the `libs` sub-calls. - wrapper is returned from `__getitem__` only if spec is concrete - allows packagers to access build information easily

…pack#3382) introduced by PR spack#1875

- Added a new interface for Specs to pass build information - Calls forwarded from Spec to Package are now explicit - Added descriptor within Spec to manage forwarding - Added state in Spec to maintain query information - Modified a few packages (the one involved in spack install pexsi) to showcase changes - This uses an object wrapper to `spec` to implement the `libs` sub-calls. - wrapper is returned from `__getitem__` only if spec is concrete - allows packagers to access build information easily

…pack#3382) introduced by PR spack#1875

- Added a new interface for Specs to pass build information - Calls forwarded from Spec to Package are now explicit - Added descriptor within Spec to manage forwarding - Added state in Spec to maintain query information - Modified a few packages (the one involved in spack install pexsi) to showcase changes - This uses an object wrapper to `spec` to implement the `libs` sub-calls. - wrapper is returned from `__getitem__` only if spec is concrete - allows packagers to access build information easily

…pack#3382) introduced by PR spack#1875

- Added a new interface for Specs to pass build information - Calls forwarded from Spec to Package are now explicit - Added descriptor within Spec to manage forwarding - Added state in Spec to maintain query information - Modified a few packages (the one involved in spack install pexsi) to showcase changes - This uses an object wrapper to `spec` to implement the `libs` sub-calls. - wrapper is returned from `__getitem__` only if spec is concrete - allows packagers to access build information easily

…3382) introduced by PR #1875

- Added a new interface for Specs to pass build information - Calls forwarded from Spec to Package are now explicit - Added descriptor within Spec to manage forwarding - Added state in Spec to maintain query information - Modified a few packages (the one involved in spack install pexsi) to showcase changes - This uses an object wrapper to `spec` to implement the `libs` sub-calls. - wrapper is returned from `__getitem__` only if spec is concrete - allows packagers to access build information easily

…pack#3382) introduced by PR spack#1875

alalazo added WIP needs-response dependencies labels Sep 29, 2016

alalazo self-assigned this Sep 29, 2016

davydden reviewed Sep 29, 2016

View reviewed changes

alalazo force-pushed the features/libs_for_non_virtual branch from c7b5574 to 3ef07a6 Compare October 5, 2016 08:50

davydden mentioned this pull request Oct 11, 2016

Abinit: Added package(s) #1995

Merged

alalazo changed the title ~~[WIP] Spec interface for build information : first tentative implementation~~ Spec interface for build information : first tentative implementation Oct 13, 2016

alalazo force-pushed the features/libs_for_non_virtual branch from 3ef07a6 to 2e7f958 Compare October 15, 2016 13:42

davydden mentioned this pull request Nov 16, 2016

hdf5: fix for lib64 installation #2349

Merged

alalazo force-pushed the features/libs_for_non_virtual branch from 8c47aee to 09f7405 Compare November 28, 2016 15:07

alalazo mentioned this pull request Nov 29, 2016

py-numpy: rework blas/lapack #2365

Merged

4 tasks

alalazo changed the title ~~Spec interface for build information : first tentative implementation~~ spec : interface for build information Nov 30, 2016

alalazo added ready and removed WIP labels Nov 30, 2016

adamjstewart reviewed Nov 30, 2016

View reviewed changes

scheibelp reviewed Nov 30, 2016

View reviewed changes

ForwardQueryToPackage: corrected a typo + style fix

605aeb4

alalazo force-pushed the features/libs_for_non_virtual branch from 3b5282c to 605aeb4 Compare March 2, 2017 08:10

tgamblin approved these changes Mar 2, 2017

View reviewed changes

tgamblin merged commit ed582ce into spack:develop Mar 2, 2017

tgamblin mentioned this pull request Mar 2, 2017

variant: added support for multi-valued variants #2386

Merged

6 tasks

alalazo deleted the features/libs_for_non_virtual branch March 2, 2017 18:08

This was referenced Mar 3, 2017

Spec object has no attribute cppflags - openblas fails to build #3343

Closed

'Spec' object has no attribute 'lapack_libs' #3364

Closed

fix blas/lapack usage after PR 1875 #3365

Merged

alalazo mentioned this pull request Mar 7, 2017

spec build interface: added docs #3377

Closed

bvanessen added a commit to bvanessen/spack that referenced this pull request Mar 8, 2017

Fixes the Elemental and LBANN packages to conform to the new syntax

274d4f5

introduced by PR spack#1875

bvanessen mentioned this pull request Mar 8, 2017

Fixes the Elemental and LBANN packages to conform to the new syntax #3382

Merged

adamjstewart pushed a commit that referenced this pull request Mar 8, 2017

Fixes the Elemental and LBANN packages to conform to the new syntax (#…

acf95c9

…3382) introduced by PR #1875

adamjstewart mentioned this pull request Mar 16, 2017

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

Closed

diaena pushed a commit to diaena/spack that referenced this pull request May 26, 2017

Fixes the Elemental and LBANN packages to conform to the new syntax (s…

9d68165

…pack#3382) introduced by PR spack#1875

xavierandrade pushed a commit to xavierandrade/spack that referenced this pull request Jun 16, 2017

Fixes the Elemental and LBANN packages to conform to the new syntax (s…

b42f3e7

…pack#3382) introduced by PR spack#1875

EmreAtes pushed a commit to EmreAtes/spack that referenced this pull request Jul 10, 2017

Fixes the Elemental and LBANN packages to conform to the new syntax (s…

1fe29fb

…pack#3382) introduced by PR spack#1875

amklinv pushed a commit that referenced this pull request Jul 17, 2017

Fixes the Elemental and LBANN packages to conform to the new syntax (#…

dfbf428

…3382) introduced by PR #1875

healther pushed a commit to electronicvisions/spack that referenced this pull request Jul 26, 2017

Fixes the Elemental and LBANN packages to conform to the new syntax (s…

4bdb3b7

…pack#3382) introduced by PR spack#1875

tgamblin added this to the v0.11.0 milestone Nov 12, 2017

omor1 mentioned this pull request Aug 29, 2020

BLAS/LAPACK providers should set CMake configure options in consumers #18372

Open

2 tasks



		def _libs_default_handler(descriptor, spec, cls):
		"""Default handler when looking for 'libs' attribute

Conversation

alalazo commented Sep 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Analysis and requirements

Design

Modifications

Uh oh!

alalazo commented Sep 29, 2016

Uh oh!

davydden left a comment

Choose a reason for hiding this comment

Uh oh!

davydden Sep 29, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alalazo commented Sep 29, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alalazo commented Sep 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davydden commented Nov 8, 2016

Uh oh!

alalazo commented Nov 25, 2016

Uh oh!

davydden commented Nov 25, 2016

Uh oh!

alalazo commented Nov 30, 2016

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!

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

Uh oh!

Uh oh!

alalazo commented Mar 2, 2017

Uh oh!

tgamblin commented Mar 2, 2017

Uh oh!

alalazo commented Mar 2, 2017

Uh oh!

tgamblin commented Mar 2, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

alalazo commented Sep 29, 2016 •

edited

Loading

davydden Sep 29, 2016 •

edited

Loading

davydden commented Sep 29, 2016 •

edited

Loading

alalazo commented Sep 29, 2016 •

edited

Loading

scheibelp commented Nov 30, 2016 •

edited

Loading