Use the checkout_externals utility to checkout physics submodules with cmake#1421
Use the checkout_externals utility to checkout physics submodules with cmake#1421mgduda merged 1 commit intoMPAS-Dev:developfrom
Conversation
mgduda
added
Atmosphere
bug fix
Build System
|
@jim-p-w Could you base your branch on the v8.3.0 tag? |
Will do. |
|
FYI, we are addressing the same issue in different routes: I would advocate we adopt the git submodules universally. |
| message( FATAL_ERROR "Failed to checkout external repos via manage_externals" ) | ||
| else() | ||
| message(STATUS "Directory ${DIR_TO_CHECK} already exists, skipping clone") | ||
| message( STATUS "Finished checking out external repos via manage_externals" ) |
There was a problem hiding this comment.
It will be great to add a logic if physics_mmm and UGWP exist, skip running checkout_externals. Thanks!
There was a problem hiding this comment.
Although physics_mmm and UGWP may already exist, they may be out of sync with the tag specified in the Externals.cfg file.
There was a problem hiding this comment.
I think an "existence" check is consistent with all other "git lone" in this CMakeLists.txt.
Folks may want to make modifications inside physics_mmm or UGWP and don't want them to get overwritten by checkout_externals. This check does not break anything if one do a clean clone of MPAS-Model
There was a problem hiding this comment.
checkout_externals will not overwrite dirty checkouts.
I think keeping the logic as one-to-one with the make build where possible helps reduce issues such as this one where extra or divergent code may lead to unexpected difference in behavior.
That said, it would be nice to wholly use submodules (in my opinion).
There was a problem hiding this comment.
@islas Thanks for the discussion!
Will we get any issue if we add an existence check?
If physics_mmm or UGWP is out of sync, it is mostly due to some corner cases and in those situations, developers/users are expected to handle it manually.
It is nice that if we may use git submodules for all subcomponents.
GSL is transitioning to do that.
Adding an existence check here will help a little bit since we use git clone --recursive to get all codes and don't want check_externals to do duplicate or unexpected work in that situation. Thanks!
There was a problem hiding this comment.
Because an existence check of directories is not sufficient to know the correct code has been checked out my inclination is to always run checkout_externals. For example, if the main branch is checked out, as currently implemented the correct tag will get checked out.
Also, in the event a different tag is needed, editing src/core_atmosphere/Externals.cfg and rerunning cmake will update things properly without requiring manual deletion of directories.
There was a problem hiding this comment.
@jim-p-w Thanks for the discussion! FYI, I would like to provide a few alternate scenarios:
- One makes modifications under
physics_mmmorUGWP, commits them to personal fork/branch. So these directories are clean (not dirty). If one wants to rebuild the MPAS-Model,checkout_externalswill "overwrites" (to be accurate, switches the top reference) all changes. This is NOT what we expect. - If one add anything under
physics_mmmorUGWP, even touching a file, these directories will be dirty andcheckout_externalswill no longer take effect (per previous @islas's comment), right? - I think we can only be able to make sure checking out specified frozen checkouts from a clean clone. For other situations, if one want to update hash/tag or get back the default frozen checkout, one can run
checkout_externalsorgit checkoutmanually at the command line instead of our forcing it in the CMake runtime. - This PR changes the current behavior:
if(NOT EXISTS ${ATMOSPHERE_CORE_PHYSICS_MMM_DIR})
......
if(NOT EXISTS ${ATMOSPHERE_CORE_PHYSICS_NOAA_DIR})
......
and it may break downstream applications.
There was a problem hiding this comment.
@guoqing-noaa I think we can all agree that the existing CMakeLists.txt file is not correct. If I've understood the changes proposed in ufs-community#231 , it seems that there are a couple of potential drawbacks:
- If we create a fresh clone of MPAS-Model, compile, change one of the tags in the
Externals.cfgfile, and re-compile, the build will not use the updated physics repository tags (because, e.g., thephysics_mmmdirectory already exists). This is also a problem if we locally merge a branch that switches external physics repository tags and makes corresponding MPAS-Model code changes to work with the new external tag: compiling will make use of the modified MPAS-Model code without the corresponding updates to the external physics. - The behavior of the CMake build is inconsistent with that of the GNU Make build.
While it requires a change in expectations (i.e., the external physics used by the build will always be those specified in the Externals.cfg file), it looks to me like the changes proposed in this PR should address both of the above drawbacks.
I'm not sure if we'll reach an agreement in this PR discussion thread. If you intend to update the ufs-community fork in the near future with changes to the MPAS-Dev/MPAS-Model develop branch, perhaps that can be followed by a PR to the ufs-community fork to enforce whatever behavior you'd prefer, and subsequent merges of MPAS-Dev/MPAS-Model develop would hopefully leave those changes in place.
There was a problem hiding this comment.
@mgduda Thanks for the discussions! We have different use situations and preferences. That's fine and expected. We can make a little bit tweaks in the ufs-community fork after this PR is merged. Thanks!
jim-p-w
force-pushed
the
feature/externals
branch
from
6102275 to
b0b584e
Compare
|
I'm making this a draft until the target branch gets updated to be v8.3.0 |
What I'm suggesting is that your PR branch should begin at the |
jim-p-w
force-pushed
the
feature/externals
branch
from
b0b584e to
aaf4eaa
Compare
…h cmake. core_atmosphere requires specific versions of the physics code from https://github.com/NCAR/MMM-physics.git and https://github.com/NOAA-GSL/UGWP.git The checkout_externals utility uses the versions of those repositories specified in src/core_atmosphere/Externals.cfg. The gnu make build system uses checkout_externals; this change modifies the cmake build files to use the same mechanism as the make build system.
jim-p-w
force-pushed
the
feature/externals
branch
from
8ff5e97 to
365f28c
Compare
MPAS Version 8.4.0 This release of MPAS introduces new capabilities and improvements in the MPAS-Atmosphere model and its supporting software infrastructure, and it is the first release to provide the foundation for CheMPAS-A, a chemistry capability for MPAS-Atmosphere. Notable changes are listed below. MPAS-Atmosphere Initialization: * Fix an issue in snow initialization over areas covered by seaice. (PR #1438) * Introduce namelist control over the vertical coordinate configuration for real-data case initialization. Two new namelist options may now be specified in the &vertical_grid namelist group in the namelist.init_atmosphere file: - config_hybrid_coordinate (default: true) Whether to employ a hybrid vertical coordinate - config_hybrid_top_z (default: 30000 m) Height at which coordinate surfaces become constant height surfaces Additionally, the smoothing coefficient formula for the hybrid coordinate now uses the transition height instead of the model top height. (PR #1382) * Add computation of the edgeNormalVectors, cellTangentPlane, and localVerticalUnitVectors fields for MPAS-A initialization case 13 (CAM-MPAS 3-d grid) to better support alternative initialization workflows for CAM-MPAS. (PR #1351) MPAS-Atmosphere Dynamics: * Implement an initial Large-Eddy Simulation (LES) capability. Similar to WRF, it contains two subgrid turbulence models -- a diagnostic Turbulent Kinetic Energy (TKE) formulation based on a 3-d Smagorinsky formulation, and a 1.5-order prognostic TKE formulation. These formulations generally follow the implementation in WRF (see the WRF Technical Note Version 4, sections 4.2.3 and 4.2.4 for a description of the formulations). (PR #1404) * Generalize the epssm parameter, allowing level-dependent values to be specified through four new namelist options: - config_epssm_minimum (default value: 0.1) - config_epssm_maximum (default value: 0.5) - config_epssm_transition_bottom_z (default value: 30000.0) - config_epssm_transition_top_z (default value: 50000.0) The epssm parameter is an off-centering coefficient for the vertically semi-implicit acoustic / gravity-wave integration, and it was previously specified as a single, constant value using the config_epssm namelist option. (PR #1381) * Add the capability to turn off microphysics tendencies above a specified height with a new namelist option, config_microphysics_top, in the &physics namelist group. Ignoring tendencies from the microphysics at upper levels has been found to alleviate some instabilities in deep-domain (i.e., model tops above the stratopause) MPAS-Atmosphere simulations. (PR #1380) * Optimize OpenACC data movement throughout the MPAS-A dycore, so that the majority of fields are transferred to the device at the start of a dynamics step and from the device at the end of a dynamics step. (PR #1315) * Remove the scaling of the gravity-wave absorbing layer coefficient (dss) by the local mesh size. Previously, the absorbing layer coefficient was scaled by dx/dx_fine, and for large values of dx/dx_fine, instabilities have been encountered associated with this scaling of the absorbing layer coefficient. (PR #1379) MPAS-Atmosphere Physics: * Correct an issue with high q2 values over urban cells in Noah-MP. (PR #1383) * Fix crashes when using the CAM SW radiation scheme. (PR #1391) * Fix an indexing typo in mpas_atmphys_interface.F, where evapprod(k,k) was used instead of evapprod(k,i). The fix corrects diagnostic output, but should not affect the model state or simulation results. (PR #1396) * Correct the units designations of the gravity wave drag diagnostics variables dusfcg, dvsfcg, dtaux3d and dtauy3d in the atmosphere core's Registry.xml file. (PR #1378) MPAS-Atmosphere Misc.: * Remove PV diagnostic fields from the default "output" stream for MPAS-Atmosphere, reducing the size of the default "history" netCDF files by ~20%. (PR #1428) * Modify the MPAS-Atmosphere CMake build system to use manage_externals to obtain specific tags from the https://github.com/NCAR/MMM-physics.git and https://github.com/NOAA-GSL/UGWP.git repositories, rather than simply obtaining the HEAD of the default branch. This modification corrects build failures when using CMake. (PR #1421) CheMPAS-A: * Enable linking with the MUSICA-Fortran library to support development of chemistry capabilities based on MICM and TUV-x. (PR #1309) * Add a new namelist group, &musica, with a single option, config_micm_file, to control the MICM configuration in MPAS-Atmosphere. The new namelist group is only included in the namelist.atmosphere file (and therefore, only recognized) when MPAS is compiled with support for the MUSICA library. (PR #1376) * Introduce directory structure and stub modules for MUSICA, and more broadly, for chemistry options in the atmosphere core. (PR #1360) New directories and modules include: src/ core_atmosphere/ chemistry/ <--| mpas_atm_chemistry.F <--| musica/ <--| new in this release mpas_musica.F <--| Infrastructure: * Introduce the ability to partition meshes "online" at model startup using the PT-Scotch library. (PR #1364) * Enable execution of halo exchanges directly on GPU-resident fields when MPAS is linked with an MPI library that is GPU-aware. To enable GPU-aware halo exchanges, a new namelist option, config_gpu_aware_mpi, in the &development namelist group must be set to true at runtime. (PR #1355) * Improve support for using an external ESMF library through a new build option, MPAS_ESMF. (PR #1405) * Set the default PnetCDF header alignment to 128 KiB when creating new output files with SMIOL. This has been found to fix I/O issues when overwriting existing model output files under certain conditions. (PR #1386) * Fix an issue with macro expansion with newer Intel oneAPI compilers by adding a flag for the 'intel' build target to change the order of macro expansion in the Fortran pre-processor. This change in macro expansion order fixes an issue with some nested macro use in MPAS. (PR #1392) * Fix a buffer overflow in the streaminfo_query function in the mpas_stream_inquiry module , where the local array c_attname was allocated without space for a C null character. (PR #1352)
4 participants
core_atmosphere requires specific versions of the physics code from https://github.com/NCAR/MMM-physics.git and https://github.com/NOAA-GSL/UGWP.git
The checkout_externals utility uses the versions of those repositories specified in src/core_atmosphere/Externals.cfg.
The gnu make build system uses checkout_externals; this change modifies the cmake build files to use the same mechanism as the make build system.
This PR fixes issue 1420
This was tested by building MPAS-Model code with cmake. It was also tested by building mpas-bundle.
This change was copied from wrf-model where it has been in use for about a year.