diff --git a/CHANGELOG.md b/CHANGELOG.md index e3faf7f..3c4fd08 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -83,6 +83,17 @@ different event from one that moved because it was wrong. ### Fixed +- The JevBench loader skipped the duplicate-id check the JSONL loader makes, so + repeated ids loaded silently (#44). Both loaders now share it. The dataset + loader read a byte order mark as part of the first row and refused it, turned + a label of `null`, `true` or `1` into the text "None", "True" or "1", accepted + empty labels and descriptions of options that do not exist, and failed on a + non-UTF-8 file without naming it (#45). It now reads the mark as nothing, + refuses each of the others with the reason, and names the file and line that + is not UTF-8. The pricing loader accepted NaN, infinite and negative prices, + `as_of` values such as `20260901` or `2026-W36-1`, and dates in the future, + which kept the report from ever calling a price stale; each is refused now, + and a byte order mark is read as nothing there too. - The CLI accepted options that misbehaved or crashed (#41): `--limit 0` ran every row and a negative limit sliced from the end, `--boot 0` failed with a traceback after the run had been paid for, an option the adapter does not diff --git a/docs/datasets.md b/docs/datasets.md index debe0db..a6d1c82 100644 --- a/docs/datasets.md +++ b/docs/datasets.md @@ -7,8 +7,8 @@ for exactly this, so a clone never commits it. ## The format -One JSON object per line (JSONL), UTF-8. Blank lines are skipped. Every other -line is one case. +One JSON object per line (JSONL), UTF-8, with or without a byte order mark. +Blank lines are skipped. Every other line is one case. | field | required | what it is | |---|---|---| @@ -46,7 +46,11 @@ A row that cannot be scored honestly is refused with its line number, and the rest of the file still runs. `--strict` refuses the whole run instead. - A missing required field, an empty `id` or `text`, or `labels` that is not a - list of at least two distinct entries. + list of at least two distinct, non-empty strings. A label written as `null`, + `true` or a number is refused rather than turned into the text `"None"`, + `"True"` or `"1"`. +- A `label_descriptions` entry for something that is not one of the row's + options, or a description that is not a non-empty string. - A `gold_label` that is not one of the row's own `labels`. Scoring it would mark every model wrong on that row, which reads as a model failure and is a data error. @@ -55,8 +59,9 @@ rest of the file still runs. `--strict` refuses the whole run instead. - An `id` already used earlier in the file. - A line that is not a JSON object. -A file with no rows at all, or a path that does not exist, is an error rather -than an empty run. +A file with no rows at all, a path that does not exist, or a file that is not +UTF-8 is an error rather than an empty run; the last names the line with the +byte that is not. ## Running it diff --git a/src/plumbline/config.py b/src/plumbline/config.py index 5c32cf7..b2cdb4e 100644 --- a/src/plumbline/config.py +++ b/src/plumbline/config.py @@ -20,6 +20,8 @@ from __future__ import annotations import json +import math +import re from collections.abc import Mapping from datetime import date from pathlib import Path @@ -122,6 +124,8 @@ def _anthropic(input_usd: float, output_usd: float) -> Pricing: #: carries it has not been filled in, so it is refused rather than priced. TEMPLATE_AS_OF = "YYYY-MM-DD" +_ISO_DATE = re.compile(r"\d{4}-\d{2}-\d{2}") + def load_pricing_file(path: Path | str) -> PricingTable: """Read a pricing table the operator wrote, requiring provenance on every entry. @@ -138,7 +142,7 @@ def load_pricing_file(path: Path | str) -> PricingTable: """ path = Path(path) try: - raw = json.loads(path.read_text(encoding="utf-8")) + raw = json.loads(path.read_text(encoding="utf-8-sig")) except FileNotFoundError: raise PricingConfigError( f"no pricing file at {path}. plumbline never guesses a pricing file " @@ -245,6 +249,15 @@ def _price(name: str, value: Mapping[str, Any], field: str, path: Path) -> float "or null. Use null where the vendor publishes no price; null and 0 are " "different claims and plumbline keeps them apart." ) + if not math.isfinite(number): + raise PricingConfigError( + f"entry {name!r} in {path} has {field}={number!r}; a price must be finite, " + "or every row it prices becomes NaN or infinite." + ) + if number < 0: + raise PricingConfigError( + f"entry {name!r} in {path} has {field}={number!r}; a price cannot be negative." + ) return float(number) @@ -256,11 +269,26 @@ def _as_of(name: str, value: Any, path: Path) -> date: "them; an unedited copy of docs/pricing.example.json is refused rather than " "used to price a run." ) + # fromisoformat also accepts 20260901 and week dates such as 2026-W36-1, + # which nobody writes meaning a date, so the shape is checked first. + if not (isinstance(value, str) and _ISO_DATE.fullmatch(value)): + raise PricingConfigError( + f"entry {name!r} in {path} has as_of={value!r}, which is not an ISO date " + "written YYYY-MM-DD. The date a price was read is what lets the report say the price " + "may be out of date, so it is not optional and not free-form." + ) try: - return date.fromisoformat(str(value)) + read_on = date.fromisoformat(value) except ValueError: raise PricingConfigError( f"entry {name!r} in {path} has as_of={value!r}, which is not an ISO date " "(YYYY-MM-DD). The date a price was read is what lets the report say the " "price may be out of date, so it is not optional and not free-form." ) from None + if read_on > date.today(): + raise PricingConfigError( + f"entry {name!r} in {path} has as_of={value!r}, which is in the future. A " + "price cannot have been read on a day that has not happened, and a future date " + "would keep the report from ever calling it out of date." + ) + return read_on diff --git a/src/plumbline/datasets/loader.py b/src/plumbline/datasets/loader.py index a61c64c..a629928 100644 --- a/src/plumbline/datasets/loader.py +++ b/src/plumbline/datasets/loader.py @@ -131,14 +131,7 @@ def load_jsonl(path: Path | str) -> LoadReport: refusals.append(RowRefusal(number, case_id, str(refused))) continue if case.id in seen: - refusals.append( - RowRefusal( - number, - case.id, - f"duplicate id {case.id!r}; results are keyed by id, so a repeat would " - "overwrite an earlier row", - ) - ) + refusals.append(_duplicate(number, case.id)) continue seen.add(case.id) cases.append(case) @@ -179,6 +172,7 @@ def load_jevbench(path: Path | str) -> LoadReport: cases: list[Case] = [] refusals: list[RowRefusal] = [] types: dict[str, int] = {} + seen: set[str] = set() normalized = 0 dropped_criteria = 0 @@ -199,6 +193,10 @@ def load_jevbench(path: Path | str) -> LoadReport: refusals.append(RowRefusal(number, case_id, str(refused))) continue + if case.id in seen: + refusals.append(_duplicate(number, case.id)) + continue + seen.add(case.id) normalized += int(was_normalized) dropped_criteria += int(lost_criteria) cases.append(case) @@ -206,8 +204,8 @@ def load_jevbench(path: Path | str) -> LoadReport: notes = [ "Translated from JevBench: the case text is the row's question above its state, " "and each row is asked as the question type it states. plumbline's harness, " - "prompts and " - "scoring differ from JevBench's, so these numbers are not comparable with theirs." + "prompts and scoring differ from JevBench's, so these numbers are not comparable " + "with theirs." ] if normalized: notes.append( @@ -248,19 +246,20 @@ def _read_rows(path: Path) -> list[tuple[int, Mapping[str, Any] | str]]: ) rows: list[tuple[int, Mapping[str, Any] | str]] = [] - with path.open(encoding="utf-8") as handle: - for number, line in enumerate(handle, start=1): - if not line.strip(): - continue # A blank line is formatting, not a row. - try: - parsed = json.loads(line) - except json.JSONDecodeError as broken: - rows.append((number, f"line is not valid JSON: {broken.msg}")) - continue - if not isinstance(parsed, dict): - rows.append((number, f"line is a {type(parsed).__name__}, expected an object")) - continue - rows.append((number, parsed)) + # Decoded line by line, so a file that is not UTF-8 is named with the line + # that is not. The first line is read as utf-8-sig, so a byte order mark + # (which Windows editors and spreadsheet exports write) is read as nothing + # rather than refusing the first row. + for number, raw in enumerate(path.read_bytes().splitlines(keepends=True), start=1): + try: + line = raw.decode("utf-8-sig" if number == 1 else "utf-8") + except UnicodeDecodeError as undecodable: + raise DatasetError( + f"{path} is not UTF-8 text: line {number} holds the byte " + f"{raw[undecodable.start]:#04x}, which UTF-8 does not allow. Save the " + "file as UTF-8 and load it again." + ) from None + _parse_line(rows, number, line) if not rows: raise DatasetError( @@ -270,6 +269,31 @@ def _read_rows(path: Path) -> list[tuple[int, Mapping[str, Any] | str]]: return rows +def _parse_line(rows: list[tuple[int, Mapping[str, Any] | str]], number: int, line: str) -> None: + """One line of the file: a row, a refusal to record, or nothing if it is blank.""" + if not line.strip(): + return # A blank line is formatting, not a row. + try: + parsed = json.loads(line) + except json.JSONDecodeError as broken: + rows.append((number, f"line is not valid JSON: {broken.msg}")) + return + if not isinstance(parsed, dict): + rows.append((number, f"line is a {type(parsed).__name__}, expected an object")) + return + rows.append((number, parsed)) + + +def _duplicate(number: int, case_id: str) -> RowRefusal: + """The refusal for an id already seen, the same from either loader.""" + return RowRefusal( + number, + case_id, + f"duplicate id {case_id!r}; results are keyed by id, so a repeat would " + "overwrite an earlier row", + ) + + def _case_from_record(record: Mapping[str, Any]) -> Case: """Build one Case, or refuse with the reason spelled out.""" for name in ("id", "text", "labels", "gold_label"): @@ -287,7 +311,15 @@ def _case_from_record(record: Mapping[str, Any]) -> Case: raw_labels = record["labels"] if not isinstance(raw_labels, Sequence) or isinstance(raw_labels, str | bytes): raise DatasetError("field 'labels' must be a list of strings") - labels = tuple(str(label) for label in raw_labels) + # Each label is taken as written. str() would have turned null into "None" + # and true into "True", and loaded an option nobody wrote. + for label in raw_labels: + if not isinstance(label, str) or not label.strip(): + raise DatasetError( + f"every label must be a non-empty string, and {label!r} is not; labels are " + "the options the model chooses between, as written" + ) + labels = tuple(raw_labels) if len(labels) < 2: raise DatasetError(f"a case needs at least 2 labels, got {len(labels)}") if len(set(labels)) != len(labels): @@ -302,8 +334,21 @@ def _case_from_record(record: Mapping[str, Any]) -> Case: ) descriptions = record.get("label_descriptions") - if descriptions is not None and not isinstance(descriptions, Mapping): - raise DatasetError("field 'label_descriptions' must be an object") + if descriptions is not None: + if not isinstance(descriptions, Mapping): + raise DatasetError("field 'label_descriptions' must be an object") + unknown = sorted(str(key) for key in descriptions if key not in labels) + if unknown: + raise DatasetError( + f"label_descriptions describes {unknown!r}, which are not among this row's " + f"options {list(labels)!r}; a description has to belong to an option" + ) + for option, description in descriptions.items(): + if not isinstance(description, str) or not description.strip(): + raise DatasetError( + f"label_descriptions gives {option!r} the description {description!r}; " + "each description is a non-empty string" + ) question_type = record.get("question_type", "choice") if question_type not in QUESTION_TYPES: diff --git a/tests/test_datasets.py b/tests/test_datasets.py index 5616c5e..da985fa 100644 --- a/tests/test_datasets.py +++ b/tests/test_datasets.py @@ -341,3 +341,86 @@ def test_the_example_in_the_dataset_docs_loads_as_documented(tmp_path) -> None: kinds = sorted(case.question_type for case in report.cases) assert kinds == ["choice", "choice", "choice", "noul", "score"] assert len(report.scoreable) == 4 # the score row loads and is held back + + +# Validation the loaders were missing (#44, #45) + + +def jevbench_row(case_id: str) -> dict: + return { + "id": case_id, + "expected": "yes", + "labels": ["yes", "no"], + "question": {"type": "noul", "instructions": "Well?", "criteria": {}}, + "state": f"something happened to {case_id}", + } + + +def test_the_jevbench_loader_refuses_a_duplicate_id_like_the_jsonl_one(tmp_path: Path) -> None: + path = write_jsonl(tmp_path / "j.jsonl", [jevbench_row("dup"), jevbench_row("dup")]) + + report = loader.load_jevbench(path) + + assert len(report.cases) == 1 + assert len(report.refusals) == 1 and "duplicate id" in report.refusals[0].reason + + +def test_a_byte_order_mark_is_read_as_utf8_rather_than_refused(tmp_path: Path) -> None: + """Windows editors and spreadsheet exports write one; the first row must still load.""" + path = tmp_path / "bom.jsonl" + path.write_text(json.dumps(a_row()) + "\n", encoding="utf-8-sig") + + report = loader.load_jsonl(path) + + assert len(report.cases) == 1 and not report.refusals + + +def test_a_file_that_is_not_utf8_names_the_file_and_the_line(tmp_path: Path) -> None: + path = tmp_path / "latin1.jsonl" + path.write_bytes( + (json.dumps(a_row()) + "\n").encode("utf-8") + # ensure_ascii=False keeps the é as a character, so latin-1 writes it as 0xE9. + + (json.dumps(a_row(id="two", text="café"), ensure_ascii=False) + "\n").encode("latin-1") + ) + + with pytest.raises(DatasetError, match=r"latin1\.jsonl.*line 2"): + loader.load_jsonl(path) + + +@pytest.mark.parametrize( + ("labels", "gold"), + [ + ([None, "a"], "a"), + ([True, False], "True"), + (["a", ""], "a"), + (["a", " "], "a"), + ([1, 2], "1"), + ], + ids=["null", "bools", "empty", "blank", "numbers"], +) +def test_every_label_must_be_a_non_empty_string( + tmp_path: Path, labels: list[object], gold: str +) -> None: + """str() used to turn null into 'None' and true into 'True', and load the row.""" + path = write_jsonl(tmp_path / "d.jsonl", [a_row(labels=labels, gold_label=gold)]) + + report = loader.load_jsonl(path) + + assert not report.cases + assert "non-empty string" in report.refusals[0].reason + + +@pytest.mark.parametrize( + "descriptions", + [{"zzz": "not an option"}, {"billing": None}, {"billing": 3}], + ids=["unknown-key", "null-value", "number-value"], +) +def test_label_descriptions_must_describe_the_options_in_words( + tmp_path: Path, descriptions: dict[str, object] +) -> None: + path = write_jsonl(tmp_path / "d.jsonl", [a_row(label_descriptions=descriptions)]) + + report = loader.load_jsonl(path) + + assert not report.cases + assert "label_descriptions" in report.refusals[0].reason diff --git a/tests/test_pricing_config.py b/tests/test_pricing_config.py index 5fae58d..e1c790b 100644 --- a/tests/test_pricing_config.py +++ b/tests/test_pricing_config.py @@ -158,3 +158,30 @@ def test_a_bool_is_not_accepted_as_a_price(tmp_path: Path) -> None: payload = {"m": {**ENTRY, "output_usd_per_million": True}} with pytest.raises(config.PricingConfigError, match="not a number"): config.load_pricing_file(write(tmp_path, payload)) + + +@pytest.mark.parametrize( + ("field", "value", "expected"), + [ + ("input_usd_per_million", float("nan"), "finite"), + ("output_usd_per_million", float("inf"), "finite"), + ("input_usd_per_million", -1.0, "negative"), + ("as_of", "20260901", "YYYY-MM-DD"), + ("as_of", "2026-W36-1", "YYYY-MM-DD"), + ("as_of", "2099-01-01", "future"), + ], +) +def test_prices_must_be_finite_and_dates_real_and_past( + tmp_path: Path, field: str, value: object, expected: str +) -> None: + """A NaN price costs every row as NaN; a future date defeats the staleness check (#45).""" + path = tmp_path / "pricing.json" + path.write_text(json.dumps({"m": {**ENTRY, field: value}}), encoding="utf-8") + with pytest.raises(config.PricingConfigError, match=expected): + config.load_pricing_file(path) + + +def test_a_pricing_file_with_a_byte_order_mark_loads(tmp_path: Path) -> None: + path = tmp_path / "pricing.json" + path.write_text(json.dumps({"m": ENTRY}), encoding="utf-8-sig") + assert "m" in config.load_pricing_file(path)