libAtoms / QUIP

libAtoms/QUIP molecular dynamics framework: https://libatoms.github.io
349 stars 122 forks source link

Make build process suitable for modern Cray programming environment #357

Open pzeiger opened 3 years ago

pzeiger commented 3 years ago

Hello QUIP devs!

I am a PhD student at Uppsala University and my group uses LAMMPS quite extensively for our work. Recently we became aware of the GAP potentials and are interested in running some simulations with them. We also got some early access to a Cray EX supercomputer. Currently there is only access to the BLAS and LAPACK implementations from Cray/HPE themselves, called libsci. The compiler wrappers provided by cray automagically link BLAS and LAPACK from libsci. Therefore the MATH_LINKOPTS variable should be empty for compiling QUIP. I have checked, that it is possible to build QUIP on the system with an empty MATH_LINKOPTS variable.

The issue arises, when we try to build QUIP together with LAMMPS using cmake (default when we include the QUIP-ML package to LAMMPS and set it to download and build it automatically). It fails in the "make config"-step after asking to specify the MATH_LINKOPTS variable. I am not sure about what is going wrong, but I think the line containing @read MATH_LINKOPTS in Makefile.config is the cause for the crash of the compilation. When I uncomment that line, the build finishes successfully and I can run some quip examples packaged with LAMMPS. So ideally we would avoid the code inside the "ifndef MATH_LINKOPTS"-block to run when we build on a Cray with LAMMPS.

So what I would propose is to add a check for a variable, maybe called MATH_LINKOPTS_CRAY or so to the "ifndef MATH_LINKOPTS"-block in Makefile.config, which can be set to 1 inside the definition of an architecture and which prevents the question about MATH_LINKOPTS to be asked in the first place. Or maybe you know or come up with a better way of achieving a similar result. This was the simplest thing I could come up with with my limited understanding of Makefiles. However I think that such a change would be a small yet easy and very helpful change to QUIP.

I am looking forward to your reply and wish a nice rest of the week.

bernstei commented 3 years ago

I thunk all you need to do is to define MATH_LINKOPTS to something innocuous you can pass to ld, e.g. "-L/_DUMMYPATH", and it should work.

pzeiger commented 3 years ago

Thank you for your reply! That is an elegant solution. It would be nice if LAMMPS would automatically do that in case the MATH_LINKOPTS variable is empty. I will see with their devs if such a change can be included. With that, I think this thread can be closed. Thanks again!

bernstei commented 3 years ago

I'm confused - from what I can tell, LAMMPS is built assuming you have a manually compiled QUIP distribution, in which case you would have had to answer all the questions in response to the "make config" it runs. Is this not how it works?

pzeiger commented 3 years ago

You can tell cmake to build QUIP when compiling LAMMPS. So you don't need to do it yourself. In such case cmake configures the QUIP compilation and the issue there is, that if MATH_LINKOPTS is set to an empty value, the whole compilation crashes.

More info here: https://docs.lammps.org/Build_extras.html#ml-quip

bernstei commented 3 years ago

I doubt any of use cmake much, so I'm not surprised that mode of use has issues. It sounds like it's basically cmake's job to put in the correct value for MATH_LINKOPTS. It's not like that's a standard cmake variable. Someone must have coded the logic to set it from within cmake (in LAMMPS), and that logic needs to be extended to the situation where it is supposed to be empty, but our if statements get confused.

pzeiger commented 3 years ago

Yes, that is the issue. Maybe I should have directly gone to the LAMMPS devs, but I did not think about the option to just add a dummy path, so I thought that maybe some modification in QUIP is the easier way. Anyway, thanks for your help and have a nice evening!

bernstei commented 3 years ago

I suggest trying this patch diff --git a/cmake/Modules/Packages/ML-QUIP.cmake b/cmake/Modules/Packages/ML-QUIP.cmake index 5a80e63..ee3ca9d 100644 --- a/cmake/Modules/Packages/ML-QUIP.cmake +++ b/cmake/Modules/Packages/ML-QUIP.cmake @@ -32,7 +32,7 @@ if(DOWNLOAD_QUIP) foreach(flag ${LAPACK_LIBRARIES}) set(temp "${temp} ${flag}") endforeach()

If it works, you can perhaps contribute it (or a slightly more refined version, which only adds the dummy path if the BLAS and LAPACK_LIBRARIES are both empty) as a PR on the lammps github.

pzeiger commented 3 years ago

The patch works, many thanks! The credit is all yours, though, so maybe you want to contribute it yourself? :)

bernstei commented 3 years ago

On Oct 28, 2021, at 2:41 PM, Paul Zeiger @.***> wrote:

The patch works, many thanks! The credit is all yours, though, so maybe you want to contribute it yourself? :)

Honestly, I'll take the reduced work if you do it over the credit :). Feel free to mention me as the source for the idea.

pzeiger commented 3 years ago

Ok, will do that. Have a nice evening!

pzeiger commented 3 years ago

Just created the pull request :). Have a nice weekend!