[bug#50080] gnu: sundials, dealii: Update sundials to 5.7.0 and add to dealii inputs.

Empty Message

Ping.

Hi Ludo,

I see that you have updated SUNDIALS to 6.1.1.
When last I updated it (to 5.7.0) [1], I encountered issues in 32-bit vs 64-bit
indices, which I see no mention of in your commit.
There were also some other issues which I mentioned in my patch series'
commit messages.

Do you know if the update to 6.1.1 renders the issues extraneous?
A good test would be adding SUNDIALS as a propagated input to
deal.II.
That had been my original goal.

Thanks,
Paul

[1]: https://issues.guix.gnu.org/50080

Hi Paul,

"Paul A. Patience" <paul@apatience.com> skribis:

> I see that you have updated SUNDIALS to 6.1.1.
> When last I updated it (to 5.7.0) [1], I encountered issues in 32-bit vs 64-bit
> indices, which I see no mention of in your commit.
> There were also some other issues which I mentioned in my patch series'
> commit messages.
>
> Do you know if the update to 6.1.1 renders the issues extraneous?

Thanks for bringing it up, I had overlooked this patch of yours—my
apologies for that.  I don’t know if the new version defaults to 64-bit
or 32-bit indices.

> A good test would be adding SUNDIALS as a propagated input to
> deal.II.

I tried that; this leads to a build failure apparently due to changes in
the API of SUNDIALS:

--8<---------------cut here---------------start------------->8---
/tmp/guix-build-dealii-9.3.2.drv-0/dealii-9.3.2/source/sundials/arkode.cc: In instantiation of ‘void dealii::SUNDIALS::ARKode<VectorType>::rese
t(double, double, const VectorType&) [with VectorType = dealii::Vector<double>]’:
/tmp/guix-build-dealii-9.3.2.drv-0/dealii-9.3.2/source/sundials/arkode.cc:838:18:   required from here
/tmp/guix-build-dealii-9.3.2.drv-0/dealii-9.3.2/source/sundials/arkode.cc:635:31: error: too few arguments to function ‘void* ARKStepCreate(ARK
RhsFn, ARKRhsFn, realtype, N_Vector, SUNContext)’
  635 |     arkode_mem = ARKStepCreate(
      |                  ~~~~~~~~~~~~~^
  636 |       explicit_function ? &t_arkode_explicit_function<VectorType> : nullptr,
      |       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  637 |       implicit_function ? &t_arkode_implicit_function<VectorType> : nullptr,
      |       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  638 |       current_time,
      |       ~~~~~~~~~~~~~            
  639 |       initial_condition_nvector);
      |       ~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from /tmp/guix-build-dealii-9.3.2.drv-0/dealii-9.3.2/source/sundials/arkode.cc:45:
/gnu/store/vyl9wll72m9jqc21idxrxnmb7rm0lkf7-sundials-6.1.1/include/arkode/arkode_arkstep.h:112:23: note: declared here
  112 | SUNDIALS_EXPORT void* ARKStepCreate(ARKRhsFn fe, ARKRhsFn fi,
      |                       ^~~~~~~~~~~~~
/tmp/guix-build-dealii-9.3.2.drv-0/dealii-9.3.2/source/sundials/arkode.cc: In instantiation of ‘void dealii::SUNDIALS::ARKode<VectorType>::setu
p_system_solver(const VectorType&) [with VectorType = dealii::Vector<double>]’:
/tmp/guix-build-dealii-9.3.2.drv-0/dealii-9.3.2/source/sundials/arkode.cc:838:18:   required from here
/tmp/guix-build-dealii-9.3.2.drv-0/dealii-9.3.2/source/sundials/arkode.cc:710:30: error: too few arguments to function ‘_generic_SUNLinearSolve
r* SUNLinSol_SPGMR(N_Vector, int, int, SUNContext)’
  710 |               SUNLinSol_SPGMR(y_template,
      |               ~~~~~~~~~~~~~~~^~~~~~~~~~~~
  711 |                               PREC_NONE,
      |                               ~~~~~~~~~~
  712 |                               0 /*krylov subvectors, 0 uses default*/);
      |                               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
--8<---------------cut here---------------end--------------->8---

I’m probably less familiar than you with these two packages; what would
you suggest?

Please keep me Cc’d and hopefully we can address these issues for good
this time!

Thanks,
Ludo’.

Hi Ludovic,

On 2022-04-15 04:50:39-04:00, Ludovic Courtès wrote:
> I’m probably less familiar than you with these two packages; what would
> you suggest?

I will go through my old patches taking into consideration the latest
version of SUNDIALS.
I had discovered the mismatched integer sizes because of some
build failures, so it should be relatively easy to reproduce in the
latest version of SUNDIALS.
It is possible that deal.II does not yet support the latest version of
SUNDIALS, in which case we will need to keep the old version around,
perhaps as dealii-sundials or some such.

> Please keep me Cc’d and hopefully we can address these issues for good
> this time!

I should get around to this this week or next.

Best regards,
Paul

On 2022-04-15 04:50:39-04:00, Ludovic Courtès wrote:
> "Paul A. Patience" <paul@apatience.com> skribis:
>> A good test would be adding SUNDIALS as a propagated input to
>> deal.II.
>
> I tried that; this leads to a build failure apparently due to changes in
> the API of SUNDIALS:

I just checked and indeed, support for SUNDIALS 6.0.0 and later is
present only in the master branch of deal.II (added this past February
[1]), so we will need two versions of the SUNDIALS package for now.

I am taking a look at the integer sizes and other issues now.

Best regards,
Paul

[1]: https://github.com/dealii/dealii/commit/b124fb2887c1643b8673759e0d0b4a71479b1ba2

Hi Ludovic,

On 2022-04-15 04:50:39-04:00, Ludovic Courtès wrote:
> "Paul A. Patience" <paul@apatience.com> skribis:
>> Do you know if the update to 6.1.1 renders the issues extraneous?
>
> Thanks for bringing it up, I had overlooked this patch of yours—my
> apologies for that.  I don’t know if the new version defaults to 64-bit
> or 32-bit indices.

Ok, I figured out what happened.

--8<---------------cut here---------------start------------->8---
      ("petsc" ,petsc-openmpi)         ;support in SUNDIALS requires MPI
-       ,@(package-inputs sundials)))
+       ("petsc-openmpi" ,petsc-openmpi)      ;support in SUNDIALS requires MPI
+       ,@(alist-delete "petsc" (package-inputs sundials))))
     (arguments
      (substitute-keyword-arguments (package-arguments sundials)
        ((#:configure-flags flags '())
-        `(cons* "-DMPI_ENABLE:BOOL=ON"
-                "-DPETSC_ENABLE:BOOL=ON"
-                (string-append "-DPETSC_INCLUDE_DIR="
-                               (assoc-ref %build-inputs "petsc")
-                               "/include")
-                (string-append "-DPETSC_LIBRARY_DIR="
-                               (assoc-ref %build-inputs "petsc")
-                               "/lib")
-                ,flags))
+        `(cons* "-DENABLE_MPI:BOOL=ON" ,flags))
--8<---------------cut here---------------end--------------->8---

You accidentally disabled PETSc support in SUNDIALS by not re-adding
PETSC_ENABLE in the sundials (not sundials-openmpi) package.
If you had done so, the configure step would have failed because PETSc
is supported in SUNDIALS only with MPI (hence the comment).
Once we enable PETSc again, the index size problem appears again, even
with SUNDIALS 6.1.1.

Basically, I think all of my changes are still relevant.
I still think setting the sundials and sundials-openmpi's index sizes to
32 bits is the most logical option (see the commit message in the
“gnu: sundials: Update to 5.7.0.” patch).

I will bring my old patch series up to date and submit it again when
it's done.

Best regards,
Paul

Hi Paul,

"Paul A. Patience" <paul@apatience.com> skribis:

> On 2022-04-15 04:50:39-04:00, Ludovic Courtès wrote:
>> "Paul A. Patience" <paul@apatience.com> skribis:
>>> Do you know if the update to 6.1.1 renders the issues extraneous?
>>
>> Thanks for bringing it up, I had overlooked this patch of yours—my
>> apologies for that.  I don’t know if the new version defaults to 64-bit
>> or 32-bit indices.
>
> Ok, I figured out what happened.
>
>       ("petsc" ,petsc-openmpi)         ;support in SUNDIALS requires MPI
> -       ,@(package-inputs sundials)))
> +       ("petsc-openmpi" ,petsc-openmpi)      ;support in SUNDIALS requires MPI
> +       ,@(alist-delete "petsc" (package-inputs sundials))))
>      (arguments
>       (substitute-keyword-arguments (package-arguments sundials)
>         ((#:configure-flags flags '())
> -        `(cons* "-DMPI_ENABLE:BOOL=ON"
> -                "-DPETSC_ENABLE:BOOL=ON"
> -                (string-append "-DPETSC_INCLUDE_DIR="
> -                               (assoc-ref %build-inputs "petsc")
> -                               "/include")
> -                (string-append "-DPETSC_LIBRARY_DIR="
> -                               (assoc-ref %build-inputs "petsc")
> -                               "/lib")
> -                ,flags))
> +        `(cons* "-DENABLE_MPI:BOOL=ON" ,flags))
>
> You accidentally disabled PETSc support in SUNDIALS by not re-adding
> PETSC_ENABLE in the sundials (not sundials-openmpi) package.
> If you had done so, the configure step would have failed because PETSc
> is supported in SUNDIALS only with MPI (hence the comment).

Oops, got it; sorry about that.

> Once we enable PETSc again, the index size problem appears again, even
> with SUNDIALS 6.1.1.
>
> Basically, I think all of my changes are still relevant.
> I still think setting the sundials and sundials-openmpi's index sizes to
> 32 bits is the most logical option (see the commit message in the
> “gnu: sundials: Update to 5.7.0.” patch).
>
> I will bring my old patch series up to date and submit it again when
> it's done.

Excellent.  Please let me know when you’re done and I’ll happily apply
it.

Thanks!

Ludo’.

Hi Paul,

"Paul A. Patience" <paul@apatience.com> skribis:

> Addressed issue reported by Maxime Devos.
>
> Paul A. Patience (4):
>   gnu: petsc-openmpi: Fix header inclusions.
>   gnu: sundials: Fix various issues.
>   gnu: sundials-openmpi: Add HYPRE dependency.
>   gnu: sundials: Update to 6.2.0.

Awesome, I’ve applied the whole series.

Thanks,
Ludo’.