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

Failing Albany configures due to incorrect Trilinos version #982

Closed
ikalash opened this issue Sep 20, 2023 · 34 comments · Fixed by #984
Closed

Failing Albany configures due to incorrect Trilinos version #982

ikalash opened this issue Sep 20, 2023 · 34 comments · Fixed by #984
Assignees
Labels
build Testing Stuff related to testing Albany (including nightly tests)

Comments

@ikalash
Copy link
Collaborator

ikalash commented Sep 20, 2023

All the Albany and Albany-LCM nightlies are failing as a result of Albany finding the "wrong" Trilinos version. I checked the version of Trilinos that was built and it is 14.5. I then tried to chance Albany/CMakeLists.txt to look for 14.5 Trilinos instead of 14.1, but it did not fix the problem. My guess is this is due to a Trilinos change. Does anyone have any insight into this? Perhaps @jewatkins @bartgol @mcarlson801 @mperego @kliegeois ?

Tagging @lxmota since this impact LCM too.

@ikalash ikalash added Testing Stuff related to testing Albany (including nightly tests) build labels Sep 20, 2023
@ikalash
Copy link
Collaborator Author

ikalash commented Sep 20, 2023

Actually it looks like the issue is not the version but finding Trilinos. In LCM, we don't ask for a specific Trilinos version (https://github.com/sandialabs/LCM/blob/main/CMakeLists.txt#L40) but still get the same error of Trilinos not being found (https://sems-cdash-son.sandia.gov/cdash/build/52529/configure) even though there is a successful install of Trilinos that is being pointed to.

@bartgol
Copy link
Collaborator

bartgol commented Sep 20, 2023

I think cmake is looking in the wrong path. If you check the error log of this build, you can see that we specify this trilinos path

-DALBANY_TRILINOS_DIR:PATH=/projects/albany/nightlyAlbanyCDash/test/TrilinosIntelInstall

but CMake looks for trilinos here

  The following configuration files were considered but not accepted:

    /home/ikalash/Trilinos-clean/build-gcc/install/lib/cmake/Trilinos/TrilinosConfig.cmake, version: 13.5

I'm not sure how that path got searched. Perhaps it is set inside some script that runs in the nightlies? Either way, CMAKE_PREFIX_PATH should be the first one to be scanned. It may be worth to print CMAKE_PREFIX_PATH right after we prepend the trilinos dir, and verify it does contain the correct paths...

@ikalash
Copy link
Collaborator Author

ikalash commented Sep 20, 2023

@bartgol : thanks for looking at it. I think the problem is it's not finding the right Trilinos even though the path is set. The other Trilinos that is being pointed to exists in my home dir, but it does not appear in any of the test scripts. Please see also my previous comment about what is happening in LCM (no version is requested, just Trilinos is not found). I think the broader question is why is -DALBANY_TRILINOS_DIR being ignored now.

@bartgol
Copy link
Collaborator

bartgol commented Sep 20, 2023

Since this happened only once, I would wait a bit. Since it affected several builds, across several platforms, it is likely to not be a fluke. Alas, cmake correctly finds trilinos on my laptop (cmake 3.26) and my workstation (cmake 3.24), so I don't think we have anything wrong in our cmake logic.

@ikalash
Copy link
Collaborator Author

ikalash commented Sep 20, 2023

Are you sure you are using an up-do-date develop branch of Trilinos on your laptop? I just pulled everything from scratch and rebuilt, and am still seeing the problem. I printed CMAKE_PREFIX_PATH and it is correct, so I don't get why Trilinos is not being found. I am also using cmake 3.26.

@bartlettroscoe , did anything change recently in the Trilinos build system that would cause the issues described here? Basically Albany is no longer finding Trilinos starting today. Here is how the CMakeLists.txt file looks like: https://github.com/sandialabs/Albany/blob/master/CMakeLists.txt

@jewatkins
Copy link
Collaborator

