Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cleanup, adding unit tests for ccpp_track_variables.py, activate "chunked data" CI test, add missing MPI target in src/CMakeLists.txt #555

Merged
merged 12 commits into from
May 1, 2024
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions .github/workflows/prebuild.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -55,3 +55,12 @@ jobs:
cmake ..
make
./test_blocked_data.x
- name: ccpp-prebuild chunked data tests
run: |
cd test_prebuild/test_chunked_data
python3 ../../scripts/ccpp_prebuild.py --config=ccpp_prebuild_config.py --builddir=build
cd build
cmake ..
make
./test_chunked_data.x

2 changes: 1 addition & 1 deletion scripts/ccpp_prebuild.py
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ def import_config(configfile, builddir):
if not os.path.isfile(configfile):
logging.error("Configuration file {0} not found".format(configfile))
success = False
return
return(success, config)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the tuple? Doesn't Python do that anyway? Is that a style thing and if so, do we want to be consistent?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gold2718 Sorry, I'm not sure what you mean. This function as called from main() expects two return values, and so currently when this block of code is executed (when there is no configuration file) it fails with a TypeError because it is not returning the expected values.

> ../scripts/ccpp_track_variables.py -s test_track_variables/suite_small_suite.xml -c test_track_variables/ccpp_prebuild_config -m test_track_variables/ -v air_pressure
ERROR:root:Configuration file test_track_variables/ccpp_prebuild_config not found
Traceback (most recent call last):
  File "/glade/derecho/scratch/kavulich/CCPP/track_variables_unit_test/ccpp-framework/test_prebuild/../scripts/ccpp_track_variables.py", line 217, in <module>
    track_variables(args.sdf,args.metadata_path,args.config,args.variable,args.debug)
  File "/glade/derecho/scratch/kavulich/CCPP/track_variables_unit_test/ccpp-framework/test_prebuild/../scripts/ccpp_track_variables.py", line 191, in track_variables
    (success, config) = import_config(config, None)
TypeError: cannot unpack non-iterable NoneType object

This change fixes the error.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the lack of clarity. All I was saying is that the parentheses are not needed so I was wondering why you use them. The same goes below where you call this function.
Is this a style thing we want to adopt? A quick look shows that the prebuild code seems to consistently use the parentheses syntax while the capgen code is . . . . mixed (I'm sure for good reasons :).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the clarification. I did not realize this until you brought this up, but parentheses are not strictly necessary for tuples, so this would be purely stylistic. Since the existing return tuples use parens I'll stick with that for now, especially since PEP8 doesn't seem to specify a preference from what I can tell.


# Import the host-model specific CCPP prebuild config;
# split into path and module name for import
Expand Down
34 changes: 21 additions & 13 deletions scripts/ccpp_track_variables.py
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,6 @@ def parse_suite(sdf, run_env):
the ordered list of schemes for the suite specified by the provided sdf"""
run_env.logger.info(f'Reading sdf {sdf} and populating Suite object')
suite = Suite(sdf_name=sdf)
run_env.logger.info(f'Reading sdf {sdf} and populating Suite object')
success = suite.parse(make_call_tree=True)
if not success:
raise Exception(f'Parsing suite definition file {sdf} failed.')
Expand Down Expand Up @@ -100,9 +99,6 @@ def create_var_graph(suite, var, config, metapath, run_env):
run_env.logger.debug(f"reading .meta files in path:\n {metapath}")
metadata_dict=create_metadata_filename_dict(metapath)

run_env.logger.debug(f"reading metadata files for schemes defined in config file: "
f"{config['scheme_files']}")

# Loop through call tree, find matching filename for scheme via dictionary schemes_in_files,
# then parse that metadata file to find variable info
partial_matches = {}
Expand Down Expand Up @@ -170,20 +166,29 @@ def create_var_graph(suite, var, config, metapath, run_env):

return (success,var_graph)

def main():
def track_variables(sdf,metadata_path,config,variable,debug):
"""Main routine that traverses a CCPP suite and outputs the list of schemes that use given
variable, broken down by group"""
variable, broken down by group

args = parse_arguments()
Args:
sdf (str) : The full path of the suite definition file to parse
metadata_path (str) : path to CCPP scheme metadata files
config (str) : path to CCPP prebuild configuration file
variable (str) : variable to track through CCPP suite
debug (bool) : Enable extra output for debugging

Returns:
None
"""

logger = setup_logging(args.debug)
logger = setup_logging(debug)

#Use new capgen class CCPPFrameworkEnv
run_env = CCPPFrameworkEnv(logger, host_files="", scheme_files="", suites="")

suite = parse_suite(args.sdf,run_env)
suite = parse_suite(sdf,run_env)

(success, config) = import_config(args.config, None)
(success, config) = import_config(config, None)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You also do not need the tuple to unpack the variables. Same questions as above.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But it doesn't hurt, or? If so, then does it matter?

if not success:
raise Exception('Call to import_config failed.')

Expand All @@ -194,10 +199,10 @@ def main():
if not success:
raise Exception('Call to gather_variable_definitions failed.')

(success, var_graph) = create_var_graph(suite, args.variable, config, args.metadata_path, run_env)
(success, var_graph) = create_var_graph(suite, variable, config, metadata_path, run_env)
if success:
print(f"For suite {suite.sdf_name}, the following schemes (in order for each group) "
f"use the variable {args.variable}:")
f"use the variable {variable}:")
for group in var_graph:
if var_graph[group]:
print(f"In group {group}")
Expand All @@ -206,4 +211,7 @@ def main():


