design: pro-627 add ADR and design doc for module evaluation framework

[georgia-i-ai]

Context

themefinder/evals/ currently hard-wires its execution engine to its artefact store: each of the four
stage eval scripts (eval_generation.py, eval_mapping.py, eval_condensation.py, eval_refinement.py)
hand-writes its own Langfuse-vs-local branch, duplicating most of its logic across both paths — and mapping
is the one stage where those two paths don't even agree on scoring. This PR is the ADR + detailed design for
fixing that: a ports-and-adapters layer that makes pydantic-evals the execution engine and Langfuse purely a
(swappable) dataset/artefact store, proven by a genuine swappability test rather than assumed.

No code changes here — this is ADR-0013 plus its accompanying design doc. Implementation is intentionally
sequenced as a separate, already-scoped rollout (8 issues across 5 waves, see the design doc's "Rollout
sequencing"), so this PR is reviewable as a design decision on its own.

Decisions taking during the design:

• Scope grew beyond "swap the engine" to include: centralising the scattered os.getenv() reads across
  evals/ into one evals/settings.py, deleting evals/metrics.py outright (mapping's local scoring moves
  onto the same evaluator the Langfuse path already uses — a disclosed behaviour change), and adding a DVC
  pipeline (dvc.yaml) as a fourth way to run evals, for dependency-aware caching and experiment tracking.
• benchmark.py and generate_synthetic.py are deliberately left still coupled to Langfuse for this pass —
  but the minimal follow-up changes to de-couple each are already scoped in the design doc, not just a TODO.

Changes proposed in this pull request

• docs/architecture/decisions/0013-modular-evaluation-framework-for-themefinder.md — the ADR: decision and
  consequences.
• docs/architecture/design/modular-evaluation-framework.md — the detailed design: directory layout, the
  four ports (DatasetPort, EvaluatorPort, EvalRunnerPort, ArtefactStorePort, each an explicit
  abc.ABC) and their adapters, evals/settings.py, the DVC pipeline design, and the issue-by-issue rollout
  plan.

Guidance to review

Start with the ADR, and design is detailed but seemed worth including fully rather than shortening.

Worth focusing on:

• Whether four ports is the right cut, and whether EvaluatorPort being implemented directly by each kind
  of evaluator (custom, pydantic-evals-native, later DeepEval) rather than through a generic wrapper is the
  right call.
• The independent dataset-source/artefact-store selection in resolve_backends() — lets a run pull cases
  from Langfuse while storing results locally, or the reverse, rather than one bundled "Langfuse configured"
  decision.
• The one open, explicitly confidence-flagged risk: whether pydantic_evals.evaluators.EvaluatorContext can
  be constructed standalone outside Dataset.evaluate()'s own loop, which PydanticEvalsLLMJudgeAdapter
  depends on — flagged as a spike to do first, not assumed.
• The rollout sequencing itself (8 issues, 5 waves) — particularly whether Wave 3 (all four stage migrations
  bundled into one issue) is the right granularity, or should split per stage.

[linear-code]

PRO-627

[github-actions]

PR Review

Well-structured ADR + design for a ports-and-adapters eval framework. Clean separation of concerns, explicit risk flagging, and thorough rollout sequencing.

---

🟡 TRY: Split Wave 3 per stage
[docs/architecture/design/modular-evaluation-framework.md, Rollout sequencing, Wave 3]

Each stage migration is independently testable and reviewable. Bundling all four risks a large, hard-to-review PR. Recommend: eval_generation.py + the benchmark.py kwarg rename as one issue (two halves of one contract), eval_mapping.py + metrics.py deletion as its own (the one disclosed behaviour change), then eval_condensation.py/eval_refinement.py together.

---

🔵 CONSIDER: Four independent stage-name lists
[docs/architecture/design/modular-evaluation-framework.md, lines 86–89]

The design correctly flags that VALID_STAGES, EVAL_FUNCS, the workflow choices, and params.yaml are four independent lists with no single source of truth — missing one silently breaks a subset of entry points. Worth noting in the ADR's consequences as a concrete maintenance cost, even if a programmatic registry is a future-pass improvement.

---

🔵 CONSIDER: run_handle: Any weakens the ArtefactStorePort contract
[docs/architecture/design/modular-evaluation-framework.md, Ports section]

start_run returns Any, which each adapter interprets opaquely. A minimal RunHandle = TypeVar("RunHandle") bound at the port level (or a trivial @dataclass class RunHandle: ... each adapter subclasses) would give the type checker something to work with and make the pass-through to record_case/finish_run statically checkable without importing any adapter.

---

⚪️ NIT: Unconditional __main__ write bypasses the port
[docs/architecture/design/modular-evaluation-framework.md, Running via DVC section]

The CLI __main__ block always writing to local_eval_runs/ regardless of configured ArtefactStorePort is pragmatic for DVC, but is worth a design note clarifying this is intentional CLI-layer behaviour rather than a port responsibility leak — the current text explains that it's unconditional but not why it can't delegate to LocalJSONArtefactStore instead (answer: LangfuseArtefactStore runs don't write locally, so DVC would have no metrics file to track).

---

View job run

[github-actions]

Claude finished @georgia-i-ai's task in 1m 9s —— View job

---

PR Review

• Read ADR and design doc
• Analyze design decisions
• Post review feedback

---

Well-structured design. The ports-and-adapters cut is clean, the swappability proof is the right approach, and the EvaluatorContext spike being made explicit (rather than assumed away) is exactly the right call.

A few targeted points:

---

run_handle: Any in ArtefactStorePort — start_run returns Any and record_case/finish_run take it back. This works but loses the type connection between a run's start and its later writes.

⚪️ NIT: Consider a TypeVar bound to ArtefactStorePort to express "same adapter's own handle type" — e.g. class ArtefactStorePort(abc.ABC, Generic[H]). Not blocking, but would make the LangfuseArtefactStore vs LocalJSONArtefactStore handle types distinguishable at the type-checker level, which pays off once there are more adapters.

---

Wave 3 granularity (docs/architecture/design/modular-evaluation-framework.md, rollout section) — the PR itself flags this as a review question, and the current single-issue approach bundles a pure-structural migration with a disclosed behaviour change.

🟡 TRY: Split Wave 3 into (a) eval_generation.py + benchmark.py kwarg rename — the simplest migration, clean reference implementation; (b) eval_mapping.py + metrics.py deletion — isolates the one scoring-behaviour change; (c) eval_condensation.py + eval_refinement.py — straightforward once (a) is reviewed. Mapping's metrics.py deletion plus the Langfuse-vs-local scoring fix is a real behaviour change that deserves its own review cycle, not a comment in a larger PR.

---

Four independent stage name lists (VALID_STAGES, EVAL_FUNCS, CI choices, params.yaml) — the doc already calls this out as "all four must be updated by hand; missing one means a stage silently works in some entry points and not others."

🔵 CONSIDER: A future single source of truth (e.g. VALID_STAGES driving params.yaml generation, or a make target that diffs the four lists and errors on mismatch) would be worth adding to the deferred section. Not in scope now but worth noting while the design is being reviewed.

---

DVC dvc init location left unresolved (ADR consequences, final bullet) — repo root vs. themefinder/ subdir has downstream implications for dvc remote config, .dvc/config, and where dvc.lock lives relative to the CI workflow's checkout path.

🔵 CONSIDER: Can this be resolved now rather than left to implementation? The two options aren't symmetric — repo-root dvc init implies DVC tracks the whole monorepo's dep graph, while themefinder/-subdir init scopes it to the package. The Wave 4 issue won't be well-defined until this is decided.

[georgia-i-ai]

The design correctly flags that VALID_STAGES, EVAL_FUNCS, the workflow choices, and params.yaml are four independent lists with no single source of truth — missing one silently breaks a subset of entry points. Worth noting in the ADR's consequences as a concrete maintenance cost, even if a programmatic registry is a future-pass improvement.

Will consider this, definitely don't like the current multiple places to update so need find a better solution.

[georgia-i-ai]

Each stage migration is independently testable and reviewable. Bundling all four risks a large, hard-to-review PR. Recommend: eval_generation.py + the benchmark.py kwarg rename as one issue (two halves of one contract), eval_mapping.py + metrics.py deletion as its own (the one disclosed behaviour change), then eval_condensation.py/eval_refinement.py together.

Claude originally suggested breaking down, I prefer to add a single issue for this with a note that this should be broken down if needed. I feel like a lot of the changes will be similar and these scripts will become quite simple so should be possible to do in one go.

[Nazehs]

LGTM! in my opinion I think this would have been much better excluding the code snippets reason been that the implementation might choose to name the methods, classes or variables differently which might then drift from what is captured here.

[georgia-i-ai]

LGTM! in my opinion I think this would have been much better excluding the code snippets reason been that the implementation might choose to name the methods, classes or variables differently which might then drift from what is captured here.

Sure, that's a fair point. The snippets are only included as a guide and not meant to be prescriptive, but I can remove if you think it comes across this way.

• remove code snippets

[georgia-i-ai]

The design correctly flags that VALID_STAGES, EVAL_FUNCS, the workflow choices, and params.yaml are four independent lists with no single source of truth — missing one silently breaks a subset of entry points. Worth noting in the ADR's consequences as a concrete maintenance cost, even if a programmatic registry is a future-pass improvement.

Will consider this, definitely don't like the current multiple places to update so need find a better solution.

• specify that states should be set in just one place rather than multiple

[github-actions]

Claude finished @georgia-i-ai's task in 56s —— View job

---

PR Review

• Read ADR and design doc
• Analyze design decisions
• Post review feedback

---

Solid design. The ports-and-adapters cut is clean, the explicit risk flagging on EvaluatorContext standalone construction is exactly right, and the swappability proof is the correct approach.

⚪️ NIT: Three code snippets remain (modular-evaluation-framework.md, lines ~322–334, ~584–618, ~647–651) — PydanticEvalsLLMJudgeAdapter, EvalSettings, and the autouse cache-clearing fixture. Given the "remove code snippets" commit removed 96 lines, these three appear intentionally kept — worth confirming with Nazehs whether these are OK to retain as illustrative examples, or should also go.

🔵 CONSIDER: dvc init location still unresolved (ADR consequences, final bullet) — repo root vs. themefinder/ subdir carries downstream implications for dvc.lock placement and CI checkout paths. As flagged in the prior review, this asymmetry makes Wave 4's scope unclear until resolved.

[github-actions]

Claude finished @georgia-i-ai's task in 24s —— View job

---

PR Review

• Read ADR and design doc
• Analyze changes since last review
• Post review feedback

---

Clean update. The EvaluatorContext standalone-construction spike is correctly flagged and the Wave 3 note to split if needed is pragmatic.

⚪️ NIT: Three code snippets remain — PydanticEvalsLLMJudgeAdapter (lines 314–327), EvalSettings (lines 570–610), and the autouse fixture (lines 638–646). Given the previous review discussed removing snippets, worth confirming these three are intentionally kept as illustrative examples.

🔵 CONSIDER: dvc init location — ADR line 100–101 still flags this as unresolved. The two options (repo root vs. themefinder/ subdir) are asymmetric in their downstream effects on dvc.lock placement and CI paths, and Wave 4's scope won't be well-defined until this is settled. Can this be resolved here rather than left to implementation?

[github-actions]

Claude finished @georgia-i-ai's task in 49s —— View job

---

PR Review

• Read ADR and design doc
• Analyze design decisions
• Post review feedback

---

Clean, well-sequenced design. Ports-and-adapters cut is sharp, the independent dataset/artefact selection is a real improvement, and the explicit confidence flag on EvaluatorContext is the right call.

Two open items from prior reviews still unaddressed:

⚪️ NIT: Three code snippets remain (modular-evaluation-framework.md lines ~314–338, ~578–611, ~638–646) — PydanticEvalsLLMJudgeAdapter, EvalSettings, and the autouse fixture. Nazehs flagged these explicitly in their review. Remove or keep them →

🔵 CONSIDER: eval_dataset_source/eval_artefact_store accept invalid strings silently (settings.py design, line ~581) — typed as str | None but only "langfuse" or "local" are valid. Literal["langfuse", "local"] | None (or a small Enum) would catch THEMEFINDER_EVAL_DATASET_SOURCE=s3 at settings-read time rather than inside resolve_backends. Worth adding to the spec before Wave 0.

[georgia-i-ai]

🔵 CONSIDER: dvc init location — ADR line 100–101 still flags this as unresolved. The two options (repo root vs. themefinder/ subdir) are asymmetric in their downstream effects on dvc.lock placement and CI paths, and Wave 4's scope won't be well-defined until this is settled. Can this be resolved here rather than left to implementation?

This is fine to resolve at implementation.

[github-actions]

Claude finished @georgia-i-ai's task in 1m 15s —— View job

---

PR Review

• Read ADR and design doc
• Check for remaining code snippets
• Review current state
• Post feedback

---

Clean update. All three previously flagged code snippets (PydanticEvalsLLMJudgeAdapter, EvalSettings, autouse fixture) are now removed — the document is prose-only throughout.

🔵 CONSIDER: eval_dataset_source/eval_artefact_store accept invalid strings silently (settings.py design) — currently described as defaulting to None, but only "langfuse" or "local" are valid values. Specifying Literal["langfuse", "local"] | None (or a small Enum) in the design would catch a misconfigured THEMEFINDER_EVAL_DATASET_SOURCE=s3 at settings-read time rather than deep inside resolve_backends. Worth pinning in the spec before Wave 0 rather than leaving to implementation to decide.

[georgia-i-ai]

🔵 CONSIDER: eval_dataset_source/eval_artefact_store accept invalid strings silently (settings.py design) — currently described as defaulting to None, but only "langfuse" or "local" are valid values. Specifying Literal["langfuse", "local"] | None (or a small Enum) in the design would catch a misconfigured THEMEFINDER_EVAL_DATASET_SOURCE=s3 at settings-read time rather than deep inside resolve_backends. Worth pinning in the spec before Wave 0 rather than leaving to implementation to decide.

Fine to resolve at implementation time.

[github-actions]

Claude finished @georgia-i-ai's task in 55s —— View job

---

PR Review

• Read ADR and design doc
• Post review feedback

---

Clean final state. No Python implementation snippets remain — the doc is prose-only throughout (directory tree, mermaid diagram, and inline grep commands are all fine). Wave 3 carries the "could split if too large" note, the single-source-of-truth decision is properly framed in both ADR and design doc, and the two explicitly deferred items (dvc init location, Literal typing) have Georgia's sign-off to resolve at implementation time.

No new blockers.

[github-actions]

Claude finished @georgia-i-ai's task in 24s —— View job

---

PR Review

• Read ADR and design doc
• Post review feedback

---

Clean final state. Both documents are prose-only, the ports-and-adapters design is sharp, and swappability is proven rather than assumed.

No new issues. Ready to merge.