This was merged yesterday: trilinos/Trilinos#12258 Specifically this one: trilinos/Trilinos#12104 (comment) seems to indicate that trilinos installs libs in lib64 now. But /projects/albany/nightlyAlbanyCDash/test/TrilinosIntelInstall/lib still seems to exist. Looks like something related to exodus testing? cmake might be getting confused as to the location of lib/cmake/Trilinos/TrilinosConfig.cmake which is now in lib64/cmake/Trilinos/TrilinosConfig.cmake.

TLDR
Turning this off Trilinos_USE_GNUINSTALLDIRS=OFF in the trilinos config might fix it.

@jewatkins
Copy link
Collaborator

Could be this too: https://github.com/sandialabs/Albany/blob/master/CMakeLists.txt#L67 maybe change that to lib64

@bartgol
Copy link
Collaborator

bartgol commented Sep 20, 2023

I thought CMake would look in several subfolders of the search paths, when it looked for the package config file. This section of their docs seems to suggest that both lib and lib64 should be scanned when looking for the package config file. Namely, this line

<prefix>/(lib/<arch>\|lib*\|share)/cmake/<name>*/

should match ${ALBANY_TRILINOS_DIR}/lib/cmake/Trilinos.

As for that line in our cmake list, it should be executed after we find trilinos, so it should not be the culprit here. I also think they should not be used, since we link CMake targets, and should not need to keep track of lib/include dirs manually...

@ikalash
Copy link
Collaborator Author

ikalash commented Sep 20, 2023

Could be this too: https://github.com/sandialabs/Albany/blob/master/CMakeLists.txt#L67 maybe change that to lib64

It's not this - the issue is Trilinos_DIR is not being set.

@ikalash
Copy link
Collaborator Author

ikalash commented Sep 20, 2023

Trilinos_USE_GNUINSTALLDIRS=OFF

It actually looks like the above Trilinos configure option does resolve this! I can try to change some of the nightlies to use this to see what happens with the nightlies.

@bartlettroscoe
Copy link
Contributor

CC: @sebrowne, @rppawlo, @ccober6

Trilinos_USE_GNUINSTALLDIRS=OFF

It actually looks like the above Trilinos configure option does resolve this! I can try to change some of the nightlies to use this to see what happens with the nightlies.

@ikalash et.al.,

FYI, it is likely that using GNUInstallDirs will not be optional in the future as per:

This is just the tip of the iceberg when moving TriBITS and Trilinos to modern CMake (i.e. buckle up). The days of all backward compatible changes in TriBITS/Trilinos CMake refactorings are over. (Talk with Trilinos management about this.)

If you want recommendations on how to set up Albany's CMake usage of Trilinos to be more modern CMake standard's compliant, let me know.

@lxmota
Copy link
Contributor

lxmota commented Sep 20, 2023

CC: @sebrowne, @rppawlo, @ccober6

Trilinos_USE_GNUINSTALLDIRS=OFF

It actually looks like the above Trilinos configure option does resolve this! I can try to change some of the nightlies to use this to see what happens with the nightlies.

@ikalash et.al.,

FYI, it is likely that using GNUInstallDirs will not be optional in the future as per:

* [EPIC: Refactoring TriBITS Core and TriBITS-based CMake projects to conform to modern CMake TriBITSPub/TriBITS#411](https://github.com/TriBITSPub/TriBITS/issues/411)

This is just the tip of the iceberg when moving TriBITS and Trilinos to modern CMake (i.e. buckle up). The days of all backward compatible changes in TriBITS/Trilinos CMake refactorings are over. (Talk with Trilinos management about this.)

If you want recommendations on how to set up Albany's CMake usage of Trilinos to be more modern CMake standard's compliant, let me know.

Yes, recommendations for CMake for Albany would be great.

@mperego
Copy link
Collaborator

mperego commented Sep 20, 2023

Thanks, @bartlettroscoe. It would be good if you ping us when you are about to merge a PR that breaks backward compatibility, so that we have a bit more time to fix Albany.

@bartlettroscoe
Copy link
Contributor

Thanks, @bartlettroscoe. It would be good if you ping us when you are about to merge a PR that breaks backward compatibility, so that we have a bit more time to fix Albany.

@mperego, my recommendation is to update Albany to use Trilinos_USE_GNUINSTALLDIRS=ON as soon as you can so that when TriBITS and Trilinos are hard-coded to use GNUInstallDirs.cmake, Albany will not even notice. (Same advice for every other customer of Trilinos.)

@bartgol
Copy link
Collaborator

bartgol commented Sep 20, 2023

@bartlettroscoe it appears that having that option ON makes Albany not find Trilinos anymore. You said we can talk regarding including Trilinos in a more cmake-y way (I thought find_package was already cmake-y, but maybe not enough :) ). Do you have time in the next few days? It would be great to fix this issue sooner than later.

