Update master from gmtb/develop 2019/10/16 #230

climbfuji · 2019-10-16T00:19:56Z

Associated PRs:

#230
NCAR/ccpp-physics#340
NOAA-EMC/GFDL_atmos_cubed_sphere#5
NOAA-EMC/fv3atm#1
ufs-community/ufs-weather-model#1
NOAA-EMC/NEMS#11 (merged)
NOAA-EMC/NEMS#12

See ufs-community/ufs-weather-model#1 for a description and regression testing information.

This PR for ccpp-framework merges the previously approved changes made for the gmtb/develop branch into the master branch.

Changes specific to ccpp-framework:

cleanup of prebuild scripts (move common code into common.py module, ...)
add capability to create html pages from metadata files for all schemes in a CCPP configuration file (required for generating scientific documentation)
add out-of-source build capability to CCPP prebuild scripts (optional argument --builddir for ccpp_prebuild.py, write out list of schemes and caps to a shell script that can be sourced and sets environment variables)

…INTERNAL_VARIABLE_DEFINITON_FILE to common.py, correct typo

…uild config

… on metadata2html.py

…_batchconvert gmtb/develop: metadata2html batch processing

…call to check_fortran_type, but do not throw an error in case a DDT is not registered

…myj_with_dom_mods

…into myj_with_dom_mods

gmtb/develop: update from master, address issues from previous commit

…ntation for TRANSITION mode

gmtb/develop: remove TRANSITION mode

- Documentation has been moved to the NCAR/ccpp-doc repository.

Remove CCPPtechnical directory, new home in NCAR/ccpp-doc repo

…ired for parallel cmake builds in NEMSfv3gfs

…ld directory

…into HEAD

gold2718

Most of my comments / requested changes are suggestions but I am worried about the commented-out code in parse_checkers.py.

gold2718 · 2019-10-17T14:57:25Z

scripts/common.py

@@ -14,6 +15,16 @@

 CCPP_TYPE = 'ccpp_t'

+# SCRIPTDIR is the directory where ccpp_prebuild.py and its Python modules are located
+SCRIPTDIR = os.path.abspath(os.path.split(__file__)[0])


Any reason to not use os.path.dirname instead of split here? That function gets enhancements (e.g., support path-like objects) that I am not sure are guaranteed to work with split.

