-
Notifications
You must be signed in to change notification settings - Fork 147
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
GitHub Issue NOAA-EMC/GSI #110. Add hpc-stack to master. #121
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Several comments on first pass.
- Some of the utilities are using NETCDF_LIBRARIES_F90 instead of the norm.
- Some utilities are linking to mpi libraries. Are these actually MPI programs?
- minor comments on the order of loading modules.
util/Conventional_Monitor/nwprod/conmon_shared/sorc/conmon_grads_sfc.fd/CMakeLists.txt
Outdated
Show resolved
Hide resolved
util/Conventional_Monitor/nwprod/conmon_shared/sorc/conmon_grads_sfctime.fd/CMakeLists.txt
Outdated
Show resolved
Hide resolved
util/Conventional_Monitor/nwprod/conmon_shared/sorc/conmon_grads_sig.fd/CMakeLists.txt
Outdated
Show resolved
Hide resolved
util/Conventional_Monitor/nwprod/conmon_shared/sorc/conmon_time.fd/CMakeLists.txt
Outdated
Show resolved
Hide resolved
1056193
to
abc5229
Compare
modulefiles/modulefile.ProdGSI.hera
Outdated
set COMP ifort | ||
set COMP_MP mpfort |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are these used anywhere?
I know CMake does not use these.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lines 11 and 12 removed at 2ef1c50. Will test once Hera is returned to service.
Note: COMP
and COMP_MP
set in modulefile.ProdGSI.$machine for many machines. Shall all occurrences of COMP
and COMP_MP
be removed? What about
set COMP_MPI mpiifort
set C_COMP icc
set C_COMP_MP mpcc
These remain in modulefiles.ProdGSI.hera and other machines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
None of those are used in cmake.
If one needs to be explicit for cmake, the relevant variables are:
CC
, FC
and CXX
to be icc
, ifort
and icpc
This will work so long as the MPI_Fortran_LIBRARIES
is linked in the target_link_libraries
for parallel applications.
If one needs to avoid the target_link_libraries
line with MPI_Fortran_LIBRARIES
, then one can set the above variables to mpiicc
, mpiifort
and mpiicpc
respectively.
The trouble with doing that is that the serial applications will be compiled with mpi wrappers.
I don't suggest doing the latter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove COMP
, COMP_MP
, COMP_MPI
, C_COMP
, C_COMP_MP
from modulefiles/modulefiles.ProdGSI*
Done at f65bac9.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ush/build_all_cmake.sh
runs to completion WCOSS_C, WCOSS_D, Orion, and Jet. Can not test Hera since down for maintenance. I do not have accounts on other platforms.
2ef1c50
to
f65bac9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good.
Lots of clean-up of linkages, specifically related to NetCDF.
Thank you, Rahul, for reviewing these changes, providing valuable input, and sharing useful insights. I'm learning more about |
Test Hera build after machine returned to service.
Resolve by moving |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have completed my review. I cloned your fork on Hera and WCOSS_D and compiled. While compiling on Hera, CMake gave the following warning:
CMake Warning (dev) at CMakeLists.txt:170 (find_package):
Policy CMP0074 is not set: find_package uses _ROOT variables.
Run "cmake --help-policy CMP0074" for policy details. Use the cmake_policy
command to set the policy and suppress this warning.
Environment variable NetCDF_ROOT is set to:
/scratch2/NCEPDEV/nwprod/hpc-stack/libs/hpc-stack/intel-18.0.5.274/impi-2018.0.4/netcdf/4.7.4
For compatibility, CMake is ignoring the variable.
This warning is for project developers. Use -Wno-dev to suppress it.
As noted in my review on CMakeLists.txt, please add cmake_policy(SET CMP0074 NEW) to remove this warning message.
I then ran the regression tests and found the same results that you identified in issue #110. I then compiled in debug and ran the debug tests. All tests successfully ran to completion.
Mike
@@ -174,8 +168,6 @@ project(GSI) | |||
find_package(Baselibs REQUIRED) | |||
else() | |||
find_package( NetCDF COMPONENTS C Fortran REQUIRED) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While compiling on Hera, the following warning message appeared:
CMake Warning (dev) at CMakeLists.txt:170 (find_package):
Policy CMP0074 is not set: find_package uses _ROOT variables.
Run "cmake --help-policy CMP0074" for policy details. Use the cmake_policy
command to set the policy and suppress this warning.
Environment variable NetCDF_ROOT is set to:
/scratch2/NCEPDEV/nwprod/hpc-stack/libs/hpc-stack/intel-18.0.5.274/impi-2018.0.4/netcdf/4.7.4
For compatibility, CMake is ignoring the variable.
This warning is for project developers. Use -Wno-dev to suppress it.
Please add:
cmake_policy(SET CMP0074 NEW)
to line 111:
cmake_policy(SET CMP0009 NEW)
cmake_policy(SET CMP0054 NEW)
cmake_policy(SET CMP0074 NEW)
find_package(OpenMP)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, Mike, for your review. The requested change was made and committed at 8578601. Execution of ush/build_all_cmake.sh
on WCOSS_D and Hera show that the CMake Warning
message is no longer generated.
The due date has past for feedback from the review committee. I will now approval and merge these changes to the NOAA-EMC/GSI master. |
Changes are made to NOAA-EMC/GSI master
CMakeLists.txt
,modulefiles.ProdGSI
, andbuild_all_cmake.sh
to utilize hpc-stack (versionhpc/1.1.0
). These changes are apply to WCOSS_D, Jet, and Orion. hpc-stack was previously added to the master for Hera at b0a3bfd.