@mperego
Copy link
Collaborator

mperego commented Sep 20, 2023

Thanks, @bartlettroscoe. It would be good if you ping us when you are about to merge a PR that breaks backward compatibility, so that we have a bit more time to fix Albany.

@mperego, my recommendation is to update Albany to use Trilinos_USE_GNUINSTALLDIRS=ON as soon as you can so that when TriBITS and Trilinos are hard-coded to use GNUInstallDirs.cmake, Albany will not even notice. (Same advice for every other customer of Trilinos.)

We'll do. I was referring to other future changes to TriBITS/Trilinos CMake.

@bartlettroscoe
Copy link
Contributor

bartlettroscoe commented Sep 20, 2023

CC: @jwillenbring, @sebrowne

Yes, recommendations for CMake for Albany would be great.

First recommendation, stop assuming you can get compilers from the Trilinos install with find_package(Trilinos). That does not work for CUDA as discovered and discussed in:

and is likely to be removed from TriBITS and Trilinos in the future. And moving to GNUInstallDirs.cmake for Trilinos breaks find_package(Trilinos) as per:

So get rid of all of the code like:

Albany/CMakeLists.txt

Lines 98 to 135 in 683b9e9

if (SET_COMPILERS_AUTOMATICALLY)
MESSAGE("Setting and checking of compilers:")
if (Albany_CROSS_COMPILE)
# Force the compilers to be the same as Trilinos (GAH experimental)
# This syntax is needed when cross compiling or the compilers get checked
# again by the Albany configure, and will probably fail, because the options
# won't be right and the exes won't run on the host.
INCLUDE (CMakeForceCompiler)
SET(CMAKE_SYSTEM_NAME Generic)
CMAKE_FORCE_CXX_COMPILER(${Trilinos_CXX_COMPILER} Generic)
CMAKE_FORCE_C_COMPILER(${Trilinos_C_COMPILER} Generic)
CMAKE_FORCE_Fortran_COMPILER(${Trilinos_Fortran_COMPILER} Generic)
# SET(CMAKE_SYSTEM_NAME ${Trilinos_SYSTEM_NAME})
# CMAKE_FORCE_CXX_COMPILER(${Trilinos_CXX_COMPILER} ${Trilinos_CXX_COMPILER_ID})
# CMAKE_FORCE_C_COMPILER(${Trilinos_C_COMPILER} ${Trilinos_C_COMPILER_ID})
# CMAKE_FORCE_Fortran_COMPILER(${Trilinos_Fortran_COMPILER} ${Trilinos_Fortran_COMPILER_ID})
# SET(CMAKE_Fortran_IMPLICIT_LINK_LIBRARIES ${Trilinos_Fortran_IMPLICIT_LINK_LIBRARIES})
else ()
set (CMAKE_CXX_COMPILER ${Trilinos_CXX_COMPILER})
set (CMAKE_C_COMPILER ${Trilinos_C_COMPILER})
set (CMAKE_Fortran_COMPILER ${Trilinos_Fortran_COMPILER})
endif ()
else()
# Make sure the compilers match.
MESSAGE("Checking compilers:")
IF(NOT ${Trilinos_CXX_COMPILER} STREQUAL ${CMAKE_CXX_COMPILER})
MESSAGE(FATAL_ERROR "C++ compilers don't match (Trilinos: ${Trilinos_CXX_COMPILER}, ${PROJECT_NAME}: ${CMAKE_CXX_COMPILER}).")
ENDIF()
IF(NOT ${Trilinos_C_COMPILER} STREQUAL ${CMAKE_C_COMPILER})
MESSAGE(FATAL_ERROR "C compilers don't match (Trilinos: ${Trilinos_C_COMPILER}, ${PROJECT_NAME}: ${CMAKE_C_COMPILER}).")
ENDIF()
IF(NOT ${Trilinos_Fortran_COMPILER} STREQUAL ${CMAKE_Fortran_COMPILER})
MESSAGE(FATAL_ERROR "Fortran compilers don't match (Trilinos: ${Trilinos_Fortran_COMPILER}, ${PROJECT_NAME}: ${CMAKE_Fortran_COMPILER}).")
ENDIF()
SET(Trilinos_CXX_COMPILER_ID ${CMAKE_CXX_COMPILER_ID})
SET(Trilinos_C_COMPILER_ID ${CMAKE_C_COMPILER_ID})
SET(Trilinos_Fortran_COMPILER_ID ${CMAKE_Fortran_COMPILER_ID})
endif()

Albany needs to have its compilers explicitly passed in and defined in CMake before calling find_package(Trilinos).

I know it was really convenient to be able to get compilers from find_package(Trilinos) but that can not be supported when moving to modern CMake and issues with CUDA and Kokkos.

NOTE: We may be able to continue support for providing compilers used with installed versions of Trilinos but it can't be through find_package(Trilinos). If that is important to Albany, please request that in:

@bartgol
Copy link
Collaborator

bartgol commented Sep 20, 2023

I was never a fan of relying on a TPL to get the compilers, so this is a welcome suggestion. :)

@bartlettroscoe
Copy link
Contributor

We'll do. I was referring to other future changes to TriBITS/Trilinos CMak

@mperego, this is a good question. I will defer to @sebrowne, @ccober6, and @jwillenbring on how and when to pre-notify Trilinos customers of potentially backwards compatibility breaks to Trilinos 'develop'. Personally, I am happy to send out emails to the various Trilinos lists but it is hard to know how much notification is too much (and that is what RELEASE_NOTES are for, right?).

@bartlettroscoe
Copy link
Contributor

Could be this too: https://github.com/sandialabs/Albany/blob/master/CMakeLists.txt#L67 maybe change that to lib64

Using the permanent link:

SET(Trilinos_LIB_DIRS "${Trilinos_DIR}/../../../lib")

this needs to be removed because you can't assume the name of the library directory going forward with modern CMake installs of packages (see https://gitlab.kitware.com/cmake/cmake/-/issues/25157#note_1418009).

@bartgol
Copy link
Collaborator

bartgol commented Sep 20, 2023

Can we still do

