diff --git a/judge.py b/judge.py index 5eee7e3ae..65e26af23 100644 --- a/judge.py +++ b/judge.py @@ -12,9 +12,9 @@ from pathlib import Path from typing import Optional -from judge import judge_conversations, judge_single_conversation +from judge import judge_single_conversation, run_judging from judge.llm_judge import LLMJudge -from judge.rubric_config import ConversationData, RubricConfig, load_conversations +from judge.rubric_config import ConversationData, RubricConfig from judge.utils import ( build_judge_task_log_path, default_adhoc_parent, @@ -25,6 +25,7 @@ build_single_conversation_run_folder_name, is_judge_run_folder_basename, ) +from utils.rubric_manifest import load_manifest_rubric_paths from utils.utils import parse_key_value_list @@ -158,8 +159,62 @@ def get_parser() -> argparse.ArgumentParser: return parser +def _resolve_output_target( + args, gen_run: Optional[str] +) -> tuple[Optional[str], Optional[str]]: + """Decide where evaluations go, returning ``(output_root, output_folder)``. + + Exactly one is non-None. ``output_root`` is a parent to mint a new ``j_*`` + run folder under; ``output_folder`` is an exact existing folder to write + into, which is how resuming lands back in the same place instead of + starting a new run. + + This is CLI policy, which is why it lives here rather than in the domain. + """ + if args.resume: + if not args.output: + raise ValueError( + "Resume mode requires --output to point to an existing evaluation " + "run folder (j_*__*)." + ) + if not os.path.isdir(args.output): + raise ValueError( + "Resume mode requires --output to point to an existing " + "evaluation run folder." + ) + base = os.path.basename(os.path.normpath(args.output)) + if not is_judge_run_folder_basename(base): + raise ValueError( + "Resume mode requires --output to be a judge run folder " + f"(basename like j_*__*), got {base!r}" + ) + return None, args.output + + if args.output is not None: + return args.output, None + if gen_run is not None: + return os.path.join(gen_run, "evaluations"), None + + print( + "Note: flat conversation folder; writing evaluations under " + "evaluations/. New runs use output/p_*__/conversations/.", + file=sys.stderr, + ) + return "evaluations", None + + async def main(args) -> Optional[str]: - """Main async entrypoint for judging conversations.""" + """Legacy CLI entry point: resolve ``args``, then call the judging domain. + + This is CLI glue, not a domain entry point. It owns everything specific to + this script's argument conventions — model shorthand parsing, the + manifest-path input form, output-location policy, resume validation, and + debug setup — and hands fully resolved values to `judge.run_judging`. + + `vera judge` does not call this. It calls `run_judging` directly and + resolves its own inputs, so nothing new belongs here: this script is + retained only until `vera resume` exists (see docs/architecture.md). + """ if args.debug: from utils.debug import set_debug @@ -168,9 +223,6 @@ async def main(args) -> Optional[str]: # Parse judge models from args (supports "model" or "model:count" format) judge_models = parse_judge_models(args.judge_model) - models_str = ", ".join(f"{model}x{count}" for model, count in judge_models.items()) - print(f"🎯 LLM Judge | Models: {models_str}") - if len(args.rubrics) > 1: print( f"Warning: multiple rubrics passed ({args.rubrics}); " @@ -179,11 +231,18 @@ async def main(args) -> Optional[str]: file=sys.stderr, ) - # Load rubric configuration once at startup - print("📚 Loading rubric configuration...") - rubric_config = await RubricConfig.load_bundle(args.rubrics[0]) + rubric_paths = await load_manifest_rubric_paths(args.rubrics[0]) if args.conversation: + # Single-conversation judging is legacy-only: `vera judge` drops this + # mode, so it is not part of the resolved-value domain entry point. + models_str = ", ".join( + f"{model}x{count}" for model, count in judge_models.items() + ) + print(f"🎯 LLM Judge | Models: {models_str}") + print("📚 Loading rubric configuration...") + rubric_config = await RubricConfig.from_paths(**rubric_paths) + # Single conversation with first judge model (single instance) first_model = next(iter(judge_models.keys())) @@ -223,57 +282,23 @@ async def main(args) -> Optional[str]: return out_run transcripts_dir, gen_run, conv_basename = resolve_conversation_input(args.folder) + output_root, output_folder = _resolve_output_target(args, gen_run) - print(f"📂 Loading conversations from {transcripts_dir}...") - conversations = await load_conversations(transcripts_dir, limit=args.limit) - print(f"✅ Loaded {len(conversations)} conversations") - - judge_kwargs = dict( + _, output_folder = await run_judging( judge_models=judge_models, - conversations=conversations, - rubric_config=rubric_config, - max_concurrent=args.max_concurrent, + **rubric_paths, + transcripts_dir=transcripts_dir, conversation_folder_name=conv_basename, - verbose=True, + limit=args.limit, + output_root=output_root, + output_folder=output_folder, judge_model_extra_params=args.judge_model_extra_params, + max_concurrent=args.max_concurrent, per_judge=args.per_judge, verbose_workers=args.verbose_workers, + verbose=True, resume=args.resume, ) - if args.resume: - if not args.output: - raise ValueError( - "Resume mode requires --output to point to an existing evaluation " - "run folder (j_*__*)." - ) - if not os.path.isdir(args.output): - raise ValueError( - "Resume mode requires --output to point to an existing " - "evaluation run folder." - ) - base = os.path.basename(os.path.normpath(args.output)) - if not is_judge_run_folder_basename(base): - raise ValueError( - "Resume mode requires --output to be a judge run folder " - f"(basename like j_*__*), got {base!r}" - ) - judge_kwargs["output_folder"] = args.output - else: - if args.output is None: - if gen_run is not None: - output_root = os.path.join(gen_run, "evaluations") - else: - output_root = "evaluations" - print( - "Note: flat conversation folder; writing evaluations under " - "evaluations/. New runs use output/p_*__/conversations/.", - file=sys.stderr, - ) - else: - output_root = args.output - judge_kwargs["output_root"] = output_root - - _, output_folder = await judge_conversations(**judge_kwargs) print(f"Evaluation output: {output_folder}/") return output_folder diff --git a/judge/__init__.py b/judge/__init__.py index 8805a5c86..c1fda3293 100644 --- a/judge/__init__.py +++ b/judge/__init__.py @@ -1,6 +1,7 @@ """Judge Package - LLM Conversation Evaluation System""" from .llm_judge import LLMJudge +from .run import run_judging from .runner import ( judge_conversations, judge_single_conversation, @@ -10,4 +11,5 @@ "LLMJudge", "judge_conversations", "judge_single_conversation", + "run_judging", ] diff --git a/judge/rubric_config.py b/judge/rubric_config.py index b39d41f39..c5622e861 100644 --- a/judge/rubric_config.py +++ b/judge/rubric_config.py @@ -14,7 +14,7 @@ import pandas as pd from judge.question_navigator import QuestionNavigator -from utils.rubric_manifest import load_manifest +from utils.rubric_manifest import load_manifest_rubric_paths # Rubric TSV column names - single source of truth for rubric structure COL_QUESTION_ID = "Question ID" @@ -85,11 +85,46 @@ async def load( Raises: FileNotFoundError: If any required file doesn't exist """ - rubric_path = Path(rubric_folder) / rubric_file - rubric_prompt_beginning_path = ( - Path(rubric_folder) / rubric_prompt_beginning_file + return await cls.from_paths( + rubric_file=str(Path(rubric_folder) / rubric_file), + rubric_prompt_beginning_file=str( + Path(rubric_folder) / rubric_prompt_beginning_file + ), + question_prompt_file=str(Path(rubric_folder) / question_prompt_file), + sep=sep, ) - question_prompt_path = Path(rubric_folder) / question_prompt_file + + @classmethod + async def from_paths( + cls, + *, + rubric_file: str, + rubric_prompt_beginning_file: str, + question_prompt_file: str, + sep: str = "\t", + ) -> "RubricConfig": + """Load rubric data from three already-resolved file paths. + + This is the resolved-value entry point: callers pass complete paths and + this does no joining, defaulting, or manifest reading. `load` and + `load_bundle` are the convenience wrappers that resolve their inputs + down to these three paths. + + Args: + rubric_file: Path to the rubric TSV + rubric_prompt_beginning_file: Path to the system prompt template + question_prompt_file: Path to the question prompt template + sep: Separator for the rubric TSV (default: tab) + + Returns: + Loaded RubricConfig with all data + + Raises: + FileNotFoundError: If any required file doesn't exist + """ + rubric_path = Path(rubric_file) + rubric_prompt_beginning_path = Path(rubric_prompt_beginning_file) + question_prompt_path = Path(question_prompt_file) # Validate files exist if not rubric_path.exists(): @@ -165,15 +200,7 @@ async def load_bundle(cls, manifest_path: str) -> "RubricConfig": doesn't exist ValueError: If the manifest is missing a required key """ - manifest_file = Path(manifest_path) - manifest = await load_manifest(manifest_path) - - return await cls.load( - rubric_folder=str(manifest_file.parent), - rubric_file=manifest["rubric_file"], - rubric_prompt_beginning_file=manifest["rubric_prompt_beginning_file"], - question_prompt_file=manifest["question_prompt_file"], - ) + return await cls.from_paths(**await load_manifest_rubric_paths(manifest_path)) @staticmethod async def _read_file(file_path: Path) -> str: diff --git a/judge/run.py b/judge/run.py new file mode 100644 index 000000000..df32510d4 --- /dev/null +++ b/judge/run.py @@ -0,0 +1,125 @@ +"""Resolved-input entry point for judging conversations. + +This is the judging domain's application function: it receives fully resolved +values, loads what those values point at, and runs the evaluation. It is the +counterpart of `generate_conversations.run.run_generation` on the generation +side, and it is what `vera judge` calls. + +It deliberately does none of the CLI's work. It does not parse arguments, read +a target manifest, apply defaults, choose an output location, or configure +debug logging — every one of those is resolved by the caller. That boundary is +what lets one function serve both the unified CLI and the legacy `judge.py` +script without either one's conventions leaking into the domain. + +Unlike the generation side, this module already sits in the permanent package, +so no temporary root-level boundary function is needed. +""" + +from __future__ import annotations + +from typing import Any, Dict, List, Optional + +from .rubric_config import RubricConfig, load_conversations +from .runner import judge_conversations + + +async def run_judging( + *, + judge_models: Dict[str, int], + rubric_file: str, + rubric_prompt_beginning_file: str, + question_prompt_file: str, + transcripts_dir: str, + conversation_folder_name: Optional[str], + limit: Optional[int], + output_root: Optional[str], + output_folder: Optional[str], + judge_model_extra_params: Dict[str, Any], + max_concurrent: Optional[int], + per_judge: bool, + verbose_workers: bool, + verbose: bool, + resume: bool, +) -> tuple[List[Dict[str, Any]], str]: + """Evaluate a folder of conversations from fully resolved inputs. + + Args: + judge_models: Judge model name to instance count, already parsed from + whatever shorthand the caller accepts + rubric_file: Resolved path to the rubric TSV + rubric_prompt_beginning_file: Resolved path to the system prompt template + question_prompt_file: Resolved path to the question prompt template + transcripts_dir: Resolved directory holding the conversation `.txt` + files. The caller has already decided whether this is a nested + `conversations/` directory or a legacy flat folder. + conversation_folder_name: Basename recorded in output paths, or None + limit: Cap on conversations loaded, or None for all + output_root: Parent directory to mint a new `j_*` run folder under. + Mutually exclusive with `output_folder`. + output_folder: Exact existing run folder to write into, bypassing run + naming. Mutually exclusive with `output_root`; this is what resuming + uses to land back in the same folder. + judge_model_extra_params: Provider parameters for the judge model + max_concurrent: Worker ceiling, or None for unlimited + per_judge: Whether `max_concurrent` applies per judge model or in total + verbose_workers: Whether workers log concurrency behavior + verbose: Whether to print progress + resume: Whether to skip evaluation TSVs that already exist + + Returns: + Tuple of (results, output_folder) where output_folder is where the + evaluations were written. + + Raises: + ValueError: If the output target is not exactly one of `output_root` + or `output_folder`. + FileNotFoundError: If a rubric file or the transcripts directory is + missing. + """ + if (output_root is None) == (output_folder is None): + raise ValueError( + "run_judging requires exactly one output target: output_root to " + "create a new run folder, or output_folder to write into an " + "existing one" + ) + + if verbose: + models_str = ", ".join( + f"{model}x{count}" for model, count in judge_models.items() + ) + print(f"🎯 LLM Judge | Models: {models_str}") + print("📚 Loading rubric configuration...") + + rubric_config = await RubricConfig.from_paths( + rubric_file=rubric_file, + rubric_prompt_beginning_file=rubric_prompt_beginning_file, + question_prompt_file=question_prompt_file, + ) + + if verbose: + print(f"📂 Loading conversations from {transcripts_dir}...") + conversations = await load_conversations(transcripts_dir, limit=limit) + if verbose: + print(f"✅ Loaded {len(conversations)} conversations") + + # `judge_conversations` distinguishes the two output modes by which keyword + # it receives, so pass only the one the caller resolved. + output_target: Dict[str, Any] = ( + {"output_folder": output_folder} + if output_folder is not None + else {"output_root": output_root} + ) + + return await judge_conversations( + judge_models=judge_models, + conversations=conversations, + rubric_config=rubric_config, + conversation_folder_name=conversation_folder_name, + verbose=verbose, + judge_model_extra_params=judge_model_extra_params, + max_concurrent=max_concurrent, + per_judge=per_judge, + verbose_workers=verbose_workers, + resume=resume, + **output_target, + ) diff --git a/tests/unit/judge/test_judge_cli.py b/tests/unit/judge/test_judge_cli.py index 403568a60..1d887e066 100644 --- a/tests/unit/judge/test_judge_cli.py +++ b/tests/unit/judge/test_judge_cli.py @@ -6,6 +6,7 @@ import pytest +import judge.run as _judge_run from utils.conversation_layout import resolve_conversation_input # Load judge.py script (project root) so we can test get_parser and main @@ -144,14 +145,18 @@ async def test_main_single_conversation_calls_judge_single(self): new_callable=AsyncMock, ) as judge_single, ): - RubricConfig.load_bundle = AsyncMock(return_value="rubric_config") + RubricConfig.from_paths = AsyncMock(return_value="rubric_config") ConversationData.load = AsyncMock(return_value="conversation_data") LLMJudge.return_value = "judge_instance" result = await main(args) - RubricConfig.load_bundle.assert_called_once_with( - "data/SI/rubric_manifest.json" + # main() resolves the manifest to concrete paths, then the rubric is + # built from those paths rather than from the manifest itself. + RubricConfig.from_paths.assert_called_once_with( + rubric_file="data/SI/rubric.tsv", + rubric_prompt_beginning_file="data/SI/rubric_prompt_beginning.txt", + question_prompt_file="data/SI/question_prompt.txt", ) ConversationData.load.assert_called_once_with("conv.txt") LLMJudge.assert_called_once_with( @@ -189,27 +194,31 @@ async def test_main_folder_calls_judge_conversations(self): "-vw", ] ) + # Loading and dispatch now happen inside `judge.run.run_judging`, so the + # seams are patched there; main()'s job is to resolve and delegate. with ( - patch.object(_judge_script, "RubricConfig") as RubricConfig, + patch.object(_judge_run, "RubricConfig") as RubricConfig, patch.object( - _judge_script, + _judge_run, "load_conversations", new_callable=AsyncMock, ) as load_convos, patch.object( - _judge_script, + _judge_run, "judge_conversations", new_callable=AsyncMock, ) as judge_convos, ): - RubricConfig.load_bundle = AsyncMock(return_value="rubric_config") + RubricConfig.from_paths = AsyncMock(return_value="rubric_config") load_convos.return_value = [] judge_convos.return_value = ([], "evaluations/run1_timestamp") result = await main(args) - RubricConfig.load_bundle.assert_called_once_with( - "data/SI/rubric_manifest.json" + RubricConfig.from_paths.assert_called_once_with( + rubric_file="data/SI/rubric.tsv", + rubric_prompt_beginning_file="data/SI/rubric_prompt_beginning.txt", + question_prompt_file="data/SI/question_prompt.txt", ) expected_dir, _, _ = resolve_conversation_input("conversations/run1") load_convos.assert_called_once_with(expected_dir, limit=10) @@ -247,15 +256,15 @@ async def test_main_folder_resume_uses_output_folder(self, tmp_path: Path): ] ) with ( - patch.object(_judge_script, "RubricConfig") as RubricConfig, + patch.object(_judge_run, "RubricConfig") as RubricConfig, patch.object( - _judge_script, "load_conversations", new_callable=AsyncMock + _judge_run, "load_conversations", new_callable=AsyncMock ) as load_convos, patch.object( - _judge_script, "judge_conversations", new_callable=AsyncMock + _judge_run, "judge_conversations", new_callable=AsyncMock ) as judge_convos, ): - RubricConfig.load_bundle = AsyncMock(return_value="rubric_config") + RubricConfig.from_paths = AsyncMock(return_value="rubric_config") load_convos.return_value = [] judge_convos.return_value = ([], str(eval_folder)) @@ -283,16 +292,18 @@ async def load_rubric_config(rubrics_arg): ) with ( patch.object( - _judge_script, "load_conversations", new_callable=AsyncMock + _judge_run, "load_conversations", new_callable=AsyncMock ) as load_convos, patch.object( - _judge_script, "judge_conversations", new_callable=AsyncMock + _judge_run, "judge_conversations", new_callable=AsyncMock ) as judge_convos, - patch.object(_judge_script, "RubricConfig") as RubricConfigMock, + patch.object(_judge_run, "RubricConfig") as RubricConfigMock, ): from judge.rubric_config import RubricConfig as RealRubricConfig - RubricConfigMock.load_bundle = RealRubricConfig.load_bundle + # Real parsing, so the two fixtures must produce different + # rubrics -- proving --rubrics is live, not a no-op. + RubricConfigMock.from_paths = RealRubricConfig.from_paths load_convos.return_value = [] judge_convos.return_value = ([], "evaluations/run1_timestamp") @@ -322,22 +333,27 @@ async def test_main_warns_on_multiple_rubrics(self, capsys): ] ) with ( - patch.object(_judge_script, "RubricConfig") as RubricConfig, + patch.object(_judge_run, "RubricConfig") as RubricConfig, patch.object( - _judge_script, "load_conversations", new_callable=AsyncMock + _judge_run, "load_conversations", new_callable=AsyncMock ) as load_convos, patch.object( - _judge_script, "judge_conversations", new_callable=AsyncMock + _judge_run, "judge_conversations", new_callable=AsyncMock ) as judge_convos, ): - RubricConfig.load_bundle = AsyncMock(return_value="rubric_config") + RubricConfig.from_paths = AsyncMock(return_value="rubric_config") load_convos.return_value = [] judge_convos.return_value = ([], "evaluations/run1_timestamp") await main(args) - RubricConfig.load_bundle.assert_called_once_with( - "tests/fixtures/rubric_manifest_simple.json" + # Only the first manifest's files are resolved and loaded. + RubricConfig.from_paths.assert_called_once_with( + rubric_file="tests/fixtures/rubric_simple.tsv", + rubric_prompt_beginning_file=( + "tests/fixtures/rubric_prompt_beginning.txt" + ), + question_prompt_file="tests/fixtures/question_prompt.txt", ) captured = capsys.readouterr() assert "Warning" in captured.err diff --git a/utils/rubric_manifest.py b/utils/rubric_manifest.py index c9e03f66a..79c8a6fd5 100644 --- a/utils/rubric_manifest.py +++ b/utils/rubric_manifest.py @@ -79,6 +79,30 @@ async def load_manifest_personas(manifest_path: str) -> list[str]: return [str(manifest_dir / p) for p in manifest.get("personas", [])] +async def load_manifest_rubric_paths(manifest_path: str) -> dict[str, str]: + """Resolve a manifest's three rubric files relative to the manifest. + + Returns the paths keyed to match `RubricConfig.from_paths`, so a caller that + has a manifest can go straight from one to the other. `load_manifest` has + already validated that all three keys are present. + + This is the single place the manifest-relative rule is applied for rubric + files, so `RubricConfig.load_bundle` and callers holding a bare manifest + path (the legacy `judge.py --rubrics` form) cannot drift apart. Callers that + already hold resolved paths -- `vera`'s target resolution, which validates + them itself -- do not need this at all. + """ + manifest = await load_manifest(manifest_path) + manifest_dir = Path(manifest_path).parent + return { + "rubric_file": str(manifest_dir / manifest["rubric_file"]), + "rubric_prompt_beginning_file": str( + manifest_dir / manifest["rubric_prompt_beginning_file"] + ), + "question_prompt_file": str(manifest_dir / manifest["question_prompt_file"]), + } + + async def load_manifest_persona_context_template(manifest_path: str) -> str: """Resolve a manifest's persona context template relative to the manifest.""" manifest = await load_manifest(manifest_path)