refactor(evaluator)!: make persisting an agent-eval run an explicit call - #1069
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe SDK replaces ChangesEvaluation persistence migration
Aggregate score schema updates
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-->>EvaluationCaller: BundleLocation
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
Contributor
|
This was referenced Aug 5, 2026
SandyChapman
force-pushed
the
explicit-run-persistence/schapman
branch
from
August 7, 2026 12:07
dafe316 to
8c44bcb
Compare
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.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/nemo-evaluator/openapi/openapi.yaml (1)
2740-2740: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPreserve
countin v2 result responses.
countwas required in the previous v2 schema, but is now optional. The v2 list/get routes omitNonevalues, so unknown counts disappear and break generated clients that requirecount. Keep required integer semantics in v2, or move nullable/omittable semantics to a new API version and regenerate clients.🤖 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/openapi/openapi.yaml` at line 2740, Update the v2 result response schemas at plugins/nemo-evaluator/openapi/openapi.yaml:2740-2740 and 2825-2825 to preserve required integer count semantics, ensuring v2 list/get responses always include count even when the value is unknown; do not make count nullable or omittable in v2, or otherwise move that behavior to a new API version before regenerating clients.Source: Linters/SAST tools
🤖 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 `@plugins/nemo-evaluator/openapi/openapi.yaml`:
- Around line 2669-2672: Update the three count descriptions in
plugins/nemo-evaluator/openapi/openapi.yaml at lines 2669-2672, 2752-2755, and
2837-2840, replacing “None when the sample size is unknown” with “Omitted when
the sample size is unknown” while preserving the remaining wording.
---
Outside diff comments:
In `@plugins/nemo-evaluator/openapi/openapi.yaml`:
- Line 2740: Update the v2 result response schemas at
plugins/nemo-evaluator/openapi/openapi.yaml:2740-2740 and 2825-2825 to preserve
required integer count semantics, ensuring v2 list/get responses always include
count even when the value is unknown; do not make count nullable or omittable in
v2, or otherwise move that behavior to a new API version before regenerating
clients.
🪄 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: ff260ffb-6e5a-48e0-9db5-a1277cc46200
⛔ 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 (34)
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/README.mdpackages/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_codex_runtime.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/openapi/openapi.yamlplugins/nemo-evaluator/src/nemo_evaluator/intake/publish.pyplugins/nemo-evaluator/src/nemo_evaluator/jobs/agent_evaluate.pyplugins/nemo-optimization/src/nemo_optimization/backends/optuna/fabric_trial.pyplugins/nemo-optimization/tests/test_fabric_trial.py
🚧 Files skipped from review as they are similar to previous changes (28)
- plugins/nemo-evaluator/src/nemo_evaluator/jobs/agent_evaluate.py
- packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/docker_sandbox.py
- packages/nemo_evaluator_sdk/examples/fabric_container/run_e2e.py
- packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/gym_runtime.py
- packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/codex/runtime.py
- packages/nemo_evaluator_sdk/tests/agent_eval/test_fabric_container_runtime.py
- packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/harbor_runtime.py
- plugins/nemo-evaluator/src/nemo_evaluator/intake/publish.py
- packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/tasks.py
- packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/runtime.py
- packages/nemo_evaluator_sdk/tests/agent_eval/test_fabric_integration.py
- packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/evaluator.py
- packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/results.py
- packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/persistence.py
- packages/nemo_evaluator_sdk/tests/agent_eval/test_codex_docker_example.py
- packages/nemo_evaluator_sdk/tests/agent_eval/test_docker_sandbox_runtime.py
- packages/nemo_evaluator_sdk/tests/agent_eval/test_evaluator.py
- packages/nemo_evaluator_sdk/tests/agent_eval/test_harbor_runtime.py
- packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/container_runtime.py
- packages/nemo_evaluator_sdk/examples/run_agent_eval/workflow_runtime.py
- packages/nemo_evaluator_sdk/examples/gym/run_gym_eval.py
- packages/nemo_evaluator_sdk/examples/run_agent_eval/run_agent_eval.py
- packages/nemo_evaluator_sdk/examples/codex_docker/example.py
- packages/nemo_evaluator_sdk/examples/run_agent_eval/pipeline.py
- packages/nemo_evaluator_sdk/examples/run_agent_eval/platform_runtime.py
- packages/nemo_evaluator_sdk/examples/profbench/runner.py
- packages/nemo_evaluator_sdk/examples/agentic_eval_with_fabric.ipynb
- packages/nemo_evaluator_sdk/examples/skill_eval/run_skill_eval.py
SandyChapman
force-pushed
the
explicit-run-persistence/schapman
branch
from
August 7, 2026 12:23
8c44bcb to
66490c3
Compare
SandyChapman
force-pushed
the
explicit-run-persistence/schapman
branch
from
August 7, 2026 14:40
66490c3 to
42d5f9d
Compare
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 `@plugins/nemo-evaluator/openapi/openapi.yaml`:
- Around line 2714-2722: Update the sample variance and related range statistics
descriptions in plugins/nemo-evaluator/openapi/openapi.yaml:2714-2722, rubric
statistics at plugins/nemo-evaluator/openapi/openapi.yaml:2799-2807, and scalar
statistics at plugins/nemo-evaluator/openapi/openapi.yaml:2886-2894 to state
that values are omitted when fewer than two scores are available, rather than
saying they are None or null. Apply the same wording consistently across all
three schema sections.
🪄 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: 087f5223-80ef-4d1e-89a3-d90bc96993a9
⛔ Files ignored due to path filters (1)
sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/values/results.pyis excluded by!sdk/**
📒 Files selected for processing (2)
packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/values/results.pyplugins/nemo-evaluator/openapi/openapi.yaml
SandyChapman
force-pushed
the
explicit-run-persistence/schapman
branch
from
August 7, 2026 15:26
42d5f9d to
069e764
Compare
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 `@plugins/nemo-evaluator/openapi/openapi.yaml`:
- Around line 2913-2929: Update the scalar-statistics model documentation around
the shown description to state that mean, min, and max are optional metadata,
including that mean may be preserved alongside value when both are provided.
Regenerate the OpenAPI artifact from the updated SDK schema/docstring rather
than editing generated output alone.
🪄 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: e37a4d11-2257-420a-b580-1b50aa4aa48b
⛔ Files ignored due to path filters (1)
sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/values/results.pyis excluded by!sdk/**
📒 Files selected for processing (2)
packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/values/results.pyplugins/nemo-evaluator/openapi/openapi.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/values/results.py
SandyChapman
force-pushed
the
explicit-run-persistence/schapman
branch
from
August 7, 2026 16:03
069e764 to
8b923d7
Compare
Contributor
|
Wil docs update be seprate MR? It fixes examples but i guess we have user facing docs which may need to reflect same behaviour |
SandyChapman
force-pushed
the
explicit-run-persistence/schapman
branch
from
August 7, 2026 17:32
8b923d7 to
0f6a5d5
Compare
Contributor
Contributor
Author
I added the docs updates. Good callout. |
arpitsardhana
approved these changes
Aug 7, 2026
SandyChapman
enabled auto-merge
August 7, 2026 19:20
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Aug 7, 2026
`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>
SandyChapman
force-pushed
the
explicit-run-persistence/schapman
branch
from
August 9, 2026 20:03
0f6a5d5 to
da12b50
Compare
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.
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
Documentation