Add KLU support to sundials #94930

GuillaumeDesforges · 2020-08-08T09:38:22Z

Motivation for this change

Add KLU support to sundials.
Needed for Haskell hmatrix-sparse for instance

Things done

GuillaumeDesforges · 2020-08-08T09:40:01Z

teto · 2020-08-08T10:52:56Z

I dont think it's linked but nixpkgs-review fails with

builder for '/nix/store/is15pq5q9s83qdzniasw5qcbl61gg0ah-python2.7-scikits.odes-2.6.1.drv' failed with exit code 2; last 10 log lines:
  ImportError while importing test module '/nix/store/2p2pbr3i0g66x802f9b2kgqccz42hc07-python2.7-scikits.odes-2.6.1/lib/python2.7/site-packages/scikits/odes/tests/test_user_return_vals_ida.py'.
  Hint: make sure your test modules/packages have valid Python names.
  Traceback:
  test_user_return_vals_ida.py:5: in <module>
      from ..sundials.ida import StatusEnumIDA
  scikits/odes/sundials/ida.pyx:1: in init scikits.odes.sundials.ida
      ???
  E   ImportError: No module named scikits.odes.sundials.common_defs
  !!!!!!!!!!!!!!!!!!! Interrupted: 8 errors during collection !!!!!!!!!!!!!!!!!!!!
  =========================== 8 error in 0.45 seconds ============================
[3 built (1 failed), 63 copied (465.4 MiB), 109.3 MiB DL]
error: error: --- Error --- nix-daemon
1 dependencies of derivation '/nix/store/6pf7pka6zgksbwbgnc1yxyr8j3wwq5ya-env.drv' failed to build
https://github.com/NixOS/nixpkgs/pull/94930
1 package failed to build:
python27Packages.scikits-odes

3 packages built:
python37Packages.scikits-odes python38Packages.scikits-odes sundials

Isn't adding suitesparse to buildInputs enough ?

doronbehar · 2020-08-08T12:09:00Z

@GuillaumeDesforges you should rebase on latest master, for those that don't use nixpkgs-review :).

@teto with this patch:

