Skip to content

feat(aggregators): add general aggregator for evaluation reports - #230

Closed
venkatkrish543re wants to merge 4 commits into
strands-agents:mainfrom
venkatkrish543re:feature/aggregators
Closed

feat(aggregators): add general aggregator for evaluation reports#230
venkatkrish543re wants to merge 4 commits into
strands-agents:mainfrom
venkatkrish543re:feature/aggregators

Conversation

@venkatkrish543re

@venkatkrish543re venkatkrish543re commented May 15, 2026

Copy link
Copy Markdown

Description

Adds a general aggregator over EvaluationReport lists. Groups results by (case_key, evaluator_name), computes descriptive stats. The three overridable hooks(_group_key, _filter_entry, _build_result) for project specific subclasses

LLM summary is optional via a configurable system_prompt. JSON serialization and terminal display included.

Related Issues

#114

Documentation PR

Type of Change

New feature

Testing

How have you tested the change? Verify that the changes do not break functionality or introduce warnings in consuming repositories: agents-docs, agent-tools, agents-cli

  • I ran hatch run prepare

Checklist

  • I have read the CONTRIBUTING document
  • I have added any necessary tests that prove my fix is effective or my feature works
  • I have updated the documentation accordingly
  • I have added an appropriate example to the documentation to outline the feature, or no new docs are needed
  • My changes generate no new warnings
  • Any dependent changes have been merged and published

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Adds a general aggregator over EvaluationReport lists. Groups results by (case_key, evaluator_name), filters corrupted trials, computes descriptive stats and optional efficiency rollup, and runs paired statistics (Wilcoxon, paired-t, McNemar) when exactly two conditions are detected with trial_idx pairing.

LLM summary is optional via a configurable system_prompt. JSON serialization and terminal display included.
Comment thread src/strands_evals/aggregators/base.py Outdated
Comment thread src/strands_evals/aggregators/base.py Outdated
Comment thread src/strands_evals/aggregators/base.py Outdated
@venkatkrish543re
venkatkrish543re marked this pull request as ready for review May 15, 2026 23:19
Comment thread src/strands_evals/aggregators/base.py Outdated
Comment thread src/strands_evals/aggregators/base.py Outdated
Comment thread src/strands_evals/aggregators/base.py Outdated
Comment thread src/strands_evals/aggregators/base.py Outdated
Comment thread src/strands_evals/aggregators/base.py Outdated
@venkatkrish543re
venkatkrish543re marked this pull request as draft May 19, 2026 14:32
… rollup, raw_values, trajectory_pointers, and corruption filtering from the base. Exposed _group_key / _filter_entry / _build_result as overridable hooks so project-specific behavior can layer on top without reimplementing core grouping.
@venkatkrish543re
venkatkrish543re marked this pull request as ready for review May 19, 2026 19:03
Comment thread src/strands_evals/aggregators/base.py Outdated
Comment thread src/strands_evals/aggregators/base.py Outdated
Comment thread src/strands_evals/aggregators/base.py Outdated
Comment thread src/strands_evals/aggregators/base.py Outdated
Comment thread src/strands_evals/aggregators/base.py Outdated
Comment thread src/strands_evals/aggregators/types.py Outdated
@github-actions

Copy link
Copy Markdown

Issue: The PR description links to issue #114 (Chaos/Resiliency Evaluation) which appears unrelated to aggregation functionality. This makes it harder to trace the motivation and requirements for this feature.

Suggestion: Please link to the correct issue that tracks the aggregator feature, or create one if it doesn't exist. This helps reviewers understand the acceptance criteria and future readers understand the context.

Comment thread src/strands_evals/aggregators/base.py Outdated
@github-actions

Copy link
Copy Markdown

Issue: This PR introduces a new public API (Aggregator, AggregationReport, AggregationResult) that customers will use directly. Per the API Bar Raising guidelines, the PR description should include:

  • Expected use cases for the feature
  • Example code snippets demonstrating usage
  • Complete API signatures with default parameter values
  • Module exports

The PR also lacks the needs-api-review label, which is required for new public classes/abstractions.

Suggestion:

  1. Add the needs-api-review label
  2. Update the PR description with a usage example:
from strands_evals.aggregators import Aggregator, AggregationReport

agg = Aggregator(name="my-experiment")
report: AggregationReport = agg.aggregate(evaluation_reports)
report.run_display()
report.to_file("results.json")
  1. Document the public API surface (what's exported, what's customizable)

@github-actions

Copy link
Copy Markdown

Review Summary

Assessment: Request Changes

The aggregator concept is well-designed with clean extension points (_group_key, _filter_entry, _build_result). The overall architecture follows the existing project patterns for report types and display. However, there are several issues that need to be addressed before merging.

