Refactor capgen branch #410

gold2718 · 2021-10-27T02:59:05Z

Refactor capgen branch

Add new runtime info routine, ccpp_physics_suite_schemes
Convert framework options to DDT with methods (runtime environment).
Convert shebangs to python3
Change ccpp_error_flag to ccpp_error_code
Add polymorphic variable property
Remove optional variable property
Introduce FortranVar to allow Fortran to have undocumented optional variables
Fix hang in datatable write plus catch more bad units
Split var_props.py from metavar.py to allow independent development
Added VarCompatObject to hold compatibility and transform information about a pair of Var objects. This object looks for compatible differences in units, kind, or dimensions.
Created unit tests for variable compatibility.
Move PrettyElementTree to xml_tools
Refactor ccpp_suite to separate out SuiteObjects
Created /test/run_tests.sh to run all capgen tests.

User interface changes?: Yes

Optional keyword removed from metadata. Optional Fortran variables should be omitted from metadata tables
--kind-type argument replaces --kind-phys and takes a list of model kinds.

Fixes: [Github issue #s]
addresses #329
addresses #378
addresses #403
fixes #408

Testing:

all doctests
capgen_test
advection_test
unit tests in test/unit_tests

Convert framework options to DDT with methods (runtime environment). Convert shebangs to python3 Change ccpp_error_flag to ccpp_error_code Add polymorphic variable property Remove optional variable property Introduct FortranVar to allow Fortran to have undocumented optional variables Cleanup ccpp_error_code usage Fix hang in datatable write plus catch more bad units Split var_props.py from metavar.py to allow independent development Added VarCompatObject to hold compatibility and transform information about a pair of Var objects. This object looks for compatible differences in units, kind, or dimensions. Created unit tests for variable compatibility. Move PrettyElementTree to xml_tools Refactor ccpp_suite to separate out SuiteObjects Created <root>/test/run_tests.sh to run all capgen tests.

climbfuji

(1) Just for my understanding, in the PR description, you say:

Optional keyword removed from metadata. Optional Fortran variables should be omitted from metadata tables

From our previous conversation, my understanding was that optional Fortran variables may be omitted from metadata tables, but they can be present. It's a hardcoded choice which flavor of a scheme to run.

(2) Do you think the improved error handler will be ready before we transition the UFS to capgen? I am asking because in this case we could "shield" the UFS from the extra step to rename ccpp_error_flag to ccpp_error_code and go straight to the new error handler. If not, we'll probably annoy some developers by changing a ton of files (Fortran and metadata, in both ccpp-physics and fv3atm/scm) twice to update the error handling.

(3) I think at least one more Python file needs the Python3 shebang:

index 45e7c14..110a37b
--- a/test/capgen_test/test_reports.py
+++ b/test/capgen_test/test_reports.py
@@ -1,4 +1,4 @@
-#! /usr/bin/env python
+#! /usr/bin/env python3
 """
 -----------------------------------------------------------------------
  Description:  Test capgen database report python interface

(4) The tests are failing on my mac (with (3) above included:

-- Build files have been written to: /Users/dom.heinzeller/work/ccpp-framework/ccpp-framework-capgen-refactor/test/ct_build
Running python interface tests
Traceback (most recent call last):
  File "/Users/dom.heinzeller/work/ccpp-framework/ccpp-framework-capgen-refactor/test/capgen_test/test_reports.py", line 27, in <module>
    from ccpp_datafile import datatable_report, DatatableReport
  File "/Users/dom.heinzeller/work/ccpp-framework/ccpp-framework-capgen-refactor/scripts/ccpp_datafile.py", line 25, in <module>
    from metadata_table import UNKNOWN_PROCESS_TYPE
  File "/Users/dom.heinzeller/work/ccpp-framework/ccpp-framework-capgen-refactor/scripts/metadata_table.py", line 133, in <module>
    from metavar     import Var, VarDictionary, CCPP_CONSTANT_VARS
  File "/Users/dom.heinzeller/work/ccpp-framework/ccpp-framework-capgen-refactor/scripts/metavar.py", line 340
    context=self.context) from cperr
                             ^
SyntaxError: invalid syntax

ERROR: python interface tests failed

ERROR: Failure running capgen test

(5) In general: This PR is extremely difficult if not impossible to review in a reasonable amount of time. There are many unrelated changes, and a lot of code is moved around (which makes it almost impossible to identify if anything has changed in those parts). I understand that this is to some extend the nature of "refactor" PRs and that breaking changes up takes more time. But it would be easier to review and lead to more robust reviews if, for example, all the "error flag" -> "error code" changes were its own PR, all the single underscore to double underscore changes were its own PR, the run_env changes were its own PR, ... or combinations of those that are more manageable. I know I am guilty of big PRs myself, especially in the beginnings of CCPP, but I think we should try to be good stewards of GitHub code change practices and create smaller PRs that contain logically related changes but nothing else / not much else otherwise going forward.

(6) There are a few more comments below inline with the code.

doc/HelloWorld/hello_scheme.meta

scripts/metavar.py

climbfuji · 2021-10-27T14:14:58Z

scripts/metavar.py

-    def is_horizontal_dimension(cls, dim_name):
-        """Return True if it is a recognized horizontal
-        dimension or index, otherwise, return False
-        >>> Var.is_horizontal_dimension('horizontal_loop_extent')


What about testing

Var.is_horizontal_dimension('horizontal_dimension')

for the sake of completeness?

climbfuji · 2021-10-27T14:18:45Z

scripts/metavar.py

-        False
-        >>> Var.is_vertical_dimension('ccpp_constant_one:vertical_interface_index')
-        False
-        >>> Var.is_vertical_dimension('horizontal_loop_extent')


We could also test for those additional vertical dimensions, like

vertical_layer_dimension_for_radiation vertical_interface_dimension_for_radiation

We have never discussed the semantics of these dimensions. If the CCPP framework is going to internally handle new dimensions, please open an issue where the full semantics of their usage (including any interaction with the standard vertical dimensions) can be discussed and agreed upon.

Yes, we never discussed this, I just noticed the additional dimension in the standard name dictionary and the UFS. Let's discuss this next week at the framework meeting when you are back, I will create an issue to remind us.

climbfuji · 2021-10-27T14:26:57Z

scripts/var_props.py

+
+###############################################################################
+# Supported vertical dimensions (should be defined in CCPP_STANDARD_VARS)
+CCPP_VERTICAL_DIMENSIONS = ['ccpp_constant_one:vertical_layer_dimension',


We are missing the "decorated" versions

vertical_interface_dimension_for_radiation vertical_layer_dimension_for_radiation

(these are defined in CCPP_STANDARD_VARS)

climbfuji · 2021-10-27T14:27:32Z

scripts/var_props.py

+###############################################################################
+# Substituions for run time dimension control
+CCPP_LOOP_DIM_SUBSTS = {'ccpp_constant_one:horizontal_dimension' :
+                        'horizontal_loop_begin:horizontal_loop_end',


Do we need to add the _radiation versions here as well?

See comment above.

climbfuji · 2021-10-27T14:28:00Z

scripts/var_props.py

+    >>> is_vertical_dimension('ccpp_constant_one:vertical_layer_index')
+    False
+    >>> is_vertical_dimension('ccpp_constant_one:vertical_interface_index')
+    False


Could test for _radiation versions here

See comment above.

climbfuji · 2021-10-27T14:52:06Z

doc/HelloWorld/hello_scheme.meta

@@ -53,9 +53,9 @@
  kind = len=512
  intent = out
 [ errflg ]


We should rename those as well.

Suggested change

[ errflg ]

[ errcode ]

Why do local variable names need to match standard names? This has never been a requirement as far as I recall.

That was just a suggestion for consistency, of course they can be different (and given the comment on the long name errflg might be just as good).

gold2718 · 2021-10-27T18:24:06Z

From our previous conversation, my understanding was that optional Fortran variables may be omitted from metadata tables, but they can be present. It's a hardcoded choice which flavor of a scheme to run.

Oops, bad communication. capgen does work this way, I added a test in capgen_test to verify that an optional Fortran variable which is included in the metadata table is handled properly.

gold2718 · 2021-10-27T18:25:48Z

Do you think the improved error handler will be ready before we transition the UFS to capgen?

I have not prioritized this work because it would delay unification readiness. We need to re-evaluate the schedule if you want to add this.

gold2718 · 2021-10-27T18:27:21Z

I think at least one more Python file needs the Python3 shebang:

Yep, I already found this by testing on a different machine (it turns out that the machine I was using had python ==> python3 so I did not see the problem. Unfortunately, in trying to clean up all this test stuff, I ended up touching a bunch of files so the PR grew.

gold2718 · 2021-10-27T18:28:45Z

In general: This PR is extremely difficult if not impossible to review in a reasonable amount of time.

I agree with this comment and am happy to try and make shorter PRs. However, if we agree that I will do this, I need to go in and re-estimate all the task hours as a PR does take me a significant amount of time. Your call.

climbfuji · 2021-10-27T19:06:19Z

In general: This PR is extremely difficult if not impossible to review in a reasonable amount of time.

I agree with this comment and am happy to try and make shorter PRs. However, if we agree that I will do this, I need to go in and re-estimate all the task hours as a PR does take me a significant amount of time. Your call.

It's a judgement call, I think we need to be pragmatic. Maybe along the lines "I can split this up easily without spending a lot of time, so let's to it" and "This would be a lot of overhead, let's keep it together".

climbfuji · 2021-10-28T12:56:56Z

I ran the tests with your latest commit bbb668c on my Mac, and all tests passed.

climbfuji · 2021-11-01T16:47:16Z

@gold2718 I am ok with this PR, do you want to make more changes or should I approve and merge? Thanks.

gold2718 · 2021-11-01T16:58:09Z

@gold2718 I am ok with this PR, do you want to make more changes or should I approve and merge? Thanks.

From my point of view, it is okay to merge if you are comfortable with the current state. I can try to work in future PRs.

gold2718 added enhancement capgen bugs, requests, etc. that involve ccpp_capgen labels Oct 27, 2021

gold2718 requested a review from climbfuji October 27, 2021 02:59

gold2718 self-assigned this Oct 27, 2021

climbfuji requested changes Oct 27, 2021

View reviewed changes

add optional var test, doctest cleanup, and python3 checks

bbb668c

climbfuji approved these changes Nov 1, 2021

View reviewed changes

climbfuji merged commit fc5f951 into NCAR:feature/capgen Nov 1, 2021

climbfuji mentioned this pull request Nov 10, 2021

Update main from feature/capgen following the refactoring and implementation of strict metadata units checking #416

Closed

dustinswales mentioned this pull request Jul 6, 2023

Implement variable action class in capgen (30 hours) #403

Closed

dustinswales mentioned this pull request Jan 11, 2024

Capgen to improve on kind metadata from prebuild (5 hours) #378

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor capgen branch #410

Refactor capgen branch #410

gold2718 commented Oct 27, 2021

climbfuji left a comment

climbfuji Oct 27, 2021

climbfuji Oct 27, 2021

gold2718 Oct 27, 2021

climbfuji Oct 27, 2021

climbfuji Oct 27, 2021

climbfuji Oct 27, 2021

gold2718 Oct 27, 2021

climbfuji Oct 27, 2021

gold2718 Oct 27, 2021

climbfuji Oct 27, 2021

gold2718 Oct 27, 2021

climbfuji Oct 27, 2021

gold2718 commented Oct 27, 2021

gold2718 commented Oct 27, 2021

gold2718 commented Oct 27, 2021

gold2718 commented Oct 27, 2021

climbfuji commented Oct 27, 2021

climbfuji commented Oct 28, 2021

climbfuji commented Nov 1, 2021

gold2718 commented Nov 1, 2021

Refactor capgen branch #410

Refactor capgen branch #410

Conversation

gold2718 commented Oct 27, 2021

climbfuji 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

gold2718 commented Oct 27, 2021

gold2718 commented Oct 27, 2021

gold2718 commented Oct 27, 2021

gold2718 commented Oct 27, 2021

climbfuji commented Oct 27, 2021

climbfuji commented Oct 28, 2021

climbfuji commented Nov 1, 2021

gold2718 commented Nov 1, 2021