diff --git i/pkgs/development/libraries/sundials/default.nix w/pkgs/development/libraries/sundials/default.nix
index 74907ee1cc9..d69b09ade07 100644
--- i/pkgs/development/libraries/sundials/default.nix
+++ w/pkgs/development/libraries/sundials/default.nix
@@ -15,7 +15,18 @@ stdenv.mkDerivation rec {
   pname = "sundials";
   version = "5.3.0";
 
-  buildInputs = [ python ] ++ stdenv.lib.optionals (lapackSupport) [ gfortran blas lapack ];
+  buildInputs = [
+    python
+  ]
+  ++ stdenv.lib.optionals (lapackSupport) [
+    gfortran
+    blas
+    lapack
+  ]
+  ++ stdenv.lib.optionals (kluSupport) [
+    suitesparse
+  ]
+  ;
   nativeBuildInputs = [ cmake ];
 
   src = fetchurl {
@@ -39,8 +50,8 @@ stdenv.mkDerivation rec {
     "-DLAPACK_LIBRARIES=${lapack}/lib/liblapack${stdenv.hostPlatform.extensions.sharedLibrary}"
   ] ++ stdenv.lib.optionals (kluSupport) [
     "-DKLU_ENABLE=ON"
-    "-DKLU_INCLUDE_DIR=${suitesparse.dev}/include"
-    "-DKLU_LIBRARY_DIR=${suitesparse}/lib"
+    # "-DKLU_INCLUDE_DIR=${suitesparse.dev}/include"
+    # "-DKLU_LIBRARY_DIR=${suitesparse}/lib"
   ];
 
   doCheck = true;

I got this cmake warnings:

CMake Warning at config/SundialsCMakeMacros.cmake:83 (message):
  ------------------------------------------------------------------------

  WARNING: KLU LIBRARIES NOT Found.  Please check library path



  ------------------------------------------------------------------------
Call Stack (most recent call first):
  config/SundialsKLU.cmake:72 (print_warning)
  CMakeLists.txt:1020 (include)


-- Looking for KLU libraries... FAILED

However, I'd still put suitesparse in buildInputs, although it's probably referenced anyway - to prevent ambiguity for cross compilations (experts might know that this is still not needed, but I'm not a NixOS' cross compiles expert :)).

Regarding scikit-odes' failure in tests - perhaps we should disable it for Python2 interpreters?

idontgetoutmuch · 2020-08-08T12:44:02Z

Regarding scikit-odes' failure in tests - perhaps we should disable it for Python2 interpreters?

@doronbehar Agreed but I don't know how to do this. If I look in scikits-odes/default.nix I see nothing that specifies the version of python to use (or not use).

doronbehar · 2020-08-08T12:45:39Z

git grep disabledIf

GuillaumeDesforges · 2020-08-08T16:09:09Z

Got it rebased.

It was indeed required to explicit CMAKE flags.
I'll put it in buildInputs as well.

@idontgetoutmuch if I remember correctly there is a isPy3 attribute passed to buildPythonPackage, and one can set the attribute disabled.
I've looked at nixpkgs manual and did not find info about that, it must be missing.

EDIT: Ah got it

nixpkgs/pkgs/development/interpreters/python/default.nix

Line 32 in 1a0abf7

isPy3 = lib.strings.substring 0 1 pythonVersion == "3";

doronbehar · 2020-08-08T17:20:33Z

You need disabledIf = python.isPy2.

GuillaumeDesforges · 2020-08-10T09:26:54Z

I get an error stating

anonymous function at /home/arsleust/.cache/nixpkgs-review/rev-0d43a96e27f0f905b72c9d786de69e70a21c3565/nixpkgs/pkgs/development/python-modules/scikits-odes/default.nix:1:1 called without required argument 'isPy2'

Isn't isPy2 part of the passthru ?

pkgs/development/python-modules/scikits-odes/default.nix

doronbehar · 2020-08-10T11:19:20Z

Github's actions marking this as failed are a false negative.

@GrahamcOfBorg build python2Packages.scikit-odes python3Packages.scikit-odes

jonringer

otherwise LGTM

pkgs/development/libraries/sundials/default.nix

doronbehar · 2020-08-10T18:56:17Z

@GrahamcOfBorg build python27Packages.scikits-odes python3Packages.scikits-odes

doronbehar

Diff LGTM, but I'd only kindly ask to make the last commit message a bit more precise:

python2Packages.scikits-odes: disable

Ever since we added KLU support to sundials (#94930),
it stopped building while the Python 3 versions didn't
break. Python 2 is EOL.

doronbehar

👍.

idontgetoutmuch · 2020-08-08T12:45:52Z

pkgs/development/libraries/sundials/default.nix

-, lapackSupport ? true }:
+, suitesparse
+, lapackSupport ? true
+, kluSupport ? true }:


Should we make the default false so as not to break other packages which might use this? For example, scikits-odes does not need kluSupport.

Maybe better is make kluSupport false when building scikits-odes

are the klu related packages likely to break the build?

I don't think it is necessary to disable klu support in sundials for scikits-odes, since build+test is ok.

Via some conditionals, it could be disabled only for python2Packages.scikits-odes :) but TBH I wouldn't see benefit in this effort - according to git grep, there are no reverse dependencies of python2Packages.scikits-odes available, and Python 2 is EOL (!!). python3Packages.scikits-odes builds fine.

are the klu related packages likely to break the build?

What do you mean @jonringer?

The biggest argument against adding "optional" dependencies by default is that it makes more fragile since there's another point of potential failure (either the dependency doesn't build, or the dependency gets bumped and is no longer compatible).

That's the concern I'm expressing.

Unless the klu library is prone to breakages, we should probably add it.

either the dependency doesn't build

suitesparse is what provides KLU support for sundials (IDK what that even means). Suitesparse seems well maintained to me, both upstream and at Nixpkgs.

or the dependency gets bumped and is no longer compatible

