From 1ac10be66132ea2b0e7af74cc21b1aabdc4e1f1b Mon Sep 17 00:00:00 2001 From: Ethan Date: Wed, 28 Oct 2026 01:42:45 +0800 Subject: [PATCH 1/2] feat(cli)!: reject duplicate keys in the eval config on every entry point A key repeated within one mapping of the evaluation config was a warning under --dry-run and silently last-wins under run, eval and EvalSession, so a second api_base or capabilities: decided the run unseen. All of them now read the config through sieval.cli._config_yaml.load_eval_config, which refuses a repeat with the key and both lines. run and run --dry-run refuse before any launch planning. YAML merge keys may still be overridden. Scope: the eval config only. Recipe YAML, sieval infer target files and files sieval wrote itself keep yaml.safe_load; a test pins every yaml load under sieval/cli. Unknown top-level keys stay a warning. BREAKING CHANGE: a config that repeats a key is now refused; load_yaml_with_duplicate_check is removed. Closes #255 Co-Authored-By: Claude Opus 5.5 --- docs/guide/configuration.md | 5 + sieval/cli/_config_yaml.py | 68 ++++++++++ sieval/cli/run.py | 16 ++- sieval/cli/session.py | 13 +- sieval/cli/validation.py | 73 +++------- tests/unit/cli/test_config_yaml.py | 206 +++++++++++++++++++++++++++++ tests/unit/cli/test_validation.py | 67 ++-------- 7 files changed, 322 insertions(+), 126 deletions(-) create mode 100644 sieval/cli/_config_yaml.py create mode 100644 tests/unit/cli/test_config_yaml.py diff --git a/docs/guide/configuration.md b/docs/guide/configuration.md index a3a0782f..abf54483 100644 --- a/docs/guide/configuration.md +++ b/docs/guide/configuration.md @@ -4,6 +4,11 @@ SiEval uses YAML files for batch evaluation. A config defines models, datasets, tasks, and runner options. +A key repeated within one mapping (two `api_base` under one model, two +`capabilities:`) is an error naming the key and both lines: `sieval run`, +`sieval eval`, their `--dry-run` and `EvalSession` all refuse the file before +anything starts. YAML merge keys (`<<: *anchor`) may still be overridden. + ```yaml result_dir: "./outputs/my-eval" diff --git a/sieval/cli/_config_yaml.py b/sieval/cli/_config_yaml.py new file mode 100644 index 00000000..1756a026 --- /dev/null +++ b/sieval/cli/_config_yaml.py @@ -0,0 +1,68 @@ +"""The one reader of an evaluation config file. + +``sieval run``, ``sieval eval``, their ``--dry-run`` and ``EvalSession`` all +open the config through :func:`load_eval_config`, so a file is accepted or +refused the same way on every entry point. A repeated mapping key is refused: +PyYAML keeps the last value silently, so a second ``api_base`` or +``capabilities:`` would otherwise decide the run unseen. + +AI-Generated Code - Claude Opus 5.5 (Anthropic) +""" + +from pathlib import Path + +import yaml + +_MERGE_TAG = "tag:yaml.org,2002:merge" + + +class DuplicateKeyError(ValueError): + """The config repeats a key within one mapping.""" + + +class _EvalConfigLoader(yaml.SafeLoader): + """``SafeLoader`` that records each repeated key; one instance per load.""" + + def __init__(self, stream: str) -> None: + super().__init__(stream) + self.duplicates: list[str] = [] + + def construct_mapping(self, node, deep=False): + # Before ``flatten_mapping``: keys a ``<<`` merge brings in may be + # overridden by the mapping's own keys, as YAML intends. + if isinstance(node, yaml.MappingNode): + first_line: dict[object, int] = {} + for key_node, _ in node.value: + if key_node.tag == _MERGE_TAG: + continue + key = self.construct_object(key_node, deep=False) + line = key_node.start_mark.line + 1 + try: + seen = key in first_line + except TypeError: + continue # unhashable; SafeLoader refuses it itself + if seen: + self.duplicates.append( + f"duplicate key {key!r} at line {line} " + f"(first at line {first_line[key]})" + ) + else: + first_line[key] = line + return super().construct_mapping(node, deep=deep) + + +def load_eval_config(path: str | Path) -> object: + """Parse the evaluation config at *path*, as ``yaml.safe_load`` would. + + Raises: + DuplicateKeyError: a mapping repeats a key; names every repeat. + yaml.YAMLError: the file is not valid YAML. + """ + loader = _EvalConfigLoader(Path(path).read_text(encoding="utf-8")) + try: + data = loader.get_single_data() + finally: + loader.dispose() + if loader.duplicates: + raise DuplicateKeyError(f"{path}: " + "; ".join(loader.duplicates)) + return data diff --git a/sieval/cli/run.py b/sieval/cli/run.py index 16542880..8f3c69e8 100644 --- a/sieval/cli/run.py +++ b/sieval/cli/run.py @@ -25,6 +25,7 @@ CliModel, apply_cli_model, ) +from sieval.cli._config_yaml import DuplicateKeyError, load_eval_config from sieval.cli._resume_option import ( ResumeCommand, ResumeOption, @@ -833,10 +834,13 @@ def unplanned(result: dict[str, object]) -> dict[str, object]: if not config_path.exists(): return unplanned(checks()) - with open(config_path) as stream: - loaded = yaml.safe_load(stream) - if not isinstance(loaded, dict): + try: + parsed = load_eval_config(config_path) + except (yaml.YAMLError, DuplicateKeyError): + return unplanned(checks()) # reported as the failed yaml_syntax check + if not isinstance(parsed, dict): raise ValueError("evaluation config must be a mapping") + loaded = cast(dict, parsed) try: apply_cli_model(loaded, cli_model, model) except ValueError: @@ -1148,10 +1152,10 @@ async def _run_all( """ configure_logging(verbose) - with open(config_path) as f: - config = yaml.safe_load(f) - if not isinstance(config, dict): + parsed = load_eval_config(config_path) + if not isinstance(parsed, dict): raise ValueError("evaluation config must be a mapping") + config = cast(dict, parsed) apply_cli_model(config, cli_model, model) # Path-only models skip resolve_infer_config (which reads YAML diff --git a/sieval/cli/session.py b/sieval/cli/session.py index a27f60e5..d7c27f83 100644 --- a/sieval/cli/session.py +++ b/sieval/cli/session.py @@ -28,6 +28,7 @@ from sieval import __version__ from sieval.cli._cli_model import CliModel, apply_cli_model +from sieval.cli._config_yaml import load_eval_config from sieval.cli._filter_spec import ( VALUES_DIGEST_KEY, check_arg_names, @@ -1753,12 +1754,12 @@ def __init__( invocation if invocation is not None else shlex.join(sys.argv) ) - with open(self.config_path, encoding="utf-8") as f: - loaded_config: RootConfigDict | None = yaml.safe_load(f) - if loaded_config is None: - loaded_config = {} - if not isinstance(loaded_config, dict): - raise ValueError("Top-level YAML config must be a dictionary") + parsed = load_eval_config(self.config_path) + if parsed is None: + parsed = {} + if not isinstance(parsed, dict): + raise ValueError("Top-level YAML config must be a dictionary") + loaded_config = cast(RootConfigDict, parsed) if "alignment" in loaded_config: logger.warning( "{}: the `alignment:` block was removed in 0.11 and is ignored; " diff --git a/sieval/cli/validation.py b/sieval/cli/validation.py index 866b262e..766f6e1c 100644 --- a/sieval/cli/validation.py +++ b/sieval/cli/validation.py @@ -11,7 +11,7 @@ AI-Generated Code - Claude Opus 4.6 (Anthropic) """ -from collections.abc import Callable, Mapping +from collections.abc import Mapping from dataclasses import dataclass, field from pathlib import Path from typing import Any, NotRequired, TypedDict, cast @@ -19,6 +19,7 @@ import yaml from sieval.cli._cli_model import CliModel, apply_cli_model +from sieval.cli._config_yaml import DuplicateKeyError, load_eval_config from sieval.cli._filter_spec import ( check_arg_names, check_by, @@ -99,42 +100,6 @@ def merge(self, other: "ValidationResult") -> None: self.warnings.extend(other.warnings) -def _make_duplicate_key_checker(warnings: list[str]) -> Callable[..., dict]: - """Create a construct_mapping override that records duplicate keys.""" - - original = yaml.SafeLoader.construct_mapping - - def construct_mapping(loader, node, deep=False): - loader.flatten_mapping(node) - seen: set[str] = set() - for key_node, _value_node in node.value: - key = loader.construct_object(key_node, deep=False) - if isinstance(key, str) and key in seen: - mark = key_node.start_mark - warnings.append(f"Duplicate key '{key}' at line {mark.line + 1}") - if isinstance(key, str): - seen.add(key) - return original(loader, node, deep=deep) - - return construct_mapping - - -def load_yaml_with_duplicate_check(content: str) -> tuple[dict, list[str]]: - """Parse YAML content, returning (parsed_dict, duplicate_key_warnings). - - Preserves last-wins behavior (consistent with ``yaml.safe_load``). - """ - warnings: list[str] = [] - checker = _make_duplicate_key_checker(warnings) - # Per-call subclass to avoid shared class-level state (thread-safe) - loader_cls = type("_DuplicateKeyLoader", (yaml.SafeLoader,), {}) - cast(Any, loader_cls).construct_mapping = checker - data = yaml.load(content, Loader=loader_cls) - if data is None: - data = {} - return data, warnings - - def _validate_structure(cfg: dict, result: ValidationResult) -> bool: """Validate top-level structure. Returns False if too broken to continue.""" for key in cfg: @@ -1084,29 +1049,24 @@ def run_dry_run( checks.append({"name": "file_exists", "ok": True}) - # 2. YAML parse + duplicate key check - content = config.read_text(encoding="utf-8") + # 2. YAML parse; a repeated key is refused, as on every entry point try: - cfg, dup_warnings = load_yaml_with_duplicate_check(content) - except yaml.YAMLError as exc: + parsed = load_eval_config(config) + except (yaml.YAMLError, DuplicateKeyError) as exc: + detail = ( + str(exc) + if isinstance(exc, DuplicateKeyError) + else f"YAML syntax error: {exc}" + ) checks.append( - { - "name": "yaml_syntax", - "ok": False, - "detail": f"YAML syntax error: {exc}", - "warnings": [], - } + {"name": "yaml_syntax", "ok": False, "detail": detail, "warnings": []} ) n_err = sum(1 for c in checks if not c["ok"]) return {"checks": checks, "n_errors": n_err, "n_warnings": 0} + # A non-mapping is refused by the cli_model and schema checks below. + cfg = {} if parsed is None else cast(dict, parsed) - checks.append( - { - "name": "yaml_syntax", - "ok": True, - "warnings": list(dup_warnings), - } - ) + checks.append({"name": "yaml_syntax", "ok": True, "warnings": []}) # 2b. The models: entry --checkpoint/--api-base becomes if cli_model is not None: @@ -1116,7 +1076,7 @@ def run_dry_run( apply_cli_model(cfg, cli_model, model_override) except ValueError as exc: checks.append({"name": "cli_model", "ok": False, "detail": str(exc)}) - return {"checks": checks, "n_errors": 1, "n_warnings": len(dup_warnings)} + return {"checks": checks, "n_errors": 1, "n_warnings": 0} checks.append( {"name": "cli_model", "ok": True, "plan": {"models": cfg["models"]}} ) @@ -1191,7 +1151,6 @@ def run_dry_run( checks.append(schema_check) n_err = sum(1 for c in checks if not c["ok"]) - all_warnings = list(dup_warnings) + list(result.warnings) - n_warn = len(all_warnings) + n_warn = len(result.warnings) return {"checks": checks, "n_errors": n_err, "n_warnings": n_warn} diff --git a/tests/unit/cli/test_config_yaml.py b/tests/unit/cli/test_config_yaml.py new file mode 100644 index 00000000..b4def93e --- /dev/null +++ b/tests/unit/cli/test_config_yaml.py @@ -0,0 +1,206 @@ +"""Tests for sieval.cli._config_yaml — the evaluation config reader. + +AI-Generated Code - Claude Opus 5.5 (Anthropic) +""" + +import ast +import json +import re +from pathlib import Path +from unittest.mock import patch + +import pytest +import yaml +from typer.testing import CliRunner + +import sieval.cli +from sieval.cli import app +from sieval.cli._config_yaml import DuplicateKeyError, load_eval_config +from sieval.cli.session import EvalSession + +cli_runner = CliRunner() + +# A second ``api_base`` under one model: last-wins would run on :8001. +_DUPLICATE_API_BASE = ( + "models:\n" + " m:\n" + " model_type: chat\n" + " api_base: http://localhost:8000/v1\n" + " api_base: http://localhost:8001/v1\n" + "datasets: {}\n" + "tasks: {}\n" +) +_DUPLICATE_MESSAGE = "duplicate key 'api_base' at line 5 (first at line 4)" + + +def _write(tmp_path: Path, text: str) -> Path: + path = tmp_path / "config.yaml" + path.write_text(text, encoding="utf-8") + return path + + +class TestLoadEvalConfig: + def test_parses_as_safe_load(self, tmp_path): + text = "models:\n a:\n name: foo\n b: [1, 2]\nn: 3\n" + assert load_eval_config(_write(tmp_path, text)) == yaml.safe_load(text) + + def test_empty_file_is_none(self, tmp_path): + assert load_eval_config(_write(tmp_path, "")) is None + + def test_duplicate_top_level_key(self, tmp_path): + path = _write(tmp_path, "models: {}\ntasks: {}\nmodels: {}\n") + with pytest.raises(DuplicateKeyError) as info: + load_eval_config(path) + assert str(info.value) == ( + f"{path}: duplicate key 'models' at line 3 (first at line 1)" + ) + + def test_duplicate_nested_key_names_both_lines(self, tmp_path): + with pytest.raises(DuplicateKeyError, match=r"line 5 \(first at line 4\)"): + load_eval_config(_write(tmp_path, _DUPLICATE_API_BASE)) + + def test_names_every_duplicate(self, tmp_path): + path = _write(tmp_path, "a: 1\na: 2\nb:\n c: 1\n c: 2\n") + with pytest.raises(DuplicateKeyError) as info: + load_eval_config(path) + message = str(info.value) + assert "duplicate key 'a' at line 2" in message + assert "duplicate key 'c' at line 5" in message + + def test_duplicate_key_is_a_value_error(self): + # The CLI renders a ValueError's message as is. + assert issubclass(DuplicateKeyError, ValueError) + + def test_same_key_in_sibling_mappings_is_not_a_duplicate(self, tmp_path): + text = "models:\n a:\n name: x\n b:\n name: y\n" + assert load_eval_config(_write(tmp_path, text)) == { + "models": {"a": {"name": "x"}, "b": {"name": "y"}} + } + + def test_merge_key_may_be_overridden(self, tmp_path): + text = ( + "base: &base\n api_base: http://a/v1\n name: x\n" + "m:\n <<: *base\n api_base: http://b/v1\n" + ) + loaded = load_eval_config(_write(tmp_path, text)) + assert loaded == { + "base": {"api_base": "http://a/v1", "name": "x"}, + "m": {"api_base": "http://b/v1", "name": "x"}, + } + + def test_two_merge_keys_are_not_duplicates(self, tmp_path): + text = "a: &a {x: 1}\nb: &b {y: 2}\nm:\n <<: *a\n <<: *b\n" + loaded = load_eval_config(_write(tmp_path, text)) + assert loaded == {"a": {"x": 1}, "b": {"y": 2}, "m": {"x": 1, "y": 2}} + + def test_duplicate_after_a_merge_is_still_refused(self, tmp_path): + text = "base: &base {x: 1}\nm:\n <<: *base\n y: 1\n y: 2\n" + with pytest.raises(DuplicateKeyError, match="'y' at line 5"): + load_eval_config(_write(tmp_path, text)) + + def test_keys_equal_after_construction_are_duplicates(self, tmp_path): + # ``1`` and ``0x1`` both construct to the int 1. + with pytest.raises(DuplicateKeyError, match="duplicate key 1 at line 2"): + load_eval_config(_write(tmp_path, "1: a\n0x1: b\n")) + + def test_invalid_yaml_raises_yaml_error(self, tmp_path): + with pytest.raises(yaml.YAMLError): + load_eval_config(_write(tmp_path, "models:\n - [invalid\n")) + + def test_loads_are_independent(self, tmp_path): + dup = _write(tmp_path, "a: 1\na: 2\n") + clean = tmp_path / "clean.yaml" + clean.write_text("a: 1\nb: 2\n", encoding="utf-8") + with pytest.raises(DuplicateKeyError): + load_eval_config(dup) + assert load_eval_config(clean) == {"a": 1, "b": 2} + + +class TestEveryEntryPointRefusesADuplicate: + """``run``, ``eval``, both ``--dry-run`` and ``EvalSession`` read the config + through one loader; a new bare ``yaml.safe_load`` on any of them fails here.""" + + @pytest.mark.parametrize( + "argv", + [ + ["run"], + ["run", "--dry-run"], + ["eval"], + ["eval", "--dry-run"], + ], + ids=["run", "run-dry-run", "eval", "eval-dry-run"], + ) + def test_cli(self, tmp_path, argv): + config = _write(tmp_path, _DUPLICATE_API_BASE) + result = cli_runner.invoke(app, [*argv, str(config), "-o", "json"]) + assert result.exit_code == 1 + payload = json.loads(result.stdout) + assert payload["ok"] is False + assert _DUPLICATE_MESSAGE in json.dumps(payload) + + def test_dry_run_reports_a_failed_yaml_check(self, tmp_path): + config = _write(tmp_path, _DUPLICATE_API_BASE) + result = cli_runner.invoke(app, ["run", "--dry-run", str(config), "-o", "json"]) + checks = json.loads(result.stdout)["data"]["checks"] + assert checks[-1] == { + "name": "yaml_syntax", + "ok": False, + "detail": f"{config}: {_DUPLICATE_MESSAGE}", + "warnings": [], + } + + def test_eval_session(self, tmp_path): + config = _write(tmp_path, _DUPLICATE_API_BASE) + with pytest.raises(DuplicateKeyError, match=re.escape(_DUPLICATE_MESSAGE)): + EvalSession(config_path=str(config)) + + @pytest.mark.anyio + async def test_run_refuses_before_planning_a_launch(self, tmp_path): + # A later EvalSession refusal would come after engines started. + from sieval.cli.run import _run_all + + config = _write(tmp_path, _DUPLICATE_API_BASE) + with ( + patch("sieval.cli.run._prepare_launch_batch") as prepare, + pytest.raises(DuplicateKeyError, match=re.escape(_DUPLICATE_MESSAGE)), + ): + await _run_all(config) + prepare.assert_not_called() + + @pytest.mark.anyio + async def test_run_dry_run_plans_nothing(self, tmp_path): + from sieval.cli.run import _run_dry_run + + config = _write(tmp_path, _DUPLICATE_API_BASE) + with patch("sieval.cli.run._prepare_launch_batch") as prepare: + result = await _run_dry_run(config) + prepare.assert_not_called() + assert result["n_errors"] == 1 + assert result["launches"] is None + + +def test_no_other_yaml_load_in_the_cli(): + """Every ``yaml`` load under ``sieval/cli`` is either the eval-config + loader or one of these files, none of which is an eval config. A new + call fails here until it says which it is.""" + allowed = { + # read_persisted_config and the effective_config diff: files sieval wrote. + "session.py": 3, + # Recipe files and `sieval infer` target files. + "infer/recipe.py": 1, + "infer/commands.py": 1, + } + cli = Path(sieval.cli.__file__).parent + found: dict[str, int] = {} + for path in sorted(cli.rglob("*.py")): + for node in ast.walk(ast.parse(path.read_text(encoding="utf-8"))): + if ( + isinstance(node, ast.Call) + and isinstance(node.func, ast.Attribute) + and isinstance(node.func.value, ast.Name) + and node.func.value.id == "yaml" + and node.func.attr in {"load", "safe_load", "full_load", "unsafe_load"} + ): + rel = path.relative_to(cli).as_posix() + found[rel] = found.get(rel, 0) + 1 + assert found == allowed diff --git a/tests/unit/cli/test_validation.py b/tests/unit/cli/test_validation.py index 08aaa981..3dc3af6a 100644 --- a/tests/unit/cli/test_validation.py +++ b/tests/unit/cli/test_validation.py @@ -15,7 +15,6 @@ from sieval.cli.validation import ( ValidationResult, _is_builtin_class_spec, - load_yaml_with_duplicate_check, validate_eval_config, validate_eval_config_imports, ) @@ -45,56 +44,6 @@ def test_merge_combines_errors_and_warnings(self): assert a.warnings == ["w1", "w2"] -# --------------------------------------------------------------------------- -# YAML duplicate-key checking -# --------------------------------------------------------------------------- - - -class TestLoadYamlWithDuplicateCheck: - def test_valid_yaml_no_duplicates(self): - content = "models:\n a:\n name: foo\n b:\n name: bar\n" - data, warnings = load_yaml_with_duplicate_check(content) - assert data == {"models": {"a": {"name": "foo"}, "b": {"name": "bar"}}} - assert warnings == [] - - def test_duplicate_top_level_key(self): - content = "models:\n a:\n name: foo\nmodels:\n b:\n name: bar\n" - data, warnings = load_yaml_with_duplicate_check(content) - assert data == {"models": {"b": {"name": "bar"}}} - assert len(warnings) == 1 - assert "models" in warnings[0] - - def test_duplicate_nested_key(self): - content = "models:\n a:\n name: foo\n a:\n name: bar\n" - data, warnings = load_yaml_with_duplicate_check(content) - assert data == {"models": {"a": {"name": "bar"}}} - assert len(warnings) == 1 - assert "'a'" in warnings[0] - - def test_empty_yaml(self): - data, warnings = load_yaml_with_duplicate_check("") - assert data == {} - assert warnings == [] - - def test_invalid_yaml_raises(self): - with pytest.raises(yaml.YAMLError): - load_yaml_with_duplicate_check("models:\n - [invalid\n") - - def test_sequential_calls_produce_independent_warnings(self): - dup_content = "a: 1\na: 2\n" - clean_content = "b: 1\nc: 2\n" - - _, w1 = load_yaml_with_duplicate_check(dup_content) - _, w2 = load_yaml_with_duplicate_check(clean_content) - _, w3 = load_yaml_with_duplicate_check(dup_content) - - assert len(w1) == 1 - assert w2 == [] - assert len(w3) == 1 - # Warnings must not leak between calls - assert w1 is not w3 - - # --------------------------------------------------------------------------- # Schema validation — structure # --------------------------------------------------------------------------- @@ -2316,8 +2265,8 @@ def test_schema_warnings_attached_to_check(self, tmp_path): assert any("foobar" in w for w in schema_check.get("warnings", [])) assert result["n_warnings"] >= 1 - def test_duplicate_key_warnings(self, tmp_path): - """Config with duplicate keys returns warnings.""" + def test_duplicate_key_is_an_error(self, tmp_path): + """A repeated key fails the yaml_syntax check and stops the dry-run.""" from sieval.cli.validation import run_dry_run config = tmp_path / "dup.yaml" @@ -2327,10 +2276,14 @@ def test_duplicate_key_warnings(self, tmp_path): "datasets: {}\ntasks: {}\n" ) result = run_dry_run(config) - yaml_check = next(c for c in result["checks"] if c["name"] == "yaml_syntax") - assert yaml_check["ok"] is True - assert len(yaml_check.get("warnings", [])) > 0 - assert result["n_warnings"] > 0 + assert result["checks"][-1] == { + "name": "yaml_syntax", + "ok": False, + "detail": f"{config}: duplicate key 'models' at line 4 (first at line 1)", + "warnings": [], + } + assert result["n_errors"] == 1 + assert result["n_warnings"] == 0 def test_schema_ok_no_detail_when_sections_not_dicts(self, tmp_path): """Schema check omits detail when models/datasets/tasks aren't all dicts.""" From f3363c5fc34e00d514f756df41f6e0295a0a8e90 Mon Sep 17 00:00:00 2001 From: Ethan Date: Wed, 28 Oct 2026 02:07:57 +0800 Subject: [PATCH 2/2] fix(cli): accept a merge chain through a nested anchor in the eval config The duplicate check ran in construct_mapping, but flatten_mapping rewrites a merge source's node in place. An anchor built after a mapping merged from it already held its inherited keys, so an override read as a repeat. Check each node once, in flatten_mapping, on its keys as written. Also trims the loader docstring, test docstrings and config guide note. Co-Authored-By: Claude Opus 5.5 --- docs/guide/configuration.md | 6 ++---- sieval/cli/_config_yaml.py | 20 ++++++++++---------- sieval/cli/validation.py | 2 +- tests/unit/cli/test_config_yaml.py | 17 ++++++++++++----- 4 files changed, 25 insertions(+), 20 deletions(-) diff --git a/docs/guide/configuration.md b/docs/guide/configuration.md index abf54483..4b9d1e90 100644 --- a/docs/guide/configuration.md +++ b/docs/guide/configuration.md @@ -4,10 +4,8 @@ SiEval uses YAML files for batch evaluation. A config defines models, datasets, tasks, and runner options. -A key repeated within one mapping (two `api_base` under one model, two -`capabilities:`) is an error naming the key and both lines: `sieval run`, -`sieval eval`, their `--dry-run` and `EvalSession` all refuse the file before -anything starts. YAML merge keys (`<<: *anchor`) may still be overridden. +A key repeated within one mapping is an error on every entry point, before +anything starts. Keys a merge (`<<: *anchor`) brings in may still be overridden. ```yaml result_dir: "./outputs/my-eval" diff --git a/sieval/cli/_config_yaml.py b/sieval/cli/_config_yaml.py index 1756a026..66dc9c35 100644 --- a/sieval/cli/_config_yaml.py +++ b/sieval/cli/_config_yaml.py @@ -1,10 +1,7 @@ """The one reader of an evaluation config file. -``sieval run``, ``sieval eval``, their ``--dry-run`` and ``EvalSession`` all -open the config through :func:`load_eval_config`, so a file is accepted or -refused the same way on every entry point. A repeated mapping key is refused: -PyYAML keeps the last value silently, so a second ``api_base`` or -``capabilities:`` would otherwise decide the run unseen. +Every entry point loads the config through :func:`load_eval_config`, which +refuses a key repeated within one mapping instead of keeping the last value. AI-Generated Code - Claude Opus 5.5 (Anthropic) """ @@ -26,11 +23,14 @@ class _EvalConfigLoader(yaml.SafeLoader): def __init__(self, stream: str) -> None: super().__init__(stream) self.duplicates: list[str] = [] + self._checked: set[int] = set() - def construct_mapping(self, node, deep=False): - # Before ``flatten_mapping``: keys a ``<<`` merge brings in may be - # overridden by the mapping's own keys, as YAML intends. - if isinstance(node, yaml.MappingNode): + def flatten_mapping(self, node): + # flatten_mapping rewrites node.value in place, merge sources included, + # so check each node once, before that: keys a ``<<`` brings in may be + # overridden. + if id(node) not in self._checked: + self._checked.add(id(node)) first_line: dict[object, int] = {} for key_node, _ in node.value: if key_node.tag == _MERGE_TAG: @@ -48,7 +48,7 @@ def construct_mapping(self, node, deep=False): ) else: first_line[key] = line - return super().construct_mapping(node, deep=deep) + super().flatten_mapping(node) def load_eval_config(path: str | Path) -> object: diff --git a/sieval/cli/validation.py b/sieval/cli/validation.py index 766f6e1c..c3a69deb 100644 --- a/sieval/cli/validation.py +++ b/sieval/cli/validation.py @@ -1049,7 +1049,7 @@ def run_dry_run( checks.append({"name": "file_exists", "ok": True}) - # 2. YAML parse; a repeated key is refused, as on every entry point + # 2. YAML parse; a repeated key is an error try: parsed = load_eval_config(config) except (yaml.YAMLError, DuplicateKeyError) as exc: diff --git a/tests/unit/cli/test_config_yaml.py b/tests/unit/cli/test_config_yaml.py index b4def93e..6d8deedf 100644 --- a/tests/unit/cli/test_config_yaml.py +++ b/tests/unit/cli/test_config_yaml.py @@ -93,6 +93,16 @@ def test_two_merge_keys_are_not_duplicates(self, tmp_path): loaded = load_eval_config(_write(tmp_path, text)) assert loaded == {"a": {"x": 1}, "b": {"y": 2}, "m": {"x": 1, "y": 2}} + def test_merge_chain_through_a_nested_anchor_may_be_overridden(self, tmp_path): + # The anchor is built after ``m`` merged from it: its node already + # holds the inherited ``a``, which must not read as a repeat. + text = ( + "base: &base {a: 1}\n" + "defaults:\n inner: &d\n <<: *base\n a: 2\n" + "m:\n <<: *d\n" + ) + assert load_eval_config(_write(tmp_path, text)) == yaml.safe_load(text) + def test_duplicate_after_a_merge_is_still_refused(self, tmp_path): text = "base: &base {x: 1}\nm:\n <<: *base\n y: 1\n y: 2\n" with pytest.raises(DuplicateKeyError, match="'y' at line 5"): @@ -117,8 +127,7 @@ def test_loads_are_independent(self, tmp_path): class TestEveryEntryPointRefusesADuplicate: - """``run``, ``eval``, both ``--dry-run`` and ``EvalSession`` read the config - through one loader; a new bare ``yaml.safe_load`` on any of them fails here.""" + """A bare ``yaml.safe_load`` on any entry point fails here.""" @pytest.mark.parametrize( "argv", @@ -180,9 +189,7 @@ async def test_run_dry_run_plans_nothing(self, tmp_path): def test_no_other_yaml_load_in_the_cli(): - """Every ``yaml`` load under ``sieval/cli`` is either the eval-config - loader or one of these files, none of which is an eval config. A new - call fails here until it says which it is.""" + """A new ``yaml`` load under ``sieval/cli`` must say what it reads.""" allowed = { # read_persisted_config and the effective_config diff: files sieval wrote. "session.py": 3,