From 2e9c71a22e8fa11a7d6aa5d7ed5e5deeeed0862f Mon Sep 17 00:00:00 2001 From: Frode Aarstad Date: Wed, 18 Dec 2024 09:25:26 +0100 Subject: [PATCH] Add line numbers when validating everest config files --- src/everest/config/__init__.py | 3 +- src/everest/config/control_variable_config.py | 3 +- src/everest/config/everest_config.py | 63 +++++++++++++++++-- src/everest/config/validation_utils.py | 25 +------- .../functional/test_main_everest_entry.py | 11 +++- tests/everest/test_config_file_loader.py | 6 +- tests/everest/test_config_validation.py | 24 +++++++ tests/everest/test_res_initialization.py | 8 +-- tests/everest/test_yaml_parser.py | 5 +- 9 files changed, 107 insertions(+), 41 deletions(-) diff --git a/src/everest/config/__init__.py b/src/everest/config/__init__.py index 51c85ba9883..4ce9c72f7e5 100644 --- a/src/everest/config/__init__.py +++ b/src/everest/config/__init__.py @@ -5,7 +5,7 @@ ) from .cvar_config import CVaRConfig from .environment_config import EnvironmentConfig -from .everest_config import EverestConfig +from .everest_config import EverestConfig, EverestValidationError from .export_config import ExportConfig from .input_constraint_config import InputConstraintConfig from .install_data_config import InstallDataConfig @@ -28,6 +28,7 @@ "ControlVariableGuessListConfig", "EnvironmentConfig", "EverestConfig", + "EverestValidationError", "ExportConfig", "InputConstraintConfig", "InstallDataConfig", diff --git a/src/everest/config/control_variable_config.py b/src/everest/config/control_variable_config.py index a7896a9a60f..65f417a37f5 100644 --- a/src/everest/config/control_variable_config.py +++ b/src/everest/config/control_variable_config.py @@ -10,9 +10,8 @@ ) from ropt.enums import VariableType -from everest.config.validation_utils import no_dots_in_string, valid_range - from .sampler_config import SamplerConfig +from .validation_utils import no_dots_in_string, valid_range class _ControlVariable(BaseModel): diff --git a/src/everest/config/everest_config.py b/src/everest/config/everest_config.py index ce22b353649..505cd3e7789 100644 --- a/src/everest/config/everest_config.py +++ b/src/everest/config/everest_config.py @@ -3,6 +3,7 @@ import shutil from argparse import ArgumentParser from io import StringIO +from itertools import chain from pathlib import Path from typing import ( TYPE_CHECKING, @@ -35,7 +36,6 @@ check_for_duplicate_names, check_path_exists, check_writeable_path, - format_errors, unique_items, validate_forward_model_configs, ) @@ -96,6 +96,40 @@ def __setattr__(self, name, value): super().__setattr__(name, value) +class EverestValidationError(ValueError): + def __init__(self): + super().__init__() + self.errors: list[tuple[ErrorDetails, tuple[int, int] | None]] = [] + + @property + def error(self): + return self.errors + + def __str__(self): + return f"{self.errors!s}" + + +def _error_loc(error_dict: "ErrorDetails") -> str: + return " -> ".join( + str(e) for e in error_dict["loc"] if e is not None and e != "__root__" + ) + + +def format_errors(validation_error: EverestValidationError) -> str: + msg = f"Found {len(validation_error.errors)} validation error{'s' if len(validation_error.errors) > 1 else ''}:\n\n" + error_map = {} + for error, pos in validation_error.errors: + if pos: + row, col = pos + key = f"line: {row}, column: {col}. {_error_loc(error)}" + else: + key = f"{_error_loc(error)}" + if key not in error_map: + error_map[key] = [key] + error_map[key].append(f" * {error['msg']} (type={error['type']})") + return msg + "\n".join(list(chain.from_iterable(error_map.values()))) + + class HasName(Protocol): name: str @@ -753,14 +787,33 @@ def lint_config_dict_with_raise(config: dict): EverestConfig.model_validate(config) @staticmethod - def load_file(config_path: str) -> "EverestConfig": - config_path = os.path.realpath(config_path) + def load_file(config_file: str) -> "EverestConfig": + config_path = os.path.realpath(config_file) if not os.path.isfile(config_path): raise FileNotFoundError(f"File not found: {config_path}") config_dict = yaml_file_to_substituted_config_dict(config_path) - return EverestConfig.model_validate(config_dict) + try: + return EverestConfig.model_validate(config_dict) + except ValidationError as error: + exp = EverestValidationError() + file_content = [] + with open(config_path, encoding="utf-8") as f: + file_content = f.readlines() + + for e in error.errors( + include_context=True, include_input=True, include_url=False + ): + if e["type"] == "literal_error": + for index, line in enumerate(file_content): + if (pos := line.find(e["input"])) != -1: + exp.errors.append((e, (index + 1, pos + 1))) + break + else: + exp.errors.append((e, None)) + + raise exp from error @staticmethod def load_file_with_argparser( @@ -775,7 +828,7 @@ def load_file_with_argparser( f"The config file: <{config_path}> contains" f" invalid YAML syntax: {e!s}" ) - except ValidationError as e: + except EverestValidationError as e: parser.error( f"Loading config file <{config_path}> failed with:\n" f"{format_errors(e)}" diff --git a/src/everest/config/validation_utils.py b/src/everest/config/validation_utils.py index 3359bf20600..db92bd9abc2 100644 --- a/src/everest/config/validation_utils.py +++ b/src/everest/config/validation_utils.py @@ -3,11 +3,10 @@ import tempfile from collections import Counter from collections.abc import Sequence -from itertools import chain from pathlib import Path -from typing import TYPE_CHECKING, TypeVar +from typing import TypeVar -from pydantic import BaseModel, ValidationError +from pydantic import BaseModel from everest.config.install_data_config import InstallDataConfig from everest.util.forward_models import ( @@ -18,8 +17,6 @@ from .install_job_config import InstallJobConfig -if TYPE_CHECKING: - from pydantic_core import ErrorDetails _VARIABLE_ERROR_MESSAGE = ( "Variable {name} must define {variable_type} value either" " at control level or variable level" @@ -247,24 +244,6 @@ def check_writeable_path(path_source: str, config_path: Path): raise ValueError(f"User does not have write access to {path}") -def _error_loc(error_dict: "ErrorDetails") -> str: - return " -> ".join( - str(e) for e in error_dict["loc"] if e is not None and e != "__root__" - ) - - -def format_errors(error: ValidationError) -> str: - errors = error.errors() - msg = f"Found {len(errors)} validation error{'s' if len(errors) > 1 else ''}:\n\n" - error_map = {} - for err in error.errors(): - key = _error_loc(err) - if key not in error_map: - error_map[key] = [key] - error_map[key].append(f" * {err['msg']} (type={err['type']})") - return msg + "\n".join(list(chain.from_iterable(error_map.values()))) - - def validate_forward_model_configs( forward_model: list[str] | None, install_jobs: list[InstallJobConfig] | None ): diff --git a/tests/everest/functional/test_main_everest_entry.py b/tests/everest/functional/test_main_everest_entry.py index a185c1469bd..437f4d46f1f 100644 --- a/tests/everest/functional/test_main_everest_entry.py +++ b/tests/everest/functional/test_main_everest_entry.py @@ -20,6 +20,7 @@ pytestmark = pytest.mark.xdist_group(name="starts_everest") +@pytest.mark.integration_test def test_everest_entry_docs(): """Test calling everest with --docs @@ -40,6 +41,7 @@ def test_everest_entry_docs(): assert not err.getvalue() +@pytest.mark.integration_test def test_everest_entry_manual(): """Test calling everest with --manual""" with capture_streams() as (out, err), pytest.raises(SystemExit): @@ -55,6 +57,7 @@ def test_everest_entry_manual(): assert not err.getvalue() +@pytest.mark.integration_test def test_everest_entry_version(): """Test calling everest with --version""" with capture_streams() as (out, err), pytest.raises(SystemExit): @@ -64,6 +67,7 @@ def test_everest_entry_version(): assert any(everest_version in channel for channel in channels) +@pytest.mark.integration_test def test_everest_main_entry_bad_command(): # Setup command line arguments for the test with capture_streams() as (_, err), pytest.raises(SystemExit): @@ -76,6 +80,7 @@ def test_everest_main_entry_bad_command(): @pytest.mark.flaky(reruns=5) @pytest.mark.fails_on_macos_github_workflow +@pytest.mark.integration_test def test_everest_entry_run(copy_math_func_test_data_to_tmp): # Setup command line arguments with capture_streams(): @@ -108,6 +113,7 @@ def test_everest_entry_run(copy_math_func_test_data_to_tmp): assert status["status"] == ServerStatus.completed +@pytest.mark.integration_test def test_everest_entry_monitor_no_run(copy_math_func_test_data_to_tmp): with capture_streams(): start_everest(["everest", "monitor", CONFIG_FILE_MINIMAL]) @@ -120,6 +126,7 @@ def test_everest_entry_monitor_no_run(copy_math_func_test_data_to_tmp): assert status["status"] == ServerStatus.never_run +@pytest.mark.integration_test def test_everest_main_export_entry(copy_math_func_test_data_to_tmp): # Setup command line arguments with capture_streams(): @@ -127,6 +134,7 @@ def test_everest_main_export_entry(copy_math_func_test_data_to_tmp): assert os.path.exists(os.path.join("everest_output", "config_minimal.csv")) +@pytest.mark.integration_test def test_everest_main_lint_entry(copy_math_func_test_data_to_tmp): # Setup command line arguments with capture_streams() as (out, err): @@ -149,7 +157,7 @@ def test_everest_main_lint_entry(copy_math_func_test_data_to_tmp): type_ = "(type=float_parsing)" validation_msg = dedent( f"""Loading config file failed with: -Found 1 validation error: +Found 1 validation error: controls -> 0 -> initial_guess * Input should be a valid number, unable to parse string as a number {type_} @@ -161,6 +169,7 @@ def test_everest_main_lint_entry(copy_math_func_test_data_to_tmp): @pytest.mark.fails_on_macos_github_workflow @skipif_no_everest_models @pytest.mark.everest_models_test +@pytest.mark.integration_test def test_everest_main_configdump_entry(copy_egg_test_data_to_tmp): # Setup command line arguments with capture_streams() as (out, _): diff --git a/tests/everest/test_config_file_loader.py b/tests/everest/test_config_file_loader.py index 0544aaba712..e366d293a36 100644 --- a/tests/everest/test_config_file_loader.py +++ b/tests/everest/test_config_file_loader.py @@ -3,12 +3,12 @@ from unittest.mock import patch import pytest -from pydantic_core import ValidationError from ruamel.yaml import YAML from everest import ConfigKeys as CK from everest import config_file_loader as loader from everest.config import EverestConfig +from everest.config.everest_config import EverestValidationError from tests.everest.utils import relpath mocked_root = relpath(os.path.join("test_data", "mocked_test_case")) @@ -122,12 +122,12 @@ def test_dependent_definitions_value_error(copy_mocked_test_data_to_tmp): def test_load_empty_configuration(copy_mocked_test_data_to_tmp): with open("empty_config.yml", mode="w", encoding="utf-8") as fh: fh.writelines("") - with pytest.raises(ValidationError, match="missing"): + with pytest.raises(EverestValidationError, match="missing"): EverestConfig.load_file("empty_config.yml") def test_load_invalid_configuration(copy_mocked_test_data_to_tmp): with open("invalid_config.yml", mode="w", encoding="utf-8") as fh: fh.writelines("asdf") - with pytest.raises(ValidationError, match="missing"): + with pytest.raises(EverestValidationError, match="missing"): EverestConfig.load_file("invalid_config.yml") diff --git a/tests/everest/test_config_validation.py b/tests/everest/test_config_validation.py index 2acb73af8b7..7559c259b46 100644 --- a/tests/everest/test_config_validation.py +++ b/tests/everest/test_config_validation.py @@ -2,6 +2,7 @@ import pathlib import re import warnings +from argparse import ArgumentParser from pathlib import Path from typing import Any @@ -973,3 +974,26 @@ def test_warning_forward_model_write_objectives(objective, forward_model, warnin def test_deprecated_keyword(): with pytest.warns(ConfigWarning, match="report_steps .* can be removed"): ModelConfig(**{"report_steps": []}) + + +def test_load_file_non_existing(): + with pytest.raises(FileNotFoundError): + EverestConfig.load_file("non_existing.yml") + + +def test_load_file_with_errors(copy_math_func_test_data_to_tmp, capsys): + with open("config_minimal.yml", encoding="utf-8") as file: + content = file.read() + + with open("config_minimal_error.yml", "w", encoding="utf-8") as file: + file.write(content.replace("generic_control", "yolo_control")) + + with pytest.raises(SystemExit): + parser = ArgumentParser(prog="test") + EverestConfig.load_file_with_argparser("config_minimal_error.yml", parser) + + captured = capsys.readouterr() + + assert "Found 1 validation error" in captured.err + assert "line: 4, column: 11" in captured.err + assert "Input should be 'well_control' or 'generic_control'" in captured.err diff --git a/tests/everest/test_res_initialization.py b/tests/everest/test_res_initialization.py index 259a0d19894..bc7e989971d 100644 --- a/tests/everest/test_res_initialization.py +++ b/tests/everest/test_res_initialization.py @@ -11,7 +11,7 @@ from ert.config import ExtParamConfig from ert.config.parsing import ConfigKeys as ErtConfigKeys from everest import ConfigKeys as CK -from everest.config import EverestConfig +from everest.config import EverestConfig, EverestValidationError from everest.simulator.everest_to_ert import ( _everest_to_ert_config_dict, everest_to_ert_config, @@ -278,7 +278,7 @@ def test_summary_default_no_opm(copy_egg_test_data_to_tmp): ), ( {"source": None, "link": True, "target": "bar.json"}, - "Input should be a valid string [type=string_type, input_value=None, input_type=NoneType]", + "Input should be a valid string", ), ( {"source": "", "link": "false", "target": "bar.json"}, @@ -286,7 +286,7 @@ def test_summary_default_no_opm(copy_egg_test_data_to_tmp): ), ( {"source": "baz/", "link": True, "target": 3}, - "Input should be a valid string [type=string_type, input_value=3, input_type=int]", + "Input should be a valid string", ), ], ) @@ -313,7 +313,7 @@ def test_install_data_with_invalid_templates( yaml.default_flow_style = False yaml.dump(raw_config, f) - with pytest.raises(ValueError) as exc_info: + with pytest.raises(EverestValidationError) as exc_info: EverestConfig.load_file(config_file) assert expected_error_msg in str(exc_info.value) diff --git a/tests/everest/test_yaml_parser.py b/tests/everest/test_yaml_parser.py index 436a594c0b3..33017dcd801 100644 --- a/tests/everest/test_yaml_parser.py +++ b/tests/everest/test_yaml_parser.py @@ -12,7 +12,8 @@ @pytest.mark.parametrize("random_seed", [None, 1234]) -def test_random_seed(random_seed): +def test_random_seed(tmp_path, monkeypatch, random_seed): + monkeypatch.chdir(tmp_path) config = {"model": {"realizations": [0]}} if random_seed: config["environment"] = {"random_seed": random_seed} @@ -128,7 +129,7 @@ def test_invalid_forward_model_config_files(copy_test_data_to_tmp, monkeypatch): config_file = "valid_config_maintained_forward_models.yml" template_path = "./templates/wellopen.jinja" assert f"""Loading config file <{config_file}> failed with: -Found 1 validation error: +Found 1 validation error: * Value error, job = 'add_templates'\t-c/--config = {template_config_path}