The best we can do is to trust Sundials to test it with the current versions of suitesparse we ship. @GuillaumeDesforges perhaps it'll be nice to note in a comment near the conditional kluSupport, that this feature is tested by upstream according to the INSTALL_GUIDE.pdf, section 1.1.4. Currently they write there:

The KLU libraries are part of SuiteSparse, a suite of sparse matrix software, available from the TexasA&M University website:http://faculty.cse.tamu.edu/davis/suitesparse.html.sundialshasbeen tested with SuiteSparse version 5.3.0

While we ship version 5.7.2 of suitesparse, so IDK 🙆‍♀️.

As a side note, I also support enabling this feature by default, for GNU Octave 6.0.0 which is about to be released. It will require this feature as well from this major version of sundials (5) . Currently Octave uses an old version of sundials with this feature enabled as well, see https://github.com/NixOS/nixpkgs/pull/79864/files#diff-a71f6e05947eac8bd8360ddb7faa9693 .

I don't understand the issue. As with most nixpkgs packages, if sundials requires a bumped version of suitesparse we can just upgrade suitesparse, and if it breaks other packages we can create another bumped package like suitesparse_X_X_X (like in the Haskell infrastructure where multiple versions of a package live).

Suitesparse seems well maintained to me, both upstream and at Nixpkgs.

Agreed, I would have been more concerned if it wasn't the case but it seems we are relatively safe here

