Skip to content

refactor: Complete spack migration & move versions to geos package.py. #305

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

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

CusiniM
Copy link
Contributor

@CusiniM CusiniM commented Apr 9, 2025

  1. Force the definition of the versions of all tpls in the geosx package.py. This should ensure that we don't need to specify the commit in all environments and it should make updating a version easier. I would argue that the only library for which we may want to track a commit instead of a release is hypre. It seems that we can't specify a commit in the package.py, however, we could modify hypre's package.py by doing something like this

  2. Update uncrustify.

  3. Remove mirrors and old cmake-base tpls installation workflow

Once this PR is merged, we should be able to move everything to the geos repo and completely get rid of this repository. I think the tpls built can be set up as independent CI jobs conditionals to changes to the spack env files. And we can probably do something clever to avoid rebuilding all tpls when only one of them is updated.

@CusiniM CusiniM requested review from bmhan12 and victorapm April 9, 2025 21:46
@CusiniM CusiniM changed the title refactor: move versions to geos package.py refactor: move versions to geos package.py. Apr 9, 2025
@CusiniM CusiniM requested a review from rrsettgast April 9, 2025 22:02
@CusiniM CusiniM self-assigned this Apr 9, 2025
@CusiniM CusiniM changed the title refactor: move versions to geos package.py. refactor: Complete spack migration & move versions to geos package.py. Apr 10, 2025
@victorapm
Copy link
Contributor

victorapm commented Apr 10, 2025

Another idea: let's just point to hypre@master. Only drawback is that is a moving target...

PS: if we are removing all the tarballs, do we want to keep CMakeLists.txt?

@CusiniM
Copy link
Contributor Author

CusiniM commented Apr 14, 2025

Another idea: let's just point to hypre@master. Only drawback is that is a moving target...

The thing is that, for Hypre, sometimes we may want commits that are not on master. I also don't know whether that can be specified in the package.py or not. We could try. It's true that ideally we want to control when updates occur.

PS: if we are removing all the tarballs, do we want to keep CMakeLists.txt?
no, I got rid of it.

@CusiniM
Copy link
Contributor Author

CusiniM commented Apr 14, 2025

There is one job that fails while building caliper. First of all, I do not understand why but I have also noticed that it's trying to build caliper 2.11 even though the version specified in the geos package is 2.12. I do not understand where it's picking up that version.

@bmhan12
Copy link
Contributor

bmhan12 commented Apr 14, 2025

There is one job that fails while building caliper. First of all, I do not understand why but I have also noticed that it's trying to build caliper 2.11 even though the version specified in the geos package is 2.12. I do not understand where it's picking up that version.

Looking at the job log, for some reason, spack concretizes the spec as:
geosx@develop%[email protected] cflags=-pthread cxxflags=-pthread +addr2line~caliper+cuda~docs+hypre~ipo+mathpresso~openmp~petsc~pygeosx+scotch+shared+trilinos~uncrustify+vtk build_system=cmake build_type=Release
(note the ~caliper).

For whatever reason, spack isn't picking the expected +caliper default.
From the log, that means it is not respecting geos's specified caliper dependency (version, variants we want), and is instead building caliper as part of hypre's dependency.
I would try explicitly adding +caliper to the spec that is passed to uberenv, see if that gets spack to build geos with the right version of caliper:

--spec "%[email protected]+cuda~uncrustify~openmp~pygeosx cuda_arch=70 ^[email protected]+allow-unsupported-compilers ^caliper~gotcha~sampler~libunwind~libdw~papi" \


Alternatively, the error from the log when building caliper 2.11 is the following:
../../libcaliper.so.2.11.0: undefined reference to std::filesystem::__cxx11::path::parent_path() const

This post suggests adding -lstdc++fs to fix it:
https://stackoverflow.com/questions/53852684/stdfilesystem-link-error-on-ubuntu-18-10

Looks like that can be done in two ways through spack:

  1. Adding ldflags to the compiler section of the spack.yaml (example here: https://github.com/LLNL/UMT/blob/69648a82c524963ee2a8d1c4f0c7bdfde9548260/spack/environment/cray/spack.yaml#L25)
  2. Require caliper in the packages section to be build with the appropriate ldflags (example here: https://github.com/LLNL/axom/blob/4d8e328928a4182eb0032a6df28d81bc942dec73/scripts/spack/configs/toss_4_x86_64_ib_cray/spack.yaml#L575-L577)

@victorapm
Copy link
Contributor

It's probably time to re-evaluate if we want the unified memory option in hypre:

depends_on("hypre +cuda+superlu-dist+mixedint+mpi+umpire+unified-memory~shared cflags='-fPIC' cxxflags='-fPIC'", when='+cuda')

The reason to keep it would be to allow over-subscription of GPU RAM, but that generally leads to poor performance

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants