feat(experiment): add opt-in model routing for evaluators

[pdebjyot]

Closes #323
Related: #88

Description

Adds ModelRouter and RoutingRule (new module strands_evals/model_router.py), plus an optional model_router= parameter on Experiment, so evaluation cost/accuracy can be matched to evaluator complexity: fast models for structural/rubric checks, strong models for nuanced judgment — including case-dependent escalation (e.g., long traces get a stronger judge).

Semantics:

• Opt-in: no model_router → behavior unchanged.
• Explicit wins: an evaluator constructed with model=... is never re-routed.
• First matching rule wins; rules match on evaluator type (class or name) plus an optional per-case condition predicate. No match falls through to an optional default_model.
• Routing returns a shallow copy of the evaluator carrying the routed model, so shared instances are never mutated across concurrent workers. Evaluators without a model attribute (deterministic) pass through untouched.
• A raising condition logs a warning and skips the rule instead of failing the evaluation.
• model_router is intentionally excluded from Experiment.to_dict()/from_dict() since routing conditions are arbitrary callables.

Example

from strands_evals import Experiment, ModelRouter, RoutingRule

router = ModelRouter(rules=[
    RoutingRule(
        evaluator_types=[ToolSelectionAccuracyEvaluator, ToolParameterAccuracyEvaluator],
        model="us.anthropic.claude-haiku-4-5-20251001-v1:0",
    ),
    RoutingRule(
        evaluator_types=[GoalSuccessRateEvaluator],
        model="us.anthropic.claude-opus-4-1-20250805-v1:0",
        condition=lambda case: len(str(case.actual_trajectory or "")) > 10_000,
    ),
])

experiment = Experiment(cases=cases, evaluators=evaluators, model_router=router)

Testing

• 12 new tests: rule matching by class/name, condition gating, condition-exception safety, first-match-wins, default-model fallback, explicit-model preservation, no-mutation of shared instances, deterministic-evaluator passthrough, and two end-to-end Experiment integration tests asserting the judge agent is constructed with the routed model (and unchanged without a router).
• Full evaluator + experiment unit suites pass (454 tests); ruff check and ruff format clean.

Checklist

• Non-breaking change (opt-in)
• Unit + integration tests added
• Conventional commit
• Lint/format clean

[arielnabavian]

@strandly-the-agent review

[strandly-the-agent]

Superseded by my consolidated review below (review 5045343142) — this earlier draft was published prematurely by one of my review passes. Its findings (round-trip no-op, condition fall-through downgrade, matching semantics, SDK name collision) are folded into the consolidated review's inline comments and Questions.

[strandly-the-agent]

Requesting changes — the implementation is clean and does what the PR says (its "454 tests pass" reproduced exactly; concurrency and copy-isolation verified under max_workers=8); what's unsettled is the design, plus five reachable issues inline.

• 🔴 ModelRouter collides with the core SDK's strands.models.ModelRouter — renaming is free now, a breaking change later.
• 🟡 inline: a routing failure fails every evaluator for the case (reproduced); routing silently no-ops on any persisted/CLI-loaded experiment; an async def condition matches every case; the public docstring documents an attribute that doesn't exist and hides exact-class matching; the headline per-case routing has no end-to-end test.

Two things need a maintainer independent of the code: (1) #323 was filed by the PR author 2 minutes before this PR and its own "if this direction looks right" was never answered — per AGENTS.md that alignment should come first, and epic #88 explicitly preferred keeping Experiment unchanged; (2) this changes public API (2 new exports + an Experiment param) — this repo has no api-review label, so design looks like the closest lever — and given the two new nouns it likely warrants an API discussion rather than a solo approve.

Pre-existing serializer bug factored out to #385. All of this is offered for a human to weigh — dismiss freely.

How this was reviewed + evidence

Reviewed pr-325 @ 03e9295 against main @ 771a3fe (+341/−0, 4 files). Pipeline: triage → context build → 5 independent passes (correctness, issue-alignment, API design, adversarial, test quality) → aggregation. The API-design and adversarial passes timed out on the deep-analysis tier and were re-run on the standard tier; every load-bearing finding was re-verified during aggregation.

Check	Result
pytest tests/strands_evals/test_model_router.py tests/strands_evals/test_experiment.py -q	✅ 125 passed (14 new — PR body says 12)
PR body's "454 tests pass"	✅ reproduced exactly (454 passed)
ruff check + ruff format --diff on both new files	✅ clean
mypy	⚠️ not run (internal error in review sandbox); note routed.model = model only type-checks because attr-defined is disabled repo-wide
Repro: async def condition returning False	✅ routes anyway
Repro: to_dict()→from_dict()→route()	✅ router no-ops (same object returned)
Repro: route() raising mid-case (read-only model property)	✅ control 2/2 pass → routed 0/2 pass, actual_output lost
Both suggested fixes	✅ verified: tests + ruff/format pass; also verified moving the hook inside the try would silently disable routing (the retry closure default-binds evaluator)
copy.copy audit of all 21 model-bearing evaluators	✅ no cached Agent, no state derived from model — shallow copy + rebind is sound
Per-case routing, max_workers=8, 8 cases	✅ correct per case; no leak onto the shared instance
Mutation testing (7 mutants)	4 killed; 2 survived (memoized select_model; routing on pre-execution data) → test finding

Pass verdicts: correctness changes-requested · issue-alignment aligned, gate on design approval · API design needs-design-discussion · adversarial broke-it (1 reproduced escalation) · test-quality gaps-found. No live model calls were made; a clean full-tests/ run wasn't obtained (sandbox killed it twice at ~42%) — the affected suites above are green.

Questions

Blocking (maintainer call, not code):

1. Is this direction approved? #323 is self-filed, uncommented, and closes with "ready to submit as a PR if this direction looks right". In-runner hook, or an external compare/-style layer (per #88's stated v1 preference)?
2. Do two classes and five public members earn their keep over a plain callable? condition being an arbitrary callable already forfeits the declarative benefits (it's why model_router can't serialize). An evaluator_model: Callable[[Evaluator, EvaluationData], Model | str | None] ctor param gives the same power with zero new exports (the name collision evaporates), isinstance/name= routing for free, and typos become NameErrors instead of silent dead rules — and matches the repo's task= / @eval_task idiom. Alternative: drop condition for v1 and make it declarative Pydantic data that round-trips.
3. Apply the design label and take the two new public nouns to an API discussion? (This repo has no api-review label workflow; design is the closest lever.)
4. Does default_model belong in v1? It's the blanket single-tier behaviour #323's own Alternatives section rejects, and it's what makes the subclass-matching gap silent.

Non-blocking:
5. Should matches/select_model be public? route() is their only caller, and select_model bypasses both of route()'s eligibility guards (it returns default_model even for Contains, which has no model at all).
6. Should type matching be isinstance (subclasses free), and/or accept an evaluator's name= as a rule key? Today two Contains instances distinguished by name= can never route differently, though name= exists precisely to disambiguate them.
7. Hand-rolled top-level classes vs the #210 DiagnosisConfig precedent (Pydantic under types/, exported from both namespaces) — deliberate? Worth a line in the PR description either way.
8. README entry expected? #210 added +57 README lines and every comparable feature has a "Features at a Glance" subsection; nothing mentions model_router in README or docs today.
9. RedTeamExperiment doesn't forward model_router and ChaosExperiment doesn't expose it, though both run LLM judges — intentional for v1?

Reading order

1. src/strands_evals/model_router.py — the whole feature: route() first (the two guards and the copy are the entire contract), then RoutingRule.matches, then the module docstring/example as the user-facing spec.
2. src/strands_evals/experiment.py — the 2-line hook; read upward to the retry closure (which constrains where the hook can live) and downward to the error-isolation try (what the hook sits outside of).
3. src/strands_evals/__init__.py — the permanent public surface; the naming and callable-vs-classes questions live here.
4. tests/strands_evals/test_model_router.py — the two integration tests are the behavioural contract; what they don't cover is the test finding.
5. Context outside the diff: evaluators/evaluator.py to_dict + experiment.py from_dict (the round-trip no-op), and #210 / README for the precedent this diverges from.

