diff --git a/CHANGELOG.md b/CHANGELOG.md index 3c4fd08..8e496e5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -83,6 +83,18 @@ different event from one that moved because it was wrong. ### Fixed +- `plumbline report` combined artifacts from different datasets under the first + one's hash, as if their figures were comparable, and gave two runs of one + adapter identical headings (#43). Different datasets are now refused unless + `--allow-mixed` is passed, when every arm names its dataset and row count; + arms that share an adapter name are told apart by model, then by time. +- A credential inside a list in the run config reached the artifact, because + redaction walked only dictionaries; it now walks lists and tuples too (#46). + The report and the artifact named the dataset by the path as typed, so an + absolute path shared a username and a directory layout; a dataset inside the + working directory is named relative to it and one outside by its file name, + and the hash still says which rows they were. A backtick in a model name no + longer breaks out of its code span. - 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 diff --git a/src/plumbline/cli.py b/src/plumbline/cli.py index e89de55..dca08e7 100644 --- a/src/plumbline/cli.py +++ b/src/plumbline/cli.py @@ -13,6 +13,7 @@ from __future__ import annotations +import dataclasses import sys from collections.abc import Callable from functools import partial @@ -107,7 +108,7 @@ def run( # so a mistake costs nothing. if report is not None and report.is_dir(): _fail(f"--report {report} is a directory; give it a file path, such as {report}/report.md") - load = _load(dataset, data_format) + load = dataclasses.replace(_load(dataset, data_format), source=_shown(dataset)) _status(load.statement()) for refusal in load.refusals: _status(f" refused {refusal}") @@ -135,7 +136,7 @@ def run( guard=execute.CostGuard(max_cost_usd=max_cost_usd, max_cases=max_cases), pricing_table=_pricing(pricing), workers=workers, - extra_config={"dataset": str(dataset), "format": data_format}, + extra_config={"dataset": _shown(dataset), "format": data_format}, ) ) @@ -178,16 +179,26 @@ def report( float | None, typer.Option("--error-cost", help="What one wrong answer costs, in USD.") ] = None, n_boot: Annotated[int, typer.Option("--boot", min=1, help="Bootstrap draws per null.")] = 2000, + allow_mixed: Annotated[ + bool, + typer.Option( + "--allow-mixed", + help="Allow runs over different datasets in one document; each arm names its own.", + ), + ] = False, ) -> None: """Render one document from runs that already happened.""" results = [_guard(partial(execute.RunResult.read, path)) for path in artifacts] - document = markdown.render( - results, - options=markdown.ReportOptions( - n_boot=n_boot, - cost_escalation_usd=escalation_cost, - cost_error_usd=error_cost, - ), + document = _guard( + lambda: markdown.render( + results, + options=markdown.ReportOptions( + n_boot=n_boot, + cost_escalation_usd=escalation_cost, + cost_error_usd=error_cost, + allow_mixed_datasets=allow_mixed, + ), + ) ) if out is not None: out.parent.mkdir(parents=True, exist_ok=True) @@ -215,6 +226,20 @@ def version() -> None: typer.echo(__version__) +def _shown(dataset: Path) -> str: + """The dataset as a report or an artifact names it. + + Relative to the working directory when it is inside it, as the caller most + likely typed it; otherwise the file name alone. An absolute path in a report + that gets shared names the user and their directory layout, and the dataset + hash already says which rows these were. + """ + try: + return str(dataset.resolve().relative_to(Path.cwd().resolve())) + except ValueError: + return dataset.name + + def _load(dataset: Path, data_format: str) -> loader.LoadReport: if data_format not in FORMATS: _fail(f"--format must be one of {list(FORMATS)!r}, got {data_format!r}") diff --git a/src/plumbline/report/markdown.py b/src/plumbline/report/markdown.py index 67327e2..6cc8ccf 100644 --- a/src/plumbline/report/markdown.py +++ b/src/plumbline/report/markdown.py @@ -91,6 +91,13 @@ class ReportOptions: min_threshold_rows: int = 200 """Below this, a threshold is fitted to noise and none is printed.""" + allow_mixed_datasets: bool = False + """Put runs over different datasets in one document, each arm naming its own. + + Off by default: figures from different datasets are not comparable, and a + document that lays them side by side invites exactly that comparison. + """ + @property def has_costs(self) -> bool: return self.cost_escalation_usd is not None and self.cost_error_usd is not None @@ -111,6 +118,16 @@ def render( if not results: raise ValueError("no runs to report on") options = options or ReportOptions() + datasets = sorted({result.dataset_hash for result in results}) + if len(datasets) > 1 and not options.allow_mixed_datasets: + named = ", ".join(f"`{digest[:8]}`" for digest in datasets) + raise ValueError( + f"these runs are over {len(datasets)} different datasets ({named}), and figures " + "from different datasets are not comparable. Report them separately, or allow " + "mixed datasets (--allow-mixed) to put them in one document with each arm " + "naming its dataset." + ) + headings = _headings(results, mixed=len(datasets) > 1) lines = _header(results, load, options) lines.extend(_how_to_read()) @@ -123,7 +140,7 @@ def render( continue lines.extend(["", "---", "", f"## {GROUP_TITLES[semantics]}", "", GROUP_NOTES[semantics]]) for result in group: - lines.extend(_arm(result, options)) + lines.extend(_arm(result, options, headings[id(result)])) return "\n".join(lines).rstrip() + "\n" @@ -132,6 +149,17 @@ def _header( results: Sequence[RunResult], load: LoadReport | None, options: ReportOptions ) -> list[str]: first = results[0] + datasets = sorted({result.dataset_hash for result in results}) + if len(datasets) > 1: + named = ", ".join(f"`{digest[:8]}`" for digest in datasets) + return [ + "# plumbline report", + "", + f"Generated {options.clock.isoformat()} against {len(datasets)} datasets " + f"({named}). {len(results)} arm(s), each naming its own dataset and row " + "count. Figures from different datasets are not comparable.", + "", + ] rows = ", ".join(sorted({f"{result.dataset_rows} rows" for result in results})) return [ "# plumbline report", @@ -142,6 +170,33 @@ def _header( ] +def _headings(results: Sequence[RunResult], *, mixed: bool) -> dict[int, str]: + """A heading per arm that tells it apart from every other. + + The adapter name alone, when it is unique; then the model; then the run's + time. With mixed datasets every arm also names its dataset and row count. + """ + + def label(result: RunResult, depth: int) -> str: + parts = [result.adapter_name] + if depth >= 1: + parts.append(result.model_requested) + if depth >= 2: + parts.append(result.timestamp) + return ", ".join(parts) + + chosen: dict[int, str] = {} + for result in results: + for depth in range(3): + candidate = label(result, depth) + if sum(1 for other in results if label(other, depth) == candidate) == 1 or depth == 2: + break + if mixed: + candidate += f", dataset `{result.dataset_hash[:8]}` ({result.dataset_rows} rows)" + chosen[id(result)] = candidate + return chosen + + def _how_to_read() -> list[str]: return [ "## How to read this", @@ -171,7 +226,7 @@ def _dataset_section(load: LoadReport) -> list[str]: return lines -def _arm(result: RunResult, options: ReportOptions) -> list[str]: +def _arm(result: RunResult, options: ReportOptions, heading: str) -> list[str]: scoreable = [ record for record in result.records if record.question_type in SUPPORTED_QUESTION_TYPES ] @@ -179,7 +234,7 @@ def _arm(result: RunResult, options: ReportOptions) -> list[str]: successes = [record for record in scoreable if record.prediction is not None] failures = [record for record in scoreable if record.prediction is None] - lines = ["", f"### {result.adapter_name}", "", *_provenance(result, options)] + lines = ["", f"### {heading}", "", *_provenance(result, options)] if excluded: lines.append( f"- **Excluded**: {excluded} rows of an unsupported question type were not scored." @@ -492,10 +547,11 @@ def _scaled(prediction: Prediction, temperature: float) -> float: def _provenance(result: RunResult, options: ReportOptions) -> list[str]: reported = result.model_reported or "not reported" + endpoint = result.config.get("endpoint") line = ( - f"- **Model**: requested `{result.model_requested}`, reported `{reported}`" - + (f", revision `{result.revision}`" if result.revision else "") - + (f", endpoint `{endpoint}`" if (endpoint := result.config.get("endpoint")) else "") + f"- **Model**: requested {_code(result.model_requested)}, reported {_code(reported)}" + + (f", revision {_code(result.revision)}" if result.revision else "") + + (f", endpoint {_code(str(endpoint))}" if endpoint else "") + "." ) hits = result.cache_stats.get("hits", 0) @@ -504,6 +560,19 @@ def _provenance(result: RunResult, options: ReportOptions) -> list[str]: return [line] +def _code(value: str) -> str: + """A markdown code span that holds ``value`` whatever it contains. + + A model string is whatever the caller passed, and a backtick inside a single + backtick span ends the span early. Markdown allows a longer fence: two + backticks with a space inside each, which a single backtick cannot close. + """ + if "`" not in value: + return f"`{value}`" + fence = "``" if "``" not in value else "```" + return f"{fence} {value} {fence}" + + def _asked_as(records: Sequence[CaseRecord]) -> list[str]: """State how each kind of row was actually asked, and flag any mismatch.""" pairs: dict[tuple[str, str], int] = {} diff --git a/src/plumbline/runner/execute.py b/src/plumbline/runner/execute.py index 4b78336..2cc63a2 100644 --- a/src/plumbline/runner/execute.py +++ b/src/plumbline/runner/execute.py @@ -450,13 +450,23 @@ def redact(config: Mapping[str, Any]) -> dict[str, Any]: for key, value in config.items(): if any(marker in key.lower() for marker in _SECRET_MARKERS): cleaned[key] = "[redacted]" - elif isinstance(value, Mapping): - cleaned[key] = redact(value) else: - cleaned[key] = value + cleaned[key] = _redacted(value) return cleaned +def _redacted(value: Any) -> Any: + """A value with every mapping inside it redacted, through lists and tuples too. + + A key inside a list of endpoints is as much a secret as one at the top level. + """ + if isinstance(value, Mapping): + return redact(value) + if isinstance(value, list | tuple): + return [_redacted(item) for item in value] + return value + + def estimate_run_cost( cases: Sequence[Case], pricing: Pricing | None, diff --git a/tests/test_cli.py b/tests/test_cli.py index 5118979..c23f889 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -348,3 +348,43 @@ def test_a_run_where_every_case_fails_exits_non_zero_and_says_why(tmp_path: Path assert "every case failed" in result.stderr.lower() assert "accuracy" in result.stderr assert "for the same reason" in report.read_text(encoding="utf-8") + + +def test_a_dataset_outside_the_working_directory_is_named_not_located(tmp_path: Path) -> None: + """An absolute path in a shared report names the user and their layout (#46).""" + dataset = a_dataset(tmp_path / "d.jsonl", n_rows=12) + report = tmp_path / "report.md" + result = invoke( + "run", + str(dataset), + "--results", + str(tmp_path / "r"), + "--report", + str(report), + "--boot", + "100", + ) + + assert result.exit_code == 0 + assert str(tmp_path) not in report.read_text(encoding="utf-8") + artifact = json.loads(next((tmp_path / "r").glob("*.json")).read_text(encoding="utf-8")) + assert artifact["config"]["dataset"] == "d.jsonl" + + +def test_report_refuses_mixed_datasets_unless_allowed(tmp_path: Path) -> None: + for name, rows in (("a", 12), ("b", 13)): + invoke( + "run", + str(a_dataset(tmp_path / f"{name}.jsonl", n_rows=rows)), + "--results", + str(tmp_path / "r"), + "--boot", + "100", + ) + artifacts = [str(path) for path in sorted((tmp_path / "r").glob("*.json"))] + + refused = invoke("report", *artifacts, "--boot", "100") + allowed = invoke("report", *artifacts, "--boot", "100", "--allow-mixed") + + assert refused.exit_code == 1 and "different datasets" in refused.output + assert allowed.exit_code == 0 and "2 datasets" in allowed.stdout diff --git a/tests/test_report.py b/tests/test_report.py index 88bc3f6..3ffe107 100644 --- a/tests/test_report.py +++ b/tests/test_report.py @@ -471,3 +471,44 @@ def test_a_run_with_no_figures_names_a_shared_failure_reason(same: bool) -> None else: assert "8 different reasons, are in the artifact" in text assert "broke on" not in text + + +# Which runs belong in one document (#43), and what a heading or a code span can hold (#46) + + +def test_artifacts_from_different_datasets_are_refused_in_one_report() -> None: + first, second = a_run(n_cases=40), a_run(n_cases=41) + assert first.dataset_hash != second.dataset_hash + + with pytest.raises(ValueError, match="different datasets"): + render(first, second) + + +def test_mixed_datasets_can_be_allowed_and_then_every_arm_names_its_own() -> None: + first, second = a_run(n_cases=40), a_run(n_cases=41) + options = markdown.ReportOptions(n_boot=200, today=date(2026, 9, 20), allow_mixed_datasets=True) + + text = markdown.render([first, second], options=options) + + assert "2 datasets" in text and "not comparable" in text + assert f"dataset `{first.dataset_hash[:8]}`" in text + assert f"dataset `{second.dataset_hash[:8]}`" in text + + +def test_two_arms_with_one_adapter_name_get_headings_that_tell_them_apart() -> None: + cases = make_cases(40, labels=LABELS) + fast = execute.run(MockAdapter(gold_by_text(cases), seed=5, model_requested="mock-fast"), cases) + slow = execute.run(MockAdapter(gold_by_text(cases), seed=5, model_requested="mock-slow"), cases) + + headings = [line for line in render(fast, slow).splitlines() if line.startswith("### ")] + + assert len(headings) == len(set(headings)) == 2 + + +def test_a_backtick_in_a_model_name_cannot_break_out_of_its_code_span() -> None: + cases = make_cases(40, labels=LABELS) + odd = execute.run(MockAdapter(gold_by_text(cases), seed=5, model_requested="we`ird"), cases) + + line = next(line for line in render(odd).splitlines() if line.startswith("- **Model**")) + + assert "`` we`ird ``" in line diff --git a/tests/test_runner.py b/tests/test_runner.py index ae92f7e..9529e96 100644 --- a/tests/test_runner.py +++ b/tests/test_runner.py @@ -796,3 +796,18 @@ def test_a_second_run_never_overwrites_the_first_artifact(tmp_path: Path) -> Non assert len(list(directory.glob("*.json"))) == 2 assert execute.RunResult.read(first).probability_semantics == "calibrated_claim" assert execute.RunResult.read(second).probability_semantics == "restricted_softmax" + + +def test_a_credential_inside_a_list_is_redacted_too() -> None: + """redact walked dicts only, so a key inside a list of endpoints reached disk (#46).""" + config = { + "endpoints": [{"api_key": "sk-live-123", "url": "https://a"}], + "pair": ({"token": "t"},), + } + + cleaned = execute.redact(config) + + assert cleaned["endpoints"][0]["api_key"] == "[redacted]" + assert cleaned["endpoints"][0]["url"] == "https://a" + assert cleaned["pair"][0]["token"] == "[redacted]" + assert "sk-live-123" not in json.dumps(cleaned)