diff --git a/README.md b/README.md index e9cc057..abb25b6 100644 --- a/README.md +++ b/README.md @@ -4,14 +4,21 @@ Turns a PDF, docx, tex or md file into a validated Lambda Feedback set. [in2lambda](https://github.com/lambda-feedback/in2lambda) performs every deterministic step and every write; this agent performs the OCR, the model calls and the loop control. -One model call per document set writes a spec of selectors, which is layer 1, and -in2lambda runs that spec over the frozen source. A draft the checks fault returns to -the model as a fixing round, which repairs the draft one field at a time at layers 3 -and 4, until the checks report no error or the round limit is reached. - -[docs/how-it-works.md](docs/how-it-works.md) describes a run stage by stage: the line -each stage prints, the file each stage writes, and what each of the three model calls -is given and may write. +`convert` reads the document by two routes and compares them. Route A is one model call +that returns the set as JSON. Route B is a Lua filter, written by one model call per +folder, that pandoc runs with no further call. A field the two routes read the same way +is taken as it stands; a field they read differently goes to a small adjudicating call; +a field that call cannot settle is flagged for a person to read. Every field of either +route must be a quote of the document. [docs/plan.md](docs/plan.md) describes each step +and what it detects. + +`run --route spec` is the earlier route. One model call per document set writes a spec +of selectors, which is layer 1, and in2lambda runs that spec over the frozen source. A +draft the checks fault returns to the model as a fixing round, which repairs the draft +one field at a time at layers 3 and 4, until the checks report no error or the round +limit is reached. [docs/how-it-works.md](docs/how-it-works.md) describes that route +stage by stage: the line each stage prints, the file each stage writes, and what each of +the three model calls is given and may write. ## Install @@ -47,16 +54,92 @@ installed and `claude login` run. A run over a set whose spec is saved makes no call, and needs no key at all. A stage that needs a variable names that variable and the run exits 1. -## Run +## Convert + +```sh +poetry run in2lambda-agent convert sheet.pdf +``` + +In full: + +```sh +poetry run in2lambda-agent convert DOCUMENT [--solutions FILE] [--filter FILE | --write-filter] [--out DIR] [--cache DIR] +``` + +`DOCUMENT` is a PDF, markdown, tex or docx file. Mathpix converts a PDF first and the +agent keeps the markdown and the images under the PDF's hash in `--cache` (default +`./.in2lambda-agent`), so a second conversion of the same PDF makes no Mathpix call. +pandoc converts a tex or docx file. `--out` defaults to `./out`, where in2lambda writes +the set's JSON folder and its zip. + +`--solutions` names the document holding the solutions. Without it the agent takes the +file beside `DOCUMENT` whose name is the document's with `_solutions`, `-solutions` or +` Solutions` after it, in any case, and whose suffix is the same: `Worksheet_1.pdf` and +`Worksheet_1_solutions.pdf`. Naming the solutions document converts the pair too, and +the set is named after the questions document whichever of the two you name; a solutions +document with no questions document beside it converts on its own. Route A reads both +documents in one call, and route B reads each under its own role. The `solutions` line +of the report names the document the +conversion read. Where the two names share no stem, as they do where the platform has +put the time of the download in each, the agent finds no solutions document and the +line names none: + +``` +solutions none found beside ME2_Fluids_2024-03-11.pdf; pass --solutions FILE +``` + +A conversion that reads that line and goes on writes an empty answer and an empty +worked solution for every question. + +`--filter` names the Lua filter route B runs, which is the file `--write-filter` wrote +for another sheet of the same set. `--write-filter` writes one for this document with a +model call and keeps it at `OUT/filter.lua`. The two options together are refused: a +conversion runs one filter. With neither option route A converts the document alone, no +field is compared, and the counts line says so. + +The command prints the solutions document, one line per flagged field, the counts of +the comparison, and the zip: + +``` +solutions /home/me/sheets/sheet_solutions.pdf +flag q2.p1.worked_solution: a stray minus sign inside or beside a display maths; Mathpix reads a separator line as one +flag q4.p2.content: two readings of the source + A: Find the drag force on the plate. + B: Find the drag force on the plate, in newtons. +fields 60 fields, agreed 54, defaulted 4, adjudicated 2, flagged 2 +build /home/me/out/sheet.zip +``` + +A flag names the field, the reason, and each route's text where both routes filled the +field. `fields` counts the fields the two routes agreed on, the fields one route alone +filled, the fields the adjudicating call settled, and the fields flagged. With no +filter there is no comparison to count, so the line is `60 fields, route B did not run`: +the fields are route A's, and each one is flagged or is route A's word for it. A filter +run that fails counts route A's fields in the same way, and adds a `route B failed` line +naming pandoc's message; the set is route A's reading alone. + +`convert` exits 1 where a named file is not there, and where Mathpix, the model or +pandoc failed, and 0 otherwise. A flagged +field does not change the exit code: the zip is written whatever the flags say, and a +person reads the flags after it. + +## Run: the spec route + +`run DOCUMENT` converts the document as `convert` does and takes the same options. +`--route spec` runs the earlier route instead, which the rest of this section describes. +The two routes' options do not mix: `--review` without `--route spec`, or +`--write-filter` with it, is refused naming the route the option belongs to, because a +run that read it and ignored it would be a spec written twice or a review never stopped +for. ```sh -poetry run in2lambda-agent run sheet.pdf +poetry run in2lambda-agent run sheet.pdf --route spec ``` In full: ```sh -poetry run in2lambda-agent run SOURCE [--spec FILE] [--review none|sample|per-question] [--rounds N] [--tries N] [--sample N] [--cache DIR] [--fresh-ocr] [--out DIR] +poetry run in2lambda-agent run SOURCE --route spec [--spec FILE] [--review none|sample|per-question] [--rounds N] [--tries N] [--sample N] [--cache DIR] [--fresh-ocr] [--out DIR] ``` `SOURCE` is a PDF, markdown, tex or docx file. Mathpix converts a PDF first, and the @@ -164,7 +247,7 @@ spec, and is not one of the rounds. ### Exit codes -`run` and `review` exit 0 where they wrote a zip, and where a review is still waiting +`run --route spec` and `review` exit 0 where they wrote a zip, and where a review is still waiting for a verdict. They exit 1 where the checks still fault the draft, where in2lambda refused the build, and where a review has no question left to answer and no zip was written. A missing credential, an unavailable backend, a reply that is not a spec, a diff --git a/in2lambda_agent/cli.py b/in2lambda_agent/cli.py index 07cb389..5df4d4b 100644 --- a/in2lambda_agent/cli.py +++ b/in2lambda_agent/cli.py @@ -2,12 +2,13 @@ import argparse import getpass +import subprocess import sys import tempfile from pathlib import Path from typing import Optional, Sequence -from in2lambda_agent import compare, corpus, gate, pipeline +from in2lambda_agent import compare, corpus, gate, pair, pipeline, routes from in2lambda_agent.mathpix import MathpixClient, MathpixError from in2lambda_agent.model import ModelError, ModelUnavailable, choose_backend from in2lambda_agent.ocr import ocr_pdf @@ -85,12 +86,41 @@ def reviewer_name(given: Optional[str]) -> str: return "reviewer" +def _conversion_options(parser: argparse.ArgumentParser) -> None: + """Adds the options of a conversion, which `convert` and `run` both take. + + Args: + parser: The subcommand's parser. + """ + parser.add_argument( + "--solutions", + type=Path, + default=None, + help="The solutions document. Default: the file beside the document " + "whose name is the document's with `_solutions` after it.", + ) + filter_ = parser.add_mutually_exclusive_group() + filter_.add_argument( + "--filter", + type=Path, + default=None, + help="The Lua filter route B runs. Without one, route A converts the " + "document alone and no field is compared.", + ) + filter_.add_argument( + "--write-filter", + action="store_true", + help="Write route B's filter for this document with a model call, and " + "keep it at `OUT/filter.lua`.", + ) + + def build_parser() -> argparse.ArgumentParser: """The command line as the design spec describes it. Returns: - A parser with the `run`, `review`, `corpus`, `gate`, `compare` and - `ui` subcommands. + A parser with the `convert`, `run`, `review`, `corpus`, `gate`, + `compare` and `ui` subcommands. """ parser = argparse.ArgumentParser( prog="in2lambda-agent", @@ -98,13 +128,45 @@ def build_parser() -> argparse.ArgumentParser: ) subcommands = parser.add_subparsers(dest="command", required=True) - run = subcommands.add_parser("run", help="Convert SOURCE into a set.") + convert = subcommands.add_parser( + "convert", help="Convert DOCUMENT into a set through both routes." + ) + convert.add_argument( + "document", + type=Path, + help="The question file to convert: a PDF, markdown, tex or docx file.", + ) + _conversion_options(convert) + convert.add_argument( + "--out", + type=Path, + default=Path("out"), + help="Where to write the set's JSON folder and zip.", + ) + convert.add_argument( + "--cache", + type=Path, + default=pipeline.DEFAULT_CACHE_DIR, + help="Where the OCR of each PDF is kept.", + ) + + run = subcommands.add_parser( + "run", help="Convert SOURCE into a set; `convert` under the default route." + ) run.add_argument( "source", type=Path, help="The question file to convert. A solutions file beside it, named " "after it, is frozen with it.", ) + run.add_argument( + "--route", + choices=("direct", "spec"), + default="direct", + help="Which route converts the document: `direct` is the `convert` " + "command, and `spec` writes a spec of selectors and runs it.", + ) + _conversion_options(run) run.add_argument( "--spec", type=Path, @@ -308,6 +370,118 @@ def build_parser() -> argparse.ArgumentParser: return parser +# Each route's own options. `run` takes both sets, because argparse cannot know +# the route until it has parsed the line, so the run refuses an option of the +# route it is not taking rather than reading it and throwing it away. +_SPEC_ROUTE_OPTIONS = { + "spec": "--spec", + "review": "--review", + "rounds": "--rounds", + "tries": "--tries", + "sample": "--sample", + "fresh_ocr": "--fresh-ocr", +} +_DIRECT_ROUTE_OPTIONS = { + "solutions": "--solutions", + "filter": "--filter", + "write_filter": "--write-filter", +} + + +def misplaced_option(args: argparse.Namespace) -> Optional[str]: + """What is wrong where `run` was given an option of the other route. + + Args: + args: The parsed arguments of `run`. + + Returns: + What to print, naming the option and the route it belongs to, or None + where every option given belongs to the route the run is taking. + """ + if args.route == "direct": + options, route, fix = _SPEC_ROUTE_OPTIONS, "spec", "add --route spec" + else: + options, route, fix = _DIRECT_ROUTE_OPTIONS, "direct", "drop --route spec" + # An option counts as given where it is not the parser's default, which is + # read back from the parser rather than repeated here. + defaults = build_parser().parse_args(["run", str(args.source)]) + for dest, name in options.items(): + if getattr(args, dest) != getattr(defaults, dest): + return f"{name} is an option of the {route} route; {fix}" + return None + + +def convert_command(args: argparse.Namespace) -> int: + """Converts one document through both routes and prints the report. + + Args: + args: The parsed arguments of `convert`, or of `run` under the direct + route, which takes the same options. + + Returns: + 0 where the zip was written, and 1 where a conversion step failed. A + flagged field does not change the code: the flags are what a person + reads after the build, and no check blocks the write. + """ + # `run SOURCE` converts the same document, under the other name. + document = Path(getattr(args, "document", None) or args.source) + if args.solutions is not None: + # The user named the two documents, so the folder is not asked. + solutions = args.solutions + else: + # Either half of a pair may be named, so the pairing goes both ways: name the + # solutions document and the questions document beside it is what converts, and + # the set is named after it. A solutions document with none beside it comes back + # as the document itself, and converts on its own. + document, solutions = pair.of(document) + if solutions is not None: + print(f"solutions {solutions}") + elif pair.questions_stem(document) is None: + # A pair whose two names share no stem, which is what the platform writes where + # it puts the time of the download in each name, is a sheet whose solutions the + # run did not find. A run that said nothing would read as a sheet with none, + # and the set it writes holds an empty answer for every question. + print(f"solutions none found beside {document}; pass --solutions FILE") + out_dir = Path(args.out) + settings = load_settings() + backend = choose_backend(settings) + lua = args.filter + try: + if args.write_filter: + out_dir.mkdir(parents=True, exist_ok=True) + lua = out_dir / "filter.lua" + lua.write_text( + routes.write_filter(document, solutions, backend)[0], encoding="utf-8" + ) + result = routes.convert( + document, + solutions, + out_dir=out_dir, + cache_dir=args.cache, + backend=backend, + settings=settings, + lua=lua, + name=document.stem, + ) + except (MathpixError, ModelUnavailable, ModelError, OSError, routes.BadReply) as error: + # A document that is not there raises an OSError here, because this route reads + # the file itself and in2lambda never sees the name. A reply that is not a JSON + # list of questions raises BadReply, as a reply that is not a spec raises + # BadSpec on the other route. + print(f"in2lambda-agent: {error}", file=sys.stderr) + return 1 + except subprocess.CalledProcessError as error: + # pandoc read the document, or ran the filter, and refused. Its own + # message names the line; the exit status alone names nothing. + stderr = (error.stderr or b"").decode("utf-8", "replace").strip() + print(f"in2lambda-agent: {stderr or error}", file=sys.stderr) + return 1 + + for line in result.report(): + print(line) + return 0 if result.zip_path else 1 + + def main(argv: Optional[Sequence[str]] = None) -> int: """Runs the command. @@ -319,6 +493,15 @@ def main(argv: Optional[Sequence[str]] = None) -> int: """ args = build_parser().parse_args(argv) + if args.command == "run": + wrong = misplaced_option(args) + if wrong: + print(f"in2lambda-agent: {wrong}", file=sys.stderr) + return 1 + + if args.command == "convert" or (args.command == "run" and args.route == "direct"): + return convert_command(args) + if args.command == "corpus": rows = corpus.sweep( args.root, diff --git a/in2lambda_agent/routes.py b/in2lambda_agent/routes.py index 91626ae..581f2ce 100644 --- a/in2lambda_agent/routes.py +++ b/in2lambda_agent/routes.py @@ -221,8 +221,27 @@ def _prompt(markdown: str, solutions: Optional[str]) -> str: ) -def _json(text: str) -> Any: - return json.loads(re.sub(r"^```(json)?\s*|\s*```$", "", text.strip())) +class BadReply(ValueError): + """What the model answered with is not a JSON list.""" + + +def _json(text: str) -> list: + """The JSON list a call was asked for. + + Raises: + BadReply: the text is not JSON, or is JSON that is not a list. A model that + answers with a sentence, and an answer cut short at the output-token + limit, both arrive here; `fields` and `to_set` read a list, and neither + reports the text they were given instead. + """ + stripped = re.sub(r"^```(json)?\s*|\s*```$", "", text.strip()) + try: + answered = json.loads(stripped) + except json.JSONDecodeError as error: + raise BadReply(f"The reply is not JSON: {error}.") from None + if not isinstance(answered, list): + raise BadReply(f"A reply is a JSON list, which {_squash(stripped)[:60]!r} is not.") + return answered def direct(markdown: str, solutions: Optional[str], backend: Backend) -> tuple[Reply_, Reply]: @@ -388,6 +407,35 @@ class Converted: adjudicated: int = 0 route_b_error: Optional[str] = None + def report(self) -> list[str]: + """One line per flag, a line of the counts, and the zip. + + A flag prints each route's text where both routes filled the field, because the + reader decides between the two. A field one route filled prints the reason + alone. + """ + lines = [] + for one in self.flags: + lines.append(f"flag {one.field}: {one.reason}") + if one.a and one.b: + lines += [f" A: {_squash(one.a)}", f" B: {_squash(one.b)}"] + if self.fields: + counts = _counted( + [self.fields, self.agreed, self.defaulted, self.adjudicated, len(self.flags)] + ) + else: + # No filter was given, or the filter run failed: no field was compared, and + # every field is route A's. The counts of a comparison that did not happen + # say nothing, so the line counts route A's fields instead. + ran = "failed" if self.route_b_error else "did not run" + counts = f"{len(fields(normalise(self.reply)))} fields, route B {ran}" + lines.append(f"fields {counts}") + if self.route_b_error: + lines.append(f"route B failed: {self.route_b_error}") + if self.zip_path: + lines.append(f"build {self.zip_path}") + return lines + _UNDERLINE = Path(__file__).parent / "underline.lua" diff --git a/tests/test_cli.py b/tests/test_cli.py index ae52898..a1a3aad 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -1,16 +1,17 @@ """The command line the design spec describes.""" import getpass +import subprocess import sys from pathlib import Path from types import SimpleNamespace import pytest -from conftest import FakeMathpix +from conftest import FakeBackend, FakeMathpix -from in2lambda_agent import cli, compare, corpus, gate, pipeline +from in2lambda_agent import cli, compare, corpus, gate, pipeline, routes from in2lambda_agent.cli import build_parser, main, reviewer_name -from in2lambda_agent.model import Usage +from in2lambda_agent.model import ModelUnavailable, Usage from in2lambda_agent.settings import Settings @@ -81,6 +82,396 @@ def test_a_sample_of_no_questions_is_refused(count, capsys): assert "at least one question" in capsys.readouterr().err +# --- convert --------------------------------------------------------------- + + +def test_convert_defaults(): + args = build_parser().parse_args(["convert", "sheet.pdf"]) + + assert args.command == "convert" + assert args.document == Path("sheet.pdf") + assert args.solutions is None + assert args.filter is None + assert args.write_filter is False + assert args.out == Path("out") + assert args.cache == Path(".in2lambda-agent") + + +def test_convert_every_option(): + args = build_parser().parse_args( + [ + "convert", + "sheet.pdf", + "--solutions", + "sheet_solutions.pdf", + "--filter", + "set.lua", + "--out", + "somewhere", + "--cache", + "cached", + ] + ) + + assert args.document == Path("sheet.pdf") + assert args.solutions == Path("sheet_solutions.pdf") + assert args.filter == Path("set.lua") + assert args.out == Path("somewhere") + assert args.cache == Path("cached") + + written = build_parser().parse_args(["convert", "sheet.pdf", "--write-filter"]) + assert written.write_filter is True + assert written.filter is None + + +@pytest.mark.parametrize("command", ["convert", "run"]) +def test_a_filter_and_a_written_filter_together_are_refused(command): + # One run has one route B filter: either the file named or the file written. + with pytest.raises(SystemExit): + build_parser().parse_args( + [command, "sheet.md", "--filter", "set.lua", "--write-filter"] + ) + + +def test_run_converts_through_both_routes_unless_the_spec_route_is_asked_for(): + assert build_parser().parse_args(["run", "sheet.md"]).route == "direct" + assert build_parser().parse_args(["run", "s.md", "--route", "spec"]).route == "spec" + assert build_parser().parse_args(["run", "s.md", "--solutions", "s2.md"]).solutions + + +def converted(zip_path=None, **counts): + """What a monkeypatched `routes.convert` answers with.""" + return routes.Converted( + set=None, zip_path=zip_path, flags=counts.pop("flags", []), reply=[], **counts + ) + + +def records(given, result): + """A stand-in for `routes.convert` that records what it was given.""" + + def record(document, solutions=None, **passed): + given.update(passed, document=document, solutions=solutions) + return result + + return record + + +@pytest.fixture +def backend(monkeypatch): + """A backend the convert branch takes without reading the environment.""" + fake = FakeBackend() + monkeypatch.setattr(cli, "choose_backend", lambda settings: fake) + return fake + + +def test_convert_hands_the_document_and_the_options_to_the_route( + tmp_path, backend, monkeypatch, capsys +): + given = {} + zip_path = tmp_path / "out" / "sheet.zip" + monkeypatch.setattr( + routes, + "convert", + records(given, converted(zip_path, fields=10, agreed=9, defaulted=1)), + ) + + code = main( + [ + "convert", + str(tmp_path / "sheet.md"), + "--solutions", + str(tmp_path / "sol.md"), + "--filter", + str(tmp_path / "set.lua"), + "--out", + str(tmp_path / "out"), + "--cache", + str(tmp_path / "cache"), + ] + ) + + assert code == 0 + assert given["document"] == tmp_path / "sheet.md" + assert given["solutions"] == tmp_path / "sol.md" + assert given["lua"] == tmp_path / "set.lua" + assert given["out_dir"] == tmp_path / "out" + assert given["cache_dir"] == tmp_path / "cache" + assert given["backend"] is backend + assert given["name"] == "sheet" + out = capsys.readouterr().out + assert "10 fields, agreed 9, defaulted 1, adjudicated 0, flagged 0" in out + assert f"build {zip_path}" in out + + +def test_convert_takes_the_solutions_document_beside_the_document( + tmp_path, backend, monkeypatch +): + (tmp_path / "sheet.md").write_text("x") + (tmp_path / "sheet_solutions.md").write_text("x") + given = {} + monkeypatch.setattr(routes, "convert", records(given, converted(tmp_path / "s.zip"))) + + assert main(["convert", str(tmp_path / "sheet.md"), "--out", str(tmp_path)]) == 0 + assert given["solutions"] == tmp_path / "sheet_solutions.md" + + +@pytest.mark.parametrize("command", ["convert", "run"]) +def test_convert_named_by_its_solutions_document_converts_the_pair( + command, tmp_path, backend, monkeypatch, capsys +): + # Naming either half of a pair converts the pair, and the set is named after the + # questions document, as the spec route has always done. + (tmp_path / "Worksheet_1.md").write_text("x") + (tmp_path / "Worksheet_1_solutions.md").write_text("x") + given = {} + monkeypatch.setattr(routes, "convert", records(given, converted(tmp_path / "s.zip"))) + + code = main( + [command, str(tmp_path / "Worksheet_1_solutions.md"), "--out", str(tmp_path)] + ) + + assert code == 0 + assert given["document"] == tmp_path / "Worksheet_1.md" + assert given["solutions"] == tmp_path / "Worksheet_1_solutions.md" + assert given["name"] == "Worksheet_1" + assert f"solutions {tmp_path / 'Worksheet_1_solutions.md'}" in capsys.readouterr().out + + +def test_convert_with_solutions_named_does_not_pair(tmp_path, backend, monkeypatch): + # `--solutions` means what the user says, not what the folder holds. + (tmp_path / "Worksheet_1.md").write_text("x") + (tmp_path / "Worksheet_1_solutions.md").write_text("x") + given = {} + monkeypatch.setattr(routes, "convert", records(given, converted(tmp_path / "s.zip"))) + + code = main( + [ + "convert", + str(tmp_path / "Worksheet_1_solutions.md"), + "--solutions", + str(tmp_path / "other.md"), + "--out", + str(tmp_path), + ] + ) + + assert code == 0 + assert given["document"] == tmp_path / "Worksheet_1_solutions.md" + assert given["solutions"] == tmp_path / "other.md" + + +@pytest.mark.parametrize("named", [False, True]) +def test_convert_names_the_solutions_document_it_read( + named, tmp_path, backend, monkeypatch, capsys +): + # The document `--solutions` names and the document found beside the questions are + # reported the same way: the reader sees which file the answers came from. + solutions = tmp_path / "sheet_solutions.md" + solutions.write_text("x") + (tmp_path / "sheet.md").write_text("x") + monkeypatch.setattr(routes, "convert", records({}, converted(tmp_path / "s.zip"))) + + code = main( + [ + "convert", + str(tmp_path / "sheet.md"), + "--out", + str(tmp_path), + *(["--solutions", str(solutions)] if named else []), + ] + ) + + assert code == 0 + assert f"solutions {solutions}" in capsys.readouterr().out + + +def test_convert_says_where_it_found_no_solutions_document( + tmp_path, backend, monkeypatch, capsys +): + # A pair whose two names share no stem is a sheet whose solutions the run did not + # find. A run that said nothing would read as a sheet with none. + monkeypatch.setattr(routes, "convert", records({}, converted(tmp_path / "s.zip"))) + + assert main(["convert", str(tmp_path / "sheet.pdf"), "--out", str(tmp_path)]) == 0 + assert ( + f"solutions none found beside {tmp_path / 'sheet.pdf'}; pass --solutions FILE" + in capsys.readouterr().out + ) + + +def test_convert_says_nothing_of_the_solutions_of_a_solutions_document( + tmp_path, backend, monkeypatch, capsys +): + # A solutions document converted on its own is what the reader asked for, and there + # is no file for `--solutions` to name. + monkeypatch.setattr(routes, "convert", records({}, converted(tmp_path / "s.zip"))) + + code = main(["convert", str(tmp_path / "sheet_solutions.md"), "--out", str(tmp_path)]) + + assert code == 0 + assert "solutions" not in capsys.readouterr().out + + +@pytest.mark.parametrize("missing", ["document", "solutions"]) +def test_convert_names_a_file_that_is_not_there(missing, tmp_path, backend, capsys): + # This route reads each file itself, so in2lambda never sees the name and never + # complains about it. + if missing == "solutions": + (tmp_path / "sheet.md").write_text("# Question 1\n") + + code = main( + [ + "convert", + str(tmp_path / "sheet.md"), + "--solutions", + str(tmp_path / "sol.md"), + "--out", + str(tmp_path), + ] + ) + printed = capsys.readouterr() + + assert code == 1 + assert printed.err.startswith("in2lambda-agent: ") + assert ("sol.md" if missing == "solutions" else "sheet.md") in printed.err + + +def test_run_without_a_route_converts_the_document(tmp_path, backend, monkeypatch): + given = {} + monkeypatch.setattr(routes, "convert", records(given, converted(tmp_path / "s.zip"))) + monkeypatch.setattr( + pipeline, "run", lambda *a, **k: pytest.fail("the spec route ran") + ) + + assert main(["run", str(tmp_path / "sheet.md"), "--out", str(tmp_path)]) == 0 + assert given["document"] == tmp_path / "sheet.md" + + +def test_the_written_filter_is_kept_in_the_out_directory( + tmp_path, backend, monkeypatch +): + given = {} + monkeypatch.setattr(routes, "convert", records(given, converted(tmp_path / "s.zip"))) + monkeypatch.setattr( + routes, "write_filter", lambda document, solutions, backend: ("-- lua", None) + ) + + code = main( + [ + "convert", + str(tmp_path / "sheet.md"), + "--write-filter", + "--out", + str(tmp_path / "out"), + ] + ) + + assert code == 0 + assert (tmp_path / "out" / "filter.lua").read_text() == "-- lua" + assert given["lua"] == tmp_path / "out" / "filter.lua" + + +def test_a_flagged_field_does_not_stop_the_build(tmp_path, backend, monkeypatch, capsys): + # The zip is written whatever the flags say; a person reads the flags after it. + flag = routes.Flag("q2.p1.worked_solution", "a", "", routes.STRAY_MINUS) + monkeypatch.setattr( + routes, "convert", records({}, converted(tmp_path / "s.zip", flags=[flag])) + ) + + code = main(["convert", str(tmp_path / "sheet.md"), "--out", str(tmp_path)]) + + assert code == 0 + assert "flag q2.p1.worked_solution:" in capsys.readouterr().out + + +def test_convert_without_a_backend_says_what_to_set(tmp_path, backend, monkeypatch, capsys): + def unavailable(*args, **kwargs): + raise ModelUnavailable("set ANTHROPIC_API_KEY, or run `claude login`") + + monkeypatch.setattr(routes, "convert", unavailable) + + code = main(["convert", str(tmp_path / "sheet.md"), "--out", str(tmp_path)]) + printed = capsys.readouterr() + + assert code == 1 + assert "claude login" in printed.err + # Nothing was converted, so there is no report. + assert "fields" not in printed.out and "build" not in printed.out + + +def test_convert_reports_what_pandoc_said(tmp_path, backend, monkeypatch, capsys): + def refuses(*args, **kwargs): + raise subprocess.CalledProcessError( + 43, "pandoc", stderr=b"Error at line 3 column 1\n" + ) + + monkeypatch.setattr(routes, "convert", refuses) + + code = main(["convert", str(tmp_path / "sheet.tex"), "--out", str(tmp_path)]) + + assert code == 1 + assert "Error at line 3 column 1" in capsys.readouterr().err + + +@pytest.mark.parametrize( + "answer", ["I cannot convert this sheet.", '{"title": "A ball", "parts": []}'] +) +def test_convert_reports_a_reply_that_is_not_a_list_of_questions( + answer, tmp_path, monkeypatch, capsys +): + # Route A is asked for a JSON list. A model that answers with a sentence, and an + # answer cut short at the output-token limit, both arrive as text no step below + # route A reads. The run names the fault, as it does for pandoc and for Mathpix. + (tmp_path / "sheet.md").write_text("# Question 1\n\nFind the height.\n") + monkeypatch.setattr(cli, "choose_backend", lambda settings: FakeBackend(answer)) + + code = main(["convert", str(tmp_path / "sheet.md"), "--out", str(tmp_path / "out")]) + printed = capsys.readouterr() + + assert code == 1 + assert printed.err.startswith("in2lambda-agent: ") + assert "fields" not in printed.out and "build" not in printed.out + + +@pytest.mark.parametrize( + "given, message", + [ + ( + ["--review", "per-question"], + "--review is an option of the spec route; add --route spec", + ), + ( + ["--spec", "set.yaml"], + "--spec is an option of the spec route; add --route spec", + ), + ( + ["--route", "spec", "--write-filter"], + "--write-filter is an option of the direct route; drop --route spec", + ), + ( + ["--route", "spec", "--solutions", "sol.md"], + "--solutions is an option of the direct route; drop --route spec", + ), + ], +) +def test_an_option_of_the_other_route_is_refused(given, message, monkeypatch, capsys): + # Under the route it does not belong to the option would be parsed and + # thrown away: a saved spec ignored and written again by two model calls, a + # review never stopped for, a filter never written. So the run says so, and + # says so before it has paid for anything. + monkeypatch.setattr( + cli, "choose_backend", lambda settings: pytest.fail("a model was asked for") + ) + monkeypatch.setattr( + pipeline, "run", lambda *a, **k: pytest.fail("the spec route ran") + ) + + code = main(["run", "sheet.md", *given]) + + assert code == 1 + assert capsys.readouterr().err.strip() == f"in2lambda-agent: {message}" + + def test_corpus_defaults(): args = build_parser().parse_args(["corpus", "ExampleContents"]) @@ -412,7 +803,7 @@ def record(source, **given): monkeypatch.setattr(pipeline, "run", record) - assert main(["run", "sheet.md"]) == 0 + assert main(["run", "sheet.md", "--route", "spec"]) == 0 assert called["source"] == Path("sheet.md") @@ -425,7 +816,7 @@ def record(source, **passed): monkeypatch.setattr(pipeline, "run", record) - assert main(["run", "sheet.md", "--tries", "5"]) == 0 + assert main(["run", "sheet.md", "--route", "spec", "--tries", "5"]) == 0 assert given["tries"] == 5 diff --git a/tests/test_pipeline.py b/tests/test_pipeline.py index 22ac5f1..65ffbf1 100644 --- a/tests/test_pipeline.py +++ b/tests/test_pipeline.py @@ -23,6 +23,10 @@ from in2lambda_agent.settings import Settings from in2lambda_agent.spec import RECORD_NAME, SPEC_NAME, BadSpec +# The command line of the spec route, which these tests cover. `run` without +# `--route spec` converts the document through the two routes of routes.py. +RUN = ["run", "--route", "spec"] + FIXTURES = Path(__file__).parent / "fixtures" SOURCE = FIXTURES / "sheet.md" SPEC = (FIXTURES / "sheet-spec.yaml").read_text() @@ -1414,7 +1418,7 @@ def test_the_rounds_running_out_prints_its_stages_and_exits_one( code = main( [ - "run", + *RUN, str(faulty / "faulty.md"), "--rounds", "1", @@ -2077,7 +2081,7 @@ def test_a_pdf_without_credentials_exits_one_naming_the_variables( monkeypatch.delenv("MATHPIX_APP_ID", raising=False) monkeypatch.delenv("MATHPIX_API_KEY", raising=False) - code = main(["run", str(pdf), "--out", str(tmp_path / "out")]) + code = main([*RUN, str(pdf), "--out", str(tmp_path / "out")]) printed = capsys.readouterr() assert code == 1 @@ -2088,7 +2092,7 @@ def test_a_pdf_without_credentials_exits_one_naming_the_variables( def test_the_command_exits_zero_and_prints_a_line_per_stage(sheets, tmp_path, capsys): (sheets / SPEC_NAME).write_text(SPEC) - code = main(["run", str(sheets / "sheet.md"), "--out", str(tmp_path / "out")]) + code = main([*RUN, str(sheets / "sheet.md"), "--out", str(tmp_path / "out")]) printed = capsys.readouterr().out.splitlines() assert code == 0 @@ -2109,7 +2113,7 @@ def test_a_review_run_and_its_approvals_exit_zero_and_build(sheets, tmp_path, ca where = ["--cache", str(tmp_path / "cache")] out = ["--out", str(tmp_path / "out")] - stopped = main(["run", str(sheets / "sheet.md"), "--review", "sample", *where, *out]) + stopped = main([*RUN, str(sheets / "sheet.md"), "--review", "sample", *where, *out]) printed = capsys.readouterr().out # Waiting for a reviewer is not a failure, and nothing is built yet. @@ -2131,7 +2135,7 @@ def test_approving_a_draft_the_checks_fault_exits_one_saying_what_they_found( where = ["--cache", str(tmp_path / "cache")] out = ["--out", str(tmp_path / "out")] - main(["run", str(sheets / "sheet.md"), "--review", "sample", *where, *out]) + main([*RUN, str(sheets / "sheet.md"), "--review", "sample", *where, *out]) main(["review", "edit", EMPTIES["field"], EMPTIES["old"], EMPTIES["new"], *where]) capsys.readouterr() @@ -2156,7 +2160,7 @@ def test_the_default_out_is_the_working_directorys_out(sheets, tmp_path, monkeyp (sheets / SPEC_NAME).write_text(SPEC) monkeypatch.chdir(tmp_path) - assert main(["run", str(sheets / "sheet.md")]) == 0 + assert main([*RUN, str(sheets / "sheet.md")]) == 0 assert (tmp_path / "out" / "set.zip").exists() @@ -2166,7 +2170,7 @@ def test_a_run_the_checks_fault_prints_its_stages_and_exits_one( (sheets / SPEC_NAME).write_text(PARTLESS_SPEC) code = main( - ["run", str(sheets / "sheet.md"), "--rounds", "0", "--out", str(tmp_path)] + [*RUN, str(sheets / "sheet.md"), "--rounds", "0", "--out", str(tmp_path)] ) printed = capsys.readouterr() @@ -2182,7 +2186,7 @@ def test_a_run_with_no_backend_exits_one_naming_what_to_do( pipeline, "choose_backend", lambda settings: FakeBackend(reason="run claude login") ) - code = main(["run", str(sheets / "sheet.md"), "--out", str(tmp_path / "out")]) + code = main([*RUN, str(sheets / "sheet.md"), "--out", str(tmp_path / "out")]) printed = capsys.readouterr() assert code == 1 @@ -2236,7 +2240,7 @@ def test_a_refused_build_prints_its_stages_and_exits_one( ), ) - code = main(["run", str(figures / "figure.md"), "--out", str(tmp_path / "out")]) + code = main([*RUN, str(figures / "figure.md"), "--out", str(tmp_path / "out")]) printed = capsys.readouterr() assert code == 1 diff --git a/tests/test_routes.py b/tests/test_routes.py index 1e8ff60..f15de3a 100644 --- a/tests/test_routes.py +++ b/tests/test_routes.py @@ -21,7 +21,7 @@ from conftest import FakeBackend import in2lambda_agent.routes as routes -from in2lambda_agent import pair +from in2lambda_agent import cli, pair from in2lambda_agent.settings import Settings ME2 = Path(__file__).parent / "fixtures" / "me2" @@ -33,6 +33,10 @@ "/Users/peterbjohnson/code/lambdafeedback/in2lambda-agent/ExampleContents/" "PHYS40002-Mechanics/problem_sheets_and_figures" ) +ME2_TARGET = Path( + "/Users/peterbjohnson/code/lambdafeedback/in2lambda-agent/ExampleContents/targets/" + "ME2_Fluids_introduction" +) FILTER = Path(__file__).parent / "fixtures" / "ps1-filter.lua" live = pytest.mark.skipif(not os.environ.get("IN2LAMBDA_LIVE"), reason="calls Mathpix and a model") @@ -415,6 +419,65 @@ def test_a_sheet_whose_filter_run_fails_keeps_its_route_a_reply(tmp_path): assert (result.fields, result.agreed, result.flags) == (0, 0, []) +# --- the report of one document ------------------------------------------------------------- + + +def test_the_report_of_a_document_gives_the_flags_the_counts_and_the_zip(tmp_path): + result = routes.Converted( + set=None, zip_path=tmp_path / "sheet.zip", reply=[], + flags=[routes.Flag("q2.p2.content", "Find the drag.", "Find the drag, in N.", "two readings")], + fields=10, agreed=8, defaulted=1, adjudicated=1, + ) + assert result.report() == [ + "flag q2.p2.content: two readings", + " A: Find the drag.", + " B: Find the drag, in N.", + "fields 10 fields, agreed 8, defaulted 1, adjudicated 1, flagged 1", + f"build {tmp_path / 'sheet.zip'}", + ] + + +def test_a_flag_only_one_route_filled_shows_the_one_text(tmp_path): + result = routes.Converted( + set=None, zip_path=tmp_path / "sheet.zip", reply=[], + flags=[routes.Flag("q1.p1.worked_solution", "$$\n-x\n$$", "", routes.STRAY_MINUS)], + fields=10, agreed=10, + ) + assert result.report() == [ + f"flag q1.p1.worked_solution: {routes.STRAY_MINUS}", + "fields 10 fields, agreed 10, defaulted 0, adjudicated 0, flagged 1", + f"build {tmp_path / 'sheet.zip'}", + ] + + +def test_the_report_counts_route_as_fields_where_route_b_did_not_run(tmp_path): + # No filter, so no field was compared and every field is route A's. The counts of + # the comparison are left out rather than printed as zero. + result = routes.Converted( + set=None, zip_path=tmp_path / "sheet.zip", flags=[], fields=0, + reply=[{"title": "Ball", "main_text": "", "parts": [{"content": "Find h."}]}], + ) + assert result.report() == [ + "fields 5 fields, route B did not run", + f"build {tmp_path / 'sheet.zip'}", + ] + + +def test_the_report_says_what_the_filter_run_failed_with(tmp_path): + # The set is route A's reading alone, so the line counts route A's fields, as it + # does where no filter was given at all. + result = routes.Converted( + set=None, zip_path=tmp_path / "sheet.zip", flags=[], fields=0, + reply=[{"title": "Ball", "main_text": "", "parts": [{"content": "Find h."}]}], + route_b_error="Error running filter set.lua: attempt to index a nil value", + ) + assert result.report() == [ + "fields 5 fields, route B failed", + "route B failed: Error running filter set.lua: attempt to index a nil value", + f"build {tmp_path / 'sheet.zip'}", + ] + + # --- live ------------------------------------------------------------------------------- @@ -451,3 +514,26 @@ def test_the_me2_pair_converts_with_no_flag(tmp_path): ("q3.p1.worked_solution", routes.STRAY_MINUS), ] assert [q.title for q in result.set.questions] == [q["title"] for q in exported()] + + +@live +@pytest.mark.skipif(not ME2_TARGET.is_dir(), reason="private corpus") +def test_the_me2_pair_converts_from_the_command_line(tmp_path, capsys): + # The ticket's run: the command a reader types, the report it prints, the zip. + (pdf,) = [p for p in ME2_TARGET.glob("*.pdf") if "solutions" not in p.name] + (solutions,) = ME2_TARGET.glob("*solutions.pdf") + + code = cli.main( + ["convert", str(pdf), "--solutions", str(solutions), "--out", str(tmp_path / "out")] + ) + printed = capsys.readouterr().out + print("\n" + printed) + + assert code == 0 + # The printed solutions PDF holds separator lines that Mathpix reads as minus signs, + # so the worked solutions of Friction on a plate and Towing a submarine are flagged. + assert [line.split(":")[0] for line in printed.splitlines() if line.startswith("flag")] == [ + "flag q2.p1.worked_solution", + "flag q3.p1.worked_solution", + ] + assert printed.splitlines()[-1].startswith(f"build {tmp_path / 'out'}")