refactor(evaluator)!: make persisting an agent-eval run an explicit call - #1069
Open
SandyChapman wants to merge 1 commit into
Open
refactor(evaluator)!: make persisting an agent-eval run an explicit call#1069SandyChapman wants to merge 1 commit into
SandyChapman wants to merge 1 commit into
Conversation
`AgentEvaluator.run()` wrote a bundle and an HTML dashboard as a side effect
whenever the run config carried an `output_dir`. Computing an evaluation and
storing one are different decisions, and folding them together forced
`AgentEvalResult` to declare two fields it could not populate:
output_dir: Path | None = None
dashboard_path: Path | None = None
Both were patched on after construction — `persist_run` returned a `model_copy`
with `output_dir` set, and the dashboard writer patched `dashboard_path` after
that. A completed run was therefore indistinguishable from one still being
assembled, and the type permitted a `dashboard_path` with no `output_dir`, a
state no code produces.
- `AgentEvalResult.persist(output_dir=None, *, write_dashboard=True)` stores the
run and returns a `BundleLocation` (`output_dir` plus an optional
`dashboard_path`). Holding one means the bundle exists, so there is no
optional to re-check; a run that was never persisted simply has no location.
- `run()` computes and returns; it writes nothing. This matches
`publish_to_intake`, which is already explicit for the same reason.
- `AgentEvalRunConfig.output_dir` becomes `work_dir`, and the result carries it.
The name was already inaccurate: seven runtimes use it as the root for trial
evidence *during* the run, not as an output. Unlike a bundle location it is
known before the run starts, so it is never attached after the fact.
- `write_dashboard` moves off the run config to `persist`, where it belongs.
`persist()` defaults to `work_dir` because that is where the trials' evidence
already lives, so `persist_run` can rewrite the evidence refs bundle-relative
and the bundle survives being moved. Persisting somewhere else leaves those refs
pointing at the original directory — supported (a re-scored run may reference an
earlier run's deliverables) but only resolvable while that directory exists.
BREAKING CHANGE: `AgentEvalRunConfig.output_dir` is renamed to `work_dir` and no
longer causes `run()` to persist; call `result.persist()` instead.
`AgentEvalRunConfig.write_dashboard` is removed — pass `write_dashboard` to
`persist()`. `AgentEvalResult.output_dir` and `.dashboard_path` are gone; read
them from the `BundleLocation` that `persist()` returns.
Signed-off-by: Sandy Chapman <schapman@nvidia.com>
Contributor
📝 WalkthroughWalkthroughThe SDK replaces ChangesEvaluation persistence migration
Sequence Diagram(s)sequenceDiagram
participant EvaluationCaller
participant AgentEvaluator
participant AgentEvalResult
participant persist_run
EvaluationCaller->>AgentEvaluator: run(config)
AgentEvaluator-->>EvaluationCaller: AgentEvalResult(work_dir)
EvaluationCaller->>AgentEvalResult: persist(write_dashboard)
AgentEvalResult->>persist_run: persist bundle
persist_run-->>AgentEvalResult: BundleLocation
AgentEvalResult-->>EvaluationCaller: output_dir and dashboard_path
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: 2
🧹 Nitpick comments (1)
packages/nemo_evaluator_sdk/tests/agent_eval/test_persistence.py (1)
149-156: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover explicit persistence for in-memory results.
The test covers only the failure path. Add a success case that calls
result.persist(tmp_path)whenwork_diris unset and asserts the returnedBundleLocation.output_dir.Based on the persistence contract, an in-memory result must accept an explicit persistence target.
🤖 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/tests/agent_eval/test_persistence.py` around lines 149 - 156, Add a success case to the test after the existing failure assertion that calls result.persist(tmp_path) with an explicit persistence target and asserts the returned BundleLocation.output_dir equals tmp_path. This covers the contract that in-memory AgentEvalResult instances accept explicit persistence targets even when work_dir is unset, complementing the existing failure-path coverage.
🤖 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/runtimes/codex/runtime.py`:
- Line 264: Update the evidence-root selection in Codex runtime.py at lines
264-264 and Fabric runtime.py at lines 514-514 to prefer config.work_dir over
self._work_root when both are set, ensuring evidence remains inside
AgentEvalResult.work_dir for persistence and rescoring. Add a regression test
covering both paths being configured and persisting the result.
In `@plugins/nemo-evaluator/src/nemo_evaluator/intake/publish.py`:
- Line 197: Update the location message in the publish flow to use the
BundleLocation returned by persist() as the persisted bundle path; do not
present result.work_dir as the bundle location. Describe work_dir only as trial
evidence, and explicitly indicate when no bundle was persisted for in-memory
results.
---
Nitpick comments:
In `@packages/nemo_evaluator_sdk/tests/agent_eval/test_persistence.py`:
- Around line 149-156: Add a success case to the test after the existing failure
assertion that calls result.persist(tmp_path) with an explicit persistence
target and asserts the returned BundleLocation.output_dir equals tmp_path. This
covers the contract that in-memory AgentEvalResult instances accept explicit
persistence targets even when work_dir is unset, complementing the existing
failure-path coverage.
🪄 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: 69bc9009-35bc-47c8-8990-7cde3de192b0
⛔ Files ignored due to path filters (10)
sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/agent_eval/evaluator.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/agent_eval/persistence.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/agent_eval/results.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/agent_eval/runtimes/codex/runtime.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/agent_eval/runtimes/docker_sandbox.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/agent_eval/runtimes/fabric/container_runtime.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/agent_eval/runtimes/fabric/runtime.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/agent_eval/runtimes/gym_runtime.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/agent_eval/runtimes/harbor_runtime.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/agent_eval/tasks.pyis excluded by!sdk/**
📒 Files selected for processing (29)
packages/nemo_evaluator_sdk/examples/agentic_eval_with_fabric.ipynbpackages/nemo_evaluator_sdk/examples/codex_docker/example.pypackages/nemo_evaluator_sdk/examples/fabric_container/run_e2e.pypackages/nemo_evaluator_sdk/examples/gym/run_gym_eval.pypackages/nemo_evaluator_sdk/examples/profbench/runner.pypackages/nemo_evaluator_sdk/examples/run_agent_eval/pipeline.pypackages/nemo_evaluator_sdk/examples/run_agent_eval/platform_runtime.pypackages/nemo_evaluator_sdk/examples/run_agent_eval/run_agent_eval.pypackages/nemo_evaluator_sdk/examples/run_agent_eval/workflow_runtime.pypackages/nemo_evaluator_sdk/examples/skill_eval/run_skill_eval.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/evaluator.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/persistence.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/results.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/codex/runtime.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/docker_sandbox.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/container_runtime.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/runtime.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/gym_runtime.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/harbor_runtime.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/tasks.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_codex_docker_example.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_docker_sandbox_runtime.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_evaluator.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_fabric_container_runtime.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_fabric_integration.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_harbor_runtime.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_persistence.pyplugins/nemo-evaluator/src/nemo_evaluator/intake/publish.pyplugins/nemo-evaluator/src/nemo_evaluator/jobs/agent_evaluate.py
| root = self._work_root | ||
| if root is None: | ||
| root = (config.output_dir or Path.cwd()) / "evidence" / "codex" | ||
| root = (config.work_dir or Path.cwd()) / "evidence" / "codex" |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Make work_dir take precedence over work_root.
When both values are set, these runtimes write evidence outside AgentEvalResult.work_dir. persist() then writes the default bundle to work_dir and preserves external evidence refs. A moved bundle cannot re-score its evidence.
packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/codex/runtime.py#L264-L264: selectconfig.work_dirbeforeself._work_root.packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/runtime.py#L514-L514: selectconfig.work_dirbeforeself._work_root.
Add a regression test that sets both paths and persists the result.
📍 Affects 2 files
packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/codex/runtime.py#L264-L264(this comment)packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/runtime.py#L514-L514
🤖 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/src/nemo_evaluator_sdk/agent_eval/runtimes/codex/runtime.py`
at line 264, Update the evidence-root selection in Codex runtime.py at lines
264-264 and Fabric runtime.py at lines 514-514 to prefer config.work_dir over
self._work_root when both are set, ensuring evidence remains inside
AgentEvalResult.work_dir for persistence and rescoring. Add a regression test
covering both paths being configured and persisting the result.
| ) -> str: | ||
| """Build an actionable error: what failed, where the results are cached, how to recover.""" | ||
| location = f"cached locally at {result.output_dir}" if result.output_dir is not None else "in the local run bundle" | ||
| location = f"cached locally at {result.work_dir}" if result.work_dir is not None else "in the local run bundle" |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Report the persisted bundle location accurately.
result.work_dir is the trial-evidence directory, not always the bundle directory. persist(output_dir=...) can write the bundle elsewhere, and an in-memory result can have no local bundle. This message can direct recovery to the wrong path.
Use the returned BundleLocation, or describe this path as trial evidence and state when no bundle was persisted.
Based on the persistence contract, work_dir stores trial evidence and persist() returns the bundle location.
🤖 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 `@plugins/nemo-evaluator/src/nemo_evaluator/intake/publish.py` at line 197,
Update the location message in the publish flow to use the BundleLocation
returned by persist() as the persisted bundle path; do not present
result.work_dir as the bundle location. Describe work_dir only as trial
evidence, and explicitly indicate when no bundle was persisted for in-memory
results.
Contributor
|
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
1 participant
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.
Off latest
main, independent of #1013 / #1065.Why
AgentEvaluator.run()wrote a bundle and an HTML dashboard as a side effect whenever the run config carried anoutput_dir. Computing an evaluation and storing one are different decisions, and folding them together forcedAgentEvalResultto declare two fields it could not populate:Both were patched on after construction —
persist_runreturned amodel_copywithoutput_dirset, and the dashboard writer patcheddashboard_pathafter that. So a completed run was indistinguishable from one still being assembled, and the type permitted adashboard_pathwith nooutput_dir— a state no code produces.After
AgentEvalResult.persist(output_dir=None, *, write_dashboard=True)returns aBundleLocation(output_dir+ optionaldashboard_path). Holding one means the bundle exists — no optional to re-check, and a run that was never persisted simply has no location.run()writes nothing. Same reasoningpublish_to_intakealready documents: "optionality is structural: you make the call or you don't."output_dir→work_diron the run config, and the result carries it. The old name was already inaccurate: seven runtimes use it as the root for trial evidence during the run. Unlike a bundle location it is known before the run starts, so it is never attached after the fact.write_dashboardmoves off the run config ontopersist, where it belongs.The one subtlety worth reviewing
persist()defaults towork_dir, and that default is load-bearing. Evidence is written underwork_dirduring the run, andpersist_runrewrites evidence refs relative to the bundle so a bundle survives being moved. That only works when you persist into the tree the evidence is already under.Persisting elsewhere is still allowed, because it is a supported scenario — a re-scored run may deliberately reference an earlier run's deliverables (see
test_persist_and_read_keep_external_evidence_refs_absolute). But it produces a bundle whose evidence refs point back at the original directory: fine while that directory exists, silently dangling once it doesn't. I verified this behaviour directly rather than inferring it.Making the safe path the default is the mitigation here. A louder signal — warning when evidence lands outside the bundle — is worth doing but is a separate change, since it needs to leave the deliberate case working.
Migration
AgentEvalRunConfig(output_dir=d)AgentEvalRunConfig(work_dir=d)AgentEvalRunConfig(write_dashboard=False)result.persist(write_dashboard=False)run()result.persist()result.output_dirlocation.output_dir(orresult.work_dir)result.dashboard_pathlocation.dashboard_pathAll in-repo callers are updated: the evaluator job plugin,
intake/publish.py, six examples, the Fabric notebook, and the tests.Verification
tyat exactly themainbaseline for these packages (536 both ways).make vendorrun; mirror included.Notes for review
persist()lives on the model and importspersist_runinside the method —persistenceimportsresultsfor the types it writes, so a module-level import would be circular.persist_runremains the underlying function.AgentEvalResult.load(bundle).persist()on a run with nowork_dirand no explicit target raises rather than inventing a directory.Summary by CodeRabbit
New Features
Updates
work_dirreplacesoutput_diras the evaluation workspace and default persistence location.Documentation