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

Adding HDF5 compression plugins to superbuild #2432

Draft
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

bvanessen
Copy link
Collaborator

Adding additional HDF5 plugins and ZFP to the superbuild. Updating
the cuda-distconv.sh superbuild example.

@bvanessen bvanessen requested a review from benson31 March 11, 2024 14:58
Comment on lines +27 to +35
# if (TARGET HDF5 AND NOT LBANN_SB_FWD_ZFP_HDF5_DIR)
# set(LBANN_SB_FWD_ZFP_HDF5_DIR ${HDF5_DIR})
# endif ()

# Conduit is "cute" about finding HDF5. It's not a CMake option() --
# you opt in by setting HDF5_DIR explicitly. So let's do that.
# if (TARGET HDF5 AND NOT LBANN_SB_FWD_ZFP_HDF5_DIR)
# set(LBANN_SB_FWD_ZFP_HDF5_DIR ${HDF5_DIR})
# endif ()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
# if (TARGET HDF5 AND NOT LBANN_SB_FWD_ZFP_HDF5_DIR)
# set(LBANN_SB_FWD_ZFP_HDF5_DIR ${HDF5_DIR})
# endif ()
# Conduit is "cute" about finding HDF5. It's not a CMake option() --
# you opt in by setting HDF5_DIR explicitly. So let's do that.
# if (TARGET HDF5 AND NOT LBANN_SB_FWD_ZFP_HDF5_DIR)
# set(LBANN_SB_FWD_ZFP_HDF5_DIR ${HDF5_DIR})
# endif ()

Comment on lines +40 to +51
# option(LBANN_SB_FWD_HDF5_HDF5_USE_16_API_DEFAULT
# "Use 1.6 API by default"
# OFF)
# option(LBANN_SB_FWD_HDF5_HDF5_USE_18_API_DEFAULT
# "Use 1.8 API by default"
# OFF)
# option(LBANN_SB_FWD_HDF5_HDF5_USE_110_API_DEFAULT
# "Use 1.10 API by default"
# OFF)
# option(LBANN_SB_FWD_HDF5_HDF5_USE_112_API_DEFAULT
# "Use 1.12 API by default"
# ON)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
# option(LBANN_SB_FWD_HDF5_HDF5_USE_16_API_DEFAULT
# "Use 1.6 API by default"
# OFF)
# option(LBANN_SB_FWD_HDF5_HDF5_USE_18_API_DEFAULT
# "Use 1.8 API by default"
# OFF)
# option(LBANN_SB_FWD_HDF5_HDF5_USE_110_API_DEFAULT
# "Use 1.10 API by default"
# OFF)
# option(LBANN_SB_FWD_HDF5_HDF5_USE_112_API_DEFAULT
# "Use 1.12 API by default"
# ON)

Comment on lines +61 to +63
# option(LBANN_SB_FWD_HDF5_DEFAULT_API_VERSION
# "Only Build Shared Libraries."
# v112)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
# option(LBANN_SB_FWD_HDF5_DEFAULT_API_VERSION
# "Only Build Shared Libraries."
# v112)

Comment on lines +27 to +31
# Conduit is "cute" about finding HDF5. It's not a CMake option() --
# you opt in by setting HDF5_DIR explicitly. So let's do that.
# if (TARGET HDF5 AND NOT LBANN_SB_FWD_ZFP_HDF5_DIR)
# set(LBANN_SB_FWD_ZFP_HDF5_DIR ${HDF5_DIR})
# endif ()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
# Conduit is "cute" about finding HDF5. It's not a CMake option() --
# you opt in by setting HDF5_DIR explicitly. So let's do that.
# if (TARGET HDF5 AND NOT LBANN_SB_FWD_ZFP_HDF5_DIR)
# set(LBANN_SB_FWD_ZFP_HDF5_DIR ${HDF5_DIR})
# endif ()

Comment on lines +118 to +122
set(LBANN_SB_SUGG_CMAKE_PREFIX_PATH_TMP "${CMAKE_PREFIX_PATH}:\$\{CMAKE_PREFIX_PATH\}")
message("BVE I think that there is a prefix path ${CMAKE_PREFIX_PATH}")
message("BVE I think that there is a foo ${FOO}")
message("BVE I think that there is a suggesed prefix path ${LBANN_SB_SUGG_CMAKE_PREFIX_PATH_TMP}")
#set(LBANN_SB_SUGG_CMAKE_PREFIX_PATH_TMP "\$\{CMAKE_PREFIX_PATH\}")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
set(LBANN_SB_SUGG_CMAKE_PREFIX_PATH_TMP "${CMAKE_PREFIX_PATH}:\$\{CMAKE_PREFIX_PATH\}")
message("BVE I think that there is a prefix path ${CMAKE_PREFIX_PATH}")
message("BVE I think that there is a foo ${FOO}")
message("BVE I think that there is a suggesed prefix path ${LBANN_SB_SUGG_CMAKE_PREFIX_PATH_TMP}")
#set(LBANN_SB_SUGG_CMAKE_PREFIX_PATH_TMP "\$\{CMAKE_PREFIX_PATH\}")


# Set to the preferred build directory
BUILD_DIR=${TMPDIR}/lbann-superbuild

# -G Ninja \
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
# -G Ninja \

cmake \
-G Ninja \
-G "Unix Makefiles" \
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
-G "Unix Makefiles" \

@@ -107,12 +115,13 @@ cmake \
-D LBANN_SB_BUILD_Aluminum=${BUILD_LBANN_STACK} \
-D LBANN_SB_Aluminum_CXX_FLAGS="${EXTRA_CXX_FLAGS}" \
-D LBANN_SB_Aluminum_CUDA_FLAGS="${EXTRA_CUDA_FLAGS}" \
-D LBANN_SB_FWD_Aluminum_ALUMINUM_ENABLE_CALIPER=ON \
-D LBANN_SB_FWD_Aluminum_ALUMINUM_ENABLE_CALIPER=OFF \
Copy link
Collaborator

Choose a reason for hiding this comment

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

May as well set LBANN_SB_BUILD_adiak=OFF and LBANN_SB_BUILD_Caliper=OFF, too.

Comment on lines +80 to +82
ZFP # LBANN
H5Z-ZFP # LBANN
HDF5_PLUGINS # LBANN
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
ZFP # LBANN
H5Z-ZFP # LBANN
HDF5_PLUGINS # LBANN
ZFP # LBANN (runtime)
H5Z-ZFP # LBANN (runtime)
HDF5_PLUGINS # LBANN (runtime)

Copy link
Collaborator

Choose a reason for hiding this comment

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

This file is a bit of a mess -- it looks like this is just a copy of the cacheinit.cmake file you load with -C later on? I'd prefer to just use that cacheinit file rather than have a full copy here. If there are still a bunch of variables you'd prefer to specify manually, I'd prefer to still use a separate file (see the opencv recipe for an example of this).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, this was not executable in the repository on purpose (executing it wouldn't preserve the meaningful export statements) -- it should only ever be sourced.

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.

2 participants