Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bump minimum dune version to 2.9 #4157

Merged
merged 1 commit into from
Aug 13, 2024
Merged

bump minimum dune version to 2.9 #4157

merged 1 commit into from
Aug 13, 2024

Conversation

akva2
Copy link
Member

@akva2 akva2 commented Aug 2, 2024

Time to bump the minimum dune version. Users on ubuntu can find dune 2.9 packages for 22.04 in the ppa.

@akva2
Copy link
Member Author

akva2 commented Aug 2, 2024

jenkins build this opm-grid=748 opm-models=914 opm-simulators=5500 opm-upscaling=389 please

@akva2
Copy link
Member Author

akva2 commented Aug 2, 2024

jenkins build this opm-grid=745 opm-models=914 opm-simulators=5500 opm-upscaling=389 please

@blattms blattms merged commit b4f00ee into OPM:master Aug 13, 2024
1 check passed
@akva2 akva2 deleted the bump_dune_2.9 branch August 13, 2024 13:53
@blattms
Copy link
Member

blattms commented Aug 13, 2024

I should have tested this before merging.

Post-merge testing is indicating that we never use dune.module (if we omit dunecontrol) and CMake and compilation is still successful with DUNE 2.8.

Is this intentional or should we give an error in CMake or the build? We probably never did this even before...

@akva2
Copy link
Member Author

akva2 commented Aug 13, 2024

indeed we never enforced these versions in our buildsystem. i guess that would be a good addition.

@alfbr
Copy link
Member

alfbr commented Aug 14, 2024

This breaks build on Red Hat 7 over here. We have a patched Dune 2.8. We are in the process of transitioning to Red Hat 8, but we are still dependent on Red Hat 7 for some weeks to come.

@akva2
Copy link
Member Author

akva2 commented Aug 14, 2024

There are (patched) dune 2.9 rpm's for rh7 and rh8 in the repo.

@alfbr
Copy link
Member

alfbr commented Aug 14, 2024

It is a tedious process to get those rpms installed. We have done it for Red Hat 8, and it took several full working days for yours truly. Moreover, to what extent that even succeeded (i.e., if all nodes actually have them) is still out in the blue. On Red Hat 7 no nodes have them. Hence, if there is any chance I do not have to set a side time for getting those onto Red Hat 7, I am deeply grateful.

@bska
Copy link
Member

bska commented Aug 14, 2024

My main reservation about this is that the simulation results change, at least between using Dune 2.7.1 as a backend and using Dune 2.9.1 as a backend. I've been using the, admittedly aging, 2.7.1 release as my backend for a long time but I decided to test 2.9.1 following this PR. When targeting 2.7.1 I had O(10) regression test failures in OPM-simulators compared to the reference solutions calculated by the CI system (i.e., those available in the opm-tests repository). When targeting the 2.9.1 release, on the other hand, I got O(60) regression test failures. Same compiler, same OS, same hardware, same OPM code.

Note: I didn't check if the solutions calculated by the 2.9.1 backend are better than the 2.7.1 solutions, they may well be, but they are at least different.

@blattms
Copy link
Member

blattms commented Aug 14, 2024

@alfbr What compile errors do you get? On my system this still works with 2.8. This PR has no code changes at all. Maybe the problem was with the other (still open) PRs?

@alfbr
Copy link
Member

alfbr commented Aug 14, 2024

@alfbr What compile errors do you get? On my system this still works with 2.8. This PR has no code changes at all. Maybe the problem was with the other (still open) PRs?

Ah, sorry, I did not actually check. I just thought it would break the build. I guess not breaking the build on 2.8 was an unintended feature of the pull request :)

@atgeirr
Copy link
Member

atgeirr commented Aug 14, 2024

I guess not breaking the build on 2.8 was an unintended feature of the pull request :)

Maybe we should let that "feature" stay for a little while?

@alfbr
Copy link
Member

alfbr commented Aug 14, 2024

Just checked, build is broken over here. Last successful build was two days ago. Do you want me to do a bisect?

@atgeirr
Copy link
Member

atgeirr commented Aug 15, 2024

Just checked, build is broken over here. Last successful build was two days ago. Do you want me to do a bisect?

That may not be necessary if you have the error? I think other recent changes not related to dune may have broken compilation with gcc 9 and 10, see OPM/opm-models#922

Also: the corresponding bump PR in opm-models is not merged, is that on purpose @blattms, @akva2 ?

@bska
Copy link
Member

bska commented Aug 15, 2024

think other recent changes not related to dune may have broken compilation with gcc 9 and 10, see OPM/opm-models#922

Right. If you're using a version of GCC older than GCC 11, then the build will fail due to not having std::from_chars() for floating-point types. In that case I have a work-around below. We may or may not switch to something like this although we're probably going to start requiring at least GCC 11 for building on Linux quite soon.