set (Trilinios_LIB_DIRS ${Trilnios_DIR}/${CMAKE_INSTALL_LIBDIR}

? That is, assuming Trilinos_GNUINSTALLDIRS=ON, of course.

@kliegeois The only place where we use this is in pyAlbany. Does pyAlbany really need it?

@mperego
Copy link
Collaborator

mperego commented Sep 20, 2023

We'll do. I was referring to other future changes to TriBITS/Trilinos CMak

@mperego, this is a good question. I will defer to @sebrowne, @ccober6, and @jwillenbring on how and when to pre-notify Trilinos customers of potentially backwards compatibility breaks to Trilinos 'develop'. Personally, I am happy to send out emails to the various Trilinos lists but it is hard to know how much notification is too much (and that is what RELEASE_NOTES are for, right?).

Fair. I'll bring this up at next Trilinos leadership meeting.

@ikalash
Copy link
Collaborator Author

ikalash commented Sep 21, 2023

Thanks for all the tips, @bartlettroscoe . Could someone please volunteer to work with Ross to try to clean up Albany's CMakeLists.txt to work with the recent Trilinos changes? @bartgol perhaps you would be willing to do this? In the meantime, we can use Trilinos_USE_GNUINSTALLDIRS=OFF to not have failures in all the builds until the rework happens.

@bartlettroscoe
Copy link
Contributor

bartlettroscoe commented Sep 21, 2023

Can we still do

set (Trilinios_LIB_DIRS ${Trilnios_DIR}/${CMAKE_INSTALL_LIBDIR}

@bartgol, that does not work in general because the value of CMAKE_INSTALL_LIBDIR ussd in the configuration and installation of Trilinos can be different from the default. For example, people can configure Trilinos wtih:

-D CMAKE_INSTALL_LIBDIR:STRING=lib

which will be used instead of lib64 on Linux systems.

The only way that Albany would get the value of CMAKE_INSTALL_LIBDIR is from include(GNUInstallDirs) which has nothing to do with the value of CMAKE_INSTALL_LIBDIR when building Trilinos.

@bartgol
Copy link
Collaborator

bartgol commented Sep 21, 2023

@ikalash I'll take care of crafting a long term solution. I enjoy cmake stuff.

@bartlettroscoe
Copy link
Contributor

@ikalash I'll take care of crafting a long term solution. I enjoy cmake stuff.

@bartgol, in case you don't have a copy of the book "Professional CMake", I highly recommend getting it. See my curated content article on it in:

That explains the best idioms for using CMake.

@ikalash
Copy link
Collaborator Author

ikalash commented Sep 21, 2023

@ikalash I'll take care of crafting a long term solution. I enjoy cmake stuff.

@bartgol : that makes one of us (unless you are kidding :))! It's all yours!

@bartgol
Copy link
Collaborator

bartgol commented Sep 25, 2023

It appears the issue is the order in which find_package(Trilinos REQUIRED) and project(Albany CXX C) are called. The following reproducer highlights the issue, assuming the Trilinos_ROOT env var is set to the proper installation folder

Works:

cmake_policy (SET CMP0074 NEW) # To use the PackageName_ROOT env var
project (Foo CXX)
find_package(Trilinos REQUIRED)

Doesn't work:

cmake_policy (SET CMP0074 NEW) # To use the PackageName_ROOT env var
find_package(Trilinos REQUIRED)
project (Foo CXX)

@bartlettroscoe I looked at the CMake docs, as well as the professional CMake book, but could not find anythink specifying the order of ops for project and find_package.

I think we had find_package() before project() b/c we wanted to use Trilinos to set the compilers. We agreed that this is not the best approach, so now we can of course move the find_package() call down below, after proect(). Still, I'm interested in your opinion on whether this reflects a CMake rule, or if something else is causing the failure.

Notice that the case that did not work failed very early, to the point where even adding message (FATAL_ERROR "hello world!") at the top of TrilinosConfig.cmake would not get printed. It seems that project is needed to trigger find_package at all.

Also notice that I tried with several approaches for the specification of the pkg folder (env var, CMAKE_PREFIX_PATH var, PATHS arg to find_package(),...). In all cases I got the same behavior.

@bartlettroscoe
Copy link
Contributor

bartlettroscoe commented Sep 25, 2023

It appears the issue is the order in which find_package(Trilinos REQUIRED) and project(Albany CXX C) are called.

...

@bartlettroscoe I looked at the CMake docs, as well as the professional CMake book, but could not find anythink specifying the order of ops for project and find_package.

@bartgol, yup, that is exactly what I was trying to point out above which points to:

You have to define the compilers before calling find_package() or it will not look under architecture-specific paths like <prefix>/lib64/. As you noted, that is undocumented in all of the CMake documentation but will be in the next CMake 3.28 release (based on my request). See:

If you don't get compilers from Trilinos, there is no reason to not define the compilers before calling find_package(Trilinos).

Sorry you had to discover this on your own again ☹️

@bartgol
Copy link
Collaborator

bartgol commented Sep 25, 2023

@bartlettroscoe Ah, sorry, I didn't catch what you were hinting at. Ok, great, it's nice to know this is not just some weird coincidence, but the way things are supposed to be.

Follow up question: we should not assume that libraries are in the lib subfolder of the install dir, since that's arch-dependent. Is it safe, however, to assume that binaries will be in the bin subfolder? We do rely on a few of the seacas binaries in our workflow, so I need to find them.

Edit: I'd be happy to rely on stuff like find_program, but I still need to provide possible path suffixes, which is not much better than specifying <prefix>/bin directly...

@bartlettroscoe
Copy link
Contributor

Follow up question: we should not assume that libraries are in the lib subfolder of the install dir, since that's arch-dependent. Is it safe, however, to assume that binaries will be in the bin subfolder? We do rely on a few of the seacas binaries in our workflow, so I need to find them.

@bartgol, to be modern CMake compliant, I would not assume the libraries are in the lib subfolder. I am not sure about bin but, technically, any of these paths can be modified by the installer. You can see the implementation in:

The documentation for that module is:

which tries to follow the GNU standard:

(But the latter does not mention lib64 so not clear why GNUInstallDirs.cmake was written the way it is.)

Edit: I'd be happy to rely on stuff like find_program, but I still need to provide possible path suffixes, which is not much better than specifying <prefix>/bin directly...

I don't know how to address this but one option might be to extend TriBITS to write the paths used with GNUInstallDirs.cmake when Trilinos was configured into the Trilinos-installed package config files. For example, TrilinosConfig.cmake could define the following vars:

set(Trilinos_CMAKE_INSTALL_BINDIR "...")
set(Trilinos_CMAKE_INSTALL_LIBDIR "...")
...

set(Trilinos_CMAKE_INSTALL_FULL_BINDIR "...")
set(Trilinos_CMAKE_INSTALL_FULL_LIBDIR "...")
...

and then downstream customers like Albany could get these by calling find_package(Trilinos ...). Then downstream customers like Albany could get the install paths like:

set(SEACAS_bin "${Trilinos_CMAKE_INSTALL_BINDIR}")  # Defined by find_package(Trilinos ...)

Thoughts?

@bartgol
Copy link
Collaborator

bartgol commented Sep 25, 2023

Yes, I think that would be a good idea. For libs, it's probably not that important (we link them as targets, which are already found by find_package() with all their properties), but for binaries, it would help. If trilinos told downstream project where to find them, then they can safely (and portably) add code that depends on those execs (as I said, Albany does that: we add a pre-process step in some tests, to call seacas binaries to partition meshes). Of course, that implies that all trilinos pkgs install binaries via something like

install (PROGRAMS my_exec DESTINATION ${CMAKE_INSTALL_BINDIR})

but I think that's probably already done and safe to assume.

Btw, I saw some hard-coded relative paths in packages/seacas/scripts/CMakeLists.txt:

   INSTALL(PROGRAMS ${CMAKE_CURRENT_BINARY_DIR}/exodus2.py DESTINATION lib)
   INSTALL(PROGRAMS ${CMAKE_CURRENT_BINARY_DIR}/exodus3.py DESTINATION lib)
   INSTALL(PROGRAMS ${CMAKE_CURRENT_BINARY_DIR}/tests/test_exodus3.py DESTINATION lib/tests/)
   INSTALL(PROGRAMS ${CMAKE_CURRENT_BINARY_DIR}/tests/exomerge_unit_test.py DESTINATION lib/tests/)
   INSTALL(PROGRAMS ${CMAKE_CURRENT_SOURCE_DIR}/tests/test-assembly.exo DESTINATION lib/tests/)
   INSTALL(PROGRAMS ${CMAKE_CURRENT_SOURCE_DIR}/tests/exomerge_unit_test.e DESTINATION lib/tests/)
   INSTALL(PROGRAMS ${CMAKE_CURRENT_BINARY_DIR}/exomerge2.py DESTINATION lib)
   INSTALL(PROGRAMS ${CMAKE_CURRENT_BINARY_DIR}/exomerge3.py DESTINATION lib)

I guess that's fine if they are not going to change, but it's odd to see this when everything else seems to be put in a path prescribed by a var. I'm not interested to change this, just brought it up in case this was a not-intended cmake usage in trilinos.

@bartlettroscoe
Copy link
Contributor

Btw, I saw some hard-coded relative paths in packages/seacas/scripts/CMakeLists.txt:

@bartgol, you should post a GitHub issue to:

suggesting it call include(GNUInstallDirs) and use:

   INSTALL(PROGRAMS ${CMAKE_CURRENT_BINARY_DIR}/exodus2.py DESTINATION ${CMAKE_INSTALL_LIBDIR})
   ...

Using GNUInstallDirs.cmake is considered modern CMake and is recommend in "Professional CMake".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Testing Stuff related to testing Albany (including nightly tests)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants