feat(evaluator): add by-name aggregate lookup to AgentEvalSummary - #1198
Merged
Conversation
Reading one aggregate from a run meant a hand-written scan over a doubly-nested
attribute:
next(s for s in result.summary.scores.scores if s.name == "...").mean
Seven sites did this -- six in tests (one via a private `_score` helper) and one
in the user-facing Hermes example, which is precisely the audience an ergonomics
gap costs most.
Adds `score(name)` and a `scores_by_name` mapping view to AggregatedMetricResult,
so the dataset path (`EvaluationResult.aggregate_scores`) gains the same surface,
with one-hop delegates on AgentEvalSummary.
A miss raises KeyError rather than returning None: an unknown name is nearly
always a typo or a metric that did not run, and failing where the name is in hand
beats an AttributeError on `.mean` downstream. `scores_by_name.get()` covers
legitimate absence.
The miss message leads with close matches rather than enumerating everything --
a run with several metrics times pass@k carries dozens of names, and a wall of
them buries the answer. It reports how many others exist so a wrong suggestion is
not a dead end, and truncates the fallback listing.
`score` is a method and `scores_by_name` a property, so neither enters the JSON
schema; verified against AggregatedMetricResult, AgentEvalSummary, and
EvaluationResult. The committed OpenAPI spec is unchanged.
Left alone deliberately: harbor_runtime and the dashboard iterate rather than
look up, and plugin_examples filters on two candidate names -- converting those
would change behaviour, not just style.
Signed-off-by: Sandy Chapman <schapman@nvidia.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe SDK now provides named aggregate-score access through ChangesAggregate score accessors
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/values/results.py`:
- Around line 466-486: Update the available-name collection in the
aggregate-score miss diagnostics to build a set before sorting, ensuring each
aggregate name appears only once in suggestions and fallback enumeration. Add a
miss-case test covering duplicate aggregate names and verifying the diagnostic
contains no repeated names.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 6be49b3c-d3cc-4014-b6b8-9be74059f7ba
📒 Files selected for processing (8)
packages/nemo_evaluator_sdk/examples/hermes/example.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/results.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/values/results.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_evaluator.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_gym_aggregate_scores.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_hermes_example.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_pass_at_k.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_summary_accessors.py
Contributor
|
…he vendored SDK Two follow-ups on the by-name lookup. **Duplicate names counted twice.** `available` was built from a generator, so a repeated aggregate name was suggested twice, listed twice in the fallback enumeration, and counted twice in the "N other aggregates" tally -- making one collision read as two distinct near-misses. `scores_by_name` already collapsed duplicates first-wins; the diagnostic path did not, and the two disagreed. Deduplicated, with tests for both the suggestion and enumeration branches. Reported by CodeRabbit on #1198. **Vendored SDK out of sync.** `nemo_evaluator_sdk` is vendored into `sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/`, so any change to its source needs `make vendor` re-run and the result committed. Without it `lint-sdk-vendored` fails, and `lint-cli` fails after it because the vendor step leaves `sdk/python/` dirty and lint-cli diffs that same path. Verifying the JSON schema was unchanged was necessary but not sufficient: the vendor copies source, not schema. Signed-off-by: Sandy Chapman <schapman@nvidia.com>
…sage Review question on #1198: what counts as a close match, and would listing the names sorted be simpler? Documents the answer where the code is rather than in a review thread. "Close" is difflib.get_close_matches -- SequenceMatcher (Ratcliff/Obershelp) similarity of at least 0.6, best three first -- which is a subsequence-overlap ratio, not an edit distance. Also records why the sorted list is not enough on its own: it is the better answer left whole, but truncating one breaks it, because the name a caller meant is not reliably in the first N. A typo'd `view.solved` sits behind a page of `gym_reward.*` in a run carrying pass@1..8 for two metrics. _MISS_NAME_LIMIT now says 10 is a judgement call rather than implying a measured optimum, which is what the comment read like. No behaviour change; the vendored SDK copy is re-synced. Signed-off-by: Sandy Chapman <schapman@nvidia.com>
15 tasks
arpitsardhana
approved these changes
Aug 10, 2026
15 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Reading a single aggregate from an eval run required a hand-written scan over a doubly-nested attribute (
next(s for s in result.summary.scores.scores if s.name == "...")). This addsscore(name)and ascores_by_namemapping view so the same read is one call, and converts the seven sites that were scanning by hand.Linear: AALGO-445
Changes
AggregatedMetricResult.score(name)— returns the aggregate, raisesKeyErroron a miss. Placed here rather than only on the agent-eval summary so the dataset path (EvaluationResult.aggregate_scores) gains the same surface.AggregatedMetricResult.scores_by_name— mapping view forin,.get(), and iteration, for when a score's absence is a legitimate outcome. First-wins on a duplicate name, matching thenext(...)scans it replaces.AgentEvalSummary._scorehelper plus 4 call sites intest_evaluator.py, one each intest_gym_aggregate_scores.py,test_hermes_example.py, two intest_pass_at_k.py, and the user-facingexamples/hermes/example.py.Design notes
A miss raises instead of returning
None. An unknown name is nearly always a typo or a metric that did not run. Both are bugs worth surfacing at the lookup, where the name is in hand, rather than as anAttributeErroron.meanfurther downstream.scores_by_name.get()covers legitimate absence.The miss message leads with close matches. A run with several metrics times pass@k carries dozens of names, so enumerating them all buries the answer exactly when it is most needed. It reports how many others exist so a wrong suggestion is not a dead end, and truncates the fallback listing at 10.
Left alone deliberately.
harbor_runtime.pyanddashboard.pyiterate rather than look up by name;plugin_examples.pyfilters on two candidate names. Converting any of them would change behaviour, not just style.No schema impact.
scoreis a method andscores_by_namea property, so neither enters the JSON schema — verified againstAggregatedMetricResult,AgentEvalSummary, andEvaluationResult. The committed OpenAPI spec is unchanged.Type of Change
Quality Gates
Verification
Signed-off-by:traileruv run pre-commit run -apasses, or any blocked checks are identified belowTargeted validation:
pytest packages/nemo_evaluator_sdk/tests/agent_eval packages/nemo_evaluator_sdk/tests/valuespytest .../test_summary_accessors.pyruff check+ruff format --checkon changed filestools/lint/lint-python-types.shuv run pre-commit run -auv run pre-commit run -a— two failures, neither caused by this change:uv-lock— refuses to run because the local uv is 0.9.30 and the repo pins>=0.9.14for lockfile writes. This is a toolchain mismatch, not lock drift: the separateCheck for uv.lock drifthook passed, and this branch touches nopyproject.tomloruv.lock.studio-lint-staged—misehas nopnpmshim configured in this worktree. This branch touches no web files.Every other hook passed, including
ruff,ty, copyright headers, config-reference doc, and Helm docs. Flagging both rather than marking the gate green; happy to re-run under a corrected local toolchain if a reviewer wants the clean sweep.Summary by CodeRabbit
New Features
Tests