From 0029e24a55912cb5a6c9a044fb7cb950564ad4bf Mon Sep 17 00:00:00 2001 From: James Frost Date: Mon, 19 Jun 2023 16:27:48 +0100 Subject: [PATCH 1/7] Allow combine constraint operator to ignore non-constraints as input --- src/CSET/operators/constraints.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/CSET/operators/constraints.py b/src/CSET/operators/constraints.py index 5d00b4fd0..875c7d620 100644 --- a/src/CSET/operators/constraints.py +++ b/src/CSET/operators/constraints.py @@ -119,7 +119,7 @@ def generate_time_constraint( def combine_constraints( - constraint: iris.Constraint = iris.Constraint(), **kwargs + constraint: iris.Constraint = None, **kwargs ) -> iris.Constraint: """ Operator that combines multiple constraints into one. @@ -144,8 +144,13 @@ def combine_constraints( TypeError If the provided arguments are not constraints. """ + # If the first argument is not a constraint, it is ignored. This handles the + # automatic passing of the previous step's output. + if type(constraint) == iris.Constraint: + combined_constraint = constraint + else: + combined_constraint = iris.Constraint() - combined_constraint = constraint for constr in kwargs.values(): combined_constraint = combined_constraint & constr return combined_constraint From de2a54d2f69948f1852d217886a05dad6d36a1e9 Mon Sep 17 00:00:00 2001 From: James Frost Date: Mon, 19 Jun 2023 16:29:18 +0100 Subject: [PATCH 2/7] Better error messages from execute_recipe function --- src/CSET/operators/_internal.py | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/src/CSET/operators/_internal.py b/src/CSET/operators/_internal.py index 0ce086f2f..d4d064c34 100644 --- a/src/CSET/operators/_internal.py +++ b/src/CSET/operators/_internal.py @@ -92,6 +92,7 @@ def execute_recipe( """ def step_parser(step: dict, step_input: any, output_file_path: Path) -> str: + """Executes a recipe step, recursively executing any sub-steps.""" logging.debug(f"Executing step: {step}") kwargs = {} for key in step.keys(): @@ -105,10 +106,8 @@ def step_parser(step: dict, step_input: any, output_file_path: Path) -> str: kwargs[key] = output_file_path else: kwargs[key] = step[key] - logging.debug(f"args: {kwargs}") logging.debug(f"step_input: {step_input}") - # If first argument of operator is explicitly defined, use that rather # than step_input. This is known through introspection of the operator. first_arg = next(iter(inspect.signature(operator).parameters.keys())) @@ -126,18 +125,25 @@ def step_parser(step: dict, step_input: any, output_file_path: Path) -> str: except ruamel.yaml.error.YAMLStreamError: raise TypeError("Must provide a stream (with a read method)") + # Checking that the recipe actually has some steps, and providing helpful + # error messages otherwise. logging.debug(recipe) - step_input = input_file try: if len(recipe["steps"]) < 1: raise ValueError("Recipe must have at least 1 step.") - for step in recipe["steps"]: - step_input = step_parser(step, step_input, output_file) except KeyError as err: raise ValueError("Invalid Recipe:", err) except TypeError as err: if recipe is None: raise ValueError("Recipe must have at least 1 step.") + if type(recipe) != dict: + raise ValueError("Recipe must either be YAML, or a Path.") # This should never be reached. raise err # pragma: no cover + + # Execute the recipe. + step_input = input_file + for step in recipe["steps"]: + step_input = step_parser(step, step_input, output_file) + logging.info(f"Recipe output: {step_input}") From 695cedad732fd84d0cfa5374f619761dfe0236bc Mon Sep 17 00:00:00 2001 From: James Frost Date: Mon, 19 Jun 2023 16:36:37 +0100 Subject: [PATCH 3/7] Rename constraint argument to be meaningful --- src/CSET/operators/RECIPES/extract_instant_air_temp.yaml | 2 +- tests/test_data/noop_recipe.yaml | 2 ++ tests/test_data/plot_instant_air_temp.yaml | 2 +- 3 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/CSET/operators/RECIPES/extract_instant_air_temp.yaml b/src/CSET/operators/RECIPES/extract_instant_air_temp.yaml index 96f208707..fa82c0ecf 100644 --- a/src/CSET/operators/RECIPES/extract_instant_air_temp.yaml +++ b/src/CSET/operators/RECIPES/extract_instant_air_temp.yaml @@ -17,7 +17,7 @@ steps: - operator: filters.filter_cubes constraint: operator: constraints.combine_constraints - constraint: + stash_constraint: operator: constraints.generate_stash_constraint stash: m01s03i236 cell_methods_constraint: diff --git a/tests/test_data/noop_recipe.yaml b/tests/test_data/noop_recipe.yaml index 221c5b477..859f988d3 100644 --- a/tests/test_data/noop_recipe.yaml +++ b/tests/test_data/noop_recipe.yaml @@ -5,3 +5,5 @@ steps: - operator: misc.noop test_argument: Banana dict_argument: {"key": "value"} + substep: + operator: constraints.combine_constraints diff --git a/tests/test_data/plot_instant_air_temp.yaml b/tests/test_data/plot_instant_air_temp.yaml index 2ab306317..b9b32790f 100644 --- a/tests/test_data/plot_instant_air_temp.yaml +++ b/tests/test_data/plot_instant_air_temp.yaml @@ -12,7 +12,7 @@ steps: - operator: filters.filter_cubes constraint: operator: constraints.combine_constraints - constraint: + stash_constraint: operator: constraints.generate_stash_constraint stash: m01s03i236 cell_methods_constraint: From b73df3d6081f040f2bf8fb815c5e39aac6f10206 Mon Sep 17 00:00:00 2001 From: James Frost Date: Mon, 19 Jun 2023 17:03:52 +0100 Subject: [PATCH 4/7] Allow operators to take arbitrary arguments This is needed to allow adding dummy arguments that only exist to chain other operators to run as sub-steps. An example of this is when wanting to produce output both as a NetCDF file, and a plot. --- src/CSET/operators/constraints.py | 8 ++++---- src/CSET/operators/filters.py | 2 +- src/CSET/operators/plot.py | 2 +- src/CSET/operators/read.py | 2 +- src/CSET/operators/write.py | 2 +- 5 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/CSET/operators/constraints.py b/src/CSET/operators/constraints.py index 875c7d620..8c83e261e 100644 --- a/src/CSET/operators/constraints.py +++ b/src/CSET/operators/constraints.py @@ -21,7 +21,7 @@ from datetime import datetime -def generate_stash_constraint(stash: str) -> iris.AttributeConstraint: +def generate_stash_constraint(stash: str, **kwargs) -> iris.AttributeConstraint: """ Operator that takes a stash string, and uses iris to generate a constraint to be passed into the read operator to minimize the CubeList the read @@ -43,7 +43,7 @@ def generate_stash_constraint(stash: str) -> iris.AttributeConstraint: return stash_constraint -def generate_var_constraint(varname: str) -> iris.Constraint: +def generate_var_constraint(varname: str, **kwargs) -> iris.Constraint: """ Operator that takes a CF compliant variable name string, and uses iris to generate a constraint to be passed into the read operator to minimize the @@ -63,7 +63,7 @@ def generate_var_constraint(varname: str) -> iris.Constraint: return varname_constraint -def generate_cell_methods_constraint(cell_methods: list) -> iris.Constraint: +def generate_cell_methods_constraint(cell_methods: list, **kwargs) -> iris.Constraint: """ Operator that takes a list of cell methods and generates a constraint from that. @@ -89,7 +89,7 @@ def check_cell_methods(cube: iris.cube.Cube): def generate_time_constraint( - time_start: str, time_end: str = None + time_start: str, time_end: str = None, **kwargs ) -> iris.AttributeConstraint: """ Operator that takes one or two ISO 8601 date strings, and returns a diff --git a/src/CSET/operators/filters.py b/src/CSET/operators/filters.py index 1ab919614..03bbf957a 100644 --- a/src/CSET/operators/filters.py +++ b/src/CSET/operators/filters.py @@ -21,7 +21,7 @@ def filter_cubes( - cubelist: iris.cube.CubeList, constraint: iris.Constraint + cubelist: iris.cube.CubeList, constraint: iris.Constraint, **kwargs ) -> iris.cube.Cube: """ Filters a cubelist down to a single cube based on a constraint. diff --git a/src/CSET/operators/plot.py b/src/CSET/operators/plot.py index 06b59a05e..adf824e00 100644 --- a/src/CSET/operators/plot.py +++ b/src/CSET/operators/plot.py @@ -23,7 +23,7 @@ import matplotlib.pyplot as plt -def spatial_contour_plot(cube: iris.cube.Cube, file_path: Path) -> Path: +def spatial_contour_plot(cube: iris.cube.Cube, file_path: Path, **kwargs) -> Path: """ Plots a spatial variable onto a map. diff --git a/src/CSET/operators/read.py b/src/CSET/operators/read.py index 333ec0813..3605aee6c 100644 --- a/src/CSET/operators/read.py +++ b/src/CSET/operators/read.py @@ -23,7 +23,7 @@ def read_cubes( - loadpath: Path, constraint: iris.Constraint = None + loadpath: Path, constraint: iris.Constraint = None, **kwargs ) -> iris.cube.CubeList: """ Read operator that takes a path string (can include wildcards), and uses diff --git a/src/CSET/operators/write.py b/src/CSET/operators/write.py index 038d96790..d1e473317 100644 --- a/src/CSET/operators/write.py +++ b/src/CSET/operators/write.py @@ -24,7 +24,7 @@ def write_cube_to_nc( - cube: Union[iris.cube.Cube, iris.cube.CubeList], file_path: Path + cube: Union[iris.cube.Cube, iris.cube.CubeList], file_path: Path, **kwargs ) -> str: """ A write operator that sits after the read operator. This operator expects From 017978f8c1fbef232995b12e34784c87819a28ff Mon Sep 17 00:00:00 2001 From: James Frost Date: Mon, 19 Jun 2023 17:13:19 +0100 Subject: [PATCH 5/7] Accept any subclass of Constraint in check --- src/CSET/operators/constraints.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/CSET/operators/constraints.py b/src/CSET/operators/constraints.py index 8c83e261e..5b6db30aa 100644 --- a/src/CSET/operators/constraints.py +++ b/src/CSET/operators/constraints.py @@ -146,7 +146,7 @@ def combine_constraints( """ # If the first argument is not a constraint, it is ignored. This handles the # automatic passing of the previous step's output. - if type(constraint) == iris.Constraint: + if isinstance(constraint, iris.Constraint): combined_constraint = constraint else: combined_constraint = iris.Constraint() From 7804faa182ebc2711d847ef28f7cf732e898c00a Mon Sep 17 00:00:00 2001 From: James Frost Date: Mon, 19 Jun 2023 17:14:19 +0100 Subject: [PATCH 6/7] Produce multiple outputs in test recipe This more closely matches the actual usage, and tests sub-step behaviour. --- tests/test_data/plot_instant_air_temp.yaml | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/test_data/plot_instant_air_temp.yaml b/tests/test_data/plot_instant_air_temp.yaml index b9b32790f..3aa635d82 100644 --- a/tests/test_data/plot_instant_air_temp.yaml +++ b/tests/test_data/plot_instant_air_temp.yaml @@ -23,6 +23,9 @@ steps: time_start: 2022-09-21T03:00:00 time_end: 2022-09-21T03:30:00 - - operator: plot.spatial_contour_plot + - operator: write.write_cube_to_nc # This is a magic value that becomes the runtime output file path. file_path: CSET_OUTPUT_PATH + plot_data_as_well: + operator: plot.spatial_contour_plot + file_path: CSET_OUTPUT_PATH From 686c3bac4cfddcac98ab7f5f10cd3d56f123fdc4 Mon Sep 17 00:00:00 2001 From: James Frost Date: Mon, 19 Jun 2023 17:21:47 +0100 Subject: [PATCH 7/7] Test for recipe that doesn't parse to a dict --- tests/test_recipe_parser.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/tests/test_recipe_parser.py b/tests/test_recipe_parser.py index 9fd4bcada..699d02382 100644 --- a/tests/test_recipe_parser.py +++ b/tests/test_recipe_parser.py @@ -109,6 +109,14 @@ def test_execute_recipe(): exception_happened = True assert exception_happened + # Test exception for recipe that parses to a non-dict. + exception_happened = False + try: + internal.execute_recipe("[]", os.devnull, os.devnull) + except ValueError: + exception_happened = True + assert exception_happened + # Test happy case (this is really an integration test). output_file = Path(f"{tempfile.gettempdir()}/{uuid4()}.nc") recipe_file = RECIPES.extract_instant_air_temp