Update CMakeLists.txt files to use imported targets #172
Conversation
ufs-weather-model does not depend directly on these libraries. g2 does.
| ) | ||
| add_dependencies(NEMS.exe ufs-weather-model-lib) | ||
|
|
||
| target_compile_definitions(NEMS.exe PRIVATE -DESMF_VERSION_MAJOR=${ESMF_VERSION_MAJOR}) |
There was a problem hiding this comment.
Is this needed? Wouldn't it inherit from ufs-weather-model-lib?
There was a problem hiding this comment.
It is needed at this moment, because ESMF_VERSION_MAJOR is added as PRIVATE definition to ufs-weather-model-lib. We can add it as PUBLIC but then all users of ufs-weather-model-lib (ie. JEDI) will get it. Do we want that? I thought it makes more sense to keep it private. In any case this will all be replaced pretty soon.
|
|
||
| add_executable( | ||
| NEMS.exe | ||
| add_library( |
There was a problem hiding this comment.
Can this be made into libnems consisting solely of the NEMS source code?
ufs-weather-model-lib is a collection of libnems, fv3atm, ww3_nems
It will simplify the logic under hear as everything is target_link_libraries or target_compile_definitions to ufs-weather-model-lib.
There was a problem hiding this comment.
Are you suggesting to rename ufs-weather-model-lib to nems? I can do that. But I'm not sure I know how to make ufs-weather-model-lib a collection of nems, fv3atm, and optionally ww3_nems.
There was a problem hiding this comment.
yes.
you can create ufs-weather-model-lib from nems, fv3atm etc. same way you created fv3atm from fv3cpl, gfsphys, etc.
There was a problem hiding this comment.
But fv3atm, in addition to depending on fv3cpl, gfsphysics also contains code (atmos_model.F90, fv3_cap.F90, ...). How do I create library without code and then add other libraries as dependencies. I guess I do not understand what you mean by 'collection'? I do not know how to "create ufs-weather-model-lib from nems, fv3atm". I can only add nems and fv3atm as dependencies to ufs-weather-model-lib.
cmake/configure_hera.gnu.cmake
Outdated
| option(32BIT "Enable 32BIT (single precision arithmetic in dycore)" OFF) | ||
| option(OPENMP "Enable OpenMP threading" ON) | ||
| option(AVX2 "Enable AVX2 instruction set" OFF) | ||
| option(32BIT "Enable 32BIT (single precision arithmetic in dycore)" OFF) |
There was a problem hiding this comment.
rather than have the same information in all the configure_${PLATFORM}.${compiler}.cmake,
can we reduce this to 2 sets?
configure.gnu.cmake and configure.intel.cmake.
If there is a platform that has something that needs something different that the "stock" options, only those options can be placed in a file with ${PLATFORM}
There was a problem hiding this comment.
We (@climbfuji and @junwang-noaa) discussed this and decided that to keep all options defined in separate files for all 'Tier-1' platforms. Your suggestion tho have configure.gnu.cmake and configure.intel.cmake is basically what is now configure_linux.gnu.cmake and configure_linux.intel.cmake (ie. generic Linux platforms)
There was a problem hiding this comment.
and why that is not sufficient? Are Tier-1 platforms not built on generic Linux platforms, give or take some options e.g. AVX2 or inline POST
There was a problem hiding this comment.
When we discussed this initially, the suggestion was to only have the tier-1 platform configuration and users would have to always specify deviations from it on the command line. This is not ok, the model should build out of the box on other systems as well. Rahul's suggestion will work, and so does Dusan's. As long as users do not have to manually specify -DINLINE_POST=OFF every time the build the model, I don't care which version we use.
There was a problem hiding this comment.
My original suggestion was to just put all options in the top-level CMakeLists.txt. Because configure_linux.gnu.cmake and configure_linux.intel.cmake are identical we do not need them both.
There was a problem hiding this comment.
@DusanJovic-NOAA
I second that suggestion.
Looking ahead with a coupled model, these options are just going to increase and become too many. And placing/repeating them in all platform/tier specific files is going to get messy, hard to maintain and prone to errors.
There was a problem hiding this comment.
How about this. In the top-level CMakeLists.txt we define these options (with the default values equal to what's currently in 'generic' linux configuration files):
set(32BIT OFF CACHE BOOL "Enable 32BIT (single precision arithmetic in dycore)")
set(AVX2 ON CACHE BOOL "Enable AVX2 instruction set")
set(CCPP ON CACHE BOOL "Enable CCPP")
set(DEBUG OFF CACHE BOOL "Enable DEBUG mode")
set(INLINE_POST OFF CACHE BOOL "Enable inline post")
set(MULTI_GASES OFF CACHE BOOL "Enable MULTI_GASES")
set(OPENMP ON CACHE BOOL "Enable OpenMP threading")
set(PARALLEL_NETCDF OFF CACHE BOOL "Enable parallel NetCDF")
set(QUAD_PRECISION ON CACHE BOOL "Enable QUAD_PRECISION (for certain grid metric terms in dycore)")
set(REPRO OFF CACHE BOOL "Enable REPRO mode")
set(WW3 OFF CACHE BOOL "Enable WW3")
Then based on CMAKE_Platform, if file exists, we include platform specific file:
if(EXISTS ${PROJECT_SOURCE_DIR}/cmake/configure_${CMAKE_Platform}.cmake)
message("")
message("Setting configuration for $ENV{CMAKE_Platform}")
message("")
include(${PROJECT_SOURCE_DIR}/cmake/configure_${CMAKE_Platform}.cmake)
endif()
On Hera, for example, this file will be:
$ cat cmake/configure_hera.intel.cmake
set(INLINE_POST ON CACHE BOOL "Enable inline post" FORCE)
set(PARALLEL_NETCDF ON CACHE BOOL "Enable parallel NetCDF" FORCE)
which will override INLINE_POST and PARALLEL_NETCDF.
There was a problem hiding this comment.
That's fine with me. How about compiler-dependent options? AVX2 only makes sense if Intel is used, for GNU that option doesn't exist (or could be replaced by another option).
There was a problem hiding this comment.
We do not use AVX2 option to define architecture specific compiler flags for GNU, but GNU compilers do have such options (-march=...). Maybe we'll start using them.
https://gcc.gnu.org/onlinedocs/gcc/x86-Options.html
If the CMAKE_Platform is defined include platform specific configuration file, which will override default options.
|
|
||
| module load gfsio/1.4.0 | ||
| module load sfcio/1.4.0 | ||
| module load sigio/2.3.0 |
There was a problem hiding this comment.
May I ask not to load gfsio, sfcio and sigio? Model does not need those extra old libraries? Also what is nceppost/dceca26, why do we need to add them to model module file?
There was a problem hiding this comment.
nceppost/dceca26 is post library.
| set( ${_prefix}_${_COMP}_INCLUDE_DIRS ${NetCDF_${_comp}_INCLUDE_DIRS} ) | ||
| set( ${_prefix}_${_arg_comp}_INCLUDE_DIRS ${NetCDF_${_comp}_INCLUDE_DIRS} ) | ||
| endforeach() | ||
| endforeach() |
There was a problem hiding this comment.
Looks like this is the same FindNetCDF.cmake that is in the CMakeModules repo. Wondering if we should remove the contents of cmake in ufs-weather-model in future commit and use https://github.com/NOAA-EMC/CMakeModules as a submodule instead?
There was a problem hiding this comment.
We can not remove cmake directory, as it contains config files and Intel.cmake and GNU.cmake with compiler flags. Those are ufs-weather-model specific.
There was a problem hiding this comment.
We can not remove cmake directory, as it contains config files and Intel.cmake and GNU.cmake with compiler flags. Those are ufs-weather-model specific.
That's right, of course. Let's think about this later. Every file we duplicate between repositories is bad.
There was a problem hiding this comment.
@climbfuji Really? ufs-s2s-model duplicates an entire repository.
There was a problem hiding this comment.
Yes, and as you might be aware of there is an effort to merge it with ufs-weather-model in the future.
| @@ -1,37 +0,0 @@ | |||
| message("") | |||
There was a problem hiding this comment.
I don't see any cleanup of cmake/configure_macosx.*.cmake, and several others are also unchanged - is this intentional?
There was a problem hiding this comment.
Yes. I do not have macosx to test, so I left it as it is. It should be cleaned up by somebody who can run a test on mac. Also other config files (cheyenne, gaea, stampede) should be updated.
There was a problem hiding this comment.
So should we merge it and then commit a cleanup PR for the other platforms within a day, while Moorthi is working on his PR? These changes will be totally independent, won't affect Moorthi's work and any of the tier-1 platforms.
There was a problem hiding this comment.
What is the long-term strategy for keeping unsupported platforms?
This should not be a 2 step process where supported platforms are updated in one PR and then the unsupported in another. This is bound to be unmaintainable.
We either remove the unsupported ones from this repo and make that a responsibility of the org that is supporting these platforms. PR's should not be held up for platforms that developers have no access to. NOAA Tier-1 platforms is a must.
There was a problem hiding this comment.
These platforms are not unsupported, they are tier-2 and tier-3. I am absolutely against removing the configurations that we use for daily work (How many real-time parallels are running on jet?! How much development for CMEPS etc is done on Cheyenne?! DTC relies on generic macOS and Linux platforms to work to do their work, and the community will too). The tier-1,2,3 concept makes sense as defined - tier-1 testing always pre-commit, tier-2/3 ideally too, but if need be retrospectively. I'd rather hold back this PR for another few hours until I tested the remaining platforms.
There was a problem hiding this comment.
Yes. These platforms are supported. They are just not in "Tier-1" category, meaning we do not require full regression test to pass before each commit. But we try our best to make sure model is running on those platforms, by providing libraries, etc. That support is not done by EMC, because we do not have access to those machines.
There was a problem hiding this comment.
Ok. I understand the concept of tier 1-2-3.
What is the agreed wait period for tier 2-3 platform to be tested. 1 week? Surely that is enough. Who is responsible for that?
What if there is an issue? Who will resolve the issues and how?
Is this information documented somewhere?
Seems like we are locked in this type of conversation often.
I am not suggesting removing generic configs. Those are necessary for development on generic platforms.
There was a problem hiding this comment.
Have a look here https://github.com/ufs-community/ufs-weather-model/wiki/Regression-Test-Policy-for-Weather-Model-Platforms-and-Compilers and here https://github.com/ufs-community/ufs/wiki/Regression-Test-Policy-for-UFS-Platforms-and-Compilers (the second is referenced in the first). Obviously still work in progress to some extend.
Description
Top level CMakeLists.txt and CMakeLists.txt file in FV3 have been changed (and cleaned up) to use imported targets (mostly for NCEPLIBS).
Remove *.appBuilder compsets and log.
Remove outdated doc directory
Testing
Full Regression tests have been run on WCOSS (Cray and Dell), Hera (GNU and Intel) and Orion.
GNU tests on Hera require new baselines, as well as test on WCOSS Cray that runs inline post.
Dependencies