Skip to content

feat: add failure cohort analysis for evaluation reports - #360

Open
max-rattray-aws wants to merge 2 commits into
strands-agents:mainfrom
max-rattray-aws:feat/failure-cohorts
Open

feat: add failure cohort analysis for evaluation reports#360
max-rattray-aws wants to merge 2 commits into
strands-agents:mainfrom
max-rattray-aws:feat/failure-cohorts

Conversation

@max-rattray-aws

Copy link
Copy Markdown
Contributor

Summary

Adds a new strands_evals.analysis module with analyze_failure_cohorts() that groups failed evaluation cases by evaluator name, sorted largest-first. This surfaces systemic problems (e.g. 14 Faithfulness failures out of 20 total) vs scattered one-off edge cases, without requiring users to write their own grouping code.

Usage

from strands_evals import Experiment
from strands_evals.analysis import analyze_failure_cohorts, print_cohort_summary

experiment = Experiment(cases=my_cases, evaluators=[correctness, faithfulness, harmfulness])
report = experiment.run_evaluations(task=my_task)

analysis = analyze_failure_cohorts(report)

for cohort in analysis.systemic_cohorts:
    print(f"{cohort.evaluator_name}: {cohort.count} failures")
    print(f"  Cases: {', '.join(cohort.failed_case_names)}")

# Or use the Rich display helper:
print_cohort_summary(analysis)

What's included

  • FailureCohort Pydantic model with is_systemic property
  • CohortAnalysis model with systemic_cohorts and one_off_failures properties
  • analyze_failure_cohorts(report) pure function (no side effects, no model calls)
  • print_cohort_summary(analysis) Rich table display helper (no new deps)
  • Works on any EvaluationReport including from EvaluationReport.from_file() and EvaluationReport.flatten()

What's tested

  • FailureCohort construction and is_systemic boundary
  • CohortAnalysis property filtering (systemic vs one-off)
  • Pydantic serialization roundtrips
  • All-pass, all-fail, mixed pass/fail scenarios
  • Multi-evaluator sorting (count descending, alphabetical tiebreaker)
  • Missing evaluator/name keys default gracefully
  • Integration with EvaluationReport.from_file() and EvaluationReport.flatten()
  • Large cohort (100 cases)
  • Rich display helper output and truncation
  • Module importability from top-level package

Related to #348

Add strands_evals.analysis module with analyze_failure_cohorts() that
groups failed cases by evaluator name, sorted largest-first. This helps
identify systemic problems (e.g. 14 Faithfulness failures out of 20
total) vs scattered one-off edge cases.

New types:
- FailureCohort: a group of cases that failed the same evaluator
- CohortAnalysis: sorted list of cohorts with summary counts

Also includes a print_cohort_summary() Rich display helper.
@max-rattray-aws
max-rattray-aws requested a review from a team as a code owner August 10, 2026 16:56
@max-rattray-aws
max-rattray-aws requested a review from pgrayy August 10, 2026 16:56
@github-actions github-actions Bot added enhancement New feature or request area-core Core eval framework: Case, Experiment, task handler, evaluation data stores area-evaluators Evaluators: output, trajectory, tool use, interactions, and LLM-as-judge quality metrics strands-running labels Aug 10, 2026
"""
failures_by_evaluator: dict[str, list[tuple[int, str]]] = {}

for i, (case, passed) in enumerate(zip(report.cases, report.test_passes, strict=False)):

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Issue: zip(..., strict=False) silently stops at the shorter of cases/test_passes. Since total_cases and total_failures are computed independently from report.test_passes (lines below), a length mismatch between the two lists would produce a silently inconsistent CohortAnalysis (e.g. total_failures counting rows that were never bucketed) rather than an error.

Suggestion: Use strict=True so a corrupted/mismatched report surfaces loudly, or if lenient behavior is intentional (e.g. for from_file on hand-edited JSON), add a one-line comment documenting the invariant so the choice is clear.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Addressed in 69bea41 — now zip(..., strict=True), so a cases/test_passes length mismatch surfaces loudly instead of silently truncating.

Minor follow-up (non-blocking): consider adding a test that asserts the mismatch case raises (e.g. with pytest.raises(ValueError): analyze_failure_cohorts(report_with_mismatched_lengths)), so this new contract is locked in against regressions.

Args:
analysis: A CohortAnalysis returned by analyze_failure_cohorts.
"""
from rich.console import Console

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Issue: rich is imported lazily inside print_cohort_summary, but rich is a hard runtime dependency (rich>=14.0.0,<15.0.0 in pyproject.toml). AGENTS.md is explicit that hard deps must be imported at module top and that lazy imports of them are not allowed (the allowed exceptions are optional extras, genuinely expensive rare-path imports, or breaking real circular imports — none apply here).

Suggestion: Move from rich.console import Console and from rich.table import Table to the top of the module alongside the other imports. (The linked issue #348 sketched the lazy-import version, but the repo convention takes precedence.)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Resolved in 69bea41from rich.console import Console and from rich.table import Table are now at module top (lines 11–12). Thanks for the quick fix.

@github-actions

Copy link
Copy Markdown

Issue (API review): This PR introduces a new public surface — a new strands_evals.analysis module, two public Pydantic models (FailureCohort, CohortAnalysis), and a public function/helper exported from the top-level package. Per the repo's API bar-raising process, new public abstractions customers build on should go through API review, but the PR isn't carrying a needs-api-review label and the description doesn't include the API-review details (use cases ✓ present, but explicit signatures/defaults and abstraction/extensibility rationale aren't called out).

Suggestion: Add the needs-api-review label and a short API section. A couple of design questions worth resolving during that review:

  • Placement: the issue left "new module vs. method on EvaluationReport" open. A free function is fine, but worth a deliberate call since it's the entry point other analysis tools will build on.
  • is_systemic threshold: hardcoded at >= 2. Is a fixed threshold the right default, or should it be configurable (e.g. a parameter on analyze_failure_cohorts or CohortAnalysis)? "2 = systemic" is a strong claim for small suites.

@github-actions

Copy link
Copy Markdown

Assessment: Request Changes

Clean, well-scoped, well-tested feature that faithfully implements #348. The main blocker is a repo-convention violation on imports; the rest are design/process items to settle before this public API lands.

Review Categories
  • Convention compliance: rich (a hard dependency) is imported lazily inside print_cohort_summary; AGENTS.md requires hard deps at module top. This should be fixed.
  • API review: New public module + models + top-level export should go through API review (needs-api-review label) — a couple of design defaults (module placement, hardcoded is_systemic threshold) are worth confirming there.
  • Robustness: zip(strict=False) can silently mask cases/test_passes length mismatches given the counts are derived independently.
  • Testing: Coverage is broad (edge cases, sorting, serialization, integration with from_file/flatten). Prefer whole-object equality over per-field asserts in the deterministic cases so regressions in unasserted fields can't slip through.
  • Composability: cli/commands/run.py::_print_summary already groups by evaluator and could reuse this new primitive (follow-up, not blocking).

Nice work — the pure-function design, docstrings, and graceful defaulting for missing keys are all solid.

@github-actions

Copy link
Copy Markdown

Assessment: Approve (pending API review)

Re-reviewed after 69bea41 (fix: address review feedback). All the code-level feedback from the previous pass has been addressed cleanly:

Verification of fixes
  • Importsrich (Console, Table) moved to module top; no more lazy import of a hard dependency.
  • Robustnesszip(..., strict=True) now surfaces cases/test_passes length mismatches instead of silently truncating.
  • Tests — deterministic cases now assert whole-object equality (assert analysis == CohortAnalysis(...)) instead of per-field checks, so regressions in unasserted fields can't slip through. Ran the suite locally: 26 passed.

Two open items, neither blocking a code approval:

  • API review (process): still no needs-api-review label. As a new public module + models + top-level export, this should get an API-review sign-off before merge — including a decision on the hardcoded is_systemic >= 2 threshold and module-vs-method placement.
  • Test coverage (minor follow-up): worth adding a pytest.raises test for the new strict=True mismatch behavior to lock in that contract.

Nice, responsive iteration — the module is clean and well-tested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-core Core eval framework: Case, Experiment, task handler, evaluation data stores area-evaluators Evaluators: output, trajectory, tool use, interactions, and LLM-as-judge quality metrics enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant