Skip to content

release/public-v1: fix Cheyenne build, update CCPP suites etc#50

Merged
JeffBeck-NOAA merged 7 commits into
ufs-community:release/public-v1from
climbfuji:cheyenne_build_fixes
Nov 5, 2020
Merged

release/public-v1: fix Cheyenne build, update CCPP suites etc#50
JeffBeck-NOAA merged 7 commits into
ufs-community:release/public-v1from
climbfuji:cheyenne_build_fixes

Conversation

@climbfuji
Copy link
Copy Markdown
Collaborator

This PR fixes a number of issues with building the latest version of the ufs-weather-model and supersedes #48. It also fixes #49.

Changes:

  • Use fixed hash instead of branch for ufs-weather-model in Externals.cfg
  • CMakeLists.txt: cmake 3.15 is required by the submodules
  • src/CMakeLists.txt: update CCPP suites
  • Update documentation for cheyenne.intel and cheyenne.gnu
  • Also: convert tabs in various CMakeLists.txt files into spaces and remove unnecessary CMake modules import (this directory doesn't exist)

This has been tested successfully on Cheyenne with the Intel compiler. Somebody else should try to test this with GNU on Cheyenne, and on other platforms if possible.

Copy link
Copy Markdown

@JulieSchramm JulieSchramm left a comment

Choose a reason for hiding this comment

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

GNU build works on Cheyenne. Thanks for fixing this.

@climbfuji
Copy link
Copy Markdown
Collaborator Author

GNU build works on Cheyenne. Thanks for fixing this.

Yay. Thanks for trying it out.

@mkavulich
Copy link
Copy Markdown
Collaborator

@climbfuji Can you incorporate the changes from #47 into this PR? Specifically, using if(NOT CCPP_SUITES) so that the default CCPP_SUITES value can be overwritten by the cmake command?

Comment thread Externals.cfg
repo_url = https://github.com/ufs-community/ufs-weather-model
# Specify either a branch name or a hash but not both.
branch = release/public-v2
#hash = 8165575
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@climbfuji Is this how the MRW App handles their Externals.cfg file for the release? Since we're still under rapid-development (even for bug fixes for the release), I'm concerned that this change will require us to constantly update Externals.cfg to stay up to date.

Note that we've had discussions about creating a stable branch of regional_workflow that would contain specific hashes in the Externals.cfg file, while the develop branch would point to the HEAD of the branch. New hashes would then have to be extensively tested before they're updated in the Externals.cfg file for the stable branch, while the develop branch would always point to the HEAD and users would be made aware that it may not work.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

In my opinion a certain hash of the srweather-app code is guaranteed to work with exactly one hash of each of its submodules / externals, everything else is gambling. Yes, everytime you update one of the submodules you need to test it with the app and as part of the testing update the hash. The PR that is created to update it documents the successful testing with those particular hashes.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

But note that you don't have to update to a new hash every time, you can do that whenever necessary and as such accumulate a few changes to the submodules (although this increases the risk to break stuff).

Copy link
Copy Markdown
Collaborator

@JeffBeck-NOAA JeffBeck-NOAA Nov 5, 2020

Choose a reason for hiding this comment

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

OK, that makes sense. I see that ufs-mrweather-app points to tags for each repo in the Externals.cfg file. I'm assuming those correspond to specific hashes, so that would be equivalent.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yes, tags and hashes are equivalent in a sense that they are static; tags however can be updated by force to a new hash (re-tagging) - not recommended unless absolutely necessary.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Yes, if you don't mind, that would be great. I'll approve right afterward!

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done. I'll try to check out the PR again to make sure I got all the hashes correct.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

OK, I'll preliminarily approve this PR.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

The hashes do match the head of the branches at this point in time, so all good.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

OK, merging!

@JeffBeck-NOAA
Copy link
Copy Markdown
Collaborator

JeffBeck-NOAA commented Nov 5, 2020

@climbfuji Can you incorporate the changes from #47 into this PR? Specifically, using if(NOT CCPP_SUITES) so that the default CCPP_SUITES value can be overwritten by the cmake command?

@climbfuji @mkavulich, I assume #47 will be closed when those changes are migrated to this PR?

@climbfuji
Copy link
Copy Markdown
Collaborator Author

@climbfuji Can you incorporate the changes from #47 into this PR? Specifically, using if(NOT CCPP_SUITES) so that the default CCPP_SUITES value can be overwritten by the cmake command?

Done, please check. If #50 gets merged, #47 should be flagged as merged, too (i.e. no need to close).

Copy link
Copy Markdown
Collaborator

@JeffBeck-NOAA JeffBeck-NOAA left a comment

Choose a reason for hiding this comment

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

Changes to Externals.cfg hashes and from PR #47 now incorporated, so approving.

@JeffBeck-NOAA JeffBeck-NOAA merged commit 90cb164 into ufs-community:release/public-v1 Nov 5, 2020
mkavulich pushed a commit to mkavulich/ufs-srweather-app that referenced this pull request Nov 24, 2020
… update CCPP suites etc (ufs-community#50)

* Remove all but the two supported CCPP suites (FV3_GFS_v15p2,FV3_RRFS_v1beta) for the release. Also, rather than hard-coding the list, allow for the "CCPP_SUITES" variable to be set to overwrite the default

* CMakeLists.txt: cmake 3.15 is required by the submodules

* src/CMakeLists.txt: allow CCPP suites to be specified on the command-line

* Update documentation for cheyenne.intel and cheyenne.gnu
natalie-perlin pushed a commit to natalie-perlin/ufs-srweather-app that referenced this pull request Jun 2, 2024
…erturbations (ufs-community#386)

* Update .gitmodules and submodule pointer for fv3atm for gsl/develop branch
* RUC ice for gsl/develop (replaces ufs-community#47) (ufs-community#49)Implementation of RUC LSM ice model in CCPP
* Squash-merge climbfuji:rucice_gfsv16dzmin into gsl/develop
* Add kice=9 to tests/tests/fv3_ccpp_rap and tests/tests/fv3_ccpp_hrrr
* Change NEW_BASELINE directory for gsl/develop to avoid conflicts with development work on the authoritative branches
* Add KICE=9 to tests/tests/fv3_ccpp_gsd_unified_ugwp and tests/tests/fv3_ccpp_gsd_drag_suite_unified_ugwp
* Revert change to .gitmodules and update submodule pointer for fv3atm
* Update gsl/develop from develop 2020/12/08 (ufs-community#50)
* Updates to stochastic_physics_wrapper (ufs-community#280)
Fix to stochastic_physics_wrapper to allow for random patterns to update at a longer time-step than model
* Update for Jet, bug fixes in running with frac_grid=T and GFDL MP, and in restarting with frac_grid=T  (ufs-community#304)
Update the modulefile for jet.intel to enable UPP v10.0.0. The hpc-stack v1.0.0 pre-release is used for this. Small changes are made to tests.rt.sh for jet.intel and gaea.intel (consistency with other platforms).
The submodule pointer update for fv3atm addresses bugs in the ufs-weather-model with frac_grid=T and GFDL microphysics, and with restarting the model when frac_grid=T (from @shansun6 and @SMoorthi-emc).
* Land stochastic perturbations (ufs-community#57)
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