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

CMake updates; flush with upstream #246

Merged
merged 75 commits into from
Aug 3, 2023
Merged

Conversation

mauneyc-LANL
Copy link
Collaborator

@mauneyc-LANL mauneyc-LANL commented Mar 29, 2023

PR Summary

Update 20230727:

CI is passing on all systems. I did need to update the CI yamls to point to main branches of upstream spiner, so there needs to be some synchronization on versions, versions listed in spackages, spackage files in the repo folder, and versions in the CI to get everything to line up.

@jonahm-LANL @jhp-lanl @dholladay00 I've re-written the building docs with some expanded information. No doubt more can be put there, but what's there should cover everything. Reused some of the old documentation, as well, though a bit of it was out-of-date. If you feel there should be some more info there, let me know.

@rbberger I made some changes in the spackage file to reflect changes made elsewhere. I also put in some TODO comments on a few things that arn't required, but may be useful to look over.

Update 20230627:

Update 20230623:

Several changes have been done.

  1. Moved to bifurcated "submodule mode/standalone mode" for building. This should make it easier to maintain. In submodule mode, install/export steps are disabled.
  2. Options compacted using cmake_dependent_option, which simplifies the required cmake invokes. Further, a CMakePresets.json file has been mocked-up to provide common configuration options when building. (see about README.build.md for more details)
  3. Updated the ports-of-call and spiner submodules to latest versions. These should get proper releases before merging this PR so that the submodules can be correctly pinned. As of writting I'm trying to mangle the submodules to correctly work on the github CI, if you spot how my bumbling can be improved, let me know.
  4. Some code changes necessary for spiner-related code due to the templating of Spiner::DataBox. There may be a cleaner approach to this so I'm open to suggestions; however, the change was simple (if verbose).
  5. Clarifying spiner/hdf5 dependency in the code defines; I've replaced the checks on SPINER_USE_HDF with the more descriptive SINGULARITY_USE_SPINER_WITH_HDF5 define.
  6. I'm drafting a build doc README.build.md with instructions for building in various contexts. Feel free to provide suggestions.
  7. some package.py changes to reflect changes in build setup, and CI adjustments

spiner, HDF5 clarity

As of writting, there is still a lot of HDF5 code in eos_spiner,eos_stellarcollapse that is distinct (at least on glancing) from spiner. I haven't disentangled it enough for us to have a possible "HDF5-less" form of spiner in singularity-eos. There may be some parallel work needed in spiner itself, but right now I'm focused on getting everything for a stable release. Right now it's possible to tell singularity-eos to build with spiner but without HDF5, but the effect is just to disable eos_spiner, so it's not ideal.

Status

I'm currently assessing where things stand in terms of completeness. Most of what I wanted done is complete but there are some loose ends to tie up (finishing docs, making sure spack is in-line, cleaning up some hacks). I'm able to build and test in all environments expected to be run on, but I still need to demonstrate this on the CI.

==/Update

This is an update to streamline the CMake build and bring in changes from upstream codes.

  • removed generic dependency handling into ad-hoc scripts, now more readable and flexible for particular packages
  • better HDF5 handling
  • Code cleanup; some re-ordering and formatting is done along with other changes

PR Checklist

  • Ensure minimal disruption to downstream projects
  • Sort out any hdf5+spiner issues
  • Document + readable diagnostics
  • Prune the cmake for out-dated, redundant, or unnecessary code
  • Ensure install stage is correct with changes
  • Adds a test for any bugs fixed. Adds tests for new features.
  • Format your changes by using the make format command after configuring with cmake.
  • Document any new features, update documentation for changes made.
  • Make sure the copyright notice on any files you modified is up to date.
  • After creating a pull request, note it in the CHANGELOG.md file
  • If preparing for a new release, update the version in cmake.

@mauneyc-LANL
Copy link
Collaborator Author

@dholladay00 @Yurlungur This is ongoing, but hopefully what remains is straightforward.

@rbberger I haven't given much thought to any spack/CI changes with these. I anticipate it to be minimal, but will ping you if any major issues arise

Copy link
Collaborator

