Add support (and tests) for optional arguments in ccpp_prebuild #552

climbfuji · 2024-03-22T17:12:05Z

Add support (and tests) for optional arguments in ccpp_prebuild

See #526 for a detailed description. This PR goes even further and creates an array of pointers for each conditionally allocated variable dimensioned to the number of OpenMP threads. This was required to fix multi-threading issues with GNU compilers (9.2.0 on Hera - might have to do with this being an old version). Since pointers do not cost anything in terms of memory, it's not an issue at all to have an array of number-of-openmp-thread pointers instead of a scalar.

User interface changes?: No

Resolves #526
Resolves #540

Testing:
test removed: none
unit tests: added test_prebuild/test_opt_arg/* and wrapper script test_prebuild/run_all_tests.sh that runs all tests - ALL PASS
system tests: ran end-to-end regression tests for ufs-weather-model on Hera with Intel and GNU, all tests b4b identical with existing baseline
manual testing: compile on macOS for all suites (cannot run due to ESMF/UFS-W-M bug on macOS, since about 5 years ago)

… it in metadata_parser.py and raise conflict in ccpp_prebuild.py if host model variable is not always active and scheme variable isn't optional

…vide run_test.sh convenience scripts and remove README.md; mv individual unit tests into unit_tests

…scheme metadata

…ines, add support for pointers

…s in scripts/mkstatic.py

…ature/prebuild_optarg_based_on_merge_feature_capgen_into_main_20240308

…/mkstatic.py

…ionally allocated

…prebuild.py for inactive/optional arguments

scripts/mkstatic.py

…ature/prebuild_optarg_based_on_merge_feature_capgen_into_main_20240308

climbfuji · 2024-05-11T02:46:14Z

scripts/mkstatic.py


                            # Add to argument list
-                            arg = '{local_name}={var_name}{dim_string},'.format(local_name=var.local_name, 
-                                var_name=local_vars[var_standard_name]['name'].replace(dim_string_target_name, ''), dim_string=dim_string)
+                            if conditional == '.true.':


Very repetitive and complicated in prebuild. Time to move to capgen!

climbfuji · 2024-05-11T02:57:17Z

Everyone, this PR is finally open for review!

climbfuji · 2024-05-13T15:12:43Z

test_prebuild/run_all_tests.sh

+echo "" && echo "Running test_blocked_data" && cd test_blocked_data && ./run_test.sh  && cd ..
+echo "" && echo "Running test_chunked_data" && cd test_chunked_data && ./run_test.sh  && cd ..
+
+echo "" && echo "Running test_track_variables" && pytest test_track_variables.py


Suggested change

echo "" && echo "Running test_track_variables" && pytest test_track_variables.py

echo "" && echo "Running test_track_variables" && pytest test_track_variables.py

gold2718

Not the most efficient thing to have to do. Let's hope this is the last of these before we can get everyone on the same platform.
I just skimmed the build and test scripts but it looks good (could not think of any edge cases you did not cover).

scripts/mkstatic.py

dustinswales

@climbfuji Looks good to me.
I have a few small questions, but nothing to hold this up.

dustinswales · 2024-05-15T14:41:20Z

scripts/mkcap.py

@@ -270,62 +296,128 @@ def print_def_intent(self, metadata):
        dictionary to resolve lower bounds for array dimensions.'''
        # Resolve dimensisons to local names using undefined upper bounds (assumed shape)
        dimstring = self.dimstring_local_names(metadata, assume_shape = True)
+        # It is an error for host model variables to have the optional attribute in the metadata


I need to check if hosts having the optional attribute in Capgen is an error. I don't recall adding this, but maybe it's in there somewhere. @gold2718 Do you know by chance?

I don't see where you could have put it.
What exactly is the rule here? I thought the host model is the source of the need for optional variables. That is, if a host model can choose to not allocate one of its variables, it needs to be declared optional by routines that use it.

Is it possible that the distinction is really about not allowing the optional attribute for host or module metadata tables? This could easily be done in metavar.py by moving the VariableProperty, optional from the __spec_props list to the __var_props list. Since HostModel only allows 'module' and 'host' metadata tables, I think this would act as the correct barrier.

I'm not sure where this leaves DDTs. For a host DDT, can an element have the active attribute? What happens if you try that with a DDT that is used in a scheme? We may need to add logic here as well.

Thoughts?

Yes, this is about the metadata only. The rule is quite simple (if I understood it correctly):

host models can have the active attribute for a variable or not (default value is true = always allocated)

schemes can have the optional attribute for a variable or not (default value is false = always allocated by the host model)

it is an error of a host model has optional in the metadata attribute

it is an error of a scheme has active in the metadata attribute

if a host model has an active attribute with a value other than true, any scheme using that environment variable must use optional = true (requirement3)

the opposite is not true or at least should not be true: a scheme can have optional = true, because model A requires that, but there can be a model B that doesn't declare it as active in its metadata.

prebuild checks for 1-5 in its own layer after receiving the metadata from capgen. This is because we only harvest information on host metadata and scheme metadata separately (except for passing known host DDTs) and do not rely on the metadata comparison host <--> model from capgen. I will note that at this point, prebuild spits out a warning for 6 if it finds a variable with optional being true but no non-default active attribute in the model. I used this to clean up the metadata, knowing that the current ccpp physics are tied to the GFS data structures used by UFS, SCM, NEPTUNE.

Regarding your pointer question. That gets tricky. The only way out I can see here is that if a host ddt has an active attribute other than the default true, all it's members (if declared explicitly in the metadata) need that to have active to, with the allocation condition being that of the DDT (if the member is always allocated when the DDT is allocated) OR a Fortran logical && of the DDTs allocation condition and its own. We need to be explicit here and have both allocation conditions in the member var's active attribute instead of implicitly inheriting it from the DDT. That would greatly confuse the physics/model developers and users.

@climbfuji, I think your rules 1-6 plus thoughts on DDTs all make sense. We should make sure all of these are tested in capgen, do we need a new issue for that?

Yes, that would be good. I'll work with @dustinswales on that

1,2,5, and 6 are applied In the Capgen implementation, with the exception of 3 and 4. I will add these soon, and revisit the DDTs.

scripts/mkcap.py

dustinswales · 2024-05-15T15:01:40Z

scripts/mkstatic.py

+
+                        # If the host variable is conditionally allocated, create a pointer for it
+                        if not conditional == '.true.':
+                            # Reuse existing temporary pointer variable, if possible; otherwise add a local pointer (tmpptr)


If it costs nothing to create a pointer, why bother reusing them?
Below I see that the local pointer is nullified after the scheme call, so there shouldn't be any conflicts in subsequent calls. Or am I misinterpreting the context all together?

reusing refers to the SAME pointer being reused for the SAME variable in the different scheme calls. Otherwise, it would be creating one pointer N times for the same variable if it gets passed to N schemes in that group.

…mbinations.

Adding additional operating systems in testing

…nto_main_20240308' of https://github.com/climbfuji/ccpp-framework into feature/more_gnu_versions_in_CI

Feature/more gnu versions in ci

grantfirl · 2024-05-17T14:40:33Z

scripts/ccpp_prebuild.py

+                    logging.error("Conditionally allocated host-model variable {0} is not optional in {1}".format(
+                                  var_name, var.container))
+                    success = False
+        # TEMPORARY CHECK - IF THE VARIABLE IS ALWAYS ALLOCATED, THE SCHEME VARIABLE SHOULDN'T BE OPTIONAL


Is this really temporary?

Yes. This is only there because all models using ccpp_prebuild are in lockstep in terms of allocating or not allocating variables. I used this to check that there are no unnecessary optional variables in the physics.

But going forward, CESM may declare some variables as potentially allocated, whereas the UFS doesn't, or vice versa. We still need the schemes make this variable optional, but the UFS would have the active attribute.

mwaxmonsky

Just had one or two recommendations for potentially easier readability but definitely nothing that should hold up this PR. Thanks @climbfuji!

mwaxmonsky · 2024-05-20T12:48:30Z

scripts/metadata_parser.py

                if not new_var.get_prop_value('active'):
-                    # If it doesn't have an active attribute, then the variable is always active (default)
-                    active = 'T'
+                    raise Exception("Unexpected result: no active attribute received from capgen metadata parser for {} / {}".format(standard_name,table_name))
+                elif scheme_name and not new_var.get_prop_value('active').lower() == '.true.':
+                    raise Exception("Scheme variable {} in table {} has metadata attribute active={}, which is not allowed".format(
+                                    standard_name, table_name, new_var.get_prop_value('active').lower()))
                elif new_var.get_prop_value('active').lower() == '.true.':
                    active = 'T'
-                elif new_var.get_prop_value('active') and new_var.get_prop_value('active').lower() == '.false.':
+                elif new_var.get_prop_value('active').lower() == '.false.':
                    active = 'F'
                else:
                    # Replace multiple whitespaces, preserve case
                    active = ' '.join(new_var.get_prop_value('active').split())



Because we use the "active" new_var property frequently here, we might want to simplify to:

active_prop = new_var.get_prop_value('active') if not active_prop: raise Exception(f"Unexpected result: no active attribute received from capgen metadata parser for {standard_name} / {table_name}") active_prop = active_prop.lower() if active_prop == '.true': active = 'T' elif active_prop == '.false': active = 'F' else: active = ' '.join(active_prop.split())

Thanks, @mwaxmonsky I do agree but given that the regression testing with the UFS is about halfway through I'll refrain from making this change. That code will go away as soon as we move to capgen anyway :-)

mwaxmonsky · 2024-05-20T12:51:42Z

scripts/metadata_parser.py

+                if not new_var.get_prop_value('optional') in [False, True]:
+                    raise Exception("Unexpected result: no optional attribute received from metadata parser for {} / {}".format(standard_name,table_name))
+                elif not scheme_name and new_var.get_prop_value('optional'):
+                    raise Exception("Host variable {} in table {} has metadata attribute optional={}, which is not allowed".format(
+                                    standard_name,table_name, new_var.get_prop_value('optional').lower()))
+                elif new_var.get_prop_value('optional'):
+                    optional = 'T'
+                else:
+                    optional = 'F'


Similarly for the optional property:

Suggested change

if not new_var.get_prop_value('optional') in [False, True]:

raise Exception("Unexpected result: no optional attribute received from metadata parser for {} / {}".format(standard_name,table_name))

elif not scheme_name and new_var.get_prop_value('optional'):

raise Exception("Host variable {} in table {} has metadata attribute optional={}, which is not allowed".format(

standard_name,table_name, new_var.get_prop_value('optional').lower()))

elif new_var.get_prop_value('optional'):

optional = 'T'

else:

optional = 'F'

optional_prop = new_var.get_prop_value('optional')

if not optional_prop in [False, True]:

raise Exception(f"Unexpected result: no optional attribute received from metadata parser for {standard_name} / {table_name}")

elif not scheme_name and optional_prop:

raise Exception(f"Host variable {standard_name} in table {table_name} has metadata attribute optional={optional_prop.lower()}, which is not allowed")

elif optional_prop:

optional = 'T'

else:

optional = 'F'

FernandoAndrade-NOAA · 2024-05-21T17:39:24Z

Testing for #2205 has completed successfully, please continue with merging this PR.

climbfuji added 13 commits March 20, 2024 15:29

Add optional attribute to variable properties in ccpp_prebuild, parse…

cd5eae8

… it in metadata_parser.py and raise conflict in ccpp_prebuild.py if host model variable is not always active and scheme variable isn't optional

Update test_blocked_data, test_chunked_data to run in debug mode; pro…

28b15cc

…vide run_test.sh convenience scripts and remove README.md; mv individual unit tests into unit_tests

Add test_prebuild/test_opt_arg

853f43d

Add wrapper to run all prebuild tests

6556d35

Fix target_link_libraries for ccpp-framework in src/CMakeLists.txt

5e4bae1

Revert me: nasty workaround to inject missing optional attribute int …

363e8e5

…scheme metadata

Add 'optional' attribute in scripts/mkcap.py, update print_def_* rout…

f4d4d7f

…ines, add support for pointers

Add support for optional variables and pointers in auto-generated cap…

413e9ec

…s in scripts/mkstatic.py

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

61b0c61

…ature/prebuild_optarg_based_on_merge_feature_capgen_into_main_20240308

Comment out workaround to inject optional attribute into scheme metadata

8709a48

Bug fixes and workarounds for allocatable strings with GNU in scripts…

f9976d6

…/mkstatic.py

Issue warning if scheme variable is optional but variable is uncondit…

01e2d8d

…ionally allocated

Declare maximum CCPP thread count and use arrays of pointers in ccpp_…

8138e22

…prebuild.py for inactive/optional arguments

dustinswales reviewed Apr 29, 2024

View reviewed changes

scripts/mkstatic.py Show resolved Hide resolved

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

a6e9f04

…ature/prebuild_optarg_based_on_merge_feature_capgen_into_main_20240308

climbfuji commented May 11, 2024

View reviewed changes

Address self-review

f2b9a55

climbfuji marked this pull request as ready for review May 11, 2024 02:56

climbfuji requested review from gold2718, mwaxmonsky, peverwhee, grantfirl and mkavulich as code owners May 11, 2024 02:57

Update prebuild tests

259a494

climbfuji mentioned this pull request May 11, 2024

Add support for optional arguments in ccpp physics and framework AND add end of run restart functionality to MOM6 (was #2286) ufs-community/ufs-weather-model#2205

Merged

15 tasks

climbfuji commented May 13, 2024

View reviewed changes

gold2718 approved these changes May 13, 2024

View reviewed changes

scripts/mkstatic.py Show resolved Hide resolved

dustinswales added 2 commits May 14, 2024 21:53

Update CI

fba4ce0

Update CI w/ GNU v9 and v13

0e00c17

dustinswales approved these changes May 15, 2024

View reviewed changes

dustinswales mentioned this pull request May 15, 2024

Add script to check fortran vs metadata without a host model #558

Closed

mwaxmonsky and others added 4 commits May 15, 2024 14:37

Adding additional matrix param for both os's and excluding invalid co…

7d2af1d

…mbinations.

Removing spaces.

c84c9f0

Fixing syntax.

bb20702

Nullify pointers in declaration in mkcap.py

d769bb8

This was referenced May 16, 2024

Complete implementation of optional arguments / active variables in capgen #566

Open

Add optional attribute to Capgen/Prebuild #526

Closed

dustinswales added 4 commits May 16, 2024 08:40

Merge pull request #3 from mwaxmonsky/feature/more_gnu_versions_in_CI

b8b754c

Adding additional operating systems in testing

Update CI

87f9323

Update CI

caae916

Update CI

788d251

dustinswales mentioned this pull request May 16, 2024

Update CI to use more GNU versons with Capgen tests/ #563

Closed

dustinswales and others added 5 commits May 16, 2024 15:41

Update CI

ba27abd

Update CI

f73bbe8

Update CI

3839c63

Merge branch 'feature/prebuild_optarg_based_on_merge_feature_capgen_i…

f1fde4a

…nto_main_20240308' of https://github.com/climbfuji/ccpp-framework into feature/more_gnu_versions_in_CI

Merge pull request #3 from dustinswales/feature/more_gnu_versions_in_CI

c298399

Feature/more gnu versions in ci

grantfirl reviewed May 17, 2024

View reviewed changes

grantfirl approved these changes May 17, 2024

View reviewed changes

mwaxmonsky approved these changes May 21, 2024

View reviewed changes

grantfirl merged commit ccfefcd into NCAR:main May 21, 2024
9 checks passed

grantfirl deleted the feature/prebuild_optarg_based_on_merge_feature_capgen_into_main_20240308 branch May 21, 2024 17:55

mkavulich mentioned this pull request Aug 13, 2024

Update CCPP tech doc: pointer variables are not allowed in CCPP-compliant parameterizations #541

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support (and tests) for optional arguments in ccpp_prebuild #552

Add support (and tests) for optional arguments in ccpp_prebuild #552

climbfuji commented Mar 22, 2024 •

edited

Loading

climbfuji May 11, 2024

climbfuji commented May 11, 2024

climbfuji May 13, 2024

gold2718 left a comment

dustinswales left a comment

dustinswales May 15, 2024

gold2718 May 15, 2024

climbfuji May 16, 2024

gold2718 May 16, 2024

climbfuji May 16, 2024

climbfuji May 16, 2024

dustinswales May 16, 2024

dustinswales May 15, 2024

climbfuji May 16, 2024

grantfirl May 17, 2024

climbfuji May 17, 2024

mwaxmonsky left a comment

mwaxmonsky May 20, 2024

climbfuji May 21, 2024

mwaxmonsky May 20, 2024

FernandoAndrade-NOAA commented May 21, 2024

	echo "" && echo "Running test_track_variables" && pytest test_track_variables.py
	echo "" && echo "Running test_track_variables" && pytest test_track_variables.py

Add support (and tests) for optional arguments in ccpp_prebuild #552

Add support (and tests) for optional arguments in ccpp_prebuild #552

Conversation

climbfuji commented Mar 22, 2024 • edited Loading

Choose a reason for hiding this comment

climbfuji commented May 11, 2024

Choose a reason for hiding this comment

gold2718 left a comment

Choose a reason for hiding this comment

dustinswales 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

mwaxmonsky 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

FernandoAndrade-NOAA commented May 21, 2024

climbfuji commented Mar 22, 2024 •

edited

Loading