feat(evaluator): give AgentEvalResult the dataset path's display surface - #1201
Merged
SandyChapman merged 3 commits intoAug 10, 2026
Merged
Conversation
`EvaluationResult` ships format_summary/print_summary/to_records/to_table/ to_pandas/__str__. `AgentEvalResult` had none of them, so anyone inspecting an agent-eval run wrote their own formatting -- two result types in one SDK with unrelated ergonomics. Adds the same six methods, with the same signatures and the same `view` values. ## No mixin The ticket proposed extracting a shared mixin. `BenchmarkEvaluationResult` (values/multi_metric_results.py) already faced this and answered it differently: import the module-level helpers, implement the methods directly. The genuinely shared logic -- format_table, summary_aggregate_record, serialize_value, flatten_dict -- is already free functions, so a mixin would share method *names* while every body still needed overriding, and would add a third pattern to unify two that already agree. This follows the established one instead. ## What a row is here A record per metric score. The fan-out is preserved rather than collapsed: task_id and trial_id are columns, so a consumer can still group by task, which is what pass@k depends on. The aggregate view is byte-for-byte the dataset path's -- percentiles flattened, histograms as JSON strings. The error section is agent-eval's own, because it has a distinction the dataset path cannot make: a failed *trial* is an attempt the agent is answerable for, a failed *metric* is a measurement that never happened. Both arrive as FAILED. ## to_table column union (deliberate divergence) `pa.Table.from_pylist` takes its schema from the first record alone. Here `error` and `diagnostics.*` appear only on failed scores, so a run whose first score passed exported a table missing exactly the columns you needed, while to_pandas -- which unions keys -- included them. Verified: 5 columns vs 7. Columns are now unioned before the table is built. Both siblings have the same latent behaviour; matching a known-lossy export seemed worse than diverging and saying so. Worth a follow-up for EvaluationResult and BenchmarkEvaluationResult. 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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughChangesAgent result presentation
Sequence Diagram(s)sequenceDiagram
participant AgentEvalResult
participant PyArrow
participant pandas
AgentEvalResult->>AgentEvalResult: Build row or aggregate records
AgentEvalResult->>PyArrow: Create table from records
AgentEvalResult->>pandas: Create dataframe from records
Possibly related PRs
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/agent_eval/results.py`:
- Around line 352-371: Update _score_record to retain AgentEvalTaskScore.id,
run_id, and metadata in every row export, serializing or flattening metadata
consistently with the existing export conventions. Ensure _score_preview_record
and downstream table/DataFrame exports preserve these fields as applicable, and
add coverage verifying their presence and values.
🪄 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: ff1e6c2d-09d1-4a59-963c-378d8c5cd9a8
📒 Files selected for processing (2)
packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/results.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_result_display.py
Contributor
|
`nemo_evaluator_sdk` is vendored into `sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/`, so a 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. Signed-off-by: Sandy Chapman <schapman@nvidia.com>
arpitsardhana
approved these changes
Aug 10, 2026
arpitsardhana
left a comment
arpitsardhana
left a comment
Contributor
There was a problem hiding this comment.
minor nits comments
…ports Review on #1201: `_score_record` dropped `AgentEvalTaskScore.id`, `run_id`, and `metadata`, so every row, table, and DataFrame export lost them. An export is the thing a caller joins, concatenates, and keeps. `id` is what a row is addressable by, `run_id` keeps a frame self-describing once several runs are stacked into one, and `metadata` is caller-supplied -- dropping it silently discarded data the SDK never owned. Metadata is flattened into dotted columns rather than JSON-encoded: it is free-form but usually shallow and scalar, so flattening keeps it queryable. Diagnostics keep the JSON treatment, because their shape is metric-defined and variable-width. The summary preview deliberately stays narrow -- it is read on a terminal -- which is the same split the dataset path makes between `to_records` and `summary_row_base_record`. Covered by a test so the two don't drift together. Also documents why the error-row limit is clamped with max(0, ...): slicing would read a negative limit as an offset from the end, so `failed[:-2]` would show all but the last two rather than none. An over-large limit needs no guard. Vendored SDK re-synced. Signed-off-by: Sandy Chapman <schapman@nvidia.com>
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
EvaluationResultshipsformat_summary(),print_summary(),to_records(),to_table(),to_pandas(), and__str__.AgentEvalResulthad none of them, so anyone inspecting an agent-eval run wrote their own formatting — two result types in one SDK with unrelated ergonomics. This adds the same six methods, with the same signatures and the sameviewvalues.Linear: AALGO-447
Changes
to_records(view="rows"|"aggregate")onAgentEvalResult, plusto_table,to_pandas,format_summary,print_summary,__str__Design: no mixin
The ticket proposed extracting a shared mixin.
BenchmarkEvaluationResult(values/multi_metric_results.py) already faced this and answered it differently — import the module-level helpers, implement the methods directly. The genuinely shared logic (format_table,summary_aggregate_record,serialize_value,flatten_dict) is already free functions, so a mixin would share method names while every body still needed overriding, and would add a third pattern to unify two that already agree. This follows the established one.Reversible, but reversing it means refactoring all three result types rather than just this one.
What a "row" is here
One record per metric score. The fan-out is preserved rather than collapsed:
task_idandtrial_idare columns, so a consumer can still group by task — which is what pass@k depends on. The aggregate view matches the dataset path exactly (percentiles flattened, histograms as JSON strings).The error section is agent-eval's own, because it has a distinction the dataset path cannot make: a failed trial is an attempt the agent is answerable for; a failed metric is a measurement that never happened. Both arrive as
FAILED.One deliberate divergence —
to_tablecolumn unionpa.Table.from_pylisttakes its schema from the first record alone. Hereerroranddiagnostics.*appear only on failed scores, so a run whose first score passed exported a table missing exactly the columns you needed — whileto_pandas, which unions keys, included them. Verified: 5 columns vs 7 on the same run.Columns are now unioned before the table is built. Both siblings have the same latent behaviour. Matching a known-lossy export seemed worse than diverging and saying so — but that makes this the odd one out until
EvaluationResultandBenchmarkEvaluationResultget the same fix. Happy to file that follow-up, or to drop this and match the siblings if reviewers would rather keep the three identical.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/testspytest .../test_result_display.pyruff check+ruff format --checkon changed filestools/lint/lint-python-types.shuv run pre-commit run -aThree adversarial review rounds were run before this was opened. Round 1 surfaced three tests that passed against a broken implementation — most notably, the failed-trial/failed-metric assertion still passed when the two labels were swapped — plus an unreachable
if countguard, now removed. Round 2 left one survivor (max_error_rowsdefaulting tomax_rowswas documented but untested). Round 3: 14/14 caught, zero survivors.uv 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 while the repo pins>=0.9.14for lockfile writes. 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 in this worktree. This branch touches no web files.Every other hook passed, including
ruff,ruff format,ty, copyright headers, config-reference doc, Helm docs, and the plugin import-boundary check.Summary by CodeRabbit
New Features
Bug Fixes