feat(evaluator): expose per-task attempts with trial identity - #1224
Open
ngoncharenko wants to merge 4 commits into
Open
feat(evaluator): expose per-task attempts with trial identity#1224ngoncharenko wants to merge 4 commits into
ngoncharenko wants to merge 4 commits into
Conversation
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 (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughChangesAgent evaluation summaries now retain ordered, trial-linked metric attempts. Failed trials use Task metric aggregation
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Evaluation
participant AgentEvalSummary
participant summary_json
participant GymInspector
Evaluation->>AgentEvalSummary: build task_metric_attempts
AgentEvalSummary->>AgentEvalSummary: aggregate pass@k values
AgentEvalSummary->>summary_json: persist trial-linked attempts
GymInspector->>summary_json: load summary
summary_json-->>GymInspector: return attempts and aggregates
GymInspector->>GymInspector: classify measured and unmeasured tasks
🚥 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 591-604: Update the output-schema tracking around output_keys and
excluded so exclusions are keyed by task.id as well as (metric_type, spec.name).
Apply exclusion checks only within the current task when filtering persisted
values and pass@k inputs, preserving retained outputs for other tasks; add
coverage where one task retains a model output and another excludes the same key
as a continuous score.
🪄 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: a940a624-881c-44d9-bf66-e89732790587
⛔ Files ignored due to path filters (1)
sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/agent_eval/results.pyis excluded by!sdk/**
📒 Files selected for processing (6)
packages/nemo_evaluator_sdk/examples/gym/README.mdpackages/nemo_evaluator_sdk/examples/gym/inspect_results.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/results.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_pass_at_k.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_persistence.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_task_metric_values.py
Contributor
|
ngoncharenko
changed the title
ngoncharenko
force-pushed
the
ngoncharenko/aalgo-310-passatk-harbor-runner
branch
from
August 11, 2026 18:51
b8d501a to
3d85361
Compare
Contributor
Contributor
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/nemo_evaluator_sdk/examples/gym/inspect_results.py (1)
137-164: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocstring omits the new
unmeasuredverdict.The function now emits a fourth classification and counter. Add it to the docstring so the four printed states are documented.
📝 Proposed edit
``None`` is a trial that died: it counts as an attempt and never as a pass, so a task that passed once and crashed once reads as flaky rather than solved. + + An empty attempt list is ``unmeasured``: the task produced no usable measurement, either because + its metric failed or because no trial ran. """🤖 Prompt for 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. In `@packages/nemo_evaluator_sdk/examples/gym/inspect_results.py` around lines 137 - 164, Update the show_per_task docstring to document all four emitted verdicts: solved, flaky, failed, and unmeasured, including that unmeasured applies when a task has no recorded attempts. Keep the existing attempt and pass@k behavior description unchanged.
🤖 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.
Nitpick comments:
In `@packages/nemo_evaluator_sdk/examples/gym/inspect_results.py`:
- Around line 137-164: Update the show_per_task docstring to document all four
emitted verdicts: solved, flaky, failed, and unmeasured, including that
unmeasured applies when a task has no recorded attempts. Keep the existing
attempt and pass@k behavior description unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a2eb5db6-98da-4d10-8bf5-c6d10e2e8fb1
⛔ Files ignored due to path filters (1)
sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/agent_eval/results.pyis excluded by!sdk/**
📒 Files selected for processing (8)
docs/evaluator/agent-eval/reading-results.mdxpackages/nemo_evaluator_sdk/examples/gym/README.mdpackages/nemo_evaluator_sdk/examples/gym/inspect_results.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/results.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_harbor_runtime.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_pass_at_k.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_persistence.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_task_metric_attempts.py
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/nemo_evaluator_sdk/tests/agent_eval/test_pass_at_k.py
- packages/nemo_evaluator_sdk/examples/gym/README.md
ngoncharenko
force-pushed
the
ngoncharenko/aalgo-310-passatk-harbor-runner
branch
from
August 11, 2026 19:18
378ba22 to
8193be9
Compare
Contributor
Author
|
@coderabbitai continue the review |
Contributor
|
|
| "'<metric_type>.<output>', plus per-semantic-view rollups named 'view.<name>'. " | ||
| "Failed or missing scores are surfaced as nan_count." | ||
| ), | ||
| examples=[ |
Contributor
There was a problem hiding this comment.
nit: Will these examples show up in openapi.yaml? In service mode, i am wondering if extensive example can blow up the size of openapi spec
Contributor
Author
There was a problem hiding this comment.
- On the service side, this is stored in summary.json and not exposed in the API spec
- Added this on purpose because the results is not a straightforward data structure and having illustrative examples should help both agents and humans.
| ), | ||
| examples=[ | ||
| { | ||
| "contract-review-msa-indemnity": { |
Contributor
There was a problem hiding this comment.
So this essentially builds the reverse index of results ie task : {runner: [{trials: (name, score)}]}
Contributor
Author
There was a problem hiding this comment.
yes, to match harbor format
Add `AgentEvalSummary.task_metric_values`: the ordered per-attempt values for each task, keyed `<metric_type>.<output>`, persisted into `summary.json`. Answering "which tasks were flaky, and on which attempt?" previously meant regrouping the flat task x trial x metric score list by hand. Rebuild pass@k on top of that mapping instead of rescanning the scores, so the per-attempt view and the published pass@k figures cannot disagree. pass@k means are unchanged; a task that produced no trial at all now surfaces in `nan_count` rather than silently shrinking the denominator. Retention follows the declared output schema: continuous, discrete and boolean values are kept, while labels and free models (token measurements) stay out even when their emitted value happens to be numeric. Rework the gym `inspect_results.py` example to read the summary directly rather than re-deriving per-task outcomes from `scores.jsonl`. Signed-off-by: Nick Goncharenko <ngoncharenko@nvidia.com>
Signed-off-by: Nick Goncharenko <ngoncharenko@nvidia.com>
Signed-off-by: Nick Goncharenko <ngoncharenko@nvidia.com>
Signed-off-by: Nick Goncharenko <ngoncharenko@nvidia.com>
ngoncharenko
force-pushed
the
ngoncharenko/aalgo-310-passatk-harbor-runner
branch
from
August 12, 2026 03:55
328f40d to
3eebc8b
Compare
| *, | ||
| metric_type: str, | ||
| output_name: str, | ||
| ) -> dict[str, list[float | None]]: |
Contributor
There was a problem hiding this comment.
I wonder if we should provide a stronger type here. Instead of:
{
"task-47": { "harbor_reward.reward": [
{ "trial_id": "task-47__7f3a9c", "value": 1.0 },
{ "trial_id": "task-47__1b8e42", "value": null }
]}
}
maybe use a model like:
class TrialOutcome:
trial_id: str
value: float | int | bool | None
class PerTaskOutcome(BaseModel):
metric_name: str
trials: list[TrialOutcome]
class PerTaskOutcomes(BaseModel):
task_id: str
outcomes: list[PerTaskOutcome]
We actually have the type AgentEvalAttemptValue which seems to match this TrialOutcome.
Comment on lines
+114
to
+118
| if "task_metric_attempts" not in payload: | ||
| raise SystemExit( | ||
| f"{bundle / 'summary.json'} predates summary.task_metric_attempts, which this script reads " | ||
| "per-task outcomes from. Re-run the eval to produce a current bundle." | ||
| ) |
Contributor
There was a problem hiding this comment.
SystemExit seems extreme for the exception, right? Maybe a custom exception would be appropriate here. Also, I assume the AgentEvalSummary.model_validate would have failed anyway? Why couldn't we just handle the model_validate failure instead?
| "(trials.jsonl) and AgentEvalTaskScore.trial_id (scores.jsonl)." | ||
| ) | ||
| ) | ||
| value: float | None = Field( |
Contributor
There was a problem hiding this comment.
Are there more values than float | None? I think this would correspond to int | bool too, right? What about str? I know our native metrics can output labels too.
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
3 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
AgentEvalSummary.task_metric_attempts— the ordered attempts per task and metric output — and persists it tosummary.json.trials.jsonl:Important notes
value: null= the trial died before scoring — an attempt that did not pass. A failed metric leaves no entry at all: unmeasured, not unsuccessful.Related Issue
reward_stats) and AALGO-428 (P3.2, exception rollup).How this change moves us closer to Harbor parity
Target shape:
AgentDatasetStats— Harbor's per-dataset aggregate, the thingsummary.jsonhas to be able to reproduce. Per-trial source isVerifierResult.rewards; it is populated byJobStats.incrementasreward_stats.setdefault(value, []).append(trial_result.trial_name).pass_at_ksummary.score("<metric>.<output>.pass@k").mean— pre-existingreward_statstask_metric_attempts— trial ids + raw floatsn_trialssummary.trial_countn_errorstrial_idwherevalue is Noneexception_statstrial_name; we grouped bytask_id.harbor_runtimealready stampstrial_nameontoAgentEvalTrial.id, so carryingtrial_idon each attempt makes the shapes the same, not merely similar:trial_name, nottask_id) and inner key type (rawfloat | int, not stringified). Pinned bytest_harbor_reward_stats_is_derivable_from_summary_task_metric_attempts.n_errorsneeds the trial id specifically — one dead trial contributes anullto every metric key, so a naive count over-counts by the number of metrics. Deduping requires identity.pass_at_kmatches in content but not shape (Harbor keys{k: float}, we emit named aggregates), andn_errorsis derivable but not implemented.Why attempts carry an id but no ordinal
Worth stating, since "shouldn't each attempt have an index?" is the obvious review question:
n_attemptsby discarding the loop index —job.py:416:for _ in range(self.config.n_attempts)— and runs the repeats concurrently (job.py:129). Each repeat gets a fresh randomtrial_name. There is no first attempt._pass_at_k(n, c, k)is a function of counts and never of position. An ordinal would imply an ordering the runs do not have.sorted(glob("*/result.json"))over a random suffix, i.e. arbitrary;trial_idis the only meaningful handle, which is why it is carried rather than parsed. For contrast, the experimentalist's_trial_attemptrecovers the ordinal by parsing the trial-name suffix, whichisdigit()-fails against Harbor's ShortUUID — so it returnsNoneon every real Harbor run (verified against a liven_attempts=2job:hello-world__4c3VrKY,hello-world__NXRG3pE).JobStats.n_retries) and its per-trialTrialResultcarries no retry field, so "did the retry do better?" is a gap in Harbor's model — not something the evaluator can synthesize. Flagging it in case it matters later.Follow-ups
reward_payload_from_resultstill walksresult.scoresand emits the legacy task-keyed, stringified shape; rewiring it onto the summary is now mechanical.exception_statsis the last gap. Withtrial_idpresent it's a join againsttrials.jsonl(already in the bundle), not a schema change. Recommend it first add a typederrortoAgentEvalTrial, replacing the untypedmetadata["exception_type"]convention.What to review
_task_metric_attemptsinagent_eval/results.py— the core. Docstring carries anin → outworked example; the two bullet lists map 1:1 onto the branches below them.value: null(counted inn); a failed metric is no entry (kept out ofn, so a judge timeout is never charged to the agent). Most likely thing to get wrong later.trial_id—persistence.pywrites withsort_keys=True(would reorder attempts lexicographically), and nothing enforces trial-id uniqueness, so a dict would collapse two attempts into one and silently drop pass@k'sn. Pinned bytest_duplicate_trial_ids_are_two_attempts_not_one.AgentEvalAttemptValueis frozen, andvaluehas no default — both guard the same class of silent corruption:persistwould save the rewrite.value:Nonemeans "the trial died" and pass@k counts it towardn, so a record that merely omits the key must not quietly become a failed attempt. Explicit"value": nullstill works.nan_countsemantics —test_a_task_that_produced_no_trial_is_unmeasured_and_counted_in_pass_at_k_nan. Previously such a task was absent from the denominator; now reported as missing coverage. Means unaffected. Note this is afrom_scorespath only — a full run still fails loudly on a trial-less task, and this PR deliberately does not relax that guard.trial.metadata["exception_type"]that only Harbor stamps, andexception_typealready means two opposite things in this codebase. Withtrial_idpresent it becomes a join againsttrials.jsonl.Changes
task_metric_attempts: dict[task_id, dict["<metric_type>.<output>", list[AgentEvalAttemptValue]]], whereAgentEvalAttemptValue = {trial_id, value}.values_by_task = [attempt_values(outputs[key]) ...]. Everything below it (measured,unmeasured,max_n,_pass_at_k) is byte-identical.tasks=Nonethere are no specs to filter on, so every numeric output observed is kept.pass@knan_countinstead of silently shrinking the denominator. Reachable only viaAgentEvalSummary.from_scorescalled directly with a task list wider than the scores; a full run cannot get here, because_score_trialsrefuses to score when a task produced no trial (test_run_rejects_tasks_without_trials).trial_idis Harbor'strial_name(_trial_from_harbor_resultstamps it), so Harbor's ownreward_statsis rebuildable from the summary alone. See the parity section above.NaNtoken and madesummary.jsonunparseable by strict readers. Serialized as"NaN", matchingMetricOutput; round-trips back to a float.(task_id, metric_type)once, replacing a per-task/per-key rescan.inspect_results.pyreads the summary directly;per_task_outcomeskeeps its bare-value shape, newper_task_attemptsexposes the records. It now rejects a bundle predating the field rather than loading it and silently showing no per-task section.make vendor, pinned byte-exact by a test.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(live/docker deselected)pytest packages/nemo_evaluator_sdk/tests/agent_eval(live/docker deselected)pytest .../test_task_metric_attempts.pypytest plugins/nemo-evaluator/tests plugins/nemo-optimization/testsuv run ruff check packages/nemo_evaluator_sdkuv run ruff format --check packages/nemo_evaluator_sdkuv run --frozen ty check <changed files>make vendorpersist_run→inspect_resultssummary.json; per-task output unchangedpass@k invariance — a golden
{name: (mean, count, nan_count)}table was captured from the pre-change implementation before the producer was touched, over a fixture covering every branch (always-passes, dead trial, metric-raised, two never-measured). Post-change it compares identical, and is pinned bytest_pass_at_k_aggregates_are_unchanged_by_carrying_trial_ids.Not passing / not run:
uv-lockpre-commit hook: needs uv0.9.14to match CI; local toolchain is0.9.30. Environment mismatch, not from this PR — nopyproject.tomlis touched, anduv-lock-checkpasses.test_harbor_runtime_e2e.py,test_codex_runtime_live.py,test_sandbox_docker_provider_live.py,test_sandbox_compose_provider_live.pyneed Docker and provider credentials.Summary by CodeRabbit
New Features
Bug Fixes
Documentation