if __name__ == '__main__':
main()

args = parse_arguments()

track_variables(args.sdf,args.metadata_path,args.config,args.variable,args.debug)
130 changes: 130 additions & 0 deletions test_prebuild/test_track_variables.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,130 @@
#! /usr/bin/env python3
"""
-----------------------------------------------------------------------
Description: Contains unit tests for ccpp_track_variables.py script

Assumptions: Assumes user has correct environment for running ccpp_track_variables.py script.
This script should not be run directly, but rather invoked with pytest.

Command line arguments: none

Usage: pytest test_track_variables.py # run the unit tests
-----------------------------------------------------------------------
"""
import sys
import os
import pytest

TEST_DIR = os.path.dirname(os.path.abspath(__file__))
SCRIPTS_DIR = os.path.abspath(os.path.join(TEST_DIR, os.pardir, "scripts"))
SAMPLE_FILES_DIR = "test_track_variables"
SUITE_FILE = f'{SAMPLE_FILES_DIR}/suite_TEST_SUITE.xml'
SMALL_SUITE_FILE = f'{SAMPLE_FILES_DIR}/suite_small_suite.xml'
CONFIG_FILE = f'{SAMPLE_FILES_DIR}/ccpp_prebuild_config.py'
if not os.path.exists(SCRIPTS_DIR):
raise ImportError(f"Cannot find scripts directory {SCRIPTS_DIR}")

sys.path.append(SCRIPTS_DIR)

from ccpp_track_variables import track_variables

def test_successful_match(capsys):
"""Tests whether test_track_variables.py produces expected output from sample suite and
metadata files for a case with a successful match (user provided a variable that exists
within the schemes specified by the test suite)"""
expected_output = """For suite test_track_variables/suite_small_suite.xml, the following schemes (in order for each group) use the variable air_pressure:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A doc string explaining the purpose of the test (what is being tested, test limits, etc.) would be helpful here and for all the test functions below.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed

In group group1
scheme_1_run (intent in)
scheme_1_run (intent in)"""
track_variables(SMALL_SUITE_FILE,SAMPLE_FILES_DIR,CONFIG_FILE,'air_pressure',False)
streams = capsys.readouterr()
expected_output_list = expected_output.splitlines()
streams_err_list = streams.err.splitlines()
for (err, expected) in zip(streams_err_list, expected_output_list):
assert err.strip() == expected.strip()

def test_successful_match_with_subcycles(capsys):
"""Tests whether test_track_variables.py produces expected output from sample suite and
metadata files for a case with a successful match (user provided a variable that exists
within the schemes specified by the test suite). In this case, the test suite file
contains subcycles, so the output should reflect this."""

expected_output = """For suite test_track_variables/suite_TEST_SUITE.xml, the following schemes (in order for each group) use the variable surface_air_pressure:
In group group1
scheme_3_run (intent inout)
scheme_3_timestep_finalize (intent inout)
scheme_3_timestep_finalize (intent out)
scheme_4_run (intent in)
scheme_3_run (intent inout)
scheme_3_timestep_finalize (intent inout)
scheme_3_timestep_finalize (intent out)
scheme_4_run (intent in)
In group group2
scheme_4_run (intent in)
scheme_4_run (intent in)
scheme_4_run (intent in)"""
track_variables(SUITE_FILE,SAMPLE_FILES_DIR,CONFIG_FILE,'surface_air_pressure',False)
streams = capsys.readouterr()
expected_output_list = expected_output.splitlines()
streams_err_list = streams.err.splitlines()
for (err, expected) in zip(streams_err_list, expected_output_list):
assert err.strip() == expected.strip()


def test_partial_match(capsys):
"""Tests whether test_track_variables.py produces expected output from sample suite and
metadata files for a case with a partial match: user provided a variable that does not
exist in the test suite, but is a substring of one or more other variables that do
exist."""

expected_output = """Variable surface not found in any suites for sdf test_track_variables/suite_TEST_SUITE.xml

ERROR:ccpp_track_variables:Variable surface not found in any suites for sdf test_track_variables/suite_TEST_SUITE.xml

Did find partial matches that may be of interest:

In scheme_2_init found variable(s) ['surface_emissivity_data_file']
In scheme_2_run found variable(s) ['surface_roughness_length', 'surface_ground_temperature_for_radiation', 'surface_air_temperature_for_radiation', 'surface_skin_temperature_over_ice', 'baseline_surface_longwave_emissivity', 'surface_longwave_emissivity', 'surface_albedo_components', 'surface_albedo_for_diffused_shortwave_on_radiation_timestep']
In scheme_3_init found variable(s) ['flag_for_mellor_yamada_nakanishi_niino_surface_layer_scheme']
In scheme_3_timestep_init found variable(s) ['flag_for_mellor_yamada_nakanishi_niino_surface_layer_scheme']
In scheme_3_run found variable(s) ['do_compute_surface_scalar_fluxes', 'do_compute_surface_diagnostics', 'surface_air_pressure', 'reference_air_pressure_normalized_by_surface_air_pressure']
In scheme_3_timestep_finalize found variable(s) ['surface_air_pressure']
In scheme_4_run found variable(s) ['surface_air_pressure']
In scheme_B_run found variable(s) ['flag_nonzero_wet_surface_fraction', 'sea_surface_temperature', 'surface_skin_temperature_after_iteration_over_water']
"""
track_variables(SUITE_FILE,SAMPLE_FILES_DIR,CONFIG_FILE,'surface',False)
streams = capsys.readouterr()
expected_output_list = expected_output.splitlines()
streams_err_list = streams.err.splitlines()
for (err, expected) in zip(streams_err_list, expected_output_list):
assert err.strip() == expected.strip()


def test_no_match(capsys):
"""Tests whether test_track_variables.py produces expected output from sample suite and
metadata files for a case with no match (user provided a variable that does not exist
within the schemes specified by the test suite)"""

expected_output = """Variable abc not found in any suites for sdf test_track_variables/suite_TEST_SUITE.xml

ERROR:ccpp_track_variables:Variable abc not found in any suites for sdf test_track_variables/suite_TEST_SUITE.xml"""
track_variables(SUITE_FILE,SAMPLE_FILES_DIR,CONFIG_FILE,'abc',False)
streams = capsys.readouterr()
expected_output_list = expected_output.splitlines()
streams_err_list = streams.err.splitlines()
for (err, expected) in zip(streams_err_list, expected_output_list):
assert err.strip() == expected.strip()


def test_bad_config(capsys):
"""Tests whether test_track_variables.py fails gracefully when provided a config file that does
not exist."""
with pytest.raises(Exception) as excinfo:
track_variables(SUITE_FILE,SAMPLE_FILES_DIR,f'{SAMPLE_FILES_DIR}/nofile','abc',False)
assert str(excinfo.value) == "Call to import_config failed."


if __name__ == "__main__":
print("This test file is designed to be run with pytest; can not be run directly")
sys.exit(1)

69 changes: 69 additions & 0 deletions test_prebuild/test_track_variables/ccpp_prebuild_config.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
#!/usr/bin/env python

# CCPP prebuild config for CCPP track variables tool test


###############################################################################
# Definitions #
###############################################################################

HOST_MODEL_IDENTIFIER = "TEST"

# Add all files with metadata tables on the host model side and in CCPP,
# relative to basedir = top-level directory of host model. This includes
# kind and type definitions used in CCPP physics. Also add any internal
# dependencies of these files to the list.
VARIABLE_DEFINITION_FILES = [
# actual variable definition files
'../src/ccpp_types.F90',
]

# Add all physics scheme files relative to basedir
SCHEME_FILES = [ ]

# Default build dir, relative to current working directory,
# if not specified as command-line argument
DEFAULT_BUILD_DIR = 'build'

# Auto-generated makefile/cmakefile snippets that contain all type definitions
TYPEDEFS_MAKEFILE = '{build_dir}/CCPP_TYPEDEFS.mk'
TYPEDEFS_CMAKEFILE = '{build_dir}/CCPP_TYPEDEFS.cmake'
TYPEDEFS_SOURCEFILE = '{build_dir}/CCPP_TYPEDEFS.sh'

# Auto-generated makefile/cmakefile snippets that contain all schemes
SCHEMES_MAKEFILE = '{build_dir}/CCPP_SCHEMES.mk'
SCHEMES_CMAKEFILE = '{build_dir}/CCPP_SCHEMES.cmake'
SCHEMES_SOURCEFILE = '{build_dir}/CCPP_SCHEMES.sh'

# Auto-generated makefile/cmakefile snippets that contain all caps
CAPS_MAKEFILE = '{build_dir}/CCPP_CAPS.mk'
CAPS_CMAKEFILE = '{build_dir}/CCPP_CAPS.cmake'
CAPS_SOURCEFILE = '{build_dir}/CCPP_CAPS.sh'

# Directory where to put all auto-generated physics caps
CAPS_DIR = '{build_dir}'

# Directory where the suite definition files are stored
SUITES_DIR = '.'

# Optional arguments - only required for schemes that use
# optional arguments. ccpp_prebuild.py will throw an exception
# if it encounters a scheme subroutine with optional arguments
# if no entry is made here. Possible values are: 'all', 'none',
# or a list of standard_names: [ 'var1', 'var3' ].
OPTIONAL_ARGUMENTS = {}

# Directory where to write static API to
STATIC_API_DIR = '{build_dir}'
STATIC_API_CMAKEFILE = '{build_dir}/CCPP_API.cmake'
STATIC_API_SOURCEFILE = '{build_dir}/CCPP_API.sh'

# Directory for writing HTML pages generated from metadata files
METADATA_HTML_OUTPUT_DIR = '{build_dir}'

# HTML document containing the model-defined CCPP variables
HTML_VARTABLE_FILE = '{build_dir}/CCPP_VARIABLES_BLOCKED_DATA.html'

# LaTeX document containing the provided vs requested CCPP variables
LATEX_VARTABLE_FILE = '{build_dir}/CCPP_VARIABLES_BLOCKED_DATA.tex'

25 changes: 25 additions & 0 deletions test_prebuild/test_track_variables/scheme_1.meta
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
[ccpp-table-properties]
name = scheme_1
type = scheme
dependencies = ../../hooks/machine.F

########################################################################
[ccpp-arg-table]
name = scheme_1_run
type = scheme
[p_lay]
standard_name = air_pressure
long_name = mean layer pressure
units = Pa
dimensions = (horizontal_loop_extent,vertical_layer_dimension)
type = real
kind = kind_phys
intent = in
[p_lev]
standard_name = air_pressure_at_interface
long_name = air pressure at model layer interfaces
units = Pa
dimensions = (horizontal_loop_extent,vertical_interface_dimension)
type = real
kind = kind_phys
intent = in
Loading
Loading