@Yurlungur Yurlungur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This all seems pretty reasonable. Some nitpicks below. Mainly want to make sure everything is behind a target guard, so that everything still works for the downstream codes doing submodules. Also when you're ready I'd like to test that build path and add some docs for it.

CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
cmake/singularity/Eigen3.cmake Outdated Show resolved Hide resolved
cmake/singularity/hdf5.cmake Outdated Show resolved Hide resolved
cmake/singularity/kokkos.cmake Outdated Show resolved Hide resolved
cmake/singularity/Eigen3.cmake Outdated Show resolved Hide resolved
cmake/singularity/eospac.cmake Outdated Show resolved Hide resolved
cmake/singularity/hdf5.cmake Outdated Show resolved Hide resolved
cmake/singularity/mpark_variant.cmake Outdated Show resolved Hide resolved
Copy link
Collaborator

@rbberger rbberger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall all looks good to me. We use a similar macro scheme for FleCSI, so I see it as a bonus that this seems to be converging into the same direction. As for the CMake version, even EAP requires 3.20, so I'd support that change.

@mauneyc-LANL
Copy link
Collaborator Author

@Yurlungur @dholladay00 some proposed updates:

I am working on a setup with the following constraints:

# There are two modes for building:
# 1.) Submodule mode
#     this mode is for projects where singularity-eos,
#     and it's dependencies, are present in the source tree. 
#     This mode is only appropriate for projects that have been
#     designed around using git submodules for dependency
#     management.
#     Submodule mode disables all export/install steps.
# 2.) Standalone mode
#     this mode will build and install singularity-eos as
#     as a complete software package for the intended platform. 
#     In standalone mode, all dependencies are expected to be 
#     found with `find_package`, and an error will be produced 
#     if the packages required for building are not located outside
#     of the source directory, except for explicit cases.

Long story short: Dependencies (save for explicit cases) are either all managed in submodules or all resolved with find_package(). This means a few changes that could be debatable:

  • There is no "mix-and-match" capabilities. This is (mostly) possible with the current CMake but: a.) it's almost never used in the "mixed" case and b.) it has made maintaining everything overly-complicated.
  • kokkos-kernels is back as a submodule. This is in-line with the "all-one-or-the-other" approach.
  • The "target-guards" on find_package() are no longer applicable - the build with either define a target, or find it will find a target.

There are other "in-flight" updates that deserve some attention, but I wanted to start discussion on this sooner rather than later.

Keep in mind I still consider this "WIP", so there is still plenty to be done/decided.

@jhp-lanl
Copy link
Collaborator

jhp-lanl commented Apr 4, 2023

* There is no "mix-and-match" capabilities. This is (mostly) possible with the current CMake but: a.) it's almost never used in the "mixed" case and b.) it has made maintaining everything overly-complicated.

How does this impact building with EOSPAC?

@mauneyc-LANL
Copy link
Collaborator Author

mauneyc-LANL commented Apr 4, 2023

@jhp-lanl Packages like eospac or HDF5 have to be used as normal system software - I'm not aware of anyone using eospac as a submodule.

By "mix-and-match" I mean the ability to do a "non-find_package()" import with a find_package() import, e.g. using kokkos submodule but ports-of-call system install. (Basically, anything in utils/<some-submodule>). My attempts to: 1.) streamline submodules configurations and 2.) remove them entirely kept getting pushback, so I figured the status quo belle ante would, at the very least, clarify any next steps.

Stripped down a little, my CMakeLists.txt looks like

if(SINGULARITY_SUBMODULE_MODE)
  # add all submodules with `add_subdirectory`, does pre-configure of cache vars
  message(STATUS "singularity-eos configuring in submodule mode.")
  singularity_import_mpark_variant()
  singularity_import_ports_of_call()
  singularity_import_spiner()
# ... others in utils/<some_sm>  
else()
  # here, just use `find_package()` with optional component requirements
  singularity_find_mpark_variant()
  singularity_find_ports_of_call()
  singularity_find_spiner()
# ... others ect.
endif()

# attach the libraries, headers, ect to the library target
singularity_enable_mpark_variant(singularity-eos)
singularity_enable_ports_of_call(singularity-eos)
singularity_enable_spiner(singularity-eos)
# others &t.

