OP-DSL / OP2-Common

OP2: open-source framework for the execution of unstructured grid applications on clusters of GPUs or multi-core CPUs
https://op-dsl.github.io
Other
98 stars 46 forks source link

Improve handling of implicit dependencies #221

Closed bozbez closed 2 years ago

bozbez commented 2 years ago

Some build environments such as Spack, Nix and various environment module implementations automatically provide libraries through the LIBRARY_PATH and CPATH environment variables or by using compiler wrappers.

To allow for compilation in these environments without having to manually specify the X_INSTALL_PATH variables this PR optimistically tries to build the libraries and apps with simply the -lx compiler link flags if the X_INSTALL_PATH variables are not supplied.

This is simple for ParMETIS and PT-Scotch, but for HDF5 we were previously using grep on the H5pubconf.h file to automatically determine if the HDF5 installation was sequential or parallel. With an implicitly provided HDF5 this is no longer possible as we do not know the location of the header file, so instead a test executable is compiled and run from makefiles/test_hdf5.cpp to determine the HDF5 variant.

For some environments such as the CCE on ARCHER2 the dependencies are injected even more intrusively with the -lx flags automatically provided, and the dependency libraries are compiled with alternate names. To handle such environments a new make variable NONSTANDARD_IMPLICIT_LIBS is introduced.

reguly commented 2 years ago

In this situation how do you know if a library is present? E.g. if I unset PTSCOTCH_INSTALL_PATH on Archer2, I am currently getting a compile error.

bozbez commented 2 years ago

From the build system point of view we can either try to check early by compiling a test executable as with HDF5, and then print an error message (replicating autoconf, CMake...) or we can just go ahead with the build and it will fail with most likely a header not found error if the libraries aren't provided by the environment.

For ARCHER2 you can unset the X_INSTALL_PATH variables and then run module load for each of the dependencies which will inject the link and include paths into the CC wrapper. Then you will also need NONSTANDARD_IMPLICIT_LIBS=true to disable the optimistic -lscotch for example, since the libraries on ARCHER2 are named e.g. libscotch_crayclang.a.

gihanmudalige commented 2 years ago

@reguly did you manage to resolve this issue on ARCHER2 ?

reguly commented 2 years ago

Not really - it's something we should document. Right now it looks like OP2 won't compile unless all the libs are present.

On 2021. Dec 14., at 14:11, Gihan R. Mudalige @.***> wrote:

 @reguly did you manage to resolve this issue on ARCHER2 ?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android.

gihanmudalige commented 2 years ago

But does this mean that not even plain old sequential can be built without having CUDA ?

reguly commented 2 years ago

it does not look like it's trying to build CUDA if it's not present - with/without NONSTANDARD_IMPLICIT_LIBS

bozbez commented 2 years ago

Ah yes currently the build will fail unless you have both ParMETIS and PT-Scotch... I think this all points to us needing some actual autoconf/CMake style dependency detection with compilation tests, similar to the updated HDF5 check.

reguly commented 2 years ago

So I am not sure I understand all the dependencies here :) previously, OP2 decided if parmetis or ptscotsch was present based on the environment variables alone, and you didn't have to have both to compile. Would it still be possible to do that? Not with NONSTANDARD_IMPLICIT_LIBS of course.

On 2021. Dec 15., at 12:23, Joe Kaushal @.***> wrote:  Ah yes currently the build will fail unless you have both ParMETIS and PT-Scotch... I think this all points to us needing some actual autoconf/CMake style dependency detection with compilation tests, similar to the updated HDF5 check.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android.

bozbez commented 2 years ago

The problem is we can't use the X_INSTALL_PATH variables to sensibly decide if the library is actually present, for example when building with Spack you can just go ahead and do the #include and -lparmetis and the build will work.

With this PR we just optimistically assume that the build will work out which ends up meaning you need both ParMETIS and PT-Scotch to compile. HDF5 has a more involved check so it doesn't suffer from this issue which should probably be brought across to the other dependencies. CUDA is checked for currently by the presence of the nvcc executable and MPI the presence of the mpicxx executable, but both of these checks could do with some extending.

Alternatively we could have some IMPLICIT_LIBS environment variable to enable the optimistic compilation, but really it would be nice if you could just run make if the environment is properly set up.

reguly commented 2 years ago

I think this is fine as it is, let's not complicate things. I don't really think anyone should have an issue with compiling both of these libs, especially if they are available from spack.

On 2021. Dec 15., at 12:37, Joe Kaushal @.***> wrote:

 The problem is we can't use the X_INSTALL_PATH variables to sensibly decide if the library is actually present, for example when building with Spack you can just go ahead and do the #include and -lparmetis and the build will work.

With this PR we just optimistically assume that the build will work out which ends up meaning you need both ParMETIS and PT-Scotch to compile. HDF5 has a more involved check so it doesn't suffer from this issue which should probably be brought across to the other dependencies. CUDA is checked for currently by the presence of the nvcc executable and MPI the presence of the mpicxx executable, but both of these checks could do with some extending.

Alternatively we could have some IMPLICIT_LIBS environment variable to enable the optimistic compilation, but really it would be nice if you could just run make if the environment is properly set up.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android.

gihanmudalige commented 2 years ago

May be I am not understanding things here. What happens if I want to introduce a new partitioner karhiplater down the line ? I would not mind the SPACK build needing all of these, but what about the plain manual build with Makefiles ? Are we asking for every partitioner available /built by a user before they can get OP2 MPI working ?

bozbez commented 2 years ago

At the moment yes we are asking for all of the partitioners... I'll give a minimal detection system a go and see how complex it ends up being - we would get a lot of general build robustness improvements from this regardless.

bozbez commented 2 years ago

Added proper detection of the dependencies with small test executables, and standardised various HAVE_X flags to make the calculation of buildable libraries easier. There should also now be a summary of the found dependencies and compilers printed at the start of the build, and you can get only that summary if you run make detect.

It does add a bit of complexity, but now you can build with/without each of the partitioners, and the build works well with both implicit (tested with ARCHER2 environment modules, Spack and Nix) and explicit environments (tested with my locally built version) without need for setting NONSTANDARD_IMPLICIT_LIBS or anything else.

gihanmudalige commented 2 years ago

@reguly , is this OK to be merged ?

reguly commented 2 years ago

yes I think so!

gihanmudalige commented 2 years ago

@bozbez you can merge this. I am assuming you have nothing more to add to this particular PR ? If so, time to move on to the next !

bozbez commented 2 years ago

Alright thanks, will update the makefiles dir README and then merge.