From 13b746a851bdbce9f9275be8319b8bf83cd2bb70 Mon Sep 17 00:00:00 2001 From: TMHSDigital <154358121+TMHSDigital@users.noreply.github.com> Date: Wed, 23 Sep 2026 15:29:25 -0400 Subject: [PATCH] fix(cli): validate before anything is sent, put status on stderr, and exit 1 when nothing was measured Options (#41): --limit, --workers, --max-cases and --boot must be positive, checked by the parser, and limit is tested with "is not None" rather than truthiness, so --limit 0 no longer runs every row and a negative limit no longer slices from the end. --boot 0 used to fail with a traceback after the run and the artifact, when the calls were already paid for. An option the chosen adapter does not take (--revision on typesafe_wire), a missing API key, and a --report path that is a directory are now each one line, before anything is sent. Streams and exit code (#42): status lines and errors go to stderr, so stdout carries only the report when no --report is given and `run > report.md` captures just the report. A run where no case produced a prediction writes its artifact and report, then exits 1, saying the shared reason when there is one. Report (#15): with no figures and one shared failure reason, the report prints that reason instead of pointing at the artifact; with several, it says how many and leaves them to the artifact. README says what goes to which stream and what the exit codes mean. Fixes #41. Fixes #42. Fixes #15. Co-Authored-By: Claude Opus 5.5 (1M context) --- CHANGELOG.md | 14 ++++ README.md | 7 ++ src/plumbline/cli.py | 73 +++++++++++++++------ src/plumbline/report/markdown.py | 19 +++++- tests/test_cli.py | 107 +++++++++++++++++++++++++++++-- tests/test_report.py | 32 +++++++++ 6 files changed, 226 insertions(+), 26 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fce944a..f6928ba 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -83,6 +83,20 @@ different event from one that moved because it was wrong. ### Fixed +- 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 + take and a missing API key each printed a traceback, and a `--report` path + that was a directory crashed after the run. Counts must now be positive, and + each of the rest is one line, before anything is sent. +- `plumbline run` exited 0 when every case failed, and printed its status and + its errors on stdout, so `run > report.md` captured them and a script could + not tell a run with no figures from a good one (#42). Status and errors now + go to stderr, stdout carries only the report, and a run with no figures + exits 1 after writing the artifact and the report. +- When every case failed for the same reason (usually a missing extra or key), + the report said only that the failures were in the artifact (#15). It now + prints that reason, once; different reasons are still left to the artifact. - The cascade's threshold was chosen and scored on the same rows, so the coverage and cost it printed were its best case rather than what it would do; with no temperature recommended it used every row and still called them diff --git a/README.md b/README.md index 70ab744..825c5f1 100644 --- a/README.md +++ b/README.md @@ -157,6 +157,13 @@ artifact: results/20260921T222053+0000-mock-c18e9496.json report: results/report.md ``` +Those lines go to stderr. Without `--report`, the report itself is the only +thing on stdout, so `plumbline run ... > report.md` captures just the report. +The exit code is 0 when the run produced figures and 1 when it could not start +(a bad option, a missing key, an unreadable file) or when every case failed; in +that last case the artifact and the report are still written, and the reason is +printed when all the cases share one. + ``` uv run plumbline adapters # what this install can run uv run plumbline version diff --git a/src/plumbline/cli.py b/src/plumbline/cli.py index 6f21218..b5fdb87 100644 --- a/src/plumbline/cli.py +++ b/src/plumbline/cli.py @@ -1,9 +1,9 @@ """The command line: load a dataset, run an arm over it, render the report. -Three commands and no cleverness. ``run`` does one arm over one dataset and +Four commands and no cleverness. ``run`` does one arm over one dataset and writes both an artifact and a report; ``report`` renders artifacts that already -exist, so a finished run is never repeated to get a document out of it; and -``adapters`` says what can be run at all. +exist, so a finished run is never repeated to get a document out of it; +``adapters`` says what can be run at all; and ``version`` says which build this is. Two defaults are deliberately absent. There is no default dataset and no default results directory on ``report``: a relative default resolves against whatever @@ -53,11 +53,11 @@ def run( report: Annotated[Path | None, typer.Option(help="Write the report here too.")] = None, data_format: Annotated[str, typer.Option("--format", help="jsonl or jevbench.")] = "jsonl", strict: Annotated[bool, typer.Option(help="Refuse to run unless every row loaded.")] = False, - limit: Annotated[int | None, typer.Option(help="Run only the first N cases.")] = None, - workers: Annotated[int, typer.Option(help="Concurrent requests.")] = 8, + limit: Annotated[int | None, typer.Option(min=1, help="Run only the first N cases.")] = None, + workers: Annotated[int, typer.Option(min=1, help="Concurrent requests.")] = 8, cache_dir: Annotated[Path | None, typer.Option("--cache", help="Cache directory.")] = None, max_cost_usd: Annotated[float | None, typer.Option(help="Abort above this.")] = None, - max_cases: Annotated[int | None, typer.Option(help="Abort above this many.")] = None, + max_cases: Annotated[int | None, typer.Option(min=1, help="Abort above this many.")] = None, semantics: Annotated[ str | None, typer.Option(help="Override probability_semantics (mock only).") ] = None, @@ -74,17 +74,26 @@ def run( Path | None, typer.Option("--pricing", help="JSON pricing table to lay over the shipped one."), ] = None, - n_boot: Annotated[int, typer.Option("--boot", help="Bootstrap draws per null.")] = 2000, + n_boot: Annotated[int, typer.Option("--boot", min=1, help="Bootstrap draws per null.")] = 2000, ) -> None: - """Run one adapter over one dataset, and write what it found.""" + """Run one adapter over one dataset, and write what it found. + + Status lines and errors go to stderr, so stdout carries only the report when + no --report is given. The exit code is 1 when no case produced a prediction, + after the artifact and the report are written. + """ + # Everything that can be checked before a call goes out is checked here, + # 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) - typer.echo(load.statement()) + _status(load.statement()) for refusal in load.refusals: - typer.echo(f" refused {refusal}") + _status(f" refused {refusal}") if strict: _guard(load.require_complete) - cases = list(load.scoreable)[:limit] if limit else list(load.scoreable) + cases = list(load.scoreable)[:limit] if limit is not None else list(load.scoreable) if not cases: _fail("no scoreable rows in this dataset, so there is nothing to run.") @@ -110,7 +119,7 @@ def run( ) artifact = result.write(results) - typer.echo(f"artifact: {artifact}") + _status(f"artifact: {artifact}") document = markdown.render( [result], @@ -124,11 +133,17 @@ def run( if report is not None: report.parent.mkdir(parents=True, exist_ok=True) report.write_text(document, encoding="utf-8") - typer.echo(f"report: {report}") + _status(f"report: {report}") else: - typer.echo("") typer.echo(document) + if not result.successes: + # The artifact and the report are written first, since they hold the + # per-case errors; then the run says it produced nothing, in its exit code. + reasons = {record.error for record in result.records if record.error} + why = f" All for the same reason: {reasons.pop()}" if len(reasons) == 1 else "" + _fail(f"every case failed or was refused, so there is nothing to measure.{why}") + @app.command() def report( @@ -141,7 +156,7 @@ def report( error_cost: Annotated[ float | None, typer.Option("--error-cost", help="What one wrong answer costs, in USD.") ] = None, - n_boot: Annotated[int, typer.Option("--boot", help="Bootstrap draws per null.")] = 2000, + n_boot: Annotated[int, typer.Option("--boot", min=1, help="Bootstrap draws per null.")] = 2000, ) -> None: """Render one document from runs that already happened.""" results = [_guard(partial(execute.RunResult.read, path)) for path in artifacts] @@ -156,7 +171,7 @@ def report( if out is not None: out.parent.mkdir(parents=True, exist_ok=True) out.write_text(document, encoding="utf-8") - typer.echo(f"report: {out}") + _status(f"report: {out}") else: typer.echo(document) @@ -209,7 +224,24 @@ def _build( elif semantics is not None: _fail("--semantics applies to the mock only; a real adapter declares its own.") - return _guard(lambda: registry.create(adapter, **config)) + try: + return registry.create(adapter, **config) + except (PlumblineError, ValueError) as refused: + _fail(str(refused)) + except TypeError: + given = [f"--{name.replace('_requested', '')}" for name in config if name in _OPTIONS] + _fail(f"the {adapter} adapter does not take {', '.join(given) or 'these settings'}.") + except Exception as unavailable: # an SDK that cannot start, such as a missing key + variable = _KEY_VARIABLES.get(adapter) + hint = f" Set {variable} in the environment." if variable else "" + _fail(f"could not set up the {adapter} adapter: {unavailable}.{hint}") + + +#: Settings the CLI passes to an adapter from its own options. +_OPTIONS = ("model_requested", "revision") + +#: Where each hosted adapter reads its key, for the message when it is missing. +_KEY_VARIABLES = {"typesafe_wire": "TYPESAFE_API_KEY", "generative": "ANTHROPIC_API_KEY"} def _semantics(value: str) -> ProbabilitySemantics: @@ -245,8 +277,13 @@ def _guard[T](call: Callable[[], T]) -> T: _fail(str(refused)) +def _status(message: str) -> None: + """A line about the run rather than its result, so it goes to stderr.""" + typer.echo(message, err=True) + + def _fail(message: str) -> NoReturn: - typer.echo(message) + typer.echo(message, err=True) raise typer.Exit(code=1) diff --git a/src/plumbline/report/markdown.py b/src/plumbline/report/markdown.py index c3c517e..6ae7c1c 100644 --- a/src/plumbline/report/markdown.py +++ b/src/plumbline/report/markdown.py @@ -187,8 +187,23 @@ def _arm(result: RunResult, options: ReportOptions) -> list[str]: lines.extend(_asked_as(scoreable)) if not successes: - lines.append("- **No figures**: every case failed or was refused, so there is nothing") - lines.append(" to measure. The failures are in the artifact.") + # One shared reason is almost always an install or setup step (a missing + # extra, a bad key), and it is the one thing the reader needs, so it is + # said here rather than left inside the artifact. Different reasons are + # a log, and a report is not a log. + reasons = {record.error for record in failures if record.error} + if failures and len(reasons) == 1: + lines.append( + f"- **No figures**: all {len(failures)} cases failed for the same reason, " + "so there is nothing to measure:" + ) + lines.append(f" {reasons.pop()}") + else: + lines.append("- **No figures**: every case failed or was refused, so there is nothing") + lines.append( + f" to measure. The {len(failures)} failures, for {len(reasons)} different " + "reasons, are in the artifact." + ) return lines outcomes = [bool(record.correct) for record in successes] diff --git a/tests/test_cli.py b/tests/test_cli.py index 986efad..5118979 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -108,8 +108,8 @@ def test_a_run_prints_what_it_loaded_and_what_it_refused(tmp_path: Path) -> None result = invoke("run", str(dataset), "--results", str(tmp_path / "r"), "--boot", "100") assert result.exit_code == 0 - assert "6 loaded" in result.stdout - assert "1 refused" in result.stdout + assert "6 loaded" in result.output + assert "1 refused" in result.output def test_strict_refuses_to_run_a_dataset_with_an_unscoreable_row(tmp_path: Path) -> None: @@ -132,7 +132,7 @@ def test_strict_refuses_to_run_a_dataset_with_an_unscoreable_row(tmp_path: Path) ) assert result.exit_code != 0 - assert "typo" in result.stdout + assert "typo" in result.output def test_the_jevbench_loader_is_selectable(tmp_path: Path) -> None: @@ -150,7 +150,7 @@ def test_the_jevbench_loader_is_selectable(tmp_path: Path) -> None: ) assert result.exit_code == 0 - assert "111 rows read" in result.stdout + assert "111 rows read" in result.output def test_a_report_can_be_rendered_from_a_stored_artifact(tmp_path: Path) -> None: @@ -190,7 +190,7 @@ def test_a_missing_dataset_fails_with_the_path_it_looked_at(tmp_path: Path) -> N result = invoke("run", str(tmp_path / "nope.jsonl"), "--results", str(tmp_path / "r")) assert result.exit_code != 0 - assert "nope.jsonl" in result.stdout + assert "nope.jsonl" in result.output def test_an_unknown_adapter_names_the_ones_that_exist(tmp_path: Path) -> None: @@ -199,7 +199,7 @@ def test_an_unknown_adapter_names_the_ones_that_exist(tmp_path: Path) -> None: result = invoke("run", str(dataset), "--adapter", "telepathy", "--results", str(tmp_path / "r")) assert result.exit_code != 0 - assert "mock" in result.stdout + assert "mock" in result.output @pytest.mark.parametrize("command", ["run", "report", "adapters", "version"]) @@ -253,3 +253,98 @@ def test_without_the_costs_the_report_says_they_are_yours_to_supply(tmp_path: Pa text = report_path.read_text(encoding="utf-8") assert "--escalation-cost" in text + + +# Validation before anything is sent (#41), and what the exit code and the +# streams say afterwards (#42) + + +@pytest.mark.parametrize( + "option", + [["--limit", "0"], ["--limit", "-1"], ["--boot", "0"], ["--workers", "0"]], + ids=["limit-0", "limit-negative", "boot-0", "workers-0"], +) +def test_a_count_that_must_be_positive_is_refused_before_the_run( + tmp_path: Path, option: list[str] +) -> None: + dataset = a_dataset(tmp_path / "d.jsonl", n_rows=6) + result = invoke("run", str(dataset), "--results", str(tmp_path / "r"), *option) + + assert result.exit_code != 0 + assert "Traceback" not in result.output + assert not (tmp_path / "r").exists() + + +def test_an_option_the_adapter_does_not_take_is_named_not_a_traceback(tmp_path: Path) -> None: + dataset = a_dataset(tmp_path / "d.jsonl", n_rows=6) + result = runner.invoke( + cli.app, + ["run", str(dataset), "--adapter", "typesafe_wire", "--revision", "abc"], + env={"TYPESAFE_API_KEY": "not-a-key"}, + ) + + assert result.exit_code == 1 + assert result.exception is None or isinstance(result.exception, SystemExit) + assert "revision" in result.output + + +def test_a_missing_api_key_is_one_line_naming_the_variable(tmp_path: Path) -> None: + dataset = a_dataset(tmp_path / "d.jsonl", n_rows=6) + result = runner.invoke( + cli.app, + ["run", str(dataset), "--adapter", "typesafe_wire"], + env={"TYPESAFE_API_KEY": None}, + ) + + assert result.exit_code == 1 + assert isinstance(result.exception, SystemExit) + assert "TYPESAFE_API_KEY" in result.output + + +def test_a_report_path_that_is_a_directory_is_refused_before_the_run(tmp_path: Path) -> None: + dataset = a_dataset(tmp_path / "d.jsonl", n_rows=6) + (tmp_path / "out").mkdir() + result = invoke( + "run", str(dataset), "--results", str(tmp_path / "r"), "--report", str(tmp_path / "out") + ) + + assert result.exit_code != 0 + assert "directory" in result.output + assert not (tmp_path / "r").exists() + + +def test_status_and_errors_go_to_stderr_and_only_the_report_to_stdout(tmp_path: Path) -> None: + dataset = a_dataset(tmp_path / "d.jsonl", n_rows=24) + result = invoke("run", str(dataset), "--results", str(tmp_path / "r"), "--boot", "100") + + assert result.exit_code == 0 + assert result.stdout.lstrip().startswith("# plumbline report") + assert "rows read" in result.stderr and "artifact:" in result.stderr + + wrong = invoke("run", str(dataset), "--format", "csv") + assert wrong.exit_code != 0 + assert "--format" in wrong.stderr and wrong.stdout == "" + + +def test_a_run_where_every_case_fails_exits_non_zero_and_says_why(tmp_path: Path) -> None: + """The mock refuses an accuracy below chance on every case: one reason, said once.""" + 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), + "--accuracy", + "0.05", + "--boot", + "100", + ) + + assert result.exit_code == 1 + assert list((tmp_path / "r").glob("*.json")), "the artifact is still written" + 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") diff --git a/tests/test_report.py b/tests/test_report.py index b48d973..88bc3f6 100644 --- a/tests/test_report.py +++ b/tests/test_report.py @@ -17,6 +17,7 @@ from datetime import date from pathlib import Path +from typing import ClassVar import pytest from typesafe_sdk import NoulAnswer, SystemOneResponse, Usage @@ -439,3 +440,34 @@ def test_the_artifact_and_the_report_name_a_non_default_endpoint() -> None: assert "endpoint `http://self-hosted.example`" in render(result) # The vendor's default endpoint is the ordinary case and says nothing. assert "endpoint `" not in render(a_run(n_cases=40)) + + +class FailingAdapter: + """Fails every case, with the same message or with one per case.""" + + name = "failing" + model_requested = "failing-1" + revision = None + probability_semantics = "calibrated_claim" + reports_tokens = False + call_params: ClassVar[dict[str, object]] = {} + + def __init__(self, same: bool) -> None: + self.same = same + + def classify(self, text: str, labels: list[str], **asked: object) -> object: + raise ValueError("the local extra is not installed" if self.same else f"broke on {text}") + + +@pytest.mark.parametrize("same", [True, False], ids=["one-reason", "many-reasons"]) +def test_a_run_with_no_figures_names_a_shared_failure_reason(same: bool) -> None: + """One reason is the thing the reader needs; many reasons are a log (#15).""" + cases = make_cases(8, labels=LABELS) + text = render(execute.run(FailingAdapter(same), cases, workers=1)) # type: ignore[arg-type] + + if same: + assert "all 8 cases failed for the same reason" in text + assert "the local extra is not installed" in text + else: + assert "8 different reasons, are in the artifact" in text + assert "broke on" not in text