From bc246ad2ddd22bf5edd6fd192923c93156aef98e 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/everest_config.py | 47 +++++++++++++++++-- src/everest/config/validation_utils.py | 27 ++++++++--- .../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_yaml_parser.py | 3 +- 6 files changed, 103 insertions(+), 15 deletions(-) diff --git a/src/everest/config/everest_config.py b/src/everest/config/everest_config.py index ce22b353649..34c746908b8 100644 --- a/src/everest/config/everest_config.py +++ b/src/everest/config/everest_config.py @@ -96,6 +96,20 @@ def __setattr__(self, name, value): super().__setattr__(name, value) +class EverestValidationError(ValueError): + def __init__(self): + super().__init__() + # self.errors: dict[tuple[int, int], ErrorDetails] = {} + self.errors: list[tuple[ErrorDetails, tuple[int, int] | None]] = [] + + @property + def error(self): + return self.errors + + def __str__(self): + return f"{self.errors!s}" + + class HasName(Protocol): name: str @@ -753,14 +767,39 @@ 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 + # elif e["type"] == "missing": + # exp.errors.append((e, None)) + + # elif e["type"] == "value_error": + # exp.errors.append((e, None)) + else: + exp.errors.append((e, None)) + + raise exp from error @staticmethod def load_file_with_argparser( @@ -775,7 +814,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 05c150b70e6..9e54a1ee13a 100644 --- a/src/everest/config/validation_utils.py +++ b/src/everest/config/validation_utils.py @@ -7,7 +7,7 @@ from pathlib import Path from typing import TYPE_CHECKING, TypeVar -from pydantic import BaseModel, ValidationError +from pydantic import BaseModel from everest.config.install_data_config import InstallDataConfig from everest.util.forward_models import ( @@ -16,6 +16,7 @@ parse_forward_model_file, ) +from .everest_config import EverestValidationError from .install_job_config import InstallJobConfig if TYPE_CHECKING: @@ -253,12 +254,26 @@ def _error_loc(error_dict: "ErrorDetails") -> str: ) -def format_errors(error: ValidationError) -> str: - errors = error.errors() - msg = f"Found {len(errors)} validation error{'s' if len(errors) > 1 else ''}:\n\n" +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 err in error.errors(): - key = _error_loc(err) + + for error, pos in validation_error.errors: + print(error, pos) + + 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()))) + + for (row, col), err in error.errors.items(): + # msg +=f"line: {row}, column: {col}. " + key = f"line: {row}, column: {col}. {_error_loc(err)}" if key not in error_map: error_map[key] = [key] error_map[key].append(f" * {err['msg']} (type={err['type']})") 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 01439396f37..3e630a4f99c 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_yaml_parser.py b/tests/everest/test_yaml_parser.py index 436a594c0b3..ccca819d397 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}