Appendix — non-blocking (12)

1. ⚪ A typo'd or empty evaluator_types yields a silently dead rule; an evaluator instance fails with a cryptic AttributeError. Cheap: ValueError on empty, and a warning at Experiment.__init__ for rule names matching none of the configured evaluators (the list is known there). Contrast the SDK sibling, which validates its inputs.
2. ⚪ select_model applies neither of route()'s eligibility guards — document as rule-resolution-only or make it private (also Q5).
3. ⚪ experiment.py docstring lists model_router under Attributes: but no attribute or property exists (diagnosis_config, the precedent, is deliberately not listed).
4. ⚪ The guard getattr(...) is not None or not hasattr(...) reads backwards; a getattr(evaluator, "model", _MISSING) sentinel states the intent directly.
5. ⚪ rules/default_model/model/condition are public-mutable with no re-validation; ModelRouter() and RoutingRule(evaluator_types=[]) are legal dead configs.
6. ⚪ (contrived) A user __copy__ returning self breaks the no-mutation guarantee, which the route() docstring states unconditionally.
7. ⚪ RoutingRule(model=None) (violates the type hint, unvalidated) matches and then silently skips default_model too.
8. ⚪ Three meanings of None meet on this surface (router default_model=None = leave alone; evaluator model=None = strands' default BedrockModel; DEFAULT_BEDROCK_MODEL_ID = serialization-only) — one clarifying docstring line would help.
9. ⚪ The module's own example re-stringifies the whole trajectory once per (evaluator, rule) pair per case, unmemoized — a span-count predicate would be a better example.
10. ⚪ New file uses RST-style double backticks where AGENTS.md mandates single — pattern-consistent with ~20 existing files, so noted only.
11. ⚪ Callable from typing while experiment.py uses collections.abc — AGENTS.md allows either; matching experiment.py would be tidier.
12. Pre-existing (filed #385): Evaluator.to_dict() erases the model=None vs explicit-model distinction — the root enabler of the round-trip no-op finding; needs its own decision since an existing test asserts today's behaviour.

[strandly-the-agent]

🔴 strands_evals.ModelRouter collides with the core SDK's strands.models.ModelRouter — same name, unrelated semantics, and evals users import both packages.

The SDK class is a failover plugin among candidate models within one invocation; this one picks a judge model per evaluation. RoutingRule also lands next to the SDK's RoutingStrategy/RoutingCandidate, so the shared vocabulary now means two different things. Anyone using both packages hits this from their first import.

Suggestion: rename to something that says what it selects (e.g. EvaluatorModelPolicy / JudgeModelSelector), or drop the new nouns entirely in favor of a callable parameter (see Questions in the summary). Cost: a mechanical rename across 4 files today; after release it's a deprecation cycle.

[strandly-the-agent]

🟡 A failure inside routing fails every evaluator for the case and drops the task output from the report — including discarding a healthy evaluator's already-computed result.

Reproduced: a control run (no router) passes 2/2 evaluators with actual_output='hi'; the identical run with a router reports both as pass=False, "An error occurred: property 'model' of ... has no setter" and the report row loses actual_output. Reached by a user-defined evaluator (a documented extension point) whose model isn't a plain settable attribute or that isn't copy.copy-able; no built-in evaluator triggers it.

Suggestion: keep routing advisory, per the module's own "opt-in and advisory" contract — degrade to the unrouted evaluator with a warning (this must stay above the retry closure, which default-binds evaluator). Cost: a genuine routing bug surfaces as a warning rather than a loud failure.

Suggested change

	if self._model_router is not None:
	evaluator = self._model_router.route(evaluator, evaluation_context)
	if self._model_router is not None:
	try:
	evaluator = self._model_router.route(evaluator, evaluation_context)
	except Exception as e:
	logger.warning(
	"evaluator=<%s>, error=<%s> | model routing failed, running evaluator as configured",
	evaluator.get_name(),
	e,
	)

[strandly-the-agent]

🟡 Routing is a silent total no-op for any experiment loaded from a file, and nothing observable records whether routing happened.

Evaluator.to_dict() writes a default model=None out as a concrete model_id, and from_dict maps it back to an explicit model= — so after any to_file/from_file round trip this guard sees every LLM evaluator as "explicitly configured" and does nothing. That's the whole CLI path (strands-evals run/validate) plus anyone who persists experiments, and the only trace of a routing decision is a DEBUG log — nothing in the report or spans says which model judged a case.

Suggestion: in this PR, document the limitation here and on Experiment(model_router=...), pin it with a characterization test, and put the resolved model on the eval span (or a report field) so "did it route?" is answerable. The underlying serializer behavior is pre-existing — filed as #385, since fixing it changes behavior an existing test asserts.

[strandly-the-agent]

🟡 An async def condition makes the rule match every case — the exact inverse of a predicate that returns False.

bool(coroutine) is always True, so an async predicate (an easy mistake in a framework whose evaluators and runner are all async) routes every case to that rule's model, with only a RuntimeWarning buried in the logs — a silently wrong, possibly far more expensive judge on all cases.

Suggestion (also add import inspect to the imports):

Suggested change

	if self.condition is not None:
	try:
	return bool(self.condition(evaluation_data))
	except Exception as e:
	logger.warning(
	"rule_types=<%s>, error=<%s> | routing condition raised, skipping rule", self._type_names, e
	)
	return False
	return True
	if self.condition is not None:
	try:
	result = self.condition(evaluation_data)
	except Exception as e:
	logger.warning(
	"rule_types=<%s>, error=<%s> | routing condition raised, skipping rule", self._type_names, e
	)
	return False
	if inspect.isawaitable(result):
	if inspect.iscoroutine(result):
	result.close()
	logger.warning(
	"rule_types=<%s> | routing condition must be synchronous, skipping rule", self._type_names
	)
	return False
	return bool(result)
	return True

[strandly-the-agent]

🟡 This public docstring promises things the code doesn't do: evaluator_types is documented as an attribute but never stored (hasattr is False); the predicate is presented as taking a case when it receives an EvaluationData (a different exported type — no session_id here); exact class-name matching is unstated, so a rule for OutputEvaluator silently doesn't cover the shipped MultimodalOutputEvaluator; and a raising condition doesn't just "skip" — it falls through to later rules and default_model, silently downgrading an escalation rule like the module's own example.

Suggestion (verified against ruff/format; also rename case → data in the module example above):

Suggested change

	Attributes:
	evaluator_types: Evaluator classes or class names this rule applies to.
	model: The model (Model instance or Bedrock model-id string) to use when
	this rule matches.
	condition: Optional predicate over the evaluation data. When provided,
	the rule only matches cases for which it returns True. This enables
	complexity-based routing (e.g., trace length, span count).
	"""
	Attributes:
	model: The model (Model instance or Bedrock model-id string) to use when
	this rule matches.
	condition: Optional predicate over the case's `EvaluationData` (note: not
	a `Case`). When provided, the rule only matches cases for which it
	returns True, which enables complexity-based routing (e.g., trace
	length, span count). It must be synchronous and side-effect-free: the
	`EvaluationData` it receives is the live object that later lands in
	the report. A predicate that raises is logged and the rule is skipped,
	so matching falls through to the next rule and then to the router's
	`default_model` - which can silently downgrade the model tier.
	Note:
	`evaluator_types` is matched by exact class name (`get_type_name()`), so a
	rule for `OutputEvaluator` does not cover subclasses such as
	`MultimodalOutputEvaluator` - list those explicitly. The constructor
	argument is not retained as an attribute.
	"""

[strandly-the-agent]

🟡 The feature's headline — per-case, condition-driven routing — is never exercised through the runner. Both integration tests use one case and a condition-free rule, so a refactor that memoizes model selection per evaluator type, or routes on pre-execution data, passes the whole suite while silently mis-routing (checked by mutation: both survive).

Suggestion: one test closes this — two cases, a condition on actual_output length, max_workers=2; assert the two Agent(model=...) kwargs via call_args_list are the fast and strong models respectively, and evaluator.model is None afterwards (no leak onto the shared instance). Such a test was written and confirmed to pass on this head and kill both surviving mutants. While here: Mock() without spec=Agent, the dead mock_agent.return_value = mock_result (the runner goes through invoke_async), and call_args reading only the last call all become traps the moment a second case exists — a small shared _agent_mock() fixture removes the duplication too.

[pdebjyot]

Thank you for this — the depth is appreciated, and I agree with the verdict. Taking the findings and the design question in turn.

The three findings are real — I reproduced all of them against 03e9295.

1. Round-trip disables routing. Confirmed, and it's two compounding bugs. Evaluator.to_dict() writes model=None out as model_id=DEFAULT_BEDROCK_MODEL_ID, Experiment.from_dict() restores it as an explicit model=, and Experiment.to_dict() never serializes the router. So after to_file/from_file every judge is pinned to Sonnet, the model is None sentinel that gates route() is destroyed, and the router is gone — with no warning, on the CLI's load path. This is the most serious of the three.
2. Fail-open conditions. Confirmed: a raising condition is warn-logged and treated as no-match, then falls through to default_model, silently swapping the measurement instrument for a cheaper one. Agree fail-open is the wrong default for an eval library.
3. Name-equality matching + evaluator-invisible conditions. Confirmed: evaluator_types matches on __name__ string equality (a subclass doesn't match its base), and condition only receives EvaluationData, so two evaluators distinguished by name= can't be routed differently. The rule surface really is "larger than a callable and less capable than one."

I'm pivoting to the callable seam you sketched. Experiment(model_selector: Callable[[Evaluator, EvaluationData], Model | str | None] | None = None), consulted for every evaluator, None = leave as-is, exceptions propagate (fail-fast), runner owns copy-and-apply, no model is None eligibility sentinel ("explicit wins" becomes visible user policy). The deciding argument for me is the one-way door: callable→rules is additive later (a future rule-set object can implement __call__ and drop into a callable parameter), rules→callable is a deprecation — so for unreleased public names the reversible choice wins. It also dissolves all three findings at once (no sentinel to poison, fail-fast, receives the evaluator so isinstance and per-name routing both work), keeps the declarative door open, and sidesteps the strands.models.routing.ModelRouter name collision entirely. If a one-knob default_judge_model= is wanted as the v0 for "all judges on haiku," happy to add it alongside.

What I've split out already: the model=None → DEFAULT_BEDROCK_MODEL_ID serialization rewrite is a pre-existing bug on main, independent of routing, and it silently pins every reloaded experiment's judges regardless of this feature. I've put it up as its own small PR (#392) so it can land on its own merits; it's also a prerequisite for any "explicit model wins" logic to survive persistence.

What I'd ask a maintainer to settle before I re-push the routing surface:

1. Priority — is per-(evaluator, case) judge routing something this package wants to own? The [FEATURE] Model routing for evaluators — match model tier to evaluation complexity #323 two-tier production pattern (materially lower judge cost at equal accuracy) is the motivation; I'd like an explicit yes/no before shipping any public surface.
2. Provenance first? Reports don't record which model scored each case today. Routing is unauditable without per-case judge-model provenance in EvaluationReport — happy to build that first as the foundation if you agree it's the prerequisite.
3. design label on both this and [FEATURE] Model routing for evaluators — match model tier to evaluation complexity #323, since the repo has no API-review workflow.

I've kept the current branch as-is for reference rather than force-pushing, so the reproductions above still line up with the code. Glad to pair on the model_selector shape or the provenance piece whenever there's a direction.