From 3e85c6852ca847d067e312b16468918709c5bcce Mon Sep 17 00:00:00 2001 From: Michael Kavulich Date: Thu, 11 Jul 2024 13:43:14 -0600 Subject: [PATCH 1/6] Remove requirement that suite definition file names begin with "suite_" (#569) Remove hard-coded requirement in prebuild that suite definition file names begin with the literal string `suite_`. Includes back-compatibility logic to allow for the previous naming convention to continue to work. --- scripts/ccpp_prebuild.py | 20 ++++++++++++++++--- scripts/common.py | 2 +- scripts/mkstatic.py | 15 +++++++++----- ..._data_suite.xml => blocked_data_suite.xml} | 0 4 files changed, 28 insertions(+), 9 deletions(-) rename test_prebuild/test_blocked_data/{suite_blocked_data_suite.xml => blocked_data_suite.xml} (100%) diff --git a/scripts/ccpp_prebuild.py b/scripts/ccpp_prebuild.py index b67de540..3cb4ee19 100755 --- a/scripts/ccpp_prebuild.py +++ b/scripts/ccpp_prebuild.py @@ -55,7 +55,7 @@ def parse_arguments(): verbose = args.verbose debug = args.debug if args.suites: - sdfs = ['suite_{0}.xml'.format(x) for x in args.suites.split(',')] + sdfs = ['{0}.xml'.format(x) for x in args.suites.split(',')] else: sdfs = None builddir = args.builddir @@ -181,8 +181,22 @@ def parse_suites(suites_dir, sdfs): logging.info('Parsing suite definition files ...') suites = [] for sdf in sdfs: - logging.info('Parsing suite definition file {0} ...'.format(os.path.join(suites_dir, sdf))) - suite = Suite(sdf_name=os.path.join(suites_dir, sdf)) + sdf_file=os.path.join(suites_dir, sdf) + if not os.path.exists(sdf_file): + # If suite file not found, check old filename convention (suite_[suitename].xml) + sdf_file_legacy=os.path.join(suites_dir, f"suite_{sdf}") + if os.path.exists(sdf_file_legacy): + logging.warning("Parsing suite definition file using legacy naming convention") + logging.warning(f"Filename {os.path.basename(sdf_file_legacy)}") + logging.warning(f"Suite name {sdf}") + sdf_file=sdf_file_legacy + else: + logging.critical(f"Suite definition file {sdf_file} not found.") + success = False + return (success, suites) + + logging.info(f'Parsing suite definition file {sdf_file} ...') + suite = Suite(sdf_name=sdf_file) success = suite.parse() if not success: logging.error('Parsing suite definition file {0} failed.'.format(sdf)) diff --git a/scripts/common.py b/scripts/common.py index c29ccda2..f5ba7f1d 100755 --- a/scripts/common.py +++ b/scripts/common.py @@ -73,7 +73,7 @@ CCPP_STATIC_SUBROUTINE_NAME = 'ccpp_physics_{stage}' # Filename pattern for suite definition files -SUITE_DEFINITION_FILENAME_PATTERN = re.compile('^suite_(.*)\.xml$') +SUITE_DEFINITION_FILENAME_PATTERN = re.compile('^(.*)\.xml$') # Maximum number of concurrent CCPP instances per MPI task CCPP_NUM_INSTANCES = 200 diff --git a/scripts/mkstatic.py b/scripts/mkstatic.py index 4f00b9b9..889b2c1d 100755 --- a/scripts/mkstatic.py +++ b/scripts/mkstatic.py @@ -691,11 +691,16 @@ def parse(self, make_call_tree=False): suite_xml = tree.getroot() self._name = suite_xml.get('name') # Validate name of suite in XML tag against filename; could be moved to common.py - if not (os.path.basename(self._sdf_name) == 'suite_{}.xml'.format(self._name)): - logging.critical("Invalid suite name {0} in suite definition file {1}.".format( - self._name, self._sdf_name)) - success = False - return success + if not (os.path.basename(self._sdf_name) == '{}.xml'.format(self._name)): + if (os.path.basename(self._sdf_name) == 'suite_{}.xml'.format(self._name)): + logging.debug("Parsing suite using legacy naming convention") + logging.debug(f"Filename {os.path.basename(self._sdf_name)}") + logging.debug(f"Suite name {format(self._name)}") + else: + logging.critical("Invalid suite name {0} in suite definition file {1}.".format( + self._name, self._sdf_name)) + success = False + return success # Check if suite name is too long if len(self._name) > SUITE_NAME_MAX_CHARS: diff --git a/test_prebuild/test_blocked_data/suite_blocked_data_suite.xml b/test_prebuild/test_blocked_data/blocked_data_suite.xml similarity index 100% rename from test_prebuild/test_blocked_data/suite_blocked_data_suite.xml rename to test_prebuild/test_blocked_data/blocked_data_suite.xml From f28313171643ce5a92eff32c86df2297ebfcacdb Mon Sep 17 00:00:00 2001 From: Courtney Peverley Date: Mon, 15 Jul 2024 08:50:19 -0600 Subject: [PATCH 2/6] update target branch for pull requests --- .github/workflows/capgen_unit_tests.yaml | 2 +- .github/workflows/python.yaml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/capgen_unit_tests.yaml b/.github/workflows/capgen_unit_tests.yaml index 441250c9..363d19e3 100644 --- a/.github/workflows/capgen_unit_tests.yaml +++ b/.github/workflows/capgen_unit_tests.yaml @@ -3,7 +3,7 @@ name: Capgen Unit Tests on: workflow_dispatch: pull_request: - branches: [feature/capgen, main] + branches: [develop, main] jobs: unit_tests: diff --git a/.github/workflows/python.yaml b/.github/workflows/python.yaml index c57461cb..791c5084 100644 --- a/.github/workflows/python.yaml +++ b/.github/workflows/python.yaml @@ -3,7 +3,7 @@ name: Python package on: workflow_dispatch: pull_request: - branches: [feature/capgen, main] + branches: [develop, main] jobs: build: From 5f338ddf02638c06548e54e0a218d154b34faff9 Mon Sep 17 00:00:00 2001 From: goldy <1588651+gold2718@users.noreply.github.com> Date: Fri, 19 Jul 2024 20:17:35 +0200 Subject: [PATCH 3/6] Use absolute paths for dependencies (#576) Use absolute paths for dependencies In order to make the dependencies in the CCPP datatable more usable, store absolute pathnames where possible. User interface changes?: Yes This may require a change in any build system that uses the dependencies. Fixes: #575 Testing: test removed: None unit tests: passed system tests: ? manual testing: ran Fortran tests --------- Co-authored-by: Steve Goldhaber --- scripts/metadata_table.py | 8 +++++++ test/capgen_test/run_test | 2 +- test/capgen_test/temp_adjust.meta | 2 ++ test/capgen_test/temp_calc_adjust.meta | 1 - test/unit_tests/test_metadata_table.py | 22 +++++++++++-------- .../unit_tests/test_metadata_parser.py | 2 +- 6 files changed, 25 insertions(+), 12 deletions(-) diff --git a/scripts/metadata_table.py b/scripts/metadata_table.py index d8b68000..c05fa5b4 100755 --- a/scripts/metadata_table.py +++ b/scripts/metadata_table.py @@ -348,6 +348,14 @@ def __init__(self, run_env, table_name_in=None, table_type_in=None, # end if self.__start_context = ParseContext(context=self.__pobj) self.__init_from_file(known_ddts, self.__run_env, skip_ddt_check=skip_ddt_check) + # Set absolute path for all dependencies + path = os.path.dirname(self.__pobj.filename) + if self.relative_path: + path = os.path.join(path, self.relative_path) + # end if + for ind, dep in enumerate(self.__dependencies): + self.__dependencies[ind] = os.path.abspath(os.path.join(path, dep)) + # end for # end if def __init_from_file(self, known_ddts, run_env, skip_ddt_check=False): diff --git a/test/capgen_test/run_test b/test/capgen_test/run_test index e5ac6e12..3d21e2c2 100755 --- a/test/capgen_test/run_test +++ b/test/capgen_test/run_test @@ -130,7 +130,7 @@ ccpp_files="${ccpp_files},${build_dir}/ccpp/ccpp_ddt_suite_cap.F90" ccpp_files="${ccpp_files},${build_dir}/ccpp/ccpp_temp_suite_cap.F90" process_list="adjusting=temp_calc_adjust,setter=temp_set" module_list="environ_conditions,make_ddt,setup_coeffs,temp_adjust,temp_calc_adjust,temp_set" -dependencies="bar.F90,foo.F90,qux.F90" +dependencies="${scriptdir}/adjust/qux.F90,${scriptdir}/bar.F90,${scriptdir}/foo.F90" suite_list="ddt_suite;temp_suite" required_vars_ddt="ccpp_error_code,ccpp_error_message,horizontal_dimension" required_vars_ddt="${required_vars_ddt},horizontal_loop_begin" diff --git a/test/capgen_test/temp_adjust.meta b/test/capgen_test/temp_adjust.meta index 02b5fa73..420e9112 100644 --- a/test/capgen_test/temp_adjust.meta +++ b/test/capgen_test/temp_adjust.meta @@ -1,6 +1,8 @@ [ccpp-table-properties] name = temp_adjust type = scheme + dependencies = qux.F90 + relative_path = adjust [ccpp-arg-table] name = temp_adjust_run type = scheme diff --git a/test/capgen_test/temp_calc_adjust.meta b/test/capgen_test/temp_calc_adjust.meta index 60a86865..437de934 100644 --- a/test/capgen_test/temp_calc_adjust.meta +++ b/test/capgen_test/temp_calc_adjust.meta @@ -2,7 +2,6 @@ name = temp_calc_adjust type = scheme dependencies = foo.F90, bar.F90 - dependencies = qux.F90 [ccpp-arg-table] name = temp_calc_adjust_run type = scheme diff --git a/test/unit_tests/test_metadata_table.py b/test/unit_tests/test_metadata_table.py index fe6154ad..08248150 100755 --- a/test/unit_tests/test_metadata_table.py +++ b/test/unit_tests/test_metadata_table.py @@ -15,9 +15,10 @@ import os import unittest -TEST_DIR = os.path.dirname(os.path.abspath(__file__)) -SCRIPTS_DIR = os.path.abspath(os.path.join(TEST_DIR, os.pardir, os.pardir, "scripts")) -SAMPLE_FILES_DIR = os.path.join(TEST_DIR, "sample_files") +UNIT_TEST_DIR = os.path.dirname(os.path.abspath(__file__)) +TEST_DIR = os.path.abspath(os.path.join(UNIT_TEST_DIR, os.pardir)) +SCRIPTS_DIR = os.path.abspath(os.path.join(TEST_DIR, os.pardir, "scripts")) +SAMPLE_FILES_DIR = os.path.join(UNIT_TEST_DIR, "sample_files") if not os.path.exists(SCRIPTS_DIR): raise ImportError("Cannot find scripts directory") @@ -374,11 +375,15 @@ def test_dependencies_rel_path(self): titles = [elem.table_name for elem in result] self.assertEqual(len(dependencies), 4) - self.assertIn('machine.F', dependencies, msg="Dependency 'machine.F' is expected but not found") - self.assertIn('physcons.F90', dependencies, msg="Dependency 'physcons.F90' is expected but not found") - self.assertIn('GFDL_parse_tracers.F90', dependencies, msg="Dependency 'GFDL_parse_tracers.F90' is expected but not found") - self.assertIn('rte-rrtmgp/rrtmgp/mo_gas_optics_rrtmgp.F90', dependencies, \ - msg="Header name 'rte-rrtmgp/rrtmgp/mo_gas_optics_rrtmgp.F90' is expected but not found") + phys_dir = os.path.join(TEST_DIR, "ccpp", "physics", "physics") + self.assertIn(os.path.join(phys_dir, 'machine.F'), dependencies, \ + msg="Dependency 'machine.F' is expected but not found") + self.assertIn(os.path.join(phys_dir, 'physcons.F90'), dependencies, \ + msg="Dependency 'physcons.F90' is expected but not found") + self.assertIn(os.path.join(phys_dir, 'GFDL_parse_tracers.F90'), dependencies, \ + msg="Dependency 'GFDL_parse_tracers.F90' is expected but not found") + self.assertIn(os.path.join(phys_dir, 'rte-rrtmgp/rrtmgp/mo_gas_optics_rrtmgp.F90'), dependencies, \ + msg="Header name 'rte-rrtmgp/rrtmgp/mo_gas_optics_rrtmgp.F90' is expected but not found") self.assertIn(rel_path, "../../ccpp/physics/physics") self.assertEqual(len(result), 1) @@ -399,4 +404,3 @@ def test_invalid_table_properties_type(self): if __name__ == "__main__": unittest.main() - diff --git a/test_prebuild/unit_tests/test_metadata_parser.py b/test_prebuild/unit_tests/test_metadata_parser.py index febc3d93..1d4b4819 100644 --- a/test_prebuild/unit_tests/test_metadata_parser.py +++ b/test_prebuild/unit_tests/test_metadata_parser.py @@ -45,7 +45,7 @@ def test_MetadataTable_parse_table(tmpdir): assert metadata_header.table_name == "" assert metadata_header.table_type == "scheme" assert metadata_header.relative_path == "path" - assert metadata_header.dependencies == ["a.f", "b.f"] + assert metadata_header.dependencies == [os.path.join(tmpdir, metadata_header.relative_path,"a.f"), os.path.join(tmpdir, metadata_header.relative_path,"b.f")] # check metadata section assert len(metadata_header.sections()) == 1 From 58308e4f6b42df7ed735c9287f31d9699739e53d Mon Sep 17 00:00:00 2001 From: Courtney Peverley Date: Wed, 7 Aug 2024 17:17:04 -0600 Subject: [PATCH 4/6] Fix for "none" units edge case (#583) Tiny bugfix for unit conversion edge case that was trying to convert "None" to "none" ## Description Convert "none" units to lowercase before comparing. User interface changes?: No Fixes: closes #567 Testing: - Updated var_compatability_test to include a "None" to "none" comparison Co-authored-by: Courtney Peverley --- scripts/var_props.py | 7 +++++++ test/var_compatibility_test/test_host.meta | 2 +- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/scripts/var_props.py b/scripts/var_props.py index 9a2fd7d0..53f39f5c 100755 --- a/scripts/var_props.py +++ b/scripts/var_props.py @@ -936,6 +936,13 @@ def __init__(self, var1_stdname, var1_type, var1_kind, var1_units, # end if # end if if self.__compat: + # Only "none" units are case-insensitive + if var1_units.lower() == 'none': + var1_units = 'none' + # end if + if var2_units.lower() == 'none': + var2_units = 'none' + # end if # Check units argument if var1_units != var2_units: self.__equiv = False diff --git a/test/var_compatibility_test/test_host.meta b/test/var_compatibility_test/test_host.meta index 5d861764..da71b182 100644 --- a/test/var_compatibility_test/test_host.meta +++ b/test/var_compatibility_test/test_host.meta @@ -26,7 +26,7 @@ [ errmsg ] standard_name = ccpp_error_message long_name = Error message for error handling in CCPP - units = none + units = None dimensions = () type = character kind = len=512 From af90b948b85b88649547b49d28ba98fd5cb8c4b1 Mon Sep 17 00:00:00 2001 From: Michael Kavulich Date: Thu, 15 Aug 2024 14:16:18 -0600 Subject: [PATCH 5/6] Fix for ccpp_track_variables.py: Need "recursive=True" for glob on .meta files (#581) Because of the change in directory structure for CCPP physics (https://github.com/ufs-community/ccpp-physics/pull/99), there are now `.meta` files at different levels in the directory tree. The `ccpp_track_variables.py` script needs the location of these `.meta` files as an input argument to the script, but the call to `glob.glob` in the script does not use the `recursive=True` argument, so even if the user passes in the argument `-m './physics/physics/**/'` (which should include all subdirectories), the call to `glob.glob` only searches one level. Our simple test case only has `.meta` files at a single directory level, so we never caught this issue. Simply adding the `recursive=True` argument fixes this issue. --- scripts/ccpp_track_variables.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/ccpp_track_variables.py b/scripts/ccpp_track_variables.py index 60d31c1e..64dfc518 100755 --- a/scripts/ccpp_track_variables.py +++ b/scripts/ccpp_track_variables.py @@ -69,7 +69,7 @@ def create_metadata_filename_dict(metapath): with that scheme""" metadata_dict = {} - scheme_filenames = glob.glob(os.path.join(metapath, "*.meta")) + scheme_filenames = glob.glob(os.path.join(metapath, "*.meta"), recursive=True) if not scheme_filenames: raise Exception(f'No files found in {metapath} with ".meta" extension') From 49a3c3fc221de993640ccc7b6f4a0efc8d0705a9 Mon Sep 17 00:00:00 2001 From: Courtney Peverley Date: Thu, 3 Oct 2024 09:36:34 -0600 Subject: [PATCH 6/6] Return full error message from xmllint (#586) Update call_command function to include stderr output in CCPPError message. Right now, when xmllint-ing is done via xml_tools.py (validate_xml_file), the output of the linter is not returned. This PR adds the stderr output (if any) to the returned error message. PR also decodes error messages for cleaner output. User interface changes?: No Testing: test removed: N/A unit tests: Added new doctest to test error message system tests: N/A manual testing: Ran with/parsed new error messages within CAM-SIMA --- scripts/parse_tools/xml_tools.py | 29 ++++++++++++++++++++++++++--- 1 file changed, 26 insertions(+), 3 deletions(-) diff --git a/scripts/parse_tools/xml_tools.py b/scripts/parse_tools/xml_tools.py index 0ff56b3a..164b169b 100644 --- a/scripts/parse_tools/xml_tools.py +++ b/scripts/parse_tools/xml_tools.py @@ -51,6 +51,21 @@ def call_command(commands, logger, silent=False): False >>> call_command(['ls'], _LOGGER) True + >>> try: + ... call_command(['ls','--invalid-option'], _LOGGER) + ... except CCPPError as e: + ... print(str(e)) + Execution of 'ls --invalid-option' failed with code: 2 + Error output: ls: unrecognized option '--invalid-option' + Try 'ls --help' for more information. + >>> try: + ... os.chdir(os.path.dirname(__file__)) + ... call_command(['ls', os.path.basename(__file__), 'foo.bar.baz'], _LOGGER) + ... except CCPPError as e: + ... print(str(e)) + Execution of 'ls xml_tools.py foo.bar.baz' failed with code: 2 + xml_tools.py + Error output: ls: cannot access 'foo.bar.baz': No such file or directory """ result = False outstr = '' @@ -66,9 +81,17 @@ def call_command(commands, logger, silent=False): result = False else: cmd = ' '.join(commands) - emsg = "Execution of '{}' failed with code:\n" - outstr = emsg.format(cmd, err.returncode) - outstr += "{}".format(err.output) + outstr = f"Execution of '{cmd}' failed with code: {err.returncode}\n" + outstr += f"{err.output.decode('utf-8', errors='replace').strip()}" + if hasattr(err, 'stderr') and err.stderr: + stderr_str = err.stderr.decode('utf-8', errors='replace').strip() + if stderr_str: + if err.output: + outstr += os.linesep + # end if + outstr += f"Error output: {stderr_str}" + # end if + # end if raise CCPPError(outstr) from err # end if # end of try