Ever since we added KLU support to sundials (NixOS#94930), it stopped building while the Python 3 versions didn't break. Python 2 is EOL.

doronbehar

👍 on adding commenting the link to upstream's INSTALL_GUIDE.pdf.

GuillaumeDesforges · 2020-08-27T10:43:19Z

@jonringer is there anything left to do in order for this to be merged ?

doronbehar · 2020-09-20T18:40:48Z

@jonringer I'm now testing the release candidate of Octave 6 which needs sundials with this patch, otherwise:

configure: WARNING: SUNDIALS IDA library not configured with SUNLINSOL_KLU or sunlinksol_klu.h is not usable.  The solvers ode15i and ode15s will not support the sparse Jacobian feature.

Is there anything left here to do / any objections left?

nixos-discourse · 2020-09-21T11:44:25Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/openblas-vs-reference-blas-implementation/9086/1

idontgetoutmuch · 2020-09-21T15:35:48Z

@flokli @FRidh @jonringer Can I help with anything to get this PR accepted?

jonringer

LGTM

Result of nixpkgs-review pr 94930 1

3 packages built:

python37Packages.scikits-odes
python38Packages.scikits-odes
sundials

GuillaumeDesforges requested a review from flokli August 8, 2020 09:40

ofborg bot requested a review from idontgetoutmuch August 8, 2020 09:46

ofborg bot added 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 labels Aug 8, 2020

GuillaumeDesforges force-pushed the pr/sundials-klu branch from b7c2869 to 922cae2 Compare August 8, 2020 15:54

GuillaumeDesforges force-pushed the pr/sundials-klu branch from 922cae2 to e34415a Compare August 10, 2020 09:15

GuillaumeDesforges requested review from FRidh and jonringer as code owners August 10, 2020 09:15

GuillaumeDesforges force-pushed the pr/sundials-klu branch from e34415a to 0d43a96 Compare August 10, 2020 09:21

doronbehar reviewed Aug 10, 2020

View reviewed changes

pkgs/development/python-modules/scikits-odes/default.nix Outdated Show resolved Hide resolved

ofborg bot added the 6.topic: python label Aug 10, 2020

GuillaumeDesforges force-pushed the pr/sundials-klu branch from 0d43a96 to e107e75 Compare August 10, 2020 10:15

ofborg bot added the 8.has: clean-up label Aug 10, 2020

jonringer reviewed Aug 10, 2020

View reviewed changes

pkgs/development/libraries/sundials/default.nix Outdated Show resolved Hide resolved

GuillaumeDesforges force-pushed the pr/sundials-klu branch from e107e75 to b9be3b3 Compare August 10, 2020 20:39

GuillaumeDesforges requested review from jonringer and doronbehar August 10, 2020 20:40

doronbehar requested changes Aug 11, 2020

View reviewed changes

GuillaumeDesforges force-pushed the pr/sundials-klu branch from b9be3b3 to cd81c8d Compare August 11, 2020 10:53

GuillaumeDesforges requested a review from doronbehar August 11, 2020 10:53

doronbehar approved these changes Aug 11, 2020

View reviewed changes

idontgetoutmuch approved these changes Aug 11, 2020

View reviewed changes

Guillaume Desforges added 2 commits August 19, 2020 10:33

sundials: add KLU support

ee6ff90

python2Packages.scikits-odes: disable

12759d3

Ever since we added KLU support to sundials (NixOS#94930), it stopped building while the Python 3 versions didn't break. Python 2 is EOL.

GuillaumeDesforges requested a review from doronbehar August 19, 2020 08:34

GuillaumeDesforges force-pushed the pr/sundials-klu branch from cd81c8d to 12759d3 Compare August 19, 2020 08:34

ofborg bot requested a review from idontgetoutmuch August 19, 2020 17:32

idontgetoutmuch approved these changes Aug 25, 2020

View reviewed changes

doronbehar approved these changes Aug 25, 2020

View reviewed changes

GuillaumeDesforges mentioned this pull request Aug 27, 2020

Add parallel execution tweag/funflow2#40

Merged

jonringer approved these changes Sep 21, 2020

View reviewed changes

jonringer merged commit 4e888fb into NixOS:master Sep 21, 2020

GuillaumeDesforges deleted the pr/sundials-klu branch September 22, 2020 15:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add KLU support to sundials #94930

Add KLU support to sundials #94930

GuillaumeDesforges commented Aug 8, 2020

GuillaumeDesforges commented Aug 8, 2020

teto commented Aug 8, 2020

doronbehar commented Aug 8, 2020

idontgetoutmuch commented Aug 8, 2020

doronbehar commented Aug 8, 2020

GuillaumeDesforges commented Aug 8, 2020 •

edited

doronbehar commented Aug 8, 2020

GuillaumeDesforges commented Aug 10, 2020

doronbehar commented Aug 10, 2020

jonringer left a comment

doronbehar commented Aug 10, 2020

doronbehar left a comment

doronbehar left a comment

idontgetoutmuch Aug 8, 2020

idontgetoutmuch Aug 11, 2020

jonringer Aug 11, 2020

GuillaumeDesforges Aug 12, 2020

doronbehar Aug 12, 2020

jonringer Aug 12, 2020

doronbehar Aug 12, 2020

GuillaumeDesforges Aug 19, 2020

doronbehar left a comment

GuillaumeDesforges commented Aug 27, 2020

doronbehar commented Sep 20, 2020

nixos-discourse commented Sep 21, 2020

idontgetoutmuch commented Sep 21, 2020

jonringer left a comment

Add KLU support to sundials #94930

Add KLU support to sundials #94930

Conversation

GuillaumeDesforges commented Aug 8, 2020

Motivation for this change

Things done

GuillaumeDesforges commented Aug 8, 2020

teto commented Aug 8, 2020

doronbehar commented Aug 8, 2020

idontgetoutmuch commented Aug 8, 2020

doronbehar commented Aug 8, 2020

GuillaumeDesforges commented Aug 8, 2020 • edited

doronbehar commented Aug 8, 2020

GuillaumeDesforges commented Aug 10, 2020

doronbehar commented Aug 10, 2020

jonringer left a comment

Choose a reason for hiding this comment

doronbehar commented Aug 10, 2020

doronbehar left a comment

Choose a reason for hiding this comment

doronbehar 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

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

doronbehar left a comment

Choose a reason for hiding this comment

GuillaumeDesforges commented Aug 27, 2020

doronbehar commented Sep 20, 2020

nixos-discourse commented Sep 21, 2020

idontgetoutmuch commented Sep 21, 2020

jonringer left a comment

Choose a reason for hiding this comment

GuillaumeDesforges commented Aug 8, 2020 •

edited