# these can only be "enabled"; note that we have find or imported spiner, so `SPINER_USE_HDF5` should have been set
if(SPINER_USE_HDF)
  singularity_enable_hdf5(singularity-eos)
endif()

if(SINGULARITY_USE_EOSPAC)
  singularity_enable_eospac(singularity-eos)
endif()

@Yurlungur
Copy link
Collaborator

Big picture I am supportive of a simpler single switch with no mix-and-match. A few questions/comments:

kokkos-kernels is back as a submodule. This is in-line with the "all-one-or-the-other" approach.

Do you plan to add submodules back into the project? Or would a person using submodule mode have to add them to the parent project?

The "target-guards" on find_package() are no longer applicable - the build with either define a target, or find it will find a target.

IMO target guards should still be on the find package targets that may be needed for submodule mode, e.g., hdf5 and eospac. I don't trust the cmake find package machinery to do this correctly on its own.

@Yurlungur
Copy link
Collaborator

@dholladay00 please take a look at this when you have the chance. I'd like your eyes on it before we merge.

@Yurlungur Yurlungur mentioned this pull request Jul 31, 2023
6 tasks
@dholladay00
Copy link
Collaborator

I have kicked off most recent branch on gitlab CI. Pending results, I think this is ready.

@Yurlungur
Copy link
Collaborator

I'm happy to have this merged in when tests pass. Thanks for all the effort @mauneyc-LANL !

@dholladay00 dholladay00 merged commit 0c212f2 into main Aug 3, 2023
4 checks passed
@dholladay00 dholladay00 deleted the mauneyc/flush-with-upstream branch August 3, 2023 03:34
Copy link
Collaborator

@jhp-lanl jhp-lanl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't get a chance to post this last night but I had a few questions

====================================== =============================== ===========================================
Package Name Distribution Comment
====================================== =============================== ===========================================
`spiner`_ submodule [*]_ / external [*]_ Required
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought the new option makes spiner an optional dependency. Is this not true?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's true, @mauneyc-LANL mark for update

``SINGULARITY_BUILD_CLOSURE`` ``SINGULARITY_USE_KOKKOS=ON`` ``SINGULARITY_USE_KOKKOSKERNELS=ON`` Mixed cell closure.
``SINGULARITY_BUILD_SESAME2SPINER`` ``SINGULARITY_USE_SPINER=ON`` ``SINGULARITY_USE_SPINER_WITH_HDF5=ON`` Builds the conversion tool sesame2spiner which makes files readable by SpinerEOS.
``SINGULARITY_BUILD_STELLARCOLLAPSE2SPINER`` ``SINGULARITY_USE_SPINER=ON`` ``SINGULARITY_USE_SPINER_WITH_HDF5=ON`` Builds the conversion tool stellarcollapse2spiner which optionally makes stellar collapse files faster to read.
``SINGULARITY_TEST_SESAME`` ``SINGULARITY_BUILD_TESTS=ON`` ``SINGULARITY_BUILD_SESAME2SPINER=ON`` Test the Sesame table readers.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fact that we can't test EOSPAC without enabling spiner is a little annoying but I can live with it since we probably always want to test both.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This led me the realize an error in the testing with the update

Comment on lines +161 to +162
To further aid the developer, ``singularity-eos`` is distributed with
**Presets**, a list of common build options with naturally named labels
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great!

Comment on lines +85 to +86
depends_on("spiner@:1.6.0", when="@:1.7.0")
depends_on("spiner@1.6.1:", when="@1.7.1:") #TODO version
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need to bother changing this for this MR, but we should make a +spiner variant in the future. I'll put that on my to-do list

depends_on("spiner" + _flag, when="+kokkos" + _flag)

# specfic specs when using GPU/cuda offloading
# TODO Do we need `+aggressive_vectorization`, `+cuda_constexpr`, `~compiler_warnings` ?
depends_on("kokkos +wrapper+cuda_lambda+cuda_relocatable_device_code", when="+cuda+kokkos")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how did I let this slip by. I thought we remove rdc from all of our spackages @mauneyc-LANL.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mauneyc-LANL mauneyc-LANL mentioned this pull request Aug 4, 2023
6 tasks
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.

7 participants