Skip to content

build the dycore with cmake#141

Merged
bensonr merged 11 commits into
NOAA-GFDL:dev/emcfrom
aerorahul:feature/cmake_only_in_dycore_of_emc
Sep 23, 2021
Merged

build the dycore with cmake#141
bensonr merged 11 commits into
NOAA-GFDL:dev/emcfrom
aerorahul:feature/cmake_only_in_dycore_of_emc

Conversation

@aerorahul
Copy link
Copy Markdown

@aerorahul aerorahul commented Sep 10, 2021

Description

The build and the source code for the FV3 dynamical core resides in two different repositories, which is less than ideal.

This PR:

  • includes a CMake build for the dynamical core intended for the UFS and thus the PR is to merge into dev/emc

This is an excerpt from an email with @bensonr from 7/19/2021

Hi Rahul,

As dev/emc exists for use within GFS/UFS, contributing a CMake build system would be appropriate. The FV3 team at GFDL can then decide whether to include (git cherry-pick) this in the master/main branch for their own use with the SHiELD model.

thanks - Rusty

Fixes #139

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Please also note
any relevant details for your test configuration (e.g. compiler, OS). Include
enough information so someone can reproduce your tests.

Testing is in progress with UFS-weather-model.
Note that this PR does not have any impact on current UFS-weather-model.
A subsequent change in the UFS-weather-model will remove the FV3 dynamical core build from FV3atm at which point these changes will come in play.

Checklist:

Please check all whether they apply or not

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules

@bensonr
Copy link
Copy Markdown
Contributor

bensonr commented Sep 10, 2021

@aerorahul - who all would you recommend for reviews?

@aerorahul
Copy link
Copy Markdown
Author

@aerorahul - who all would you recommend for reviews?

@DusanJovic-NOAA @climbfuji

Copy link
Copy Markdown

@DomHeinzeller DomHeinzeller 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 not for this PR, but for the future. We have so many repositories that use FindNetCDF.cmake and FindESMF.cmake. Should we strive for using one submodule like CMakeModules for this, or even better, is there a way to include those in the official CMake distributions?

@DomHeinzeller
Copy link
Copy Markdown

I am not sure, but I think there are AVX2 flags missing for Intel. Testing this now ...

Copy link
Copy Markdown

@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 PR as-is changed the regression test results for the AWS CI tests and on Jet with Intel for yet unknown reasons.

@aerorahul
Copy link
Copy Markdown
Author

The atm dycore is handled differently than other components in the UFS.
Isolating and fixing this issue will likely result in non-reproducible baseline due to this special handling.

@aerorahul aerorahul closed this Sep 14, 2021
@aerorahul
Copy link
Copy Markdown
Author

reopening this PR.
Merging this PR without changes in the fv3atm will not impact UFS weather model as fv3atm provides the build.
The compiler flags can be fine tuned at a later time when fv3atm is cleaned up to use this functionality.

@DusanJovic-NOAA
Copy link
Copy Markdown
Contributor

Having the CMakeLists.txt in this repo but not using it in UFS just creates confusion. And is maintenance burden, because now the same change has to be applied at two places.

Copy link
Copy Markdown

@DomHeinzeller DomHeinzeller left a comment

Choose a reason for hiding this comment

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

I don't see the merit of merging this now. It just pushes the burden to fix this to "someone else" at "some point later". We would be merging code that we know is not correct (w.r.t. b4b regression testing in the UFS) and it won't be tested. If this code is urgently needed, it should be fixed now and then merged, unless we agree that we don't care about getting the same results or not.

@DusanJovic-NOAA
Copy link
Copy Markdown
Contributor

Here's my suggestion. What if we remove setting compiler flags from this CMakeLists.txt and have them inherited from the upper levels of the app, as we do with other parts of the UFS, like fv3atm, or stochastic physics, etc.

@climbfuji
Copy link
Copy Markdown

Here's my suggestion. What if we remove setting compiler flags from this CMakeLists.txt and have them inherited from the upper levels of the app, as we do with other parts of the UFS, like fv3atm, or stochastic physics, etc.

I like that idea. Is there a smart way to detect if build flags are set by a parent cmake config, or whether this is a standalone build (in which default build flags can be configured)? I'd be interested in such a solution, because it would greatly simplify the existing CCPP CMakeLists.txt files.

@aerorahul
Copy link
Copy Markdown
Author

We could do something like this on Line 163 of CMakeLists.txt

if(NOT UFS)
  include(fv3_compiler_flags)
endif()

@climbfuji
Copy link
Copy Markdown

We could do something like this on Line 163 of CMakeLists.txt

if(NOT UFS)
  include(fv3_compiler_flags)
endif()

I was thinking along checking if a CMake project has been defined already. The top-level ufs-weather-model, for example, sets the following:

project(ufs
        VERSION 1.0
        LANGUAGES C CXX Fortran)

Would testing for project being defined be a suitable mechanism to detect a standalone build?

@aerorahul
Copy link
Copy Markdown
Author

We could do something like this on Line 163 of CMakeLists.txt

if(NOT UFS)
  include(fv3_compiler_flags)
endif()

I was thinking along checking if a CMake project has been defined already. The top-level ufs-weather-model, for example, sets the following:

project(ufs
        VERSION 1.0
        LANGUAGES C CXX Fortran)

Would testing for project being defined be a suitable mechanism to detect a standalone build?

You can check on project, but this has to be just for the UFS as UFS is enforcing its flags.

@climbfuji
Copy link
Copy Markdown

We could do something like this on Line 163 of CMakeLists.txt

if(NOT UFS)
  include(fv3_compiler_flags)
endif()

I was thinking along checking if a CMake project has been defined already. The top-level ufs-weather-model, for example, sets the following:

project(ufs
        VERSION 1.0
        LANGUAGES C CXX Fortran)

Would testing for project being defined be a suitable mechanism to detect a standalone build?

You can check on project, but this has to be just for the UFS as UFS is enforcing its flags.

This is the question we have been debating for a while. Should components set their own flags, or should parents (of submodules) be in charge of setting those. It should be consistent, in my opinion.

@DusanJovic-NOAA
Copy link
Copy Markdown
Contributor

Please remove makefile from the top-level directory. It's not used anymore.

@aerorahul
Copy link
Copy Markdown
Author

Please remove makefile from the top-level directory. It's not used anymore.

done.

@DusanJovic-NOAA
Copy link
Copy Markdown
Contributor

Ready for merge. @bensonr please merge this PR. All UFS tests passed. See ufs-community/ufs-weather-model#812

@bensonr bensonr merged commit ab026b7 into NOAA-GFDL:dev/emc Sep 23, 2021
@aerorahul aerorahul deleted the feature/cmake_only_in_dycore_of_emc branch September 23, 2021 14:33
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.

5 participants