diff --git a/src/submission_checker/checker.py b/src/submission_checker/checker.py index 2bd296f..784788b 100644 --- a/src/submission_checker/checker.py +++ b/src/submission_checker/checker.py @@ -16,6 +16,7 @@ from . import layout from .drafters import ApprovedDrafter, DrafterListError, load_approved_drafters +from .messages import Invalid from .models import ( AccuracyResult, CheckResult, @@ -971,10 +972,8 @@ def _derive_regions( try: regions = compute_regions(c_max, c_min) - except ValueError as exc: - results.append( - _err("region-computation", "fail", sd_path or model_dir, detail=str(exc)) - ) + except Invalid as exc: + results.append(_err(exc.rule, exc.key, sd_path or model_dir, **exc.params)) return None, results return regions, results diff --git a/src/submission_checker/data/messages.yaml b/src/submission_checker/data/messages.yaml index 46143a1..11cbd3e 100644 --- a/src/submission_checker/data/messages.yaml +++ b/src/submission_checker/data/messages.yaml @@ -24,11 +24,46 @@ _shared: io-error: text: 'IO error reading {file}: {error}' field-invalid: - text: 'Validation error in {file}: {field} — {problem}' + text: '{file}, field `{field}`: {problem}' + file-invalid: + text: '{file}: {problem}' + unworded-error: + text: '{message}' score-not-fraction: text: 'r{concurrency}: `{dataset}` score {value} is not a fraction in [0, 1], which is what the reference scorer reports' spec: §4.3 +_pydantic: + # Pydantic's built-in validation errors, keyed by its error type. Templates may use + # the error's context values (see pydantic_core's list_all_errors) and {input}, the + # offending value. The test suite fails when this list and the types the file + # models can raise drift apart. + title: Field validation + spec: '' + messages: + missing: required, but missing + extra_forbidden: + text: not a recognised field + fix: Remove it, or check its spelling against the spec's template. + model_type: expected an object, got {input} + model_attributes_type: expected an object, got {input} + dict_type: expected an object, got {input} + list_type: expected a list, got {input} + too_short: needs at least {min_length} item(s), got {actual_length} + string_type: expected a string, got {input} + string_too_short: needs at least {min_length} character(s), got {input} + int_type: expected a whole number, got {input} + int_parsing: expected a whole number, got {input} + int_from_float: expected a whole number, got {input} + float_type: expected a number, got {input} + float_parsing: expected a number, got {input} + bool_type: expected true or false, got {input} + bool_parsing: expected true or false, got {input} + greater_than: must be greater than {gt}, got {input} + greater_than_equal: must be at least {ge}, got {input} + less_than_equal: must be at most {le}, got {input} + literal_error: must be {expected}, got {input} + enum: must be {expected}, got {input} accuracy-coverage: title: Accuracy at the mandatory points spec: §5.3 @@ -75,6 +110,14 @@ accuracy-valid: spec: §4.3 messages: fail: accuracy_result.json is empty + scores-not-collection: accuracy_scores must be a mapping of datasets or a list of entries + entry-not-mapping: each accuracy_scores entry must be an object + entry-unnamed: each accuracy_scores entry needs a non-empty dataset_name + dataset-duplicate: dataset {name!r} appears more than once + score-missing: dataset {name!r} has no score + alias-conflict: + text: dataset {name!r} gives {native} and {canonical} different values + fix: They are two spellings of one field; give one, or make them agree. agentic-accuracy: title: Agentic accuracy spec: §3.2 @@ -383,6 +426,14 @@ point-cap: spec: §2, §8 messages: fail: '{n} points exceed the {max_points}-point cap' +point-config-valid: + title: Point configuration (point.yaml) + spec: §8.3 + messages: + warmup-completed-exceeds-issued: warmup requests_completed ({completed}) exceeds requests_issued + ({issued}) + warmup-concurrency-zero: warmup concurrency must be positive when duration or requests are + nonzero point-count: title: Number of points spec: §2, §8 @@ -429,6 +480,10 @@ power-descriptor: title: Power descriptor (system_power.json) spec: §4.5.2 messages: + value-count: give exactly one of value_w, value_kw or value_pj + mlc-default-source: an mlc_default source names its Appendix D subsection ({subsections}), not + {source!r} + source-not-url: a {source_type} source must be a resolvable URL, not {source!r} pass: text: No system_power.json; power normalisation is not required for the {division} division spec: §4.5 @@ -502,7 +557,8 @@ region-computation: title: Region boundaries spec: §5.5 messages: - fail: '{detail}' + c-min-range: Minimum concurrency must be between 1 and {limit} (inclusive), got {c_min} + c-max-too-low: Maximum Supported Concurrency must be greater than {limit}, got {c_max} region-declared: title: Region declared spec: §8.3 @@ -523,6 +579,12 @@ required-dir: fail: text: 'Missing required directory: {name}/{hint}' fix: Create {name}/ at the submission root, laid out as §8.1 describes. +result-file-valid: + title: Result summary (result_summary.json) + spec: §8.3 + messages: + percentile-key: '{key!r} is not a percentile between 0 and 100' + percentile-conflict: two keys for percentile {percentile} give different values result-summary-present: title: Result summary present spec: §1 @@ -675,6 +737,13 @@ system-description-valid: messages: pass: System description valid for {rel} fail: system_desc.json is not readable as a JSON object + core-count-missing: a node type must give host_processor_core_count or host_processor_vcpu_count + division-unknown: unknown division {value!r}; must be one of standardized, serviced, rdi + availability-unknown: unknown availability {value!r}; must be one of available, preview, rdi + availability-conflict: + text: the availability fields disagree ({values}) + fix: publication_status, system_availability_status and availability_status are spellings of + one field; give one, or make them agree. system-results-dir: title: System results directory spec: §1 diff --git a/src/submission_checker/messages.py b/src/submission_checker/messages.py index 2446e77..6132a86 100644 --- a/src/submission_checker/messages.py +++ b/src/submission_checker/messages.py @@ -23,6 +23,13 @@ Templates use :meth:`str.format` syntax restricted to plain names: ``{model}``, ``{value!r}`` and ``{ratio:.2f}`` work, ``{point.name}`` and ``{scores[0]}`` do not. The code formats anything richer before passing it in. + +Validation inside the file models words its findings the same way. A validator +raises :class:`Invalid` with a rule, a key and values rather than a hand-written +``ValueError``; the loaders render it from the catalog, located at the field that +failed. Pydantic's own errors ("Field required", "Input should be a valid +integer") are worded by the :data:`PYDANTIC` section, keyed by Pydantic's error +type, so a library upgrade cannot change what a submitter reads. """ from __future__ import annotations @@ -37,6 +44,9 @@ import yaml __all__ = [ + "PYDANTIC", + "SHARED", + "Invalid", "MessageCatalogError", "Rendered", "catalog", @@ -52,6 +62,11 @@ #: field-level errors every file loader reports under its own rule. SHARED = "_shared" +#: Section of the catalog wording Pydantic's built-in validation errors, keyed by +#: Pydantic's error type (``missing``, ``int_parsing``, ...). Its templates can use +#: the error's context values and ``input``, the offending value. +PYDANTIC = "_pydantic" + #: When True, a missing key, a missing parameter or a template error raises #: instead of degrading to a generic message. The test suite turns this on, so a #: message that cannot render is a failing test rather than a quiet fallback. @@ -62,6 +77,22 @@ class MessageCatalogError(RuntimeError): """The catalog is malformed, or a result names a message it does not hold.""" +class Invalid(ValueError): + """A finding raised rather than returned, worded by the catalog. + + File-model validators raise it where they would raise ``ValueError``: Pydantic + carries it through to the loader, which reports it under the field that failed. + Code outside a model, such as the region computation, raises it for its caller + to report. ``str()`` of it is the rendered message. + """ + + def __init__(self, rule: str, key: str, /, **params: object) -> None: + self.rule = rule + self.key = key + self.params = params + super().__init__(render(rule, key, params).text) + + @dataclass(frozen=True) class Message: """One catalog entry: its template, optional fix, and spec section.""" diff --git a/src/submission_checker/models/file/accuracy.py b/src/submission_checker/models/file/accuracy.py index 3d8f9e5..b00dd8e 100644 --- a/src/submission_checker/models/file/accuracy.py +++ b/src/submission_checker/models/file/accuracy.py @@ -7,6 +7,7 @@ from pydantic import PrivateAttr, RootModel, model_validator +from ...messages import Invalid from ..results import CheckResult, err __all__ = ["AccuracyResult"] @@ -38,20 +39,20 @@ def _read_native_report(cls, data: Any) -> Any: if isinstance(data, dict) and "accuracy_scores" in data: data = data["accuracy_scores"] if not isinstance(data, (dict, list)): - raise ValueError("accuracy_scores must be a dataset mapping or list") + raise Invalid("accuracy-valid", "scores-not-collection") if not isinstance(data, list): return data indexed: dict[str, dict[str, Any]] = {} for entry in data: if not isinstance(entry, dict): - raise ValueError("Each accuracy_scores entry must be a dictionary") + raise Invalid("accuracy-valid", "entry-not-mapping") name = entry.get("dataset_name") if not isinstance(name, str) or not name.strip(): - raise ValueError("Each accuracy_scores entry must have a non-empty dataset_name") + raise Invalid("accuracy-valid", "entry-unnamed") if name in indexed: - raise ValueError(f"Duplicate accuracy dataset_name: {name!r}") + raise Invalid("accuracy-valid", "dataset-duplicate", name=name) if "score" not in entry: - raise ValueError(f"Native accuracy entry {name!r} is missing score") + raise Invalid("accuracy-valid", "score-missing", name=name) normalized = dict(entry) # Native scorers name these fields differently. Keep the originals # and expose aliases used by the existing sample-count/weight gates. @@ -61,7 +62,13 @@ def _read_native_report(cls, data: Any) -> Any: ): if native in entry: if canonical in entry and entry[canonical] != entry[native]: - raise ValueError(f"Conflicting {native} and {canonical} for {name!r}") + raise Invalid( + "accuracy-valid", + "alias-conflict", + name=name, + native=native, + canonical=canonical, + ) normalized[canonical] = entry[native] indexed[name] = normalized return indexed diff --git a/src/submission_checker/models/file/point_config.py b/src/submission_checker/models/file/point_config.py index 2d012e6..64a1f6b 100644 --- a/src/submission_checker/models/file/point_config.py +++ b/src/submission_checker/models/file/point_config.py @@ -5,7 +5,7 @@ import re from pathlib import Path -from ...messages import fragment +from ...messages import Invalid, fragment __all__ = ["DpShortfall", "NodesUsed", "PointConfig", "RuntimeSettings", "WarmupSpec"] @@ -93,14 +93,14 @@ def is_disabled(self) -> bool: @model_validator(mode="after") def _check_completed_le_issued(self) -> WarmupSpec: if self.requests_completed > self.requests_issued: - raise ValueError( - f"requests_completed ({self.requests_completed})" - f" > requests_issued ({self.requests_issued})" + raise Invalid( + "point-config-valid", + "warmup-completed-exceeds-issued", + completed=self.requests_completed, + issued=self.requests_issued, ) if self.concurrency == 0 and not self.is_disabled: - raise ValueError( - "Warmup concurrency must be positive when duration or requests are nonzero" - ) + raise Invalid("point-config-valid", "warmup-concurrency-zero") return self diff --git a/src/submission_checker/models/file/point_summary.py b/src/submission_checker/models/file/point_summary.py index c768af7..b839d7e 100644 --- a/src/submission_checker/models/file/point_summary.py +++ b/src/submission_checker/models/file/point_summary.py @@ -6,6 +6,8 @@ from pydantic import BaseModel, ConfigDict, Field, computed_field, field_validator +from ...messages import Invalid + __all__ = ["PercentileStats", "PointSummary"] @@ -28,12 +30,15 @@ def _normalize_percentile_keys(cls, values: dict[str, float]) -> dict[str, float """Treat native decimal keys and integer keys as the same percentile.""" normalized: dict[str, float] = {} for key, value in values.items(): - percentile = float(key) + try: + percentile = float(key) + except ValueError: + percentile = math.nan if not math.isfinite(percentile) or not 0 <= percentile <= 100: - raise ValueError(f"Invalid percentile key: {key!r}") + raise Invalid("result-file-valid", "percentile-key", key=key) canonical = str(int(percentile)) if percentile.is_integer() else str(percentile) if canonical in normalized and normalized[canonical] != value: - raise ValueError(f"Conflicting values for percentile {canonical}") + raise Invalid("result-file-valid", "percentile-conflict", percentile=canonical) normalized[canonical] = value return normalized diff --git a/src/submission_checker/models/file/system.py b/src/submission_checker/models/file/system.py index 3739473..d7267e0 100644 --- a/src/submission_checker/models/file/system.py +++ b/src/submission_checker/models/file/system.py @@ -15,6 +15,8 @@ model_validator, ) +from ...messages import Invalid + __all__ = [ "AcceleratorInfo", "ConfigSummary", @@ -130,10 +132,7 @@ def _lift_flat_accelerator_fields(cls, data: object) -> object: def _require_core_or_vcpu_count(self) -> NodeType: """A node must disclose at least one of physical core count or vCPU count.""" if self.host_processor_core_count is None and self.host_processor_vcpu_count is None: - raise ValueError( - "node_types entry must specify host_processor_core_count or" - " host_processor_vcpu_count" - ) + raise Invalid("system-description-valid", "core-count-missing") return self @@ -268,7 +267,7 @@ def _coerce_division(cls, v: object) -> object: normalized = mapping.get(v.strip().lower()) if normalized is not None: return normalized - raise ValueError(f"Unknown division {v!r}. Must be one of: standardized, serviced, rdi") + raise Invalid("system-description-valid", "division-unknown", value=v) return v @model_validator(mode="before") @@ -291,7 +290,7 @@ def _reconcile_availability_spellings(cls, data: object) -> object: distinct = {str(v).strip().lower() for v in present.values()} if len(distinct) > 1: pairs = ", ".join(f"{k}={v!r}" for k, v in sorted(present.items())) - raise ValueError(f"Conflicting availability values: {pairs}") + raise Invalid("system-description-valid", "availability-conflict", values=pairs) if data.get("publication_status") in (None, ""): data = {**data, "publication_status": next(iter(present.values()))} return data @@ -304,7 +303,7 @@ def _coerce_availability(cls, v: object) -> object: normalized = mapping.get(v.strip().lower()) if normalized is not None: return normalized - raise ValueError(f"Unknown availability {v!r}. Must be one of: available, preview, rdi") + raise Invalid("system-description-valid", "availability-unknown", value=v) return v @field_validator("input_token_average", "output_token_average", mode="before") diff --git a/src/submission_checker/models/file/system_power.py b/src/submission_checker/models/file/system_power.py index b47c9ee..f027eb5 100644 --- a/src/submission_checker/models/file/system_power.py +++ b/src/submission_checker/models/file/system_power.py @@ -38,7 +38,7 @@ from pydantic import BaseModel, ConfigDict, Field, model_validator -from ...messages import fragment +from ...messages import Invalid, fragment from ...power_defaults import ( PowerDefault, accelerator_default, @@ -116,16 +116,21 @@ class SourcedValue(BaseModel): def _one_value_and_a_real_source(self) -> SourcedValue: values = [v for v in (self.value_w, self.value_kw, self.value_pj) if v is not None] if len(values) != 1: - raise ValueError("exactly one of value_w, value_kw or value_pj is required") + raise Invalid("power-descriptor", "value-count") if self.source_type == "mlc_default": if self.source not in _APPENDIX_D: - raise ValueError( - f"an mlc_default source names its Appendix D subsection" - f" ({', '.join(sorted(_APPENDIX_D))}), not {self.source!r}" + raise Invalid( + "power-descriptor", + "mlc-default-source", + subsections=", ".join(sorted(_APPENDIX_D)), + source=self.source, ) elif not self.source.startswith(("https://", "http://")): - raise ValueError( - f"a {self.source_type} source must be a resolvable URL, not {self.source!r}" + raise Invalid( + "power-descriptor", + "source-not-url", + source_type=self.source_type, + source=self.source, ) return self diff --git a/src/submission_checker/models/loader.py b/src/submission_checker/models/loader.py index a489845..93aa688 100644 --- a/src/submission_checker/models/loader.py +++ b/src/submission_checker/models/loader.py @@ -18,9 +18,12 @@ import yaml from pydantic import ValidationError +from pydantic_core import ErrorDetails +from .. import messages +from ..messages import PYDANTIC, SHARED, Invalid, MessageCatalogError from .file import AccuracyResult, PointConfig, PointSummary, SystemDescription, SystemPower -from .results import CheckResult, err +from .results import CheckResult, err, field_err #: A load failure: the ``_shared`` catalog message and the values it needs. _LoadError = tuple[str, dict[str, object]] @@ -56,17 +59,30 @@ def _load_failure(rule: str, path: Path, failure: _LoadError) -> list[CheckResul return [err(rule, key, path, **params)] +def _brief(value: object, limit: int = 60) -> str: + text = repr(value) + return text if len(text) <= limit else text[: limit - 1] + "…" + + +def _problem(error: ErrorDetails) -> tuple[str, str, dict[str, object]]: + """The catalog message for one Pydantic error: (rule, key, values).""" + ctx = dict(error.get("ctx") or {}) + cause = ctx.get("error") + if isinstance(cause, Invalid): + return cause.rule, cause.key, cause.params + if error["type"] in messages.catalog()[PYDANTIC].messages: + return PYDANTIC, error["type"], {**ctx, "input": _brief(error["input"])} + if messages.strict: + raise MessageCatalogError( + f"no {PYDANTIC} message for Pydantic error {error['type']!r}: {error['msg']}" + ) + return SHARED, "unworded-error", {"message": error["msg"]} + + def _validation_errors(exc: ValidationError, rule: str, path: Path) -> list[CheckResult]: return [ - err( - rule, - "field-invalid", - path, - file=path.name, - field=".".join(str(part) for part in e["loc"]) or "(top level)", - problem=e["msg"], - ) - for e in exc.errors() + field_err(rule, path, ".".join(str(part) for part in e["loc"]), *_problem(e)) + for e in exc.errors(include_url=False) ] diff --git a/src/submission_checker/models/regions.py b/src/submission_checker/models/regions.py index 47d840a..ba951d6 100644 --- a/src/submission_checker/models/regions.py +++ b/src/submission_checker/models/regions.py @@ -20,6 +20,8 @@ import math from dataclasses import dataclass +from ..messages import Invalid + __all__ = [ "MARGIN_SATISFIES_HIGH_CONCURRENCY", "MIN_DURATION_MS", @@ -118,7 +120,7 @@ def compute_regions(c_max: int, c_min: int) -> Regions: A :class:`Regions` instance with inclusive ``[start, end]`` boundaries. Raises: - ValueError: If *c_max* is not greater than 32, or *c_min* is outside 1–32. + Invalid: If *c_max* is not greater than 32, or *c_min* is outside 1–32. Example:: @@ -126,13 +128,12 @@ def compute_regions(c_max: int, c_min: int) -> Regions: print(regions.low_concurrency) # 17–26 """ if not 1 <= c_min <= ULTRA_LOW_CONCURRENCY_MAX: - raise ValueError( - f"Minimum concurrency must be between 1 and {ULTRA_LOW_CONCURRENCY_MAX} " - f"(inclusive), got {c_min}" + raise Invalid( + "region-computation", "c-min-range", limit=ULTRA_LOW_CONCURRENCY_MAX, c_min=c_min ) if c_max <= ULTRA_LOW_CONCURRENCY_MAX: - raise ValueError( - f"Maximum Supported Concurrency must be > {ULTRA_LOW_CONCURRENCY_MAX}, got {c_max}" + raise Invalid( + "region-computation", "c-max-too-low", limit=ULTRA_LOW_CONCURRENCY_MAX, c_max=c_max ) interval = math.log2(c_max - c_min) / 3 diff --git a/src/submission_checker/models/results.py b/src/submission_checker/models/results.py index 3565922..1ec374f 100644 --- a/src/submission_checker/models/results.py +++ b/src/submission_checker/models/results.py @@ -2,14 +2,15 @@ from __future__ import annotations +from collections.abc import Mapping from enum import Enum from pathlib import Path from pydantic import BaseModel, ConfigDict, Field, computed_field -from ..messages import render +from ..messages import SHARED, catalog, render -__all__ = ["CheckResult", "Report", "Severity", "err", "ok", "warn"] +__all__ = ["CheckResult", "Report", "Severity", "err", "field_err", "ok", "warn"] class Severity(str, Enum): @@ -86,6 +87,36 @@ def err(rule: str, key: str, path: Path | None = None, /, **params: object) -> C return _result(Severity.ERROR, rule, key, path, params) +def field_err( + rule: str, path: Path, field: str, source: str, key: str, params: Mapping[str, object] +) -> CheckResult: + """An ERROR for one field of a file that failed validation. + + *source* and *key* name the catalog message saying what is wrong: a rule's own + message raised as :class:`~submission_checker.messages.Invalid`, or a + :data:`~submission_checker.messages.PYDANTIC` one. The shared ``field-invalid`` + message places it in the file and field; the result keeps its fix and spec. + """ + problem = render(source, key, params) + where = {"file": path.name, "problem": problem.text} + located = ( + render(SHARED, "field-invalid", {**where, "field": field}) + if field + else render(SHARED, "file-invalid", where) + ) + entry = catalog().get(rule) + return CheckResult( + rule=rule, + key=key, + title=entry.title if entry is not None else rule, + message=located.text, + fix=problem.fix, + severity=Severity.ERROR, + path=path, + spec_ref=problem.spec or (entry.spec if entry is not None else ""), + ) + + class Report(BaseModel): """Aggregated results from all checks against a submission. diff --git a/tests/submission_checker/test_checker.py b/tests/submission_checker/test_checker.py index 019b063..d8de634 100644 --- a/tests/submission_checker/test_checker.py +++ b/tests/submission_checker/test_checker.py @@ -11,6 +11,7 @@ from submission_checker import layout from submission_checker.checker import SubmissionChecker +from submission_checker.messages import Invalid from submission_checker.models import CheckResult, Report, Severity from .conftest import TEST_SUBMISSIONS @@ -623,16 +624,17 @@ def test_invalid_point_yaml_is_skipped(self, tmp_path): assert _errors(report, "point-config-valid") def test_region_computation_error(self, tmp_path): - """region-computation error when compute_regions raises ValueError.""" + """region-computation error when compute_regions raises Invalid.""" # compute_regions only raises if M <= 32, but SystemDescription enforces M > 32. - # Patch compute_regions to simulate an unexpected ValueError. + # Patch compute_regions to simulate the error. root = _build_submission(tmp_path) with patch( "submission_checker.checker.compute_regions", - side_effect=ValueError("C_max must be > 32"), + side_effect=Invalid("region-computation", "c-max-too-low", limit=32, c_max=16), ): report = _check(root) - assert _errors(report, "region-computation") + [error] = _errors(report, "region-computation") + assert error.message == "Maximum Supported Concurrency must be greater than 32, got 16" def test_model_name_matches_dir(self, tmp_path): """ok when point.yaml's model_name matches the model directory name.""" diff --git a/tests/submission_checker/test_loader.py b/tests/submission_checker/test_loader.py index 4890fa4..b8132fe 100644 --- a/tests/submission_checker/test_loader.py +++ b/tests/submission_checker/test_loader.py @@ -45,7 +45,7 @@ def test_load_system_description_schema_error(tmp_path): assert model is None assert all(r.severity == Severity.ERROR for r in results) assert len(results) > 1 # all missing fields reported, not just the first - assert any("Validation error" in r.message for r in results) + assert any(r.message.endswith("required, but missing") for r in results) # --------------------------------------------------------------------------- @@ -88,7 +88,7 @@ def test_load_point_config_schema_error(tmp_path): assert model is None assert len(results) >= 1 assert all(r.severity == Severity.ERROR for r in results) - assert all("Validation error" in r.message for r in results) + assert "bad.yaml, field `concurrency`: required, but missing" in [r.message for r in results] def test_load_point_config_valid_returns_check_results(tmp_path): @@ -143,7 +143,7 @@ def test_load_result_summary_schema_error(tmp_path): assert model is None assert len(results) >= 1 assert all(r.severity == Severity.ERROR for r in results) - assert any("Validation error" in r.message for r in results) + assert any(r.message.startswith("bad.json, field `") for r in results) # --------------------------------------------------------------------------- @@ -166,7 +166,7 @@ def test_load_accuracy_result_schema_error(tmp_path): assert model is None assert len(results) >= 1 assert all(r.severity == Severity.ERROR for r in results) - assert any("Validation error" in r.message for r in results) + assert any(r.message.startswith("bad.json, field `cnn_dailymail") for r in results) # --------------------------------------------------------------------------- diff --git a/tests/submission_checker/test_messages.py b/tests/submission_checker/test_messages.py index d84d5dc..d3ac9f2 100644 --- a/tests/submission_checker/test_messages.py +++ b/tests/submission_checker/test_messages.py @@ -7,22 +7,46 @@ hold both sides together: each key the code uses exists, each call passes exactly the placeholders its template needs, and the catalog carries nothing the code no longer produces. + +Validation errors are held to the same standard. The file models raise +:class:`~submission_checker.messages.Invalid` rather than ``ValueError``, and every +Pydantic error type their schemas can produce has a ``_pydantic`` message. """ from __future__ import annotations import ast +import json from dataclasses import dataclass from pathlib import Path import pytest +import yaml +from pydantic import BaseModel +from pydantic_core._pydantic_core import list_all_errors import submission_checker from submission_checker import messages -from submission_checker.messages import SHARED, MessageCatalogError, placeholders +from submission_checker.messages import PYDANTIC, SHARED, MessageCatalogError, placeholders +from submission_checker.models.file import ( + AccuracyResult, + PointConfig, + PointSummary, + SystemDescription, + SystemPower, +) +from submission_checker.models.loader import ( + load_accuracy_result, + load_point_config, + load_result_summary, + load_system_description, + load_system_power, +) _SRC = Path(submission_checker.__file__).parent -_RESULT_FUNCS = {"ok", "warn", "err", "_ok", "_warn", "_err", "fragment"} +_RESULT_FUNCS = {"ok", "warn", "err", "_ok", "_warn", "_err", "fragment", "Invalid"} +#: Catalog sections that no single rule's code produces. +_SECTIONS = {SHARED, PYDANTIC} #: Calls whose rule is a variable, and the rules it can take. The coverage loop #: picks one of three region rules; the loaders and the score helper report under @@ -138,7 +162,7 @@ def test_no_message_is_unused(self) -> None: unused = [ f"{rule}.{key}" for rule, entry in _CATALOG.items() - if rule != SHARED + if rule not in _SECTIONS for key in entry.messages if (rule, key) not in used ] @@ -149,7 +173,7 @@ def test_no_message_is_unused(self) -> None: class TestCatalog: def test_every_rule_has_a_title_and_spec(self) -> None: for rule, entry in _CATALOG.items(): - if rule == SHARED: + if rule in _SECTIONS: continue assert entry.title and entry.title != rule, rule assert entry.spec.startswith("§"), f"{rule}: spec {entry.spec!r}" @@ -189,3 +213,169 @@ def test_outside_strict_mode_a_bad_message_degrades( rendered = messages.render("model-name-valid", "no-such-key", {"name": "x"}) assert rendered.text == "model-name-valid (no-such-key): name='x'" assert rendered.title == "Benchmark model name" + + +#: The models each loader validates, and the Pydantic error types each kind of core +#: schema node can produce. A node kind missing from both maps fails the test, so a +#: new field type is classified before it reaches a submitter. +_FILE_MODELS: list[type[BaseModel]] = [ + AccuracyResult, + PointConfig, + PointSummary, + SystemDescription, + SystemPower, +] +_NODE_ERRORS = { + "model-field": {"missing"}, + "model": {"model_type"}, + "model-fields": {"model_attributes_type"}, + "str": {"string_type"}, + "int": {"int_type", "int_parsing", "int_from_float"}, + "float": {"float_type", "float_parsing"}, + "bool": {"bool_type", "bool_parsing"}, + "literal": {"literal_error"}, + "enum": {"enum"}, + "dict": {"dict_type"}, + "list": {"list_type"}, +} +_SILENT_NODES = { + "any", + "computed-field", + "default", + "definition-ref", + "definitions", + "function-after", + "function-before", + "no-info", + "nullable", + "union", + "with-info", +} +_CONSTRAINT_ERRORS = { + "gt": "greater_than", + "ge": "greater_than_equal", + "lt": "less_than", + "le": "less_than_equal", +} + + +def _reachable_errors(schema: object, found: set[str]) -> None: + if isinstance(schema, list | tuple): + for item in schema: + _reachable_errors(item, found) + return + if not isinstance(schema, dict): + return + kind = schema.get("type") + if isinstance(kind, str): + assert kind in _NODE_ERRORS or kind in _SILENT_NODES, f"unclassified schema node {kind!r}" + found |= _NODE_ERRORS.get(kind, set()) + found |= {error for name, error in _CONSTRAINT_ERRORS.items() if name in schema} + if "min_length" in schema: + found.add("string_too_short" if kind == "str" else "too_short") + if "max_length" in schema: + found.add("string_too_long" if kind == "str" else "too_long") + if "pattern" in schema: + found.add("string_pattern_mismatch") + if schema.get("allow_inf_nan") is False: + found.add("finite_number") + if (schema.get("config") or {}).get("extra_fields_behavior") == "forbid": + found.add("extra_forbidden") + for name, value in schema.items(): + if name not in {"serialization", "metadata"}: # output and annotations, not validation + _reachable_errors(value, found) + + +@pytest.mark.unit +class TestValidationWording: + def test_models_raise_invalid_not_value_error(self) -> None: + bare = [] + for path in sorted((_SRC / "models").rglob("*.py")): + for node in ast.walk(ast.parse(path.read_text(encoding="utf-8"))): + if ( + isinstance(node, ast.Raise) + and isinstance(node.exc, ast.Call) + and isinstance(node.exc.func, ast.Name) + and node.exc.func.id in {"ValueError", "AssertionError", "TypeError"} + ): + bare.append(f"{path.relative_to(_SRC)}:{node.lineno}") + assert not bare, "raise Invalid(rule, key, ...) instead:\n" + "\n".join(bare) + + def test_pydantic_messages_match_the_reachable_errors(self) -> None: + found: set[str] = set() + for model in _FILE_MODELS: + _reachable_errors(model.__pydantic_core_schema__, found) + assert "missing" in found # guards against the walk matching nothing + worded = set(_CATALOG[PYDANTIC].messages) + assert not found - worded, f"add {PYDANTIC} messages for: {sorted(found - worded)}" + assert not worded - found, f"no file model can raise: {sorted(worded - found)}" + + def test_pydantic_messages_use_only_the_error_context(self) -> None: + contexts = {e["type"]: set(e["example_context"] or {}) for e in list_all_errors()} + for key, message in _CATALOG[PYDANTIC].messages.items(): + assert key in contexts, f"{key} is not a Pydantic error type" + extra = _needs(message) - contexts[key] - {"input"} + assert not extra, f"{PYDANTIC}.{key} uses {sorted(extra)}" + + +_BAD_FILES = [ + pytest.param( + load_system_description, + "system_desc.json", + { + "division": "open", + "publication_status": "available", + "system_availability_status": "preview", + }, + "system_desc.json: the availability fields disagree" + " (publication_status='available', system_availability_status='preview')", + id="model-level-invalid", + ), + pytest.param( + load_system_power, + "system_power.json", + { + "node_sets": [ + {"published_power": {"value_w": 1, "source_type": "x", "source": "y", "bogus": 1}} + ] + }, + "system_power.json, field `node_sets.0.published_power.bogus`: not a recognised field", + id="extra-forbidden", + ), + pytest.param( + load_point_config, + "point.yaml", + {"warmup": {"requests_completed": 5, "requests_issued": "two"}}, + "point.yaml, field `warmup.requests_issued`: expected a whole number, got 'two'", + id="int-parsing", + ), + pytest.param( + load_result_summary, + "results.json", + {"ttft": {"percentiles": {"p99": 1.0}}}, + "results.json, field `ttft.percentiles`: 'p99' is not a percentile between 0 and 100", + id="field-level-invalid", + ), + pytest.param( + load_accuracy_result, + "accuracy.json", + {"accuracy_scores": [{"dataset_name": "a", "score": 1}, {"dataset_name": "a", "score": 1}]}, + "accuracy.json: dataset 'a' appears more than once", + id="before-validator-invalid", + ), +] + + +@pytest.mark.unit +@pytest.mark.parametrize(("loader", "name", "content", "expected"), _BAD_FILES) +def test_validation_errors_are_worded_by_the_catalog( + tmp_path: Path, loader, name: str, content: dict, expected: str +) -> None: + path = tmp_path / name + path.write_text(yaml.safe_dump(content) if name.endswith(".yaml") else json.dumps(content)) + model, results = loader(path) + assert model is None + assert expected in [r.message for r in results] + for result in results: # strict mode has rendered every one from the catalog + assert "Value error" not in result.message + assert "Input should" not in result.message diff --git a/tests/submission_checker/test_native_formats.py b/tests/submission_checker/test_native_formats.py index 124a03d..bf39aa3 100644 --- a/tests/submission_checker/test_native_formats.py +++ b/tests/submission_checker/test_native_formats.py @@ -153,7 +153,7 @@ def test_duplicate_percentile_aliases_must_agree(same_value): if same_value: assert PointSummary.model_validate(payload).tpot.percentiles == {"90": 100} else: - with pytest.raises(ValidationError, match="Conflicting values"): + with pytest.raises(ValidationError, match="give different values"): PointSummary.model_validate(payload)