Conversation
|
jenkins build this hipify please |
cuda 12.0 (default in ubuntu 24.04) cannot build fmt. furthermore, cuda 12.0 only supports up to g++-12 as host compiler, meaning there is no std::format either. so workaround where possible by using stringstreams the test_primary_variables_gpu pulls in too much generic code so disable it. i'm not entirely sure if 12.1 is enough for our code to compile but at least this fixes build issues on a common configuration
bska
left a comment
There was a problem hiding this comment.
With the possible exception of a single include statement, this looks good to me from a C++ point of view. I'd like to get a second opinion from a GPU expert however.
|
jenkins build this hipify please |
|
I am a bit split on this. From a technical standpoint, this looks fine, but very soon limiting ourselves to CUDA 12.0 will simply not be feasible when we include more of the code in .cu-files for the property and assembly evaluation. We can't do this change for the whole codebase, and the duplication it would cause would not be tolerable. There are other parts of the code that I do not compile with 12.0 either, though I can't remember. In my opinion: CUDA is a rather speciality feature, and getting newer CUDA versions on Ubuntu isn't that hard: Would it be easier to simply require a higher version of CUDA, and if not found, disable CUDA support? |
|
it would certainly be doable that way. all of the code builds with cuda 12.0 with this PR, but as you point out, it will be very hard to keep supporting cuda 12 (which is why i opted for disabling of the primary variables test). my thinking was that since it mostly works now, this would be a good approach for 2026.04, but i'm not insisting either way. i am however insisting that we do not break at compile time for users though, so it's either this or perm disabling cuda 12. |
|
Ok, I suppose we can keep this for the release to enable linear algebra on GPUs for a wide an audience as possible, then revert back the stringstreams afterwards. |
|
alright. for the record, i am aware it's easy to update cuda, and i think anyone serious about gpu would do so. i'm worried about joe random that happens to have cuda stuff on their box because they followed some other build instructions that happened to put the distro cuda on their box and then they want to build opm.. |
So is that an agreement, then? We use the current PR for the upcoming 2026.04 OPM release and revert back to |
agreed |
bska
left a comment
There was a problem hiding this comment.
So is that an agreement, then? We use the current PR for the upcoming 2026.04 OPM release and revert back to
fmt::format()–effectively raising the minimum CUDA version requirement–for future development once the release is out (or when we make changes to this code after the release branch exists)?agreed
Very good. In that case, I'll merge this into the master branch to prepare for the release.
cuda 12.0 (default in ubuntu 24.04) cannot build fmt. furthermore, cuda 12.0 only supports up to g++-12 as host compiler, meaning there is no std::format either. so workaround where possible by using stringstreams
the test_primary_variables_gpu pulls in too much generic code so disable it.
i'm not entirely sure if 12.1 is enough for our code to compile but at least this fixes build issues on a common configuration