feat: add ToolEfficiencyEvaluator for session-level tool usage analysis - #362
feat: add ToolEfficiencyEvaluator for session-level tool usage analysis#362max-rattray-aws wants to merge 2 commits into
Conversation
Add a new LLM-judge evaluator that classifies each tool call in a trajectory as NECESSARY, REDUNDANT, ERRORED, or UNNECESSARY. The efficiency score is computed as necessary_count / total_count. The evaluator operates at SESSION_LEVEL, reading the full conversation trajectory and producing a structured breakdown of tool call efficiency. This complements ToolSelectionAccuracyEvaluator (per-call) and ToolParameterAccuracyEvaluator (per-call parameters) by providing a global view of tool usage waste. Key features: - Per-call classification with reasoning - Configurable max_tool_result_length for context window management - JSON-serialized breakdown in EvaluationOutput.label - Versioned prompt template following existing patterns
GitHub Actions
Inactive
GitHub Actions
Inactive
github-actions
Bot
added
enhancement
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| return [ | ||
| EvaluationOutput( | ||
| score=score, | ||
| test_pass=score >= 0.5, |
There was a problem hiding this comment.
Issue (Suggestion): The pass threshold 0.5 is hardcoded. For an efficiency ratio, "half the calls were necessary = pass" is a fairly arbitrary cutoff, and callers may reasonably want a stricter bar.
Suggestion: Consider exposing it as a constructor argument (e.g. pass_threshold: float = 0.5) so the behavior is configurable and self-documenting. If it's intentionally fixed, a brief comment explaining the rationale would help.
There was a problem hiding this comment.
Resolved — pass_threshold is now a constructor argument (defaulting to 0.5), and test_evaluate_low_efficiency exercises a custom 0.8 threshold. Thanks!
| result = evaluator_agent(prompt, structured_output_model=ToolEfficiencyRating) | ||
| rating = cast(ToolEfficiencyRating, result.structured_output) | ||
|
|
||
| score = rating.necessary_count / rating.total_count if rating.total_count > 0 else 1.0 |
There was a problem hiding this comment.
Issue (Important): The score uses the LLM-provided rating.necessary_count / rating.total_count rather than deriving them from the classifications list. Since both counts and the per-call breakdown are produced independently by the model, they can silently disagree — the score and the label (classifications JSON) may tell different stories. There's also nothing preventing necessary_count > total_count, which would yield a score > 1.0. (Notably, test_evaluate_low_efficiency encodes exactly this drift: 3 classifications but total_count=5.)
Suggestion: Derive the counts deterministically from classifications so the score always matches the breakdown, e.g.:
total = len(rating.classifications)
necessary = sum(1 for c in rating.classifications if c.category == ToolCallCategory.NECESSARY)
score = necessary / total if total > 0 else 1.0This keeps necessary_count/total_count as the model's summary while making the score authoritative and bounded to [0, 1].
There was a problem hiding this comment.
Confirmed resolved in the latest push — the score is now derived directly from classifications (necessary_count = sum(... == NECESSARY), total_count = len(classifications)), and the redundant necessary_count/total_count fields were dropped from ToolEfficiencyRating entirely. Nice — that removes the drift risk and the score > 1.0 case, and the new test_evaluate_score_derived_from_classifications locks in the behavior.
One tiny follow-up: the class docstring still says "Score is calculated as necessary_count / total_count", which now refers to values that aren't model fields anymore. Consider rewording to reference the classification counts to keep it in sync.
| assert len(result) == 1 | ||
| assert result[0].score == 1.0 | ||
| assert result[0].test_pass is True | ||
| assert result[0].reason == "All tool calls contributed to the final response." |
There was a problem hiding this comment.
Issue (Suggestion): These evaluate tests assert on individual EvaluationOutput fields (score, test_pass, reason) separately. Per-field assertions silently miss unexpected or regressed fields on the returned object.
Suggestion: Where the output is fully deterministic, assert the whole object in one equality check, e.g. assert result == [EvaluationOutput(score=1.0, test_pass=True, reason=..., label=...)]. This catches regressions in any field, including label.
There was a problem hiding this comment.
Resolved — the evaluate tests now assert the full EvaluationOutput in a single equality check (including label), which will catch regressions in any field. Thanks for tightening these up.
|
Assessment: Comment Clean, well-tested addition that follows the existing evaluator conventions (versioned prompt template, Review themes
Nice complement to the existing tool-selection/parameter evaluators — the four-category framing is intuitive. |
GitHub Actions
Inactive
GitHub Actions
Inactive
|
Following up on my earlier review — one item from the summary is still outstanding (I couldn't attach it inline since Issue (Important): The new Suggestion: Add a Everything else from the review looks great — score is now derived from |
1 participant
Summary
Adds a
ToolEfficiencyEvaluatorthat operates atSESSION_LEVELand classifies each tool call in a trajectory as NECESSARY, REDUNDANT, ERRORED, or UNNECESSARY. The efficiency score isnecessary_count / total_count(1.0 if no tool calls were made).This complements
ToolSelectionAccuracyEvaluator(was calling a tool justified at this point?) andToolParameterAccuracyEvaluator(were the parameters correct?) by answering a different question: given the whole trajectory, were all these calls needed?Usage
The evaluator returns an
EvaluationOutputwith:score: efficiency ratio (0.0 to 1.0)test_pass: True if score >= 0.5reason: overall assessment from the judgelabel: JSON string with per-call classifications for programmatic analysisWhat's tested
21 unit tests, all passing.
Related to #345