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

remove h5py from spack and from tests #192

Merged
merged 6 commits into from
Nov 18, 2022
Merged

remove h5py from spack and from tests #192

merged 6 commits into from
Nov 18, 2022

Conversation

Yurlungur
Copy link
Collaborator

@Yurlungur Yurlungur commented Nov 14, 2022

PR Summary

As requested by @mauneyc-LANL @dholladay00 and motivated by issues @annapiegra is having with oneapi, this PR does the following:

  • Stores an hdf5 "gold" file as a release
  • Downloads the file in cmake if needed
  • Removes py-h5py from spack and cmake dependencies.

PR Checklist

  • 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

@Yurlungur Yurlungur self-assigned this Nov 14, 2022
@Yurlungur Yurlungur added enhancement New feature or request build Something to do with the build system labels Nov 14, 2022
@annapiegra
Copy link
Collaborator

Singularity-eos builds with a fresh spack checkout %oneApi w/ the following changes to package.py:

--- a/var/spack/repos/builtin/packages/singularity-eos/package.py
+++ b/var/spack/repos/builtin/packages/singularity-eos/package.py
@@ -19,6 +19,7 @@ class SingularityEos(CMakePackage, CudaPackage):
     maintainers = ["rbberger"]
 
     version("main", branch="main")
+    version("fixed", branch="jmm/goldfiles")    
     version("1.6.2", sha256="9c85fca679139a40cc9c72fcaeeca78a407cc1ca184734785236042de364b942")
     version("1.6.1", sha256="c6d92dfecf9689ffe2df615791c039f7e527e9f47799a862e26fa4e3420fe5d7")
 
@@ -62,7 +63,6 @@ class SingularityEos(CMakePackage, CudaPackage):
     depends_on("[email protected]", when="+tests")
     depends_on("python@3:", when="+python")
     depends_on("[email protected]:", when="+python")
-    #    depends_on("py-h5py", when="+tests build_extra=stellarcollapse")
     depends_on("py-sphinx", when="+doc")
     depends_on("[email protected]", when="+doc")
     depends_on("py-sphinx-multiversion", when="+doc")
@@ -116,9 +116,6 @@ class SingularityEos(CMakePackage, CudaPackage):
     # common MPI dependence
     for _flag in ("~mpi", "+mpi"):
         depends_on("hdf5~cxx+hl" + _flag, when=_flag)
-        depends_on("py-h5py" + _flag, when="+tests build_extra=stellarcollapse " + _flag)
-        #        depends_on("hdf5+hl" + _flag, when=_flag)
-        depends_on("py-h5py" + _flag, when=_flag)
         depends_on("kokkos-nvcc-wrapper" + _flag, when="+cuda+kokkos" + _flag)
 

@Yurlungur
Copy link
Collaborator Author

Thanks, @annapiegra ! @dholladay00 @mauneyc-LANL does this satisfy?

@dholladay00
Copy link
Collaborator

Does not including h5py fix any of the issues we see with hdf5 and mpi @Yurlungur @mauneyc-LANL ? I'm curious if we could further simplify the spackage?

@Yurlungur
Copy link
Collaborator Author

I don't think this simplifies hdf5 proper, which needs to be compiled as either with or without MPI.

@Yurlungur
Copy link
Collaborator Author

@mauneyc-LANL I switched to FetchContent. Let me know what you think now.

Comment on lines +66 to +71
FetchContent_Declare(goldfiles
URL ${SINGULARITY_GOLDFILE_URL}
URL_HASH SHA256=${SINGULARITY_GOLDFILE_HASH}
SOURCE_DIR goldfiles
)
FetchContent_MakeAvailable(goldfiles)
Copy link
Collaborator

Choose a reason for hiding this comment

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

With cmake@>=3.24, you'll get this warning blurb.

[cmake] CMake Warning (dev) at /usr/share/cmake/Modules/FetchContent.cmake:1267 (message):
[cmake]   The DOWNLOAD_EXTRACT_TIMESTAMP option was not given and policy CMP0135 is
[cmake]   not set.  The policy's OLD behavior will be used.  When using a URL
[cmake]   download, the timestamps of extracted files should preferably be that of
[cmake]   the time of extraction, otherwise code that depends on the extracted
[cmake]   contents might not be rebuilt if the URL changes.  The OLD behavior
[cmake]   preserves the timestamps from the archive instead, but this is usually not
[cmake]   what you want.  Update your project to the NEW behavior or specify the
[cmake]   DOWNLOAD_EXTRACT_TIMESTAMP option with a value of true to avoid this
[cmake]   robustness issue.

See https://cmake.org/cmake/help/latest/policy/CMP0135.html

Not breaking, and for this case I think is over-doing it. But if you want to avoid terminal bloat, you could add

# Avoid warning about DOWNLOAD_EXTRACT_TIMESTAMP in CMake 3.24:
if (CMAKE_VERSION VERSION_GREATER_EQUAL "3.24.0")
  cmake_policy(SET CMP0135 NEW)
endif()

@mauneyc-LANL mauneyc-LANL merged commit 14b1f48 into main Nov 18, 2022
@mauneyc-LANL mauneyc-LANL deleted the jmm/goldfiles branch November 18, 2022 00:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Something to do with the build system enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants