Fix error message in variable definition check (#2313)

Co-authored-by: Bouwe Andela <[email protected]>

.zenodo.json

@@ -178,6 +178,11 @@
             "name": "Kazeroni, Rémi",
             "orcid": "0000-0001-7205-9528"
         },
+        {
+            "affiliation": "GEOMAR, Germany",
+            "name": "Hohn, David",
+            "orcid": "0000-0002-5317-1247"
+        },
         {
             "affiliation": "DLR, Germany",
             "name": "Bauer, Julian"
@@ -190,6 +195,11 @@
         {
             "affiliation": "Forschungszentrum Juelich, Germany",
             "name": "Benke, Joerg"
+        },
+        {
+            "affiliation": "BSC, Spain",
+            "name": "Martin-Martinez, Eneko",
+            "orcid": "0000-0002-9213-7818"
         }
     ],
     "description": "ESMValCore: A community tool for pre-processing data from Earth system models in CMIP and running analysis scripts.",

CITATION.cff

@@ -199,6 +199,11 @@ authors:
     affiliation: "Forschungszentrum Juelich (FZJ), Germany"
     family-names: Benke
     given-names: Joerg
+  -
+    affiliation: "BSC, Spain"
+    family-names: Martin-Martinez
+    given-names: Eneko
+    orcid: "https://orcid.org/0000-0002-9213-7818"
 
 cff-version: 1.2.0
 date-released: 2023-12-19

esmvalcore/_recipe/check.py

@@ -70,21 +70,21 @@ def diagnostics(diags):
     for name, diagnostic in diags.items():
         if 'scripts' not in diagnostic:
             raise RecipeError(
-                "Missing scripts section in diagnostic {}".format(name))
+                f"Missing scripts section in diagnostic '{name}'.")
         variable_names = tuple(diagnostic.get('variables', {}))
         scripts = diagnostic.get('scripts')
         if scripts is None:
             scripts = {}
         for script_name, script in scripts.items():
             if script_name in variable_names:
                 raise RecipeError(
-                    "Invalid script name {} encountered in diagnostic {}: "
-                    "scripts cannot have the same name as variables.".format(
-                        script_name, name))
+                    f"Invalid script name '{script_name}' encountered "
+                    f"in diagnostic '{name}': scripts cannot have the "
+                    "same name as variables.")
             if not script.get('script'):
                 raise RecipeError(
-                    "No script defined for script {} in diagnostic {}".format(
-                        script_name, name))
+                    f"No script defined for script '{script_name}' in "
+                    f"diagnostic '{name}'.")
 
 
 def duplicate_datasets(
@@ -95,27 +95,31 @@ def duplicate_datasets(
     """Check for duplicate datasets."""
     if not datasets:
         raise RecipeError(
-            "You have not specified any dataset or additional_dataset groups "
-            f"for variable {variable_group} in diagnostic {diagnostic}.")
+            "You have not specified any dataset or additional_dataset "
+            f"groups for variable '{variable_group}' in diagnostic "
+            f"'{diagnostic}'.")
     checked_datasets_ = []
     for dataset in datasets:
         if dataset in checked_datasets_:
             raise RecipeError(
-                f"Duplicate dataset {dataset} for variable {variable_group} "
-                f"in diagnostic {diagnostic}.")
+                f"Duplicate dataset\n{pformat(dataset)}\nfor variable "
+                f"'{variable_group}' in diagnostic '{diagnostic}'.")
         checked_datasets_.append(dataset)
 
 
-def variable(var: dict[str, Any], required_keys: Iterable[str]):
+def variable(
+    var: dict[str, Any],
+    required_keys: Iterable[str],
+    diagnostic: str,
+    variable_group: str
+) -> None:
     """Check variables as derived from recipe."""
     required = set(required_keys)
     missing = required - set(var)
     if missing:
         raise RecipeError(
-            f"Missing keys {missing} in\n"
-            f"{pformat(var)}\n"
-            "for variable {var['variable_group']} in diagnostic "
-            f"{var['diagnostic']}")
+            f"Missing keys {missing} in\n{pformat(var)}\nfor variable "
+            f"'{variable_group}' in diagnostic '{diagnostic}'.")
 
 
 def _log_data_availability_errors(dataset):

esmvalcore/_recipe/recipe_schema.yml

@@ -46,7 +46,7 @@ variable:
 # TODO: add preprocessor item
 
 diagnostic:
-  scripts: any(null(), map(include('script')))
+  scripts: any(null(), map(include('script')), required=False)
   additional_datasets: list(include('dataset'), required=False)
   title: str(required=False)
   description: str(required=False)
@@ -55,4 +55,4 @@ diagnostic:
   variables: map(include('variable'), null(), required=False)
 
 script:
-  script: str()
+  script: str(required=False)

esmvalcore/_recipe/to_datasets.py

@@ -244,6 +244,7 @@ def _get_dataset_facets_from_recipe(
     recipe_variable: dict[str, Any],
     recipe_dataset: dict[str, Any],
     profiles: dict[str, Any],
+    diagnostic_name: str,
     session: Session,
 ) -> tuple[Facets, list[Facets]]:
     """Read the facets for a single dataset definition from the recipe."""
@@ -286,6 +287,8 @@ def _get_dataset_facets_from_recipe(
             'dataset',
             'project',
         ),
+        diagnostic=diagnostic_name,
+        variable_group=variable_group
     )
 
     preprocessor = facets.get('preprocessor', 'default')
@@ -329,6 +332,7 @@ def _get_facets_from_recipe(
             recipe_variable=recipe_variable,
             recipe_dataset=recipe_dataset,
             profiles=profiles,
+            diagnostic_name=diagnostic_name,
             session=session,
         )
 

tests/integration/recipe/test_recipe.py

@@ -165,6 +165,80 @@ def get_recipe(tempdir: Path, content: str, session: Session):
     return recipe
 
 
+def test_recipe_missing_scripts(tmp_path, session):
+    content = dedent("""
+        datasets:
+          - dataset: bcc-csm1-1
+
+        diagnostics:
+          diagnostic_name:
+            variables:
+              ta:
+                project: CMIP5
+                mip: Amon
+                exp: historical
+                ensemble: r1i1p1
+                timerange: 1999/2002
+        """)
+    exc_message = ("Missing scripts section in diagnostic 'diagnostic_name'.")
+    with pytest.raises(RecipeError) as exc:
+        get_recipe(tmp_path, content, session)
+    assert str(exc.value) == exc_message
+
+
+def test_recipe_duplicate_var_script_name(tmp_path, session):
+    content = dedent("""
+        datasets:
+          - dataset: bcc-csm1-1
+
+        diagnostics:
+          diagnostic_name:
+            variables:
+              ta:
+                project: CMIP5
+                mip: Amon
+                exp: historical
+                ensemble: r1i1p1
+                start_year: 1999
+                end_year: 2002
+            scripts:
+              ta:
+                script: tmp_path / 'diagnostic.py'
+        """)
+    exc_message = ("Invalid script name 'ta' encountered in diagnostic "
+                   "'diagnostic_name': scripts cannot have the same "
+                   "name as variables.")
+    with pytest.raises(RecipeError) as exc:
+        get_recipe(tmp_path, content, session)
+    assert str(exc.value) == exc_message
+
+
+def test_recipe_no_script(tmp_path, session):
+    content = dedent("""
+        datasets:
+          - dataset: bcc-csm1-1
+
+        diagnostics:
+          diagnostic_name:
+            variables:
+              ta:
+                project: CMIP5
+                mip: Amon
+                exp: historical
+                ensemble: r1i1p1
+                start_year: 1999
+                end_year: 2002
+            scripts:
+              script_name:
+                argument: 1
+        """)
+    exc_message = ("No script defined for script 'script_name' in "
+                   "diagnostic 'diagnostic_name'.")
+    with pytest.raises(RecipeError) as exc:
+        get_recipe(tmp_path, content, session)
+    assert str(exc.value) == exc_message
+
+
 def test_recipe_no_datasets(tmp_path, session):
     content = dedent("""
         diagnostics:
@@ -181,7 +255,56 @@ def test_recipe_no_datasets(tmp_path, session):
         """)
     exc_message = ("You have not specified any dataset "
                    "or additional_dataset groups for variable "
-                   "ta in diagnostic diagnostic_name.")
+                   "'ta' in diagnostic 'diagnostic_name'.")
+    with pytest.raises(RecipeError) as exc:
+        get_recipe(tmp_path, content, session)
+    assert str(exc.value) == exc_message
+
+
+def test_recipe_duplicated_datasets(tmp_path, session):
+    content = dedent("""
+        datasets:
+          - dataset: bcc-csm1-1
+          - dataset: bcc-csm1-1
+
+        diagnostics:
+          diagnostic_name:
+            variables:
+              ta:
+                project: CMIP5
+                mip: Amon
+                exp: historical
+                ensemble: r1i1p1
+                timerange: 1999/2002
+            scripts: null
+        """)
+    exc_message = ("Duplicate dataset\n{'dataset': 'bcc-csm1-1'}\n"
+                   "for variable 'ta' in diagnostic 'diagnostic_name'.")
+    with pytest.raises(RecipeError) as exc:
+        get_recipe(tmp_path, content, session)
+    assert str(exc.value) == exc_message
+
+
+def test_recipe_var_missing_args(tmp_path, session):
+    content = dedent("""
+        datasets:
+          - dataset: bcc-csm1-1
+
+        diagnostics:
+          diagnostic_name:
+            variables:
+              ta:
+                project: CMIP5
+                exp: historical
+                ensemble: r1i1p1
+                timerange: 1999/2002
+            scripts: null
+        """)
+    exc_message = ("Missing keys {'mip'} in\n{'dataset': 'bcc-csm1-1',"
+                   "\n 'ensemble': 'r1i1p1',\n 'exp': 'historical',\n"
+                   " 'project': 'CMIP5',\n 'short_name': 'ta',\n "
+                   "'timerange': '1999/2002'}\nfor variable 'ta' "
+                   "in diagnostic 'diagnostic_name'.")
     with pytest.raises(RecipeError) as exc:
         get_recipe(tmp_path, content, session)
     assert str(exc.value) == exc_message