Review Categories
  • Project structure: Tests are in the wrong directory (tests/aggregators/ vs tests/strands_evals/aggregators/), and the module isn't exported from the top-level __init__.py
  • API bar raising: This introduces a new public API surface but lacks the needs-api-review label and the PR description doesn't include usage examples or API signatures per the bar-raising process
  • Type safety: The model parameter uses Optional[Any] instead of the established Model | str | None pattern, and internal data flows through untyped dict objects making the extension points harder to use correctly
  • Style compliance: Logging calls don't follow the structured field format defined in STYLE_GUIDE.md
  • Linked issue: References [FEATURE] Chaos/Resiliency Evaluation of Agents #114 (chaos testing) which appears unrelated to aggregation

The extensibility design with the three hooks is thoughtful and the test coverage is solid for the current scope.

Comment thread src/strands_evals/aggregators/base.py Outdated
Comment thread src/strands_evals/aggregators/base.py Outdated
Comment thread src/strands_evals/types/evaluation_report.py Outdated
Comment thread src/strands_evals/aggregation/__init__.py Outdated
Comment thread src/strands_evals/aggregation/frame.py Outdated
Comment thread src/strands_evals/aggregation/coerce.py Outdated
_BASE_COLUMNS = [CASE, EVALUATOR, SCORE, PASSED, REASON]


def _case_key(case: dict, index: int) -> str:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Let's tuck these 2 private methods into the class where they're called.

@@ -0,0 +1,370 @@
"""Tests for the pandas-style surface on ``EvaluationReport``.

Consolidates what were previously test_coerce / test_frame / test_mixin, now

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Remove this legacy slop

"""Group rows by one or more columns. Mirrors ``DataFrame.groupby``."""
return ReportGroupBy(self._df.groupby(by, **kwargs))

def filter(

@ybdarrenwang ybdarrenwang Jun 5, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

All the helper methods — filter, describe, display, to_file/from_file, to_pandas, the __getattr__ passthrough, __len__, __repr__ — are nice to have that mirrors Pandas dataframe indeed. But these speculative API surfaces are not really consumed by EvaluationReport or any other class in the codebase. I worry that they only add dead weights to the codebase.

@@ -0,0 +1,370 @@
"""Tests for the pandas-style surface on ``EvaluationReport``.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Add these tests to test_evaluation_report.py instead of creating this new test file.

result = self._groupby.agg(*args, **kwargs)
return ReportFrame(result.reset_index())

def __getattr__(self, name: str) -> Any:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This method is not needed either. If a user genuinely needs raw pandas, they do report.frame().df.groupby(...)

"""
return self.frame().group_by(by, **kwargs)

def agg(self, *args: Any, **kwargs: Any) -> ReportFrame:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Ideally, .agg should still return a EvaluationReport, not a ReportFrame, so the users can directly

aggregated = report.group_by("evaluator").agg(
    mean_score=("score", "mean"),
    pass_rate=("passed", "mean"),
)
aggregated.display()  # ← same CollapsibleTableReportDisplay experience

and so you don't need to implement display for ReportFrame either.

The intention is all the pandas df should be hiding beneath EvaluationReport, while its interface remains the same except newly supported .group_by().agg().

Here's the shape (Delete ReportFrame entirely):

class EvaluationReport(BaseModel):

    def group_by(self, by: str | list[str]) -> "ReportGroupBy":
        """Group cases. Returns an intermediate that resolves back to EvaluationReport."""
        df = self._to_dataframe()  # private — hidden from users
        return ReportGroupBy(df.groupby(by))


class ReportGroupBy:
    """Internal. Users only see .agg() on this."""

    def __init__(self, groupby):
        self._groupby = groupby

    def agg(self, **kwargs) -> "EvaluationReport":
        """Aggregate and return a new EvaluationReport."""
        result_df = self._groupby.agg(**kwargs).reset_index()
        # Convert aggregated rows back into EvaluationReport's structure
        return _dataframe_to_report(result_df)

    def _dataframe_to_report(df: pd.DataFrame) -> EvaluationReport:
        """Convert an aggregated DataFrame back into an EvaluationReport."""
        cases = []
        scores = []
        test_passes = []
        reasons = []
    
        for _, row in df.iterrows():
            case = {"name": str(row.get("case", row.get("evaluator", "")))}
            # Stuff remaining columns into metadata
            case["metadata"] = {
                k: v for k, v in row.items()
                if k not in ("case", "evaluator", "score", "passed", "reason")
            }
            cases.append(case)
            scores.append(float(row.get("score", row.get("mean_score", 0.0))))
            test_passes.append(bool(row.get("passed", row.get("pass_rate", 0) >= 0.5)))
            reasons.append(str(row.get("reason", "")))
    
        overall = sum(scores) / len(scores) if scores else 0.0
        return EvaluationReport(
            evaluator_name="Aggregated",
            overall_score=overall,
            scores=scores,
            cases=cases,
            test_passes=test_passes,
            reasons=reasons,
        )

@venkatkrish543re

Copy link
Copy Markdown
Author

Closing per working group discussion. With #241 returning a single flattened report, grouping and aggregation are straightforward with pandas directly on the report, So a dedicated aggregator in the library is not needed

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants