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

Add support for cmake build system #14

Merged
merged 27 commits into from
Dec 11, 2019

Conversation

DusanJovic-NOAA
Copy link
Collaborator

No description provided.

Copy link
Collaborator

@climbfuji climbfuji left a comment

Choose a reason for hiding this comment

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

This is only a partial review, I am still working on it / testing. But I wanted to let you know where I am. Besides the two comments below, I am getting compilation errors for the IPD cmake version on macOS+GNU with the following flags:

./compile_cmake.sh $PWD/.. macosx.gnu 'OPENMP=N 32BIT=Y' '32bit' 2>&1 | tee compile_ipd_new.log

The errors are of this type:

/Users/heinzeller-d/TMP_NOAA/scratch/ufs-weather-model/ufs-weather-model-cmake-rt-for-develop-branch-20191208/compile/ipd_new/FV3/gfsphysics/physics/aer_cloud.F:3173:13:

 3173 |        delw0=cubicint_ice(SW, SW0, 1d0, 0d0, 1d0)
      |             1
Error: Type mismatch in argument 'y2' at (1); passed REAL(16) to REAL(8)

I hope I have time to fix this tonight and propose changes. This problem doesn't occur for CCPP, the following compile commands I do get an executable (didn't run it yet; note also that the OPENMP=N is required on the particular macbook I am currently on, it runs the latest macOS Catalina and this version messed up the OpenMP libraries):

./compile.sh $PWD/../FV3 macosx.gnu 'OPENMP=N SION=Y 32BIT=Y' '32bit' 2>&1 | tee compile_ipd_old.log
./compile.sh $PWD/../FV3 macosx.gnu 'OPENMP=N SION=Y 32BIT=Y CCPP=Y' '32bit' 2>&1 | tee compile_ccpp_dynamic_old.log
./compile.sh $PWD/../FV3 macosx.gnu 'OPENMP=N SION=Y 32BIT=Y CCPP=Y STATIC=Y SUITES=FV3_GFS_2017_gfdlmp,FV3_GSD_v0' '32bit' 2>&1 | tee compile_ccpp_static_old.log
./compile_cmake.sh $PWD/.. macosx.gnu 'OPENMP=N 32BIT=Y CCPP=Y STATIC=Y SUITES=FV3_GFS_2017_gfdlmp,FV3_GSD_v0' '32bit' 2>&1 | tee compile_ccpp_static_new.log

The cmake CCPP dynamic new fails as expected (because we don't support it):

./compile_cmake.sh $PWD/.. macosx.gnu 'OPENMP=N 32BIT=Y CCPP=Y' '32bit' 2>&1 | tee compile_ccpp_dynamic_new.log

I will also try the 64bit versions tonight, as well as DEBUG and REPRO mode, and run all possible executables for at least one setup (GFDL-MP basic setup).

@@ -41,3 +41,14 @@ module load esmf/8.0.0_bs50

# Needed at runtime:
module load alps

Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't this block be the same as in modulefiles/gaea.intel/fv3?

setenv CMAKE_CXX_COMPILER CC
setenv CMAKE_Fortran_COMPILER ftn
setenv CMAKE_Platform gaea
setenv NETCDF ${NETCDF_DIR}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need a line setenv CMAKE_Platform gaea.intel here?

@climbfuji
Copy link
Collaborator

@DusanJovic-NOAA can you please apply this patch to the fv3atm submodule?

diff --git a/gfsphysics/CMakeLists.txt b/gfsphysics/CMakeLists.txt
index d2814a0..6c2a236 100644
--- a/gfsphysics/CMakeLists.txt
+++ b/gfsphysics/CMakeLists.txt
@@ -10,7 +10,7 @@ if(CMAKE_Fortran_COMPILER_ID MATCHES "Intel")
         string (REPLACE "-i4 -real-size 32" "-i4 -real-size 64 -no-prec-div -no-prec-sqrt" CMAKE_Fortran_FLAGS "${CMAKE_Fortran_FLAGS}")
     endif()
 elseif(CMAKE_Fortran_COMPILER_ID MATCHES "GNU")
-    set(CMAKE_Fortran_FLAGS "${CMAKE_Fortran_FLAGS} -fdefault-real-8")
+    set(CMAKE_Fortran_FLAGS "${CMAKE_Fortran_FLAGS} -fdefault-real-8 -fdefault-double-8")
 endif()
 endif()

With this, the compile_cmake.sh works on macOS+GNU for IPD as well. Thanks!

Also: what do you think of changing

make -j ${MAKE_THREADS}

in compile_cmake.sh to

make VERBOSE=1 -j ${MAKE_THREADS}

automatically if DEBUG=Y is in the MAKEOPT string?

@DusanJovic-NOAA
Copy link
Collaborator Author

Added "fdefault-double-8" to gfsphysics/CMakeLists.txt in b0c4740.
We can add VERBOSE flag to make for debug builds, but I suggest we make that change in one of the future commits, simply to avoid rerunning full regression tests on all platforms.

@climbfuji
Copy link
Collaborator

Added "fdefault-double-8" to gfsphysics/CMakeLists.txt in b0c4740.
We can add VERBOSE flag to make for debug builds, but I suggest we make that change in one of the future commits, simply to avoid rerunning full regression tests on all platforms.

Thanks for making the change. Agreed, VERBOSE is not a pressing change to make.

Copy link
Collaborator

@climbfuji climbfuji left a comment

Choose a reason for hiding this comment

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

I tested this PR on macOS with both gcc+gfortran and clang+gfortran for a range of MAKEOPTS (32bit, 64bit) in DEBUG mode without OpenMP for IPD, CCPP dynamic, CCPP static, both old make and new cmake build. All versions I tested thus far compile and give b4b identical results between IPD and the two CCPP builds, also between gcc+gfortran and clang+gfortran. While I will keep testing with OpenMP and in REPRO and PROD mode, I think this PR is good to go.

Edit: the test I ran were the basic fv3_gfdlmp/fv3_ccpp_gfdlmp regression tests.

@climbfuji
Copy link
Collaborator

climbfuji commented Dec 11, 2019

Once this is merged, I will update dtc/develop and start to prepare for the commit to develop/master for Dec 20.

I did more tests with OpenMP being turned on on macOS for gcc+gfortran, and the results look good.

@junwang-noaa
Copy link
Collaborator

junwang-noaa commented Dec 11, 2019 via email

@climbfuji
Copy link
Collaborator

Dom, So you plan to combine these changes with CCPP changes for coupled system?

Yes, I owe you a detailed description of what the PR contains, and I will do this as I create the PRs tomorrow and probably Thursday.

Copy link
Collaborator

@RatkoVasic-NOAA RatkoVasic-NOAA left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@DusanJovic-NOAA DusanJovic-NOAA merged commit 3bc41ff into ufs-community:develop Dec 11, 2019
@DusanJovic-NOAA DusanJovic-NOAA deleted the cmake_dev branch December 19, 2019 13:59
XiaSun-Atmos pushed a commit to XiaSun-Atmos/ufs-weather-model that referenced this pull request Aug 8, 2020
LarissaReames-NOAA pushed a commit to LarissaReames-NOAA/ufs-weather-model that referenced this pull request Oct 22, 2021
JessicaMeixner-NOAA pushed a commit to JessicaMeixner-NOAA/ufs-weather-model that referenced this pull request Mar 14, 2023
…_init

Merge in develop of UFS and point to updated WW3
epic-cicd-jenkins pushed a commit that referenced this pull request Apr 17, 2023
Currently the workflow fails on hera at the make_ics and/or make_lbcs step because the wgrib2 module is not loaded. A PR was accepted in regional_workflow to fix this problem, but the hash was never updated here in the top-level app.
jkbk2004 pushed a commit to GeorgeGayno-NOAA/ufs-weather-model that referenced this pull request Feb 6, 2024
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