Nek5000 / nekRS

our next generation fast and scalable CFD code
https://nek5000.mcs.anl.gov/
Other
284 stars 75 forks source link

Allow external HYPRE #419

Closed aprilnovak closed 2 years ago

aprilnovak commented 2 years ago

For Cardinal, we have to maintain a separate fork of NekRS that only has changes to the CMake files to allow us to use the same HYPRE that MOOSE uses. This has become very tedious in incorporating the latest NekRS changes into Cardinal, since we need to merge upstream updates into our fork before we can try out new features.

Desired Solution Add a HYPRE_DIR variable to NekRS's CMake files that can be used during Cardinal's build process to point to the HYPRE used by MOOSE. It should be understood that this feature is not intended to support arbitrary HYPRE versions, but we are already relying on this feature for Cardinal (we just want to stop maintaining our own fork). Therefore, this would be an advanced feature that any standalone NekRS users would never need to deal with.

The proposed changes basically add this to hypre.cmake:

if (NOT "${HYPRE_DIR}" STREQUAL "")
  # ... lines to point to an external HYPRE
else()
  # ... build NekRS as usual with its own HYPRE
stgeke commented 2 years ago

Can you please add a PR for this?

aprilnovak commented 2 years ago

Yes, I was planning on it this week

stgeke commented 2 years ago

Let me understand this better. Do you want to use the same HYPRE installation or just the same source?

aprilnovak commented 2 years ago

The same installation. Ron set this up last year because we were running into seg faults if MOOSE was built with a different HYPRE version than NekRS. To help explain, the specific changes are like this for the external HYPRE option:

if (NOT "${HYPRE_DIR}" STREQUAL "")
  find_library(HYPRE_FOUND libHYPRE.dylib PATHS ${HYPRE_DIR}/lib REQUIRED NO_DEFAULT_PATH)
  find_file(HYPRE_H_FOUND HYPRE.h PATHS ${HYPRE_DIR}/include REQUIRED NO_DEFAULT_PATH)
  find_file(HYPRE_PARCSR_LS_H_FOUND HYPRE_parcsr_ls.h PATHS ${HYPRE_DIR}/include REQUIRED NO_DEFAULT_PATH)
  add_library(HYPRE SHARED IMPORTED)
  set_target_properties(HYPRE PROPERTIES
    IMPORTED_LOCATION ${HYPRE_DIR}/lib/libHYPRE.dylib
    IMPORTED_NO_SONAME FALSE)
  target_include_directories(HYPRE INTERFACE ${HYPRE_DIR}/include)
stgeke commented 2 years ago

Sharing the same installation is really tricky. How do you ensure that both codes are configured to use HYPRE in the same way? We could use HYPRE e.g. using FP32 breaking MOOSE. There are many more compile time switches which can kick-in here.

aprilnovak commented 2 years ago

I understand the concern - that's why I want to emphasize that this is an advanced feature for Cardinal. We are already relying on this feature in Cardinal, so arguments related to "things can break" are kind of a moot point unless Cardinal as a project should not exist in the first place.

So far, we haven't run into any compatibility issues. If (or when) they arise, we may need to enforce some sort of requirements in NekRS/Cardinal.

stgeke commented 2 years ago

The best solution to this issue is that each codes uses its own (private) HYPRE installation. This requires dynamically loading HYPRE at runtime. This will require a thin wrapper around HYPRE as it's not supported by default.

aprilnovak commented 2 years ago

The best solution to this issue is that each codes uses its own (private) HYPRE installation.

I'm totally open to alternative approaches, as long as we can stop maintaining our own fork eventually. Unfortunately my CMake skills are very rudimentary. Could you perhaps tackle the NekRS side of things that I can use as an example for the MOOSE side of things?

stgeke commented 2 years ago

It's not about cmake. HYPRE has to be dlsym loaded into nekRS. This way it's totally independent and no conflicts with other software packages can happen.

aprilnovak commented 2 years ago

I think I understand - at least, I see some dlsym things related to OCCA and AMGX, which we would need to also incorporate for HYPRE?

Apologies for the naive questions, this is new territory for me. So do any changes to get HYPRE dlsym loaded into NekRS only have to occur inside NekRS? Or do changes need to be made to HYPRE directly?

stgeke commented 2 years ago

Yeah AMGX supports it by default. Ideally this should go directly into HYPRE but in the meantime we could incorporate this into our HYPRE wrapper in nekRS. It's easier because you don't have to deal with the whole API of HYPRE - just the one we're using.

aprilnovak commented 2 years ago

in the meantime we could incorporate this into our HYPRE wrapper in nekRS

That would be fantastic. Is this difficult to achieve, or more like a one-line change? I don't know if I can be much help, not being familiar with NekRS's HYPRE wrapper.

emerzari commented 2 years ago

The best solution to this issue is that each codes uses its own (private) HYPRE installation. This requires dynamically loading HYPRE at runtime. This will require a thin wrapper around HYPRE as it's not supported by default.

FYI, Ron tried this route before but it did not work. We ended up with a bunch of conflicts with symbols. It does not mean it is not doable, but just alerting it may not be trivial.

stgeke commented 2 years ago

IIRC he tried something different. If you dlsym load HYPRE there will be no symbol conflicts.

stgeke commented 2 years ago

It's a bigger change. Basically one has to wrap every single API call.

aprilnovak commented 2 years ago

If you show me an example to get started, I can tackle the rest. Knowing where to start would be much appreciated!

stgeke commented 2 years ago

Fixed in https://github.com/Nek5000/nekRS/commit/9ae887200454a3409e8a0c324bde9989815716c3