Update Sundials package #5696

gardner48 · 2017-10-11T19:36:28Z

Change to CMake subclass
Update web addresses
Add 3.0.0-beta-2 version
Add new and missing Sundials configuration options
Add conflicts for some variants
Add xSDK configuration
Update lists of example program Makefiles

…s to install fixes

davydden · 2017-10-11T20:18:50Z

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

-            description='Use external BLAS/LAPACK libraries')
-    variant('klu',     default=False,
+            description='Enable MPI parallel vector')
+    variant('openmp',  default=False,


are openmp and pthread mutually exclusive (i.e. only one can be True at the same time)? If so, could you please rework it into a single variant:

variant( 'threads', default='none', description='Multithreading support', values=('pthreads', 'openmp', 'none'), multi=False )

that's what we do with all blas/lapac (i.e. see openblas)

In our case openmp and pthread are not mutually exclusive options. Turning on these variants builds separate openmp and pthread vector modules that can be used together if a user wanted.

i see, thanks for explanation

davydden · 2017-10-11T20:21:11Z

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

+            description='Enable RAJA parallel vector')
+
+    # External libraries
+    variant('blas',       default=False,


why do we need internal blas at all? I would remove the two variants all together... Imaging what could happen if one uses sundials as a part of a bigger DAG (i.e. dealii) which uses petsc, trilinos, mumps etc where everyone use exactly the same Blas/Lapack, but now you would have different Blas/Lapack within sundials itself. So if you have dynamic linking, you can easily get into troubles because you don't have control over what blas/lapack is used within sundials (threaded / openmp or not).

generally, 99% packages in Spack do not build any dependencies themselves, look at petsc which is able to compile all of its dependencies but we provide them via Spack for a good reason.

another way to look at it is that Spack promises that there is one and only one version/variant/configuration of each dependency within the DAG. If you build internal Blas/Lapack, this is no longer the case...

Sundials does not build an internal blas and only needs to link to a blas library when building with an external solver requires blas (e.g. superlu_mt). Activating the blas variant sets the path to whatever blas the user is providing.

an external solver requires blas (e.g. superlu_mt).

oh, then you don't need these variants at all. If superlu_mt is there and it needs blas/lapack, you will have it in DAG.

@tgamblin : if some dependency in the DAG needs blas/lapack, spec['blas'].libs will still work, right, meaning there is no need to declare that the root node depends on blas/lapack?

Activating the blas variant sets the path to whatever blas the user is providing.

atlernative is still to remove those variants and add something like

depends_on('blas', when='+superlu_mt') depends_on('lapack', when='+superlu_mt')

It is also possible to build superlu_mt with its own internal blas so it doesn't necessarily depend on using blas. So it seems like we don't need the blas variant or the depends_on in the package.

It is also possible to build superlu_mt with its own internal blas

that's bad for the reasons outlined above... Luckily by default it will use blas from Spack. Maybe you can make sure it is this way here by writing

depends_on(superlu_mt+blas,...)

superlu_mt encourages users to not use the internal blas unless they have to and since Spack will provides a blas installation your suggestion sounds like the way to go.

davydden · 2017-10-11T20:22:16Z

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

+            description='Install examples')
+
+    # Generic (std-c) math libraries (UNIX only)
+    variant('generic-math', default=True,


does it work on macOS? if not, maybe set default = platfom!="darwin" or alike.

Our CMake build links to std-c math libraries only if IF(UNIX) is true. This option is really for a unix user to turn this behavior off, otherwise it's not used.

ah, ok. Just wanted to be sure that it won't break the build on macOS with default values (the current version of the package builds OK).

davydden · 2017-10-11T20:22:42Z

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

+    # ==========================================================================
+
+    # Build dependencies
+    depends_on('cmake', type='build')


that's in CMakePackage already

Thanks, I'll remove this. It was left over from the original Sundials package which used Package rather than CMakePackage.

they only reason to keep this if you add some specific constraint on version of cmake. There are a couple of packages which do that.

We do require at least CMake 2.8.1 and I'll update the depends to reflect that.

davydden · 2017-10-11T20:23:26Z

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

+
+    # MPI related dependencies
+    depends_on('mpi', when='+mpi')
+    depends_on('mpi', when='+hypre')


can you build ~mpi+hypre? if not, there should be a conflict

You're right, this needs to be added as a conflict.

davydden · 2017-10-11T20:23:49Z

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

+    # MPI related dependencies
+    depends_on('mpi', when='+mpi')
+    depends_on('mpi', when='+hypre')
+    depends_on('mpi', when='+petsc')


same here, if +petsc~mpi does not make sense, declare a conflict

Same thing here, I'll add a conflict.

davydden · 2017-10-11T20:26:32Z

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

+    # SUNDIALS xSDK Settings
+    # ==========================================================================
+
+    @when('xsdk-defaults=True')


do you really need two different cmake_args? there is so much code duplication here!.. Can you not do some if/else within the same function based on xsdk-defaults? It would be much easier to maintain...

Looking over this again we don't really need the xsdk-defaults option in Spack since user's don't directly set the CMake variables. So I've removed this option and the related cmake_args function.

davydden · 2017-10-11T20:31:08Z

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

+
+    # External libraries
+    depends_on('hypre',              when='+hypre')
+    depends_on('petsc',              when='+petsc')


probably you want petsc+mpi, as petsc can also be built without mpi.

Yes I do, I'll update the dependency.

davydden · 2017-10-11T20:33:11Z

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

    depends_on('lapack',             when='+lapack')
    depends_on('suite-sparse',       when='+klu')
-    depends_on('superlu-mt+openmp',  when='+superlu+openmp')
-    depends_on('superlu-mt+pthread', when='+superlu+pthread')
+    depends_on('superlu-mt+openmp',  when='+superlu-mt+openmp')


you probably want to add a conflict that +superlu-mt needs either openmp or pthread, and if both are disabled you can't have sundials+supleru-mt.

These depend lines are not quite right since it is possible to use superlu-mt from Sundials without enabling openmp or pthreads (i.e. Sundials is run in serial but the linear solves with superlu_mt are threaded).

So what I need is to know is if superlu_mt was build with openmp or pthread and set our CMake variable accordingly. Is this the right way to do that

if spec.satisfies('^superlu-mt+openmp'):

Yes, I think it’s the right syntax

The above if works as expected for checking how superlu_mt was configured.

davydden · 2017-10-11T21:30:37Z

btw, you may want to add yourself as a maintainer of sundials :

    maintainers = ['gardner48']

which would appear in spack info sundials.

junghans · 2017-10-12T01:46:40Z

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

+    variant('idas',   default=True,
+            description='Enable IDAS')
+    variant('kinsol', default=True,
+            description='Enable KINSOL')


Can you turn them in an array and loop over them? (see lammps for an example)

Thanks for the suggestion and the pointer to lammps. I've updated these options to an array of sundials solvers.

Remove blas variant as blas is only needed when used by an external linear solver. Set related CMake blas variables as needed depending enabled external linear solvers. Add minimum required CMake version. Additional conflicts and dependencies for external libraries based on mpi, indextype, and precision. Fix SuperLU_MT logic to check which threading type SuperLU_MT was configured with. Add maintaiers. Change Sundials solver options to use an array of values. Consistently use % for formatting.

davydden

thanks for updating, some minor suggestions, otherwise LGTM.

davydden · 2017-10-12T05:36:14Z

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

-    url = "http://computation.llnl.gov/projects/sundials-suite-nonlinear-differential-algebraic-equation-solvers/download/sundials-2.6.2.tar.gz"
+    homepage = "https://computation.llnl.gov/projects/sundials"
+    url = "https://computation.llnl.gov/projects/sundials/download/sundials-2.7.0.tar.gz"
+    maintainers = ['cswoodward', 'gardner48']


davydden · 2017-10-12T05:37:07Z

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

+    variant(
+        'precision',
+        default='double',
+        description='''real type precision''',


nitpick: i think it's enough to have a single ' on both sides

davydden · 2017-10-12T05:37:18Z

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

+    variant(
+        'indextype',
+        default='int64_t',
+        description='''index integer type''',


same here, a single '

davydden · 2017-10-12T05:41:18Z

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

+        'indextype',
+        default='int64_t',
+        description='''index integer type''',
+        values=('int32_t', 'int64_t'),


to be consistent with other packages (i.e. at least petsc, mump, superlu-dist, hypre), could you please make it into a single bool variant:

int64 [off] True, False Compile with 64bit indices

I've made this a single bool option but does the default need to be False? Prior to the current beta release sundials only supported 64bit indices.

Prior to the current beta release sundials only supported 64bit indices.

i would say we shall keep it as True then, so that default values are working, i.e. spack install sundials should build.

And after you have a next stable release we can switch it to False by default to be consistent with defaults in other packages.

davydden · 2017-10-12T05:43:07Z

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

+    # Require that PETSc is built with MPI
+    depends_on('petsc+mpi', when='+petsc')
+
+    # Require that SuperLU_MT built with external blas


davydden · 2017-10-12T05:43:28Z

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


        def on_off(varstr):
            return 'ON' if varstr in self.spec else 'OFF'

-        cmake_args = std_cmake_args[:]


indeed, that's in CMakePackage already 👍

davydden · 2017-10-12T05:46:08Z

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

+            description='Enable PETSc MPI parallel vector')
+
+    # Library type
+    variant('shared', default=True,


FYI, there is currently a discussion in Spack on how to unify static/shared #5269 If you have any preferences where that go, please feel free to join the discussion

davydden · 2017-10-12T05:49:48Z

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

+
+    # External libraries
+    variant('lapack',     default=False,
+            description='Enable LAPACK direct solvers')


so that's something you use in Sundials regardless of superlu_mt or alike? So that's indeed a build option?

Yes, this activates interfaces to use LAPACKdense or banded solvers for linear systems that arise inside sundials time integrators and nonlinear solvers.

davydden · 2017-10-12T18:12:56Z

@junghans ping.

davydden · 2017-10-13T17:30:10Z

@junghans since you requested changes to this PR, do you have further comments?

tgamblin · 2017-10-14T02:11:34Z

@junghans

gardner48 added 4 commits October 10, 2017 20:06

initial update of sundials package

f5b840b

fix bugs in initial sundials update

4f59975

add xsdk cmake setup, fix generic math option, add cuda/raja Makefile…

c795936

…s to install fixes

Fix lapack install bug, add new conflicts, clean up formatting

e647e04

davydden suggested changes Oct 11, 2017

View reviewed changes

davydden reviewed Oct 11, 2017

View reviewed changes

junghans requested changes Oct 12, 2017

View reviewed changes

davydden approved these changes Oct 12, 2017

View reviewed changes

gardner48 added 2 commits October 12, 2017 08:49

change triple-single quotes to single quotes

5392a4a

Change indextype option to a single int64 option

5ef3037

davydden approved these changes Oct 12, 2017

View reviewed changes

tgamblin added the xSDK label Oct 12, 2017

junghans approved these changes Oct 14, 2017

View reviewed changes

junghans merged commit f885c89 into spack:develop Oct 14, 2017

Update Sundials package #5696

Update Sundials package #5696

Conversation

gardner48 commented Oct 11, 2017

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

gardner48 Oct 11, 2017 • 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

Choose a reason for hiding this comment

gardner48 Oct 11, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gardner48 Oct 11, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gardner48 Oct 11, 2017 • 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

davydden Oct 11, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davydden Oct 11, 2017 • 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

davydden commented Oct 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davydden left a 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

gardner48 Oct 12, 2017 • 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

davydden commented Oct 12, 2017

davydden commented Oct 13, 2017

tgamblin commented Oct 14, 2017

gardner48 Oct 11, 2017 •

edited

gardner48 Oct 11, 2017 •

edited

gardner48 Oct 11, 2017 •

edited

gardner48 Oct 11, 2017 •

edited

davydden Oct 11, 2017 •

edited

davydden Oct 11, 2017 •

edited

gardner48 Oct 12, 2017 •

edited