diff --git a/opm/models/utils/parametersystem.hh b/opm/models/utils/parametersystem.hh
index 9e49f7ee0..10b62b64a 100644
--- a/opm/models/utils/parametersystem.hh
+++ b/opm/models/utils/parametersystem.hh
@@ -41,8 +41,8 @@
 #include <dune/common/classname.hh>
 #include <dune/common/parametertree.hh>
 
-#include <cstdlib>
 #include <charconv>
+#include <cstdlib>
 #include <fstream>
 #include <iostream>
 #include <list>
@@ -916,6 +916,7 @@ auto Get(bool errorIfNotRegistered)
                                                         const char* const>, std::string,
                                          std::remove_const_t<decltype(Param::value)>>;
     ParamType defaultValue = Param::value;
+
     const std::string& defVal = MetaData::mutableRegistry()[paramName].compileTimeValue;
     if constexpr (std::is_same_v<ParamType, std::string>) {
         defaultValue = defVal;
@@ -928,19 +929,11 @@ auto Get(bool errorIfNotRegistered)
         defaultValue = std::strtold(defVal.data(), nullptr);
     }
 #endif
+    else if constexpr (std::is_floating_point_v<ParamType>) {
+        defaultValue = std::strtod(defVal.c_str(), nullptr);
+    }
     else {
-#ifdef _LIBCPP_VERSION // If this macro is defined, clang's libc++ is used
-        // For floating point types, libc++ (the llvm/clang library implementation)
-        // does not yet (as of clang 15) offer an implementation of std::from_chars().
-        if constexpr (std::is_integral_v<ParamType>) {
-            std::from_chars(defVal.data(), defVal.data() + defVal.size(), defaultValue);
-        } else {
-            // Floating point workaround.
-            defaultValue = std::strtod(defVal.c_str(), nullptr);
-        }
-#else
         std::from_chars(defVal.data(), defVal.data() + defVal.size(), defaultValue);
-#endif
     }
 
     // prefix the parameter name by the model's GroupName. E.g. If

@totto82
Copy link
Member

totto82 commented Aug 15, 2024

We are still on Ubuntu 20.04.6 LTS with (gcc 9.4) on our local clusters. We are in the process of upgrading but these things often take time.... Will your workaround also work for clang 15? I would be glad if we can support Ubuntu 20.04 to the end for this year.

@bska
Copy link
Member

bska commented Aug 15, 2024

Will your workaround also work for clang 15?

Yes. This workaround effectively broadens the workaround in OPM/opm-models#922 to apply to all compilers, not just Clang.

@totto82
Copy link
Member

totto82 commented Aug 15, 2024

I am looking forward for the PR.

@GitPaean
Copy link
Member

Yes. This workaround effectively broadens the workaround in OPM/opm-models#922 to apply to all compilers, not just Clang.

Would you mind to create a PR for it?

@alfbr
Copy link
Member

alfbr commented Aug 15, 2024

think other recent changes not related to dune may have broken compilation with gcc 9 and 10, see OPM/opm-models#922

Right. If you're using a version of GCC older than GCC 11, then the build will fail due to not having std::from_chars() for floating-point types. In that case I have a work-around below. We may or may not switch to something like this although we're probably going to start requiring at least GCC 11 for building on Linux quite soon.

Indeed, we are on GCC 9 on Red Hat 7. It is probably easier for me to get a newer GCC than to get the OPM rpms in on Red Hat 7, however, since Norce also are on GCC 9 it would be great to get the work-around in, and postpone the bumping of GCC. In our case we probably only need weeks to get past Red Hat 7, but these things are hard to predict.

@bska
Copy link
Member

bska commented Aug 15, 2024

I've submitted my workaround as PR OPM/opm-models#926.

@blattms
Copy link
Member

blattms commented Aug 16, 2024

Also: the corresponding bump PR in opm-models is not merged, is that on purpose @blattms, @akva2 ?

Kind of. I was checking whether I still can compile with 2.8 and wondering whether that is on purpose. Then the concerns came up and I halted the process. I will check/investigate Ubuntu 20.04 for @totto82 before continuing to merge.

@blattms
Copy link
Member

blattms commented Sep 10, 2024

We are still on Ubuntu 20.04.6 LTS with (gcc 9.4) on our local clusters. We are in the process of upgrading but these things often take time.... Will your workaround also work for clang 15? I would be glad if we can support Ubuntu 20.04 to the end for this year.

That is strange. Don't you run into OPM/opm-simulators#5595 ? Maybe I am wrong there. Could you double check? Maybe everybody is using self-compiled DUNE there with a higher version?

@blattms
Copy link
Member

blattms commented Sep 10, 2024

I think we need a decision about what 2024.10 should support. 2.7 seems to need extra work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants