Use OPENMP instead of OpenMP_Fortran_FOUND, cleanup compiler flags #531
Conversation
There was a problem hiding this comment.
looks good.
A couple of comments.
compiler flags e.g. Intel -g -traceback have no performance penalties but greatly aid tracing issues in the event of failures.
These can be used even in RELEASE or REPRO modes, and not just DEBUG.
I would suggest adding them to the base set of compiler flags for the COMPILER.
cmake/Intel.cmake
Outdated
| set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -O2 -debug minimal") | ||
| elseif(DEBUG) | ||
| set(CMAKE_Fortran_FLAGS "${CMAKE_Fortran_FLAGS} -g -O0 -check -check noarg_temp_created -check nopointer -warn -warn noerrors -fp-stack-check -fstack-protector-all -fpe0 -debug -traceback -ftrapuv") | ||
| set(CMAKE_Fortran_FLAGS "${CMAKE_Fortran_FLAGS} -g -O0 -check -check noarg_temp_created -check nopointer -warn -warn noerrors -fp-stack-check -fstack-protector-all -fpe0 -debug -traceback -ftrapuvi -no-prec-div -no-prec-sqrt") | ||
| if(DEBUG_LINKMPI) | ||
| if(OPENMP) | ||
| set(CMAKE_Fortran_FLAGS "${CMAKE_Fortran_FLAGS} -link_mpi=dbg_mt") |
There was a problem hiding this comment.
can we also address linking with DEBUG_LINKMPI
cmake/Intel.cmake
Outdated
| set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -O2 -debug minimal") | ||
| elseif(DEBUG) | ||
| set(CMAKE_Fortran_FLAGS "${CMAKE_Fortran_FLAGS} -g -O0 -check -check noarg_temp_created -check nopointer -warn -warn noerrors -fp-stack-check -fstack-protector-all -fpe0 -debug -traceback -ftrapuv") | ||
| set(CMAKE_Fortran_FLAGS "${CMAKE_Fortran_FLAGS} -O0 -check -check noarg_temp_created -check nopointer -warn -warn noerrors -fp-stack-check -fstack-protector-all -fpe0 -debug -ftrapuvi -no-prec-div -no-prec-sqrt") |
There was a problem hiding this comment.
This should be:
set(CMAKE_Fortran_FLAGS_DEBUG "${CMAKE_Fortran_FLAGS_DEBUG} -O0 -check -check noarg_temp_created -check nopointer -warn -warn noerrors -fp-stack-check -fstack-protector-all -fpe0 -debug -ftrapuvi -no-prec-div -no-prec-sqrt")
Also, whose DEBUG flags are these intended to be? System wide or a specific component? I think these came about for FV3 and have stuck here. In a future clean-up, we should keep system wide flags in this file and component specific ones to be applied in the component.
|
Please add @mark-a-potts to review. |
|
I cannot get "make install" to complete due to a problem with stochastic_physics/mod not existing. |
Does 'make install' work in the current develop branch? |
Good point. It does not. I'll have to put in a PR for that. |
1 similar comment
Good point. It does not. I'll have to put in a PR for that. |
|
@mark-a-potts Try now. |
It works now. Thanks. |
@BrianCurtis-NOAA did all tests complete successfully? I assume the differences are because I added the |
|
I thought the failure looked like it hadn't moved the baseline correctly. The develop-20210421/GNU is empty but /glade/work/briancurtis/FV3_RT/REGRESSION_TEST_GNU is full. |
|
Yes, either Cheyenne isn't happy with a cron job moving files, or i've just typo'd something. I'm hoping the later since it would be as easy fix. |
|
The cheyenne intel was the same issue as the GNU, the move to the storage location didn't work. I'm moving intel right now and will add the RT label once finished. |
|
Ready for merge. |
The current logic in CCPP_driver does not allow for buckets to be emptied every time-step. This one-line modification fixes that.
## DESCRIPTION OF CHANGES:
This PR reorganizes the WE2E testing system so that it is easier to comprehend, use, and modify. The major changes are as follows:
* Move the WE2E testing system from the `regional_workflow/tests` directory to `regional_workflow/tests/WE2E`. This is because we anticipate other types of test in the testing system, e.g. unit tests, which would go under `regional_workflow/tests/unit`, etc.
* Move the WE2E test configuration files that were in `regional_workflow/tests/baseline_configs` to category subdirectories under `regional_workflow/tests/WE2E/test_configs`. The category subdirectories thus far and the types of tests they contain are:
* `grids_extrn_mdls_suites_community`
Tests in community mode of various combinations of the grid, external models for ICs and LBCs, and physics suites.
* `grids_extrn_mdls_suites_nco`
Tests in NCO mode of various combinations of the grid, external models for ICs and LBCs, and physics suites.
* `release_SRW_v1`
The graduate student test (GST) used for the UFS SRW App version 1 release.
* `wflow_features`
Test of workflow features, e.g. ability to set various parameters to user-specified values instead of using the defaults, ability to fetch external model files from different models and on different dates from NOAA-HPSS, etc.
* Rename some of the WE2E test configuration files to adhere to the naming convention used in each category subdirectory.
* Remove some of the WE2E test configuration files since they are almost-duplicates, e.g. they differ with respect to another test only in the cycle date/time used or the LBC specification interval.
* Changes to contents of test configuration files:
* Rearrange the order in which experiment variables are specified in the test configuration files so that the predefined grid name and the physics suite are set first, then the external model info is set, then the cycle dates, then the forecast length and LBC update interval, finally followed by other parameters.
* Remove redundant variable specifications in the test configuration files, e.g. remove `QUILTING="TRUE"` since `QUILTING` is already set to `"TRUE"` by default, remove `GRID_GEN_METHOD="ESGgrid"` since a predefined grid is already specified that in turn implies a value for `GRID_GEN_METHOD`.
* Include a test purpose/description in each test configuration file.
* Perform bug fixes in the WE2E configuration files, e.g. a test that was supposed to run on the `RRFS_CONUS_3km` grid was actually running on the `RRFS_CONUS_13km` grid.
* Remove the file `baselines_list.txt` since it it not used.
* Add new function in `get_WE2Etest_names_subdirs_descs.sh` that:
* Searches subdirectories under the base directory in which the WE2E test configuration files are located (`regional_workflow/tests/WE2E/test_configs`) and returns a list of all available test names, the category subdirectories under the base directory in which they are located, the unique test IDs, and the test descriptions.
* Creates (if requested) a comma-separated value (CSV) file containing this WE2E information that can be opened as a spreadsheet in Google Sheets.
* Rename `run_experiments.sh` to `run_WE2E_tests.sh`.
* In `run_WE2E_tests.sh`:
* Include a detailed usage message.
* Make sure that required arguments are provided on the command line.
* Call the new function `get_WE2Etest_names_subdirs_descs` to get a full list of all available tests. Then check the list of test names that the user wants to run to make sure all exist in the full list.
* Run sanity checks on the user-specified list of tests to run, e.g. that a test is not repeated (either under the same name or under an alternate name).
* In `ush/bash_utils/filesys_cmds_vrfy.sh`, put the `local` attribute in front of variables that are supposed to be local.
* In `ush/bash_utils/is_element_of.sh`, add the `:-` at the end of `array_name_at` so that the function still works when the array passed in is empty.
* In `ush/config_defaults.sh`, edit comments and move groups of variables to more appropriate location in file.
## TESTS CONDUCTED:
On Hera, ran the following tests so far:
* get_from_HPSS_ics_FV3GFS_lbcs_FV3GFS_fmt_netcdf_2021062000
* inline_post
* specify_EXTRN_MDL_SYSBASEDIR_ICS_LBCS
* specify_RESTART_INTERVAL
All were successful. Since it is expensive to run all the new WE2E tests, the remainder will be tested gradually and the results recorded in a google spreadsheet (for now; this will have to be automated at some point).
## DEPENDENCIES:
There will likely be a companion PR into `ufs-weather-app` that updates the hash of `regional_workflow`, but that can only be created after this PR is merged.
## DOCUMENTATION:
Detailed documentation for how the new WE2E testing system works is in the comments in the files `run_WE2E_tests.sh` and `get_WE2Etest_names_subdirs_descs.sh`. Transferring these to RST files will take time and is likely better to do as part of a separate PR.
PR Checklist
Ths PR is up-to-date with the top of all sub-component repositories except for those sub-components which are the subject of this PR. Please consult the ufs-weather-model wiki if you are unsure how to do this.
This PR has been tested using a branch which is up-to-date with the top of all sub-component repositories except for those sub-components which are the subject of this PR
An Issue describing the work contained in this PR has been created either in the subcomponent(s) or in the ufs-weather-model. The Issue should be created in the repository that is most relevant to the changes in contained in the PR. The Issue and the dependent sub-component PR
are specified below.
If new or updated input data is required by this PR, it is clearly stated in the text of the PR.
Instructions: All subsequent sections of text should be filled in as appropriate.
The information provided below allows the code managers to understand the changes relevant to this PR, whether those changes are in the ufs-weather-model repository or in a subcomponent repository. Ufs-weather-model code managers will use the information provided to add any applicable labels, assign reviewers and place it in the Commit Queue. Once the PR is in the Commit Queue, it is the PR owner's responsiblity to keep the PR up-to-date with the develop branch of ufs-weather-model.
Description
Use OPENMP option to specify dependency on OpenMP instead of using OpenMP_Fortran_FOUND, which can be set by some dependent libraries. See #410
Remove redundant compiler flags in cmake/Intel.cmake and GNU.cmake
Issue(s) addressed
Link the issues to be closed with this PR, whether in this repository, or in another repository.
(Remember, issues must always be created before starting work on a PR branch!)
Testing
Due to the change in compiler flags (now '
-no-prec-div -no-prec-sqrt' is used in all tests, previously only in 64-bit tests), all 32-bit tests need new baselines (using Intel).Dependencies
If testing this branch requires non-default branches in other repositories, list them. Those branches should have matching names (ideally).
Do PRs in upstream repositories need to be merged first?
If so add the "waiting for other repos" label and list the upstream PRs