Out of curiosity, are these enhancements for Python 3? I read on the web (Python 2, https://docs.python.org/2/library/os.path.html and other sources) that os.path.dirname as well as os.path.basename internally use os.path.split to return the first/second part.

Yes, it seems that python 3 has more functionality for these functions (and probably a different internal implementation although I have not looked into that).

gold2718 · 2019-10-17T15:03:45Z

scripts/common.py

+SCRIPTDIR = os.path.abspath(os.path.split(__file__)[0])
+
+# SRCDIR is the directory where the CCPP framework source code (C, Fortran) is located
+SRCDIR = os.path.abspath(os.path.join(SCRIPTDIR, '..', 'src'))


Use os.pardir in place of ...

gold2718 · 2019-10-17T15:29:08Z

scripts/metadata2html.py

+
+    # Import the host-model specific CCPP prebuild config;
+    # split into path and module name for import
+    configpath = os.path.abspath(os.path.split(configfile)[0])


Another place to use os.path.dirname?

gold2718 · 2019-10-17T15:36:15Z

scripts/metadata2html.py

+    # Import the host-model specific CCPP prebuild config;
+    # split into path and module name for import
+    configpath = os.path.abspath(os.path.split(configfile)[0])
+    configmodule = os.path.split(configfile)[1].rstrip('.py')


I would prefer os.path.basename(configfile).rstrip('.py')
Also, is there enforcement somewhere of the .py suffix? If not, why not just strip off any suffix?
re.sub('[.]+[^.]*$', '', os.path.basename(configfile))

Will use os.path.basename(configfile).rstrip('.py') for now. Not sure, but if I use __import__ or importlib.import_module afterwards, isn't Python looking for configfile + .py ?

Actually, I am going to use

configpath = os.path.abspath(os.path.dirname(configfile)) configmodule = os.path.splitext(os.path.basename(configfile))[0]

because this relies on existing functionality of the os module only.

I like that.

gold2718 · 2019-10-17T15:40:36Z

scripts/metadata2html.py

+    configpath = os.path.abspath(os.path.split(configfile)[0])
+    configmodule = os.path.split(configfile)[1].rstrip('.py')
+    sys.path.append(configpath)
+    ccpp_prebuild_config = __import__(configmodule)


Is there a particular reason why importlib.import_module cannot be used here? __import__ is discouraged unless necessary.

gold2718 · 2019-10-17T15:48:59Z

scripts/parse_tools/parse_checkers.py

+    # DH* 20190913 - skip checking if a DDT is registered at this time
+    #if match is None:
+    #    if error:
+    #        raise CCPPError("'{}' is not a valid{} Fortran type".format(typestr, dt))
+    #    else:
+    #        typestr = None
+    #    # End if
+    ## End if
+    # *DH 20190913


This sort of change is going to make it difficult to keep the capgen development branch up to date with master. Can we find some other technique (e.g., some variable test) to ensure that this clause is not triggered for master but is active for feature/capgen?

I'd suggest to worry about this once capgen is merged into master - until then, the logic in the master-version of the scripts that capgen would be using would require this change as well, because it assumes that (a) all DDTs used are defined in the same metadata file (which is certainly not going to be the case) and that they are defined in the metadata file before they are used. I understand that the feature/capgen version completes the analysis phase and then knows about all DDTs, but the current version in the master branch does not.

I will accept that, however, this means that master and feature/capgen will diverge until then. Adding more logic to keep the if statement from being triggered in master, will allow updates to feature/capgen.

…dard libraries instead of deprecated or manual tools

codecov-io · 2019-10-18T00:54:03Z

Codecov Report

Merging #230 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #230   +/-   ##
=======================================
  Coverage   47.28%   47.28%           
=======================================
  Files          14       14           
  Lines        1343     1343           
=======================================
  Hits          635      635           
  Misses        708      708

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7c2332d...82329df. Read the comment docs.

…m/climbfuji/ccpp-framework into update_master_from_gmtb_develop_20191016

grantfirl

Looks OK to me.

climbfuji and others added 18 commits September 12, 2019 08:33

scripts/common.py, scripts/ccpp_prebuild.py: move definition of CCPP_…

d9f538e

…INTERNAL_VARIABLE_DEFINITON_FILE to common.py, correct typo

scripts/metadata2html.py: add batch processing option using CCPP preb…

7f3c188

…uild config

doc/CCPPtechnical/source/ScientificDocRules.inc: update documentation…

258bc8e

… on metadata2html.py

Update of scripts and documentation to address reviewer comments

513d6e3

Merge pull request NCAR#223 from climbfuji/gmtb_develop_metadata2html…

6abc50e

…_batchconvert gmtb/develop: metadata2html batch processing

scripts/metavar.py, scripts/parse_tools/parse_checkers.py: re-enable …

ee4cf5f

…call to check_fortran_type, but do not throw an error in case a DDT is not registered

Merge branch 'master' of https://github.com/NCAR/ccpp-framework into …

62d4c41

…myj_with_dom_mods

Merge branch 'gmtb/develop' of https://github.com/NCAR/ccpp-framework …

470d27e

…into myj_with_dom_mods

Merge pull request NCAR#224 from climbfuji/myj_with_dom_mods

9088a6c

gmtb/develop: update from master, address issues from previous commit

doc/CCPPtechnical/source/BuildingRunningHostModels.rst: remove docume…

00e63a1

…ntation for TRANSITION mode

Merge pull request NCAR#225 from climbfuji/remove_transition_mode

5b472cd

gmtb/develop: remove TRANSITION mode

- Remove CCPPtechnical directory.

94a426e

- Documentation has been moved to the NCAR/ccpp-doc repository.

Merge pull request NCAR#226 from JulieSchramm/gmtb/develop

4f60488

Remove CCPPtechnical directory, new home in NCAR/ccpp-doc repo

Modifications of code generator to support out-of-source builds, requ…

6dc90b3

…ired for parallel cmake builds in NEMSfv3gfs

scripts/mkstatic.py: add missing guard for filepath being empty

5a67e43

scripts/mkdoc.py: add missing guard for filepath being empty

e742528

scripts/ccpp_prebuild.py: write html and latex variable tables to bui…

1358ecf

…ld directory

Merge branch 'gmtb/develop' of https://github.com/NCAR/ccpp-framework …

0e3f90b

…into HEAD

climbfuji marked this pull request as ready for review October 16, 2019 23:21

climbfuji requested review from gold2718, grantfirl and llpcarson as code owners October 16, 2019 23:21

gold2718 requested changes Oct 17, 2019

View reviewed changes

Improvements of prebuild scripts based on reviewer comments; use stan…

8553108

…dard libraries instead of deprecated or manual tools

gold2718 approved these changes Oct 18, 2019

View reviewed changes

climbfuji added 2 commits October 22, 2019 06:28

Merge branch 'ccpp_prebuild_out_of_source_build' of https://github.co…

dcb3a83

…m/climbfuji/ccpp-framework into update_master_from_gmtb_develop_20191016

scripts/ccpp_prebuild.py: write schemes and caps with path

82329df

grantfirl approved these changes Oct 24, 2019

View reviewed changes

climbfuji merged commit d473ed5 into NCAR:master Oct 31, 2019

This was referenced Oct 31, 2019

Bugfix for travis test in schemes/check/ccpp_prebuild_config.py; add … #235

Merged

Bugfix #2 for travis test in schemes/check/ccpp_prebuild_config.py #236

Merged

climbfuji deleted the update_master_from_gmtb_develop_20191016 branch June 27, 2022 03:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update master from gmtb/develop 2019/10/16 #230

Update master from gmtb/develop 2019/10/16 #230

climbfuji commented Oct 16, 2019 •

edited

Loading

gold2718 left a comment

gold2718 Oct 17, 2019

climbfuji Oct 17, 2019

gold2718 Oct 18, 2019

gold2718 Oct 17, 2019

gold2718 Oct 17, 2019

gold2718 Oct 17, 2019

climbfuji Oct 17, 2019

climbfuji Oct 18, 2019

gold2718 Oct 18, 2019

gold2718 Oct 17, 2019

gold2718 Oct 17, 2019

climbfuji Oct 17, 2019

gold2718 Oct 18, 2019

codecov-io commented Oct 18, 2019 •

edited

Loading

grantfirl left a comment

Update master from gmtb/develop 2019/10/16 #230

Update master from gmtb/develop 2019/10/16 #230

Conversation

climbfuji commented Oct 16, 2019 • edited Loading

gold2718 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Oct 18, 2019 • edited Loading

Codecov Report

grantfirl left a comment

Choose a reason for hiding this comment

climbfuji commented Oct 16, 2019 •

edited

Loading

codecov-io commented Oct 18, 2019 •

edited

Loading