feat(experimentalist): wire up nemo-evaluator-sdk using HarborRunner - #955
feat(experimentalist): wire up nemo-evaluator-sdk using HarborRunner#955ngoncharenko wants to merge 11 commits into
Conversation
|
|
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:
📝 WalkthroughWalkthroughThis PR adds SDK-backed Harbor evaluation, content-addressed native Harbor caching, runnable Hello and Tau2 examples, experiment configurations, architecture and E2E documentation, and regression tests. ChangesExperimentalist evaluator and examples
Sequence Diagram(s)sequenceDiagram
participant CLI as run-eval-only.py
participant Factory as evaluator/factory.py
participant Evaluator as HarborRunnerEvaluator
participant SDK as HarborAgentTaskRunner
participant Runtime as harbor_runtime.py
CLI->>Factory: build evaluator
Factory->>Evaluator: create HarborRunnerConfig
CLI->>Evaluator: run dataset and agent
Evaluator->>Evaluator: map dataset task names
Evaluator->>SDK: execute Harbor tasks
SDK->>Runtime: resolve cache and scoped import
Runtime-->>SDK: reuse or rerun job
SDK-->>Evaluator: return trial results
Evaluator-->>CLI: return aggregate metrics
Possibly related PRs
Suggested labels: 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 |
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (3)
plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/harbor_agent_task_runner.py (1)
51-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTYPE_CHECKING-only import violates path guideline.
HarborAgentTaskRunner,HarborRuntimeConfig,AgentEvalTaskare imported only underTYPE_CHECKINGfor annotation purposes. The lazy-import rationale (documented in the module docstring) is sound, but this specific pattern is exactly what the path guideline prohibits. Consider a structuralProtocolfor the SDK surface actually used (mirroringHarborJobOptionsinharbor.py), which preserves the lazy-import contract without a TYPE_CHECKING-only import.As per path instructions,
plugins/nemo-experimentalist/**/*.py: "Use concrete type hints rather than string-based type hints, and do not hide imports underTYPE_CHECKING."Also applies to: 195-217
🤖 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-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/harbor_agent_task_runner.py` around lines 51 - 59, Replace the TYPE_CHECKING-only SDK imports in the Harbor task runner with local structural Protocols describing the HarborAgentTaskRunner, HarborRuntimeConfig, and AgentEvalTask members actually used. Update the annotations and related code in the task-runner flow to use these Protocols while preserving lazy SDK loading and concrete type hints without hidden imports.Source: Path instructions
plugins/nemo-experimentalist/README.md (1)
127-129: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a final
Next Stepssection to each documentation page.Each page provides useful links or verification commands but ends without the required cross-linked
Next Stepssection.
plugins/nemo-experimentalist/README.md#L127-L129: turn the example and architecture links into the finalNext Stepssection.plugins/nemo-experimentalist/docs/architecture.md#L703-L707: add links to the runnable examples and evaluator documentation after the verification example.plugins/nemo-experimentalist/examples/hello-harbor-agent/README.md#L212-L218: add links to the architecture guide and evaluator tests after the Docker verification instructions.🤖 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-experimentalist/README.md` around lines 127 - 129, Append a final “Next Steps” section to all three documentation sites: in plugins/nemo-experimentalist/README.md lines 127-129, convert the existing example and architecture links into that section; in plugins/nemo-experimentalist/docs/architecture.md lines 703-707, add links to the runnable examples and evaluator documentation after the verification example; and in plugins/nemo-experimentalist/examples/hello-harbor-agent/README.md lines 212-218, add links to the architecture guide and evaluator tests after the Docker verification instructions.Source: Coding guidelines
plugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/dataset/template/task_template/environment/Dockerfile (1)
10-14: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGit not purged after clone, unlike
runtime-server/Dockerfile.
runtime-server/Dockerfilepurgesgitpost-clone; this Dockerfile doesn't, leaving unnecessary packages in the final image.♻️ Align with runtime-server/Dockerfile
RUN apt-get update && apt-get install -y --no-install-recommends \ git \ && git clone --depth=1 "${TAU2_BENCH_REPO}" "${TAU2_BENCH_ROOT}" \ && pip install --no-cache-dir uv "${TAU2_BENCH_ROOT}[knowledge]" \ + && apt-get purge -y --auto-remove git \ && rm -rf /var/lib/apt/lists/*🤖 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-experimentalist/examples/tau2-nemo-oo-agent/dataset/template/task_template/environment/Dockerfile` around lines 10 - 14, Update the package-install chain in the Dockerfile to remove git immediately after cloning and installing the benchmark dependencies, matching the cleanup behavior in runtime-server/Dockerfile while retaining the existing apt lists cleanup.
🤖 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-experimentalist/docs/architecture.md`:
- Around line 308-309: Update the failed-trial aggregation statement in the
architecture documentation to accurately describe the evaluator contract: failed
trials are excluded from the mean, so they do not directly lower it but reduce
the number of contributing trials. Remove the contradictory claim that a crash
drags the aggregate down.
- Around line 690-700: Make the evaluator example runnable by importing Path,
defining an async main function, and invoking it through asyncio.run(main())
instead of awaiting at module scope. Before evaluator.run, materialize the
baseline agent using the same approach as docs/e2e/run-eval-only.py so the
referenced agent-0 path exists.
In `@plugins/nemo-experimentalist/docs/e2e/experiment-debug-round.yaml`:
- Line 5: Update the runnable config comments to use platform-root-relative
paths: in plugins/nemo-experimentalist/docs/e2e/experiment-debug-round.yaml
lines 5-5, plugins/nemo-experimentalist/docs/e2e/experiment-eval-only.yaml lines
5-5, and plugins/nemo-experimentalist/docs/e2e/experiment-fast.yaml lines 2-2,
replace the docs/e2e path with the full plugins/nemo-experimentalist/docs/e2e
path.
In `@plugins/nemo-experimentalist/docs/e2e/experiment-fast.yaml`:
- Around line 1-3: Add the standard NVIDIA copyright and Apache-2.0 SPDX header
to plugins/nemo-experimentalist/docs/e2e/experiment-fast.yaml lines 1-3 using
YAML comments, and add the corresponding Markdown-comment header before the task
text in
plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/sum-two/instruction.md
line 1.
In
`@plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/task-template/environment/Dockerfile`:
- Around line 7-11: Update both Dockerfiles at
plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/task-template/environment/Dockerfile
lines 7-11 and
plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/greet-world/environment/Dockerfile
lines 7-11: create a dedicated non-root user, assign that user ownership of
/app/artifacts and /app/traces, and set USER to run each image as that user.
In
`@plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/task-template/instruction.md`:
- Around line 1-9: Add SPDX headers to the Markdown fixtures in
plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/task-template/instruction.md
and
plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/greet-world/instruction.md.
Keep the exact-output fixture
plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/task-template/tests/expected.txt
unchanged, or strip its header in test.sh before comparison.
In
`@plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/task-template/README.md`:
- Around line 4-16: Update the task-template README to add a prerequisites
section before the existing explanation, then append a “Next Steps” section
containing links to the relevant follow-up documentation. Ensure the page ends
with navigation and preserve the existing task-template details.
In
`@plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/greet-world/tests/test.sh`:
- Around line 21-32: Update both verifier scripts at
plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/greet-world/tests/test.sh
lines 21-32 and
plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/sum-three/tests/test.sh
lines 21-32: remove first-line-only extraction, compare the full CR-normalized
output against the full expected content, and reject outputs containing any
additional lines or content.
In
`@plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/sum-two/environment/Dockerfile`:
- Around line 7-11: Create a dedicated non-root user in the Dockerfile, grant
that user ownership of /app and its artifacts and traces directories, then set
the Docker USER directive before execution so the agent-controlled task does not
run as root.
In
`@plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/sum-two/tests/expected.txt`:
- Line 1: Resolve SPDX compliance for all three fixtures using one
repository-approved metadata or exception mechanism: in
plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/sum-two/tests/expected.txt
lines 1-1 and
plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/greet-universe/tests/expected.txt
lines 1-1, preserve the exact compared payloads while satisfying or explicitly
exempting the SPDX rule; in
plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/greet-universe/instruction.md
lines 1-1, add compliant NVIDIA copyright and Apache-2.0 metadata without
changing prompt semantics.
In
`@plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/sum-two/tests/test.sh`:
- Around line 21-38: Update the test script’s output validation to fail closed:
explicitly verify that reading both `/tests/expected.txt` and `$OUTPUT`
succeeds, normalize complete file contents without `head -n 1`, and compare the
complete normalized values. Preserve zero reward and unsuccessful formatting
when either read fails or the contents differ.
In
`@plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/greet-universe/environment/Dockerfile`:
- Around line 7-11: Update the Dockerfile after creating /app/artifacts and
/app/traces to add a dedicated unprivileged user, grant that user ownership of
both directories, and set the USER directive before the container executes the
agent. Preserve the existing /app working directory and artifact/trace paths.
In
`@plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/greet-universe/instruction.md`:
- Around line 1-7: Update the instruction document by adding a top-level
Markdown heading and changing the fenced code block to declare the text
language, while preserving the existing output path and exact required line.
In `@plugins/nemo-experimentalist/examples/hello-harbor-agent/README.md`:
- Around line 119-130: Update the A/B evaluator commands in the README to pass
distinct --experiment-dir values to run-eval-only.py, ensuring the Harbor and
harbor_agent_task_runner runs use separate experiment directories and cannot
reuse each other’s cached job results.
In `@plugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/AGENT-SPEC.md`:
- Line 1: Prepend the required NVIDIA copyright SPDX header and Apache-2.0
license identifier to all six affected files:
plugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/AGENT-SPEC.md (1-1),
dataset/template/task_template/environment/docker-compose.yaml (1-1),
environment/Dockerfile (1-1), environment/runtime-server/Dockerfile (1-1),
dataset/template/task_template/instruction.md (1-1), and
dataset/template/task_template/task.toml (1-1). Use the repository’s format for
each file type, placing comments before existing content and before FROM or
<instructions> where applicable.
In `@plugins/nemo-experimentalist/pyproject.toml`:
- Around line 14-16: Update the nemo-evaluator-sdk dependency declaration in
pyproject.toml to install only for Python 3.12 and newer, while preserving the
existing ungated harbor dependency and the evaluator_type behavior. Use the
project’s dependency marker syntax to exclude nemo-evaluator-sdk from Python
3.11 environments.
---
Nitpick comments:
In
`@plugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/dataset/template/task_template/environment/Dockerfile`:
- Around line 10-14: Update the package-install chain in the Dockerfile to
remove git immediately after cloning and installing the benchmark dependencies,
matching the cleanup behavior in runtime-server/Dockerfile while retaining the
existing apt lists cleanup.
In `@plugins/nemo-experimentalist/README.md`:
- Around line 127-129: Append a final “Next Steps” section to all three
documentation sites: in plugins/nemo-experimentalist/README.md lines 127-129,
convert the existing example and architecture links into that section; in
plugins/nemo-experimentalist/docs/architecture.md lines 703-707, add links to
the runnable examples and evaluator documentation after the verification
example; and in
plugins/nemo-experimentalist/examples/hello-harbor-agent/README.md lines
212-218, add links to the architecture guide and evaluator tests after the
Docker verification instructions.
In
`@plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/harbor_agent_task_runner.py`:
- Around line 51-59: Replace the TYPE_CHECKING-only SDK imports in the Harbor
task runner with local structural Protocols describing the
HarborAgentTaskRunner, HarborRuntimeConfig, and AgentEvalTask members actually
used. Update the annotations and related code in the task-runner flow to use
these Protocols while preserving lazy SDK loading and concrete type hints
without hidden imports.
🪄 Autofix (Beta)
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: 03aa3d96-1fdb-48f5-aab9-0ba78ef0204c
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (69)
plugins/nemo-experimentalist/README.mdplugins/nemo-experimentalist/docs/architecture.mdplugins/nemo-experimentalist/docs/e2e/experiment-debug-round.yamlplugins/nemo-experimentalist/docs/e2e/experiment-eval-only-sdk.yamlplugins/nemo-experimentalist/docs/e2e/experiment-eval-only.yamlplugins/nemo-experimentalist/docs/e2e/experiment-fast.yamlplugins/nemo-experimentalist/docs/e2e/run-eval-only.pyplugins/nemo-experimentalist/examples/hello-harbor-agent/AGENT-SPEC.mdplugins/nemo-experimentalist/examples/hello-harbor-agent/README.mdplugins/nemo-experimentalist/examples/hello-harbor-agent/agent.pyplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/task-template/README.mdplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/task-template/environment/Dockerfileplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/task-template/instruction.mdplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/task-template/task.tomlplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/task-template/tests/expected.txtplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/task-template/tests/test.shplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/greet-world/environment/Dockerfileplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/greet-world/instruction.mdplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/greet-world/task.tomlplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/greet-world/tests/expected.txtplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/greet-world/tests/test.shplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/sum-two/environment/Dockerfileplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/sum-two/instruction.mdplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/sum-two/task.tomlplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/sum-two/tests/expected.txtplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/sum-two/tests/test.shplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/greet-universe/environment/Dockerfileplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/greet-universe/instruction.mdplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/greet-universe/task.tomlplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/greet-universe/tests/expected.txtplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/greet-universe/tests/test.shplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/sum-three/environment/Dockerfileplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/sum-three/instruction.mdplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/sum-three/task.tomlplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/sum-three/tests/expected.txtplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/sum-three/tests/test.shplugins/nemo-experimentalist/examples/hello-harbor-agent/harbor_wrapper.pyplugins/nemo-experimentalist/examples/hello-harbor-agent/main.pyplugins/nemo-experimentalist/examples/hello-harbor-agent/optimizer.yamlplugins/nemo-experimentalist/examples/hello-harbor-agent/tracing.pyplugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/AGENT-SPEC.mdplugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/agent.pyplugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/dataset/template/task_template/environment/Dockerfileplugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/dataset/template/task_template/environment/docker-compose.yamlplugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/dataset/template/task_template/environment/runtime-server/Dockerfileplugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/dataset/template/task_template/environment/runtime-server/server.pyplugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/dataset/template/task_template/environment/runtime-server/task_config.jsonplugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/dataset/template/task_template/instruction.mdplugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/dataset/template/task_template/task.tomlplugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/dataset/template/task_template/tests/test.shplugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/harbor_wrapper.pyplugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/main.pyplugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/optimizer.yamlplugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/pyproject.tomlplugins/nemo-experimentalist/pyproject.tomlplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/eval_author/config.yamlplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/eval_author/run.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/base.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/factory.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/harbor.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/harbor_agent_task_runner.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/deps.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/run.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/resolve.pyplugins/nemo-experimentalist/tests/experimentalist/conftest.pyplugins/nemo-experimentalist/tests/experimentalist/test_evaluator_harbor_ab_e2e.pyplugins/nemo-experimentalist/tests/experimentalist/test_evaluator_harbor_agent_task_runner.pyplugins/nemo-experimentalist/tests/experimentalist/test_hello_example_baseline.pyplugins/nemo-experimentalist/tests/test_eval_author_run.py
ba6355e to
1f903e8
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
plugins/nemo-experimentalist/pyproject.toml (1)
16-16: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGate
nemo-evaluator-sdkfor Python 3.12+.This ungated dependency breaks supported Python 3.11 installs before users can select the plain Harbor fallback. Mirror the Harbor marker.
- "nemo-evaluator-sdk", + "nemo-evaluator-sdk; python_full_version >= '3.12'",🤖 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-experimentalist/pyproject.toml` at line 16, Update the nemo-evaluator-sdk dependency entry in pyproject.toml to include the same Python-version environment marker used by the Harbor dependency, restricting it to Python 3.12 and newer while preserving the existing dependency declaration.Source: Learnings
plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/task-template/environment/Dockerfile (1)
7-11: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftRun all Harbor task images as an unprivileged user.
plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/task-template/environment/Dockerfile#L7-L11: create a user, chown/appand its writable directories, and setUSER.plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/greet-world/environment/Dockerfile#L7-L11: apply the same non-root setup.plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/sum-two/environment/Dockerfile#L7-L11: apply the same non-root setup.plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/greet-universe/environment/Dockerfile#L7-L11: apply the same non-root setup.plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/sum-three/environment/Dockerfile#L7-L11: apply the same non-root setup.
Based on learnings, Harbor uploads the agent into/appand currently executes it as root; validate upload, permissions, startup, and execution end to end for every image.🤖 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-experimentalist/examples/hello-harbor-agent/dataset/task-template/environment/Dockerfile` around lines 7 - 11, The five listed Dockerfiles must run Harbor tasks as an unprivileged user: create a dedicated user, chown /app and its writable artifacts and traces directories, then set USER to that account. Apply the same setup in the task-template, train/greet-world, train/sum-two, validation/greet-universe, and validation/sum-three Dockerfiles, and verify upload, permissions, startup, and execution end to end for every image.Sources: Learnings, Linters/SAST tools
🧹 Nitpick comments (3)
plugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/README.md (1)
11-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake this a focused how-to and move prerequisites first.
Move the component table to a linked reference page, then place Docker, registry access, network access, and
INFERENCE_API_KEYunder Prerequisites before the commands. As per coding guidelines, each page must fit one Diataxis quadrant and list prerequisites at the top.🤖 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-experimentalist/examples/tau2-nemo-oo-agent/README.md` around lines 11 - 54, Restructure the README into a focused how-to: add a Prerequisites section near the top listing Docker, dataset-registry access, task-container network access, and a valid INFERENCE_API_KEY before the setup and run commands. Remove the inline “What it contains” component table and replace it with a link to the appropriate reference page, keeping the remaining execution instructions focused on running the example.Source: Coding guidelines
plugins/nemo-experimentalist/docs/architecture.md (1)
4-19: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftSplit the mixed documentation pages by Diataxis type.
plugins/nemo-experimentalist/docs/architecture.md#L4-L19: retain architecture explanation here; move run/debug procedures and reference material to linked pages.plugins/nemo-experimentalist/examples/hello-harbor-agent/README.md#L4-L16: retain the example how-to here; move evaluator, cache, and configuration reference material to linked pages.
As per coding guidelines, each documentation page should fit one Diataxis quadrant.🤖 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-experimentalist/docs/architecture.md` around lines 4 - 19, Split the mixed documentation by Diataxis type: in plugins/nemo-experimentalist/docs/architecture.md lines 4-19, retain only the architecture explanation and link run/debug procedures and reference material to dedicated pages; in plugins/nemo-experimentalist/examples/hello-harbor-agent/README.md lines 4-16, retain the example how-to and move evaluator, cache, and configuration reference content to linked pages.Source: Coding guidelines
plugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/AGENT-SPEC.md (1)
1-167: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPolicy text duplicated verbatim in
instruction.md.The full airline policy here is copy-pasted into
dataset/template/task_template/instruction.md. Two independent copies of a ~180-line policy will drift; consider generating the<policy>block ininstruction.mdfrom this file (or vice versa) at dataset-prep time.🤖 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-experimentalist/examples/tau2-nemo-oo-agent/AGENT-SPEC.md` around lines 1 - 167, Remove the duplicated airline policy from dataset/template/task_template/instruction.md and establish a single source of truth using the policy in AGENT-SPEC.md, generating or synchronizing the instruction.md <policy> block during dataset preparation. Ensure the generated block remains verbatim and update the preparation flow rather than maintaining two independently edited copies.
🤖 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-experimentalist/docs/architecture.md`:
- Around line 584-593: Move the Prerequisites block in
plugins/nemo-experimentalist/docs/architecture.md (lines 584-593) before the
architecture overview. Also move the corresponding prerequisites block in
plugins/nemo-experimentalist/examples/hello-harbor-agent/README.md (lines
111-117) before the example layout and behavior sections, keeping each block’s
existing content unchanged.
In
`@plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/greet-world/tests/test.sh`:
- Around line 37-39: Replace the tr -d '\r' normalization in the EXPECTED and
ACTUAL assignments of all three test.sh
files—plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/greet-world/tests/test.sh
lines 37-39,
plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/sum-two/tests/test.sh
lines 37-39, and
plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/greet-universe/tests/test.sh
lines 37-39—with CRLF-only normalization that removes carriage returns only when
paired with line feeds, preserving standalone carriage returns.
In `@plugins/nemo-experimentalist/examples/hello-harbor-agent/README.md`:
- Around line 172-174: Update the force-rerun command in the README to include
the explicit --experiment-dir tmp/eval-only-sdk argument, ensuring it refreshes
the SDK evaluator arm and remains distinct from the other evaluator experiment
directory.
In
`@plugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/dataset/template/task_template/environment/docker-compose.yaml`:
- Line 1: Add the required NVIDIA copyright and Apache-2.0 SPDX license
identifiers at the top of the docker-compose.yaml file, before the existing
services declaration, following the repository’s standard header format.
In `@plugins/nemo-experimentalist/README.md`:
- Around line 115-125: Use distinct experiment directories in both onboarding
examples: update the plain-Harbor command in
plugins/nemo-experimentalist/README.md lines 115-125 to use
tmp/exp-hello-eval-plain, and update the SDK runner command in
plugins/nemo-experimentalist/docs/e2e/README.md lines 58-64 to use
tmp/exp-hello-eval-sdk.
In
`@plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/harbor_agent_task_runner.py`:
- Around line 175-176: Update the task execution flow around _cache_fingerprint
and _cache_is_stale to acquire a per-job-directory lock before rechecking cache
state. Keep the lock held through the cache recheck, run_tasks invocation, and
fingerprint write, so concurrent processes cannot run or write the same trial
directory; release it afterward.
---
Duplicate comments:
In
`@plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/task-template/environment/Dockerfile`:
- Around line 7-11: The five listed Dockerfiles must run Harbor tasks as an
unprivileged user: create a dedicated user, chown /app and its writable
artifacts and traces directories, then set USER to that account. Apply the same
setup in the task-template, train/greet-world, train/sum-two,
validation/greet-universe, and validation/sum-three Dockerfiles, and verify
upload, permissions, startup, and execution end to end for every image.
In `@plugins/nemo-experimentalist/pyproject.toml`:
- Line 16: Update the nemo-evaluator-sdk dependency entry in pyproject.toml to
include the same Python-version environment marker used by the Harbor
dependency, restricting it to Python 3.12 and newer while preserving the
existing dependency declaration.
---
Nitpick comments:
In `@plugins/nemo-experimentalist/docs/architecture.md`:
- Around line 4-19: Split the mixed documentation by Diataxis type: in
plugins/nemo-experimentalist/docs/architecture.md lines 4-19, retain only the
architecture explanation and link run/debug procedures and reference material to
dedicated pages; in
plugins/nemo-experimentalist/examples/hello-harbor-agent/README.md lines 4-16,
retain the example how-to and move evaluator, cache, and configuration reference
content to linked pages.
In `@plugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/AGENT-SPEC.md`:
- Around line 1-167: Remove the duplicated airline policy from
dataset/template/task_template/instruction.md and establish a single source of
truth using the policy in AGENT-SPEC.md, generating or synchronizing the
instruction.md <policy> block during dataset preparation. Ensure the generated
block remains verbatim and update the preparation flow rather than maintaining
two independently edited copies.
In `@plugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/README.md`:
- Around line 11-54: Restructure the README into a focused how-to: add a
Prerequisites section near the top listing Docker, dataset-registry access,
task-container network access, and a valid INFERENCE_API_KEY before the setup
and run commands. Remove the inline “What it contains” component table and
replace it with a link to the appropriate reference page, keeping the remaining
execution instructions focused on running the example.
🪄 Autofix (Beta)
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: f53b7e03-88f4-40e6-883c-c36ee933b2f5
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (72)
plugins/nemo-experimentalist/README.mdplugins/nemo-experimentalist/docs/architecture.mdplugins/nemo-experimentalist/docs/e2e/README.mdplugins/nemo-experimentalist/docs/e2e/experiment-debug-round.yamlplugins/nemo-experimentalist/docs/e2e/experiment-eval-only-sdk.yamlplugins/nemo-experimentalist/docs/e2e/experiment-eval-only.yamlplugins/nemo-experimentalist/docs/e2e/experiment-fast.yamlplugins/nemo-experimentalist/docs/e2e/run-eval-only.pyplugins/nemo-experimentalist/examples/README.mdplugins/nemo-experimentalist/examples/hello-harbor-agent/AGENT-SPEC.mdplugins/nemo-experimentalist/examples/hello-harbor-agent/README.mdplugins/nemo-experimentalist/examples/hello-harbor-agent/agent.pyplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/task-template/README.mdplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/task-template/environment/Dockerfileplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/task-template/instruction.mdplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/task-template/task.tomlplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/task-template/tests/expected.txtplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/task-template/tests/test.shplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/greet-world/environment/Dockerfileplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/greet-world/instruction.mdplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/greet-world/task.tomlplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/greet-world/tests/expected.txtplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/greet-world/tests/test.shplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/sum-two/environment/Dockerfileplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/sum-two/instruction.mdplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/sum-two/task.tomlplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/sum-two/tests/expected.txtplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/sum-two/tests/test.shplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/greet-universe/environment/Dockerfileplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/greet-universe/instruction.mdplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/greet-universe/task.tomlplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/greet-universe/tests/expected.txtplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/greet-universe/tests/test.shplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/sum-three/environment/Dockerfileplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/sum-three/instruction.mdplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/sum-three/task.tomlplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/sum-three/tests/expected.txtplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/sum-three/tests/test.shplugins/nemo-experimentalist/examples/hello-harbor-agent/harbor_wrapper.pyplugins/nemo-experimentalist/examples/hello-harbor-agent/main.pyplugins/nemo-experimentalist/examples/hello-harbor-agent/optimizer.yamlplugins/nemo-experimentalist/examples/hello-harbor-agent/tracing.pyplugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/AGENT-SPEC.mdplugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/README.mdplugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/agent.pyplugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/dataset/template/task_template/environment/Dockerfileplugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/dataset/template/task_template/environment/docker-compose.yamlplugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/dataset/template/task_template/environment/runtime-server/Dockerfileplugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/dataset/template/task_template/environment/runtime-server/server.pyplugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/dataset/template/task_template/environment/runtime-server/task_config.jsonplugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/dataset/template/task_template/instruction.mdplugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/dataset/template/task_template/task.tomlplugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/dataset/template/task_template/tests/test.shplugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/harbor_wrapper.pyplugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/main.pyplugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/optimizer.yamlplugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/pyproject.tomlplugins/nemo-experimentalist/pyproject.tomlplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/eval_author/config.yamlplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/eval_author/run.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/base.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/factory.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/harbor.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/harbor_agent_task_runner.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/deps.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/run.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/resolve.pyplugins/nemo-experimentalist/tests/experimentalist/conftest.pyplugins/nemo-experimentalist/tests/experimentalist/test_evaluator_harbor_ab_e2e.pyplugins/nemo-experimentalist/tests/experimentalist/test_evaluator_harbor_agent_task_runner.pyplugins/nemo-experimentalist/tests/experimentalist/test_hello_example_baseline.pyplugins/nemo-experimentalist/tests/test_eval_author_run.py
🚧 Files skipped from review as they are similar to previous changes (37)
- plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/greet-universe/tests/expected.txt
- plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/sum-three/task.toml
- plugins/nemo-experimentalist/docs/e2e/experiment-eval-only.yaml
- plugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/main.py
- plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/sum-two/tests/expected.txt
- plugins/nemo-experimentalist/docs/e2e/experiment-eval-only-sdk.yaml
- plugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/optimizer.yaml
- plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/task-template/task.toml
- plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/greet-world/tests/expected.txt
- plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/task-template/tests/expected.txt
- plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/task-template/tests/test.sh
- plugins/nemo-experimentalist/docs/e2e/experiment-fast.yaml
- plugins/nemo-experimentalist/examples/hello-harbor-agent/AGENT-SPEC.md
- plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/resolve.py
- plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/greet-world/task.toml
- plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/eval_author/run.py
- plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/eval_author/config.yaml
- plugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/pyproject.toml
- plugins/nemo-experimentalist/tests/experimentalist/conftest.py
- plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/run.py
- plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/deps.py
- plugins/nemo-experimentalist/examples/hello-harbor-agent/main.py
- plugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/dataset/template/task_template/task.toml
- plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/sum-three/tests/test.sh
- plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/sum-three/tests/expected.txt
- plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/task-template/README.md
- plugins/nemo-experimentalist/examples/hello-harbor-agent/optimizer.yaml
- plugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/dataset/template/task_template/environment/runtime-server/task_config.json
- plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/sum-two/task.toml
- plugins/nemo-experimentalist/tests/test_eval_author_run.py
- plugins/nemo-experimentalist/docs/e2e/experiment-debug-round.yaml
- plugins/nemo-experimentalist/examples/hello-harbor-agent/agent.py
- plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/base.py
- plugins/nemo-experimentalist/tests/experimentalist/test_hello_example_baseline.py
- plugins/nemo-experimentalist/examples/hello-harbor-agent/harbor_wrapper.py
- plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/harbor.py
- plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/greet-universe/task.toml
…ions Round-2 review fixes on #955. Verifier (all 5 copies of tests/test.sh): - Normalize CRLF at end-of-line only. `tr -d '\r'` deleted every carriage return, so `sum=4<CR>2` collapsed into a passing `sum=42` — the same reward-hacking class as the trailing-output hole fixed earlier. The agent under test is code the optimizer actively reward-maximizes, so a lax verifier is a live surface, not a hypothetical one. - Mirror the rule in test_hello_example_baseline.py, which had the identical hole via `.replace("\r", "")`, and assert the shell and Python rules stay in lockstep; that pairing has now drifted twice. Docs: - Give every evaluator arm its own --experiment-dir. README.md (plain Harbor) and docs/e2e/README.md (SDK runner) both pointed at tmp/exp-hello-eval; the two evaluators disagree about an existing job dir, so sharing one makes the result order-dependent. - Point the --force-rerun example at tmp/eval-only-sdk rather than the default. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Nick Goncharenko <ngoncharenko@nvidia.com>
…ions Round-2 review fixes on #955. Verifier (all 5 copies of tests/test.sh): - Normalize CRLF at end-of-line only. `tr -d '\r'` deleted every carriage return, so `sum=4<CR>2` collapsed into a passing `sum=42` — the same reward-hacking class as the trailing-output hole fixed earlier. The agent under test is code the optimizer actively reward-maximizes, so a lax verifier is a live surface, not a hypothetical one. - Mirror the rule in test_hello_example_baseline.py, which had the identical hole via `.replace("\r", "")`, and assert the shell and Python rules stay in lockstep; that pairing has now drifted twice. Docs: - Give every evaluator arm its own --experiment-dir. README.md (plain Harbor) and docs/e2e/README.md (SDK runner) both pointed at tmp/exp-hello-eval; the two evaluators disagree about an existing job dir, so sharing one makes the result order-dependent. - Point the --force-rerun example at tmp/eval-only-sdk rather than the default. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Nick Goncharenko <ngoncharenko@nvidia.com>
2e23b4b to
ca7e2ad
Compare
…ions Round-2 review fixes on #955. Verifier (all 5 copies of tests/test.sh): - Normalize CRLF at end-of-line only. `tr -d '\r'` deleted every carriage return, so `sum=4<CR>2` collapsed into a passing `sum=42` — the same reward-hacking class as the trailing-output hole fixed earlier. The agent under test is code the optimizer actively reward-maximizes, so a lax verifier is a live surface, not a hypothetical one. - Mirror the rule in test_hello_example_baseline.py, which had the identical hole via `.replace("\r", "")`, and assert the shell and Python rules stay in lockstep; that pairing has now drifted twice. Docs: - Give every evaluator arm its own --experiment-dir. README.md (plain Harbor) and docs/e2e/README.md (SDK runner) both pointed at tmp/exp-hello-eval; the two evaluators disagree about an existing job dir, so sharing one makes the result order-dependent. - Point the --force-rerun example at tmp/eval-only-sdk rather than the default. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Nick Goncharenko <ngoncharenko@nvidia.com>
ca7e2ad to
04528b1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
plugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/dataset/template/task_template/environment/runtime-server/server.py (1)
4-4: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDrop
from __future__ import annotations; hints are already concrete for this 3.12 target.This import stringifies all annotations in the file, conflicting with the path guideline to use concrete type hints. The file already uses native
int | None/dict[str, Any]syntax that needs no postponed evaluation on Python 3.12.As per path instructions for
plugins/nemo-experimentalist/**/*.py: "Use concrete type hints rather than string-based type hints, and do not hide imports under TYPE_CHECKING."🤖 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-experimentalist/examples/tau2-nemo-oo-agent/dataset/template/task_template/environment/runtime-server/server.py` at line 4, Remove the from __future__ import annotations directive from server.py, leaving the existing concrete Python 3.12 annotations and imports unchanged.Source: Path instructions
plugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/dataset/template/task_template/environment/Dockerfile (1)
5-14: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUnpinned tau2-bench git ref in both task Dockerfiles. Both images clone
tau2-benchwith--depth=1and no commit/tag pin, so rebuilds can silently pick up a different upstream source and dependency versions, breaking eval-image reproducibility.
plugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/dataset/template/task_template/environment/Dockerfile#L5-L14: add aTAU2_BENCH_REFbuild arg,git checkoutit after clone, pin theuvversion, and purgegitafter install (matching the sibling Dockerfile).plugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/dataset/template/task_template/environment/runtime-server/Dockerfile#L5-L16: sameTAU2_BENCH_REFpin, and capfastmcpto a known-good range instead of an open-ended>=3.0.🤖 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-experimentalist/examples/tau2-nemo-oo-agent/dataset/template/task_template/environment/Dockerfile` around lines 5 - 14, Pin tau2-bench and dependency versions in both Dockerfiles: in plugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/dataset/template/task_template/environment/Dockerfile lines 5-14, add TAU2_BENCH_REF, checkout that ref after cloning, pin uv, and remove git after installation; in plugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/dataset/template/task_template/environment/runtime-server/Dockerfile lines 5-16, add and checkout TAU2_BENCH_REF and constrain fastmcp to the known-good version range.
🤖 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-experimentalist/docs/architecture.md`:
- Around line 308-312: Update the aggregation contract in the trial-status
documentation to state that only trials with status == "completed" are averaged,
replacing the broader status != "failed" wording. Preserve the existing
explanation that failed trials remain in the denominator and all-failed rounds
aggregate to {}.
---
Nitpick comments:
In
`@plugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/dataset/template/task_template/environment/Dockerfile`:
- Around line 5-14: Pin tau2-bench and dependency versions in both Dockerfiles:
in
plugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/dataset/template/task_template/environment/Dockerfile
lines 5-14, add TAU2_BENCH_REF, checkout that ref after cloning, pin uv, and
remove git after installation; in
plugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/dataset/template/task_template/environment/runtime-server/Dockerfile
lines 5-16, add and checkout TAU2_BENCH_REF and constrain fastmcp to the
known-good version range.
In
`@plugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/dataset/template/task_template/environment/runtime-server/server.py`:
- Line 4: Remove the from __future__ import annotations directive from
server.py, leaving the existing concrete Python 3.12 annotations and imports
unchanged.
🪄 Autofix (Beta)
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: 001c2536-569b-47cb-8d78-1958cce08ea6
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (72)
plugins/nemo-experimentalist/README.mdplugins/nemo-experimentalist/docs/architecture.mdplugins/nemo-experimentalist/docs/e2e/README.mdplugins/nemo-experimentalist/docs/e2e/experiment-debug-round.yamlplugins/nemo-experimentalist/docs/e2e/experiment-eval-only-sdk.yamlplugins/nemo-experimentalist/docs/e2e/experiment-eval-only.yamlplugins/nemo-experimentalist/docs/e2e/experiment-fast.yamlplugins/nemo-experimentalist/docs/e2e/run-eval-only.pyplugins/nemo-experimentalist/examples/README.mdplugins/nemo-experimentalist/examples/hello-harbor-agent/AGENT-SPEC.mdplugins/nemo-experimentalist/examples/hello-harbor-agent/README.mdplugins/nemo-experimentalist/examples/hello-harbor-agent/agent.pyplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/task-template/README.mdplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/task-template/environment/Dockerfileplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/task-template/instruction.mdplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/task-template/task.tomlplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/task-template/tests/expected.txtplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/task-template/tests/test.shplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/greet-world/environment/Dockerfileplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/greet-world/instruction.mdplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/greet-world/task.tomlplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/greet-world/tests/expected.txtplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/greet-world/tests/test.shplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/sum-two/environment/Dockerfileplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/sum-two/instruction.mdplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/sum-two/task.tomlplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/sum-two/tests/expected.txtplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/sum-two/tests/test.shplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/greet-universe/environment/Dockerfileplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/greet-universe/instruction.mdplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/greet-universe/task.tomlplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/greet-universe/tests/expected.txtplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/greet-universe/tests/test.shplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/sum-three/environment/Dockerfileplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/sum-three/instruction.mdplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/sum-three/task.tomlplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/sum-three/tests/expected.txtplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/sum-three/tests/test.shplugins/nemo-experimentalist/examples/hello-harbor-agent/harbor_wrapper.pyplugins/nemo-experimentalist/examples/hello-harbor-agent/main.pyplugins/nemo-experimentalist/examples/hello-harbor-agent/optimizer.yamlplugins/nemo-experimentalist/examples/hello-harbor-agent/tracing.pyplugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/AGENT-SPEC.mdplugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/README.mdplugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/agent.pyplugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/dataset/template/task_template/environment/Dockerfileplugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/dataset/template/task_template/environment/docker-compose.yamlplugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/dataset/template/task_template/environment/runtime-server/Dockerfileplugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/dataset/template/task_template/environment/runtime-server/server.pyplugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/dataset/template/task_template/environment/runtime-server/task_config.jsonplugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/dataset/template/task_template/instruction.mdplugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/dataset/template/task_template/task.tomlplugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/dataset/template/task_template/tests/test.shplugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/harbor_wrapper.pyplugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/main.pyplugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/optimizer.yamlplugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/pyproject.tomlplugins/nemo-experimentalist/pyproject.tomlplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/eval_author/config.yamlplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/eval_author/run.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/base.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/factory.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/harbor.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/harbor_agent_task_runner.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/deps.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/run.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/resolve.pyplugins/nemo-experimentalist/tests/experimentalist/conftest.pyplugins/nemo-experimentalist/tests/experimentalist/test_evaluator_harbor_ab_e2e.pyplugins/nemo-experimentalist/tests/experimentalist/test_evaluator_harbor_agent_task_runner.pyplugins/nemo-experimentalist/tests/experimentalist/test_hello_example_baseline.pyplugins/nemo-experimentalist/tests/test_eval_author_run.py
🚧 Files skipped from review as they are similar to previous changes (43)
- plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/sum-three/tests/expected.txt
- plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/greet-world/tests/expected.txt
- plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/task-template/README.md
- plugins/nemo-experimentalist/examples/hello-harbor-agent/AGENT-SPEC.md
- plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/greet-universe/tests/expected.txt
- plugins/nemo-experimentalist/docs/e2e/experiment-fast.yaml
- plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/sum-two/task.toml
- plugins/nemo-experimentalist/docs/e2e/experiment-eval-only-sdk.yaml
- plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/run.py
- plugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/main.py
- plugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/optimizer.yaml
- plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/greet-world/task.toml
- plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/task-template/task.toml
- plugins/nemo-experimentalist/examples/hello-harbor-agent/optimizer.yaml
- plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/eval_author/config.yaml
- plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/greet-universe/task.toml
- plugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/pyproject.toml
- plugins/nemo-experimentalist/tests/experimentalist/conftest.py
- plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/sum-two/tests/expected.txt
- plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/eval_author/run.py
- plugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/dataset/template/task_template/environment/runtime-server/task_config.json
- plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/greet-world/tests/test.sh
- plugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/dataset/template/task_template/environment/docker-compose.yaml
- plugins/nemo-experimentalist/examples/hello-harbor-agent/main.py
- plugins/nemo-experimentalist/docs/e2e/experiment-eval-only.yaml
- plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/sum-three/task.toml
- plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/task-template/tests/expected.txt
- plugins/nemo-experimentalist/examples/hello-harbor-agent/agent.py
- plugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/dataset/template/task_template/task.toml
- plugins/nemo-experimentalist/pyproject.toml
- plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/sum-two/tests/test.sh
- plugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/README.md
- plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/base.py
- plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/deps.py
- plugins/nemo-experimentalist/tests/experimentalist/test_hello_example_baseline.py
- plugins/nemo-experimentalist/examples/hello-harbor-agent/harbor_wrapper.py
- plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/sum-three/tests/test.sh
- plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/task-template/tests/test.sh
- plugins/nemo-experimentalist/docs/e2e/experiment-debug-round.yaml
- plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/resolve.py
- plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/greet-universe/tests/test.sh
- plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/factory.py
- plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/harbor.py
04528b1 to
dc158c9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/harbor_agent_task_runner.py (2)
112-112: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
default_factory=list.Matches
HarborRuntimeConfigand avoids a shared literal default.♻️ Proposed change
- artifacts: list[str] = Field(default=[], description="Additional Harbor artifact sources to collect per trial.") + artifacts: list[str] = Field( + default_factory=list, description="Additional Harbor artifact sources to collect per trial." + )🤖 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-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/harbor_agent_task_runner.py` at line 112, Update the artifacts field in the relevant task-runner configuration model to use default_factory=list instead of a mutable list literal default, matching HarborRuntimeConfig and ensuring each instance receives its own list.
51-59: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
TYPE_CHECKING-only SDK imports conflict with the repo guideline.The docstring rationale is sound (a broken SDK install must not break the plain evaluator), so this is likely the right call — but it deviates from the stated rule. Consider annotating with
NamedTuplefield types resolved viaAny/protocols, or documenting the exemption so the next reader doesn't "fix" it.As per coding guidelines: "prefer concrete type hints over string-based type hints, and do not import those types only under
TYPE_CHECKING; import them normally when possible."🤖 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-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/harbor_agent_task_runner.py` around lines 51 - 59, Document the intentional TYPE_CHECKING-only imports in the module or nearby code, explicitly recording the Harbor SDK lazy-import requirement as an exemption to the normal concrete-type import guideline. Preserve the existing HarborAgentTaskRunner, HarborRuntimeConfig, and AgentEvalTask annotations and lazy-import behavior.Source: Coding guidelines
packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/harbor_runtime.py (1)
489-501: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUse
_safe_resolvehere too.
Path.resolve()on a vanishing symlink chain can raiseOSError(the reason_safe_resolveexists). Lines 489/500 and 588 call.resolve()directly, so a mid-walk race turns a best-effort cache check into a failed run.♻️ Proposed change
- dataset_root = dataset_path.resolve() + dataset_root = _safe_resolve(dataset_path) @@ - candidate_resolved = candidate.resolve() + candidate_resolved = _safe_resolve(candidate)🤖 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/harbor_runtime.py` around lines 489 - 501, Replace the direct Path.resolve() calls in the task-directory cache validation and the related path handling around the runtime logic with the existing _safe_resolve helper. Ensure vanishing symlink chains or other OSError cases remain best-effort and continue through normal re-discovery rather than failing the run.packages/nemo_evaluator_sdk/tests/agent_eval/test_harbor_runtime.py (1)
319-321: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop
async/asynciomarks from these three tests.Nothing is awaited in
test_changed_inputs_invalidate_the_cache,test_cosmetic_options_do_not_evict_the_cache, ortest_task_subset_of_a_cached_run_still_hits— they only call sync helpers.♻️ Proposed change
-@pytest.mark.asyncio `@pytest.mark.parametrize`("mutation", ["agent", "task", "option"]) -async def test_changed_inputs_invalidate_the_cache(tmp_path: Path, mutation: str) -> None: +def test_changed_inputs_invalidate_the_cache(tmp_path: Path, mutation: str) -> None:Also applies to: 344-345, 357-358
🤖 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_harbor_runtime.py` around lines 319 - 321, Remove the async declaration and `@pytest.mark.asyncio` decorator from test_changed_inputs_invalidate_the_cache, test_cosmetic_options_do_not_evict_the_cache, and test_task_subset_of_a_cached_run_still_hits, since each test uses only synchronous helpers and does not await anything.plugins/nemo-experimentalist/docs/architecture.md (2)
582-593: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftKeep this page in one Diataxis quadrant.
This is an EXPLANATION page, but it embeds a HOW-TO and ends without
Next Steps. Move run/debug instructions to a linked HOW-TO and add cross-links at the end.🤖 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-experimentalist/docs/architecture.md` around lines 582 - 593, Keep architecture.md focused on explanation by removing the embedded “Running and debugging it” prerequisites and command instructions, then move them into a linked HOW-TO document. Add an appropriate cross-link to that HOW-TO and a “Next Steps” section at the end of the architecture page, linking to relevant follow-up documentation.Source: Coding guidelines
582-593: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftApply the Diataxis structure rules consistently.
plugins/nemo-experimentalist/docs/architecture.md#L582-L593: keep the page as EXPLANATION, move run/debug instructions to a linked HOW-TO, and addNext Steps.plugins/nemo-experimentalist/examples/README.md#L4-L14: identify the page as REFERENCE and add prerequisites plusNext Steps.plugins/nemo-experimentalist/examples/hello-harbor-agent/README.md#L96-L117: keep one TUTORIAL/HOW-TO scope and move reference material to linked pages.🤖 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-experimentalist/docs/architecture.md` around lines 582 - 593, Apply Diataxis structure consistently: in plugins/nemo-experimentalist/docs/architecture.md lines 582-593, keep the page explanatory by moving run/debug commands to a linked HOW-TO and adding a Next Steps section; in plugins/nemo-experimentalist/examples/README.md lines 4-14, label the page as REFERENCE, add prerequisites, and add Next Steps; in plugins/nemo-experimentalist/examples/hello-harbor-agent/README.md lines 96-117, retain a single TUTORIAL/HOW-TO scope and move reference content to linked pages.Source: Coding guidelines
plugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/dataset/template/task_template/environment/runtime-server/Dockerfile (1)
11-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin
fastmcpto an exact version instead of>=3.0.FastMCP's own docs call out
fastmcp>=3.0.0as bad practice, since minor versions can ship breaking changes; they recommend exact pins for reproducible builds.♻️ Proposed fix
- && pip install --no-cache-dir uv "${TAU2_BENCH_ROOT}[knowledge]" "fastmcp>=3.0" \ + && pip install --no-cache-dir uv "${TAU2_BENCH_ROOT}[knowledge]" "fastmcp==3.4.4" \🤖 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-experimentalist/examples/tau2-nemo-oo-agent/dataset/template/task_template/environment/runtime-server/Dockerfile` around lines 11 - 16, Update the fastmcp dependency in the Dockerfile install command to use an exact version pin rather than the open-ended “>=3.0” constraint, while leaving the other package installations 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.
Inline comments:
In
`@plugins/nemo-experimentalist/tests/experimentalist/test_evaluator_harbor_agent_task_runner.py`:
- Around line 355-382: Update test_errored_cached_job_is_rerun to use the
stamped, complete cached_job_dir fixture instead of manually constructing an
unstamped job_dir with _write_trial. Before invoking HarborRunnerEvaluator._run,
modify the cached sum-three trial’s result.json to add exception_info while
preserving the cache stamp, then assert fake_job.calls confirms only that
errored trial is rerun.
---
Nitpick comments:
In
`@packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/harbor_runtime.py`:
- Around line 489-501: Replace the direct Path.resolve() calls in the
task-directory cache validation and the related path handling around the runtime
logic with the existing _safe_resolve helper. Ensure vanishing symlink chains or
other OSError cases remain best-effort and continue through normal re-discovery
rather than failing the run.
In `@packages/nemo_evaluator_sdk/tests/agent_eval/test_harbor_runtime.py`:
- Around line 319-321: Remove the async declaration and `@pytest.mark.asyncio`
decorator from test_changed_inputs_invalidate_the_cache,
test_cosmetic_options_do_not_evict_the_cache, and
test_task_subset_of_a_cached_run_still_hits, since each test uses only
synchronous helpers and does not await anything.
In `@plugins/nemo-experimentalist/docs/architecture.md`:
- Around line 582-593: Keep architecture.md focused on explanation by removing
the embedded “Running and debugging it” prerequisites and command instructions,
then move them into a linked HOW-TO document. Add an appropriate cross-link to
that HOW-TO and a “Next Steps” section at the end of the architecture page,
linking to relevant follow-up documentation.
- Around line 582-593: Apply Diataxis structure consistently: in
plugins/nemo-experimentalist/docs/architecture.md lines 582-593, keep the page
explanatory by moving run/debug commands to a linked HOW-TO and adding a Next
Steps section; in plugins/nemo-experimentalist/examples/README.md lines 4-14,
label the page as REFERENCE, add prerequisites, and add Next Steps; in
plugins/nemo-experimentalist/examples/hello-harbor-agent/README.md lines 96-117,
retain a single TUTORIAL/HOW-TO scope and move reference content to linked
pages.
In
`@plugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/dataset/template/task_template/environment/runtime-server/Dockerfile`:
- Around line 11-16: Update the fastmcp dependency in the Dockerfile install
command to use an exact version pin rather than the open-ended “>=3.0”
constraint, while leaving the other package installations unchanged.
In
`@plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/harbor_agent_task_runner.py`:
- Line 112: Update the artifacts field in the relevant task-runner configuration
model to use default_factory=list instead of a mutable list literal default,
matching HarborRuntimeConfig and ensuring each instance receives its own list.
- Around line 51-59: Document the intentional TYPE_CHECKING-only imports in the
module or nearby code, explicitly recording the Harbor SDK lazy-import
requirement as an exemption to the normal concrete-type import guideline.
Preserve the existing HarborAgentTaskRunner, HarborRuntimeConfig, and
AgentEvalTask annotations and lazy-import behavior.
🪄 Autofix (Beta)
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: 3b7ee668-add0-46c3-8b43-e941f15917c9
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (74)
packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/harbor_runtime.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_harbor_runtime.pyplugins/nemo-experimentalist/README.mdplugins/nemo-experimentalist/docs/architecture.mdplugins/nemo-experimentalist/docs/e2e/README.mdplugins/nemo-experimentalist/docs/e2e/experiment-debug-round.yamlplugins/nemo-experimentalist/docs/e2e/experiment-eval-only-sdk.yamlplugins/nemo-experimentalist/docs/e2e/experiment-eval-only.yamlplugins/nemo-experimentalist/docs/e2e/experiment-fast.yamlplugins/nemo-experimentalist/docs/e2e/run-eval-only.pyplugins/nemo-experimentalist/examples/README.mdplugins/nemo-experimentalist/examples/hello-harbor-agent/AGENT-SPEC.mdplugins/nemo-experimentalist/examples/hello-harbor-agent/README.mdplugins/nemo-experimentalist/examples/hello-harbor-agent/agent.pyplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/task-template/README.mdplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/task-template/environment/Dockerfileplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/task-template/instruction.mdplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/task-template/task.tomlplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/task-template/tests/expected.txtplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/task-template/tests/test.shplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/greet-world/environment/Dockerfileplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/greet-world/instruction.mdplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/greet-world/task.tomlplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/greet-world/tests/expected.txtplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/greet-world/tests/test.shplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/sum-two/environment/Dockerfileplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/sum-two/instruction.mdplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/sum-two/task.tomlplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/sum-two/tests/expected.txtplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/sum-two/tests/test.shplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/greet-universe/environment/Dockerfileplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/greet-universe/instruction.mdplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/greet-universe/task.tomlplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/greet-universe/tests/expected.txtplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/greet-universe/tests/test.shplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/sum-three/environment/Dockerfileplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/sum-three/instruction.mdplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/sum-three/task.tomlplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/sum-three/tests/expected.txtplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/sum-three/tests/test.shplugins/nemo-experimentalist/examples/hello-harbor-agent/harbor_wrapper.pyplugins/nemo-experimentalist/examples/hello-harbor-agent/main.pyplugins/nemo-experimentalist/examples/hello-harbor-agent/optimizer.yamlplugins/nemo-experimentalist/examples/hello-harbor-agent/tracing.pyplugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/AGENT-SPEC.mdplugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/README.mdplugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/agent.pyplugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/dataset/template/task_template/environment/Dockerfileplugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/dataset/template/task_template/environment/docker-compose.yamlplugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/dataset/template/task_template/environment/runtime-server/Dockerfileplugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/dataset/template/task_template/environment/runtime-server/server.pyplugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/dataset/template/task_template/environment/runtime-server/task_config.jsonplugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/dataset/template/task_template/instruction.mdplugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/dataset/template/task_template/task.tomlplugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/dataset/template/task_template/tests/test.shplugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/harbor_wrapper.pyplugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/main.pyplugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/optimizer.yamlplugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/pyproject.tomlplugins/nemo-experimentalist/pyproject.tomlplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/eval_author/config.yamlplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/eval_author/run.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/base.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/factory.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/harbor.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/harbor_agent_task_runner.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/deps.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/run.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/resolve.pyplugins/nemo-experimentalist/tests/experimentalist/conftest.pyplugins/nemo-experimentalist/tests/experimentalist/test_evaluator_harbor_ab_e2e.pyplugins/nemo-experimentalist/tests/experimentalist/test_evaluator_harbor_agent_task_runner.pyplugins/nemo-experimentalist/tests/experimentalist/test_hello_example_baseline.pyplugins/nemo-experimentalist/tests/test_eval_author_run.py
🚧 Files skipped from review as they are similar to previous changes (44)
- plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/sum-three/tests/expected.txt
- plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/task-template/task.toml
- plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/sum-two/tests/expected.txt
- plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/greet-universe/task.toml
- plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/task-template/tests/expected.txt
- plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/task-template/README.md
- plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/sum-two/task.toml
- plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/eval_author/run.py
- plugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/dataset/template/task_template/environment/runtime-server/task_config.json
- plugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/pyproject.toml
- plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/greet-world/tests/expected.txt
- plugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/README.md
- plugins/nemo-experimentalist/docs/e2e/README.md
- plugins/nemo-experimentalist/pyproject.toml
- plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/greet-world/task.toml
- plugins/nemo-experimentalist/docs/e2e/experiment-fast.yaml
- plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/eval_author/config.yaml
- plugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/main.py
- plugins/nemo-experimentalist/docs/e2e/experiment-eval-only.yaml
- plugins/nemo-experimentalist/tests/experimentalist/conftest.py
- plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/greet-world/tests/test.sh
- plugins/nemo-experimentalist/examples/hello-harbor-agent/optimizer.yaml
- plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/resolve.py
- plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/greet-universe/tests/expected.txt
- plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/factory.py
- plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/base.py
- plugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/optimizer.yaml
- plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/sum-three/tests/test.sh
- plugins/nemo-experimentalist/tests/test_eval_author_run.py
- plugins/nemo-experimentalist/examples/hello-harbor-agent/main.py
- plugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/dataset/template/task_template/task.toml
- plugins/nemo-experimentalist/docs/e2e/experiment-debug-round.yaml
- plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/deps.py
- plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/sum-three/task.toml
- plugins/nemo-experimentalist/examples/tau2-nemo-oo-agent/dataset/template/task_template/environment/docker-compose.yaml
- plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/task-template/tests/test.sh
- plugins/nemo-experimentalist/examples/hello-harbor-agent/agent.py
- plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/greet-universe/tests/test.sh
- plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/run.py
- plugins/nemo-experimentalist/tests/experimentalist/test_hello_example_baseline.py
- plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/sum-two/tests/test.sh
- plugins/nemo-experimentalist/README.md
- plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/harbor.py
- plugins/nemo-experimentalist/examples/hello-harbor-agent/harbor_wrapper.py
SandyChapman
left a comment
There was a problem hiding this comment.
I've got some questions around using AgentEvaluator and duplication of models in the plugin.
| # The returned SDK trials are discarded: they expose only a single | ||
| # `harbor_reward.reward`, while the loop needs every verifier metric. The | ||
| # job dir is the richer contract, so both evaluators read that instead. | ||
| sdk_trials = await runner.run_tasks(list(sdk_tasks.values())) |
There was a problem hiding this comment.
Why invoke run_tasks directly instead of using AgentEvaluator?
There was a problem hiding this comment.
we don't need to score trials. Harbor does it. trials_from_job_dir gets all the data it needs for both native_harbor and evaluator_harbor. If we make it to score, this adds computation that is not used later to construct experimentalist's TrialResult. The source of truth for both harbor's is the job results dir content. If/when native_harbor is removed, then we can switch that to AgentEvalResult ---> TrialResult
There was a problem hiding this comment.
I think the point is to give a consistent API contract based on Evaluator's conventions rather than require experimentalist to build it's own data model for evaluation. E.g. Experimentalist should use AgentEvalResult and TrialResult removed. We can pick this up in standup/parking lot today.
There was a problem hiding this comment.
Yes, it makes sense, but we will need to resolve the gaps first in our models as discussed above: #955 (comment) + P3 section gaps https://docs.google.com/document/d/1Fuqm2KkbSWHYuuGNw3uHwOKfRNCBO0NzULuYWnoClFg/edit?tab=t.7gayh93h0pc2#heading=h.28u730wv2tvv before we can replace the contract
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/harbor_runtime.py`:
- Around line 931-934: Update the package identity construction near
_digest_directory and _safe_identifier to include a digest of
agent_dir.resolve() alongside the content digest, so equal-content directories
with the same basename remain distinct. Preserve the existing import-root and
identifier format as appropriate, and add a test covering overlapping
installations of same-basename agent directories that verifies each imports from
its own resolved path.
In `@packages/nemo_evaluator_sdk/tests/agent_eval/test_harbor_runtime_e2e.py`:
- Around line 123-136: Make the resume test distinguishable from a full rerun by
preserving an observable artifact in the existing job directory. Add a marker
file before the second run and assert it still exists afterward, or retain one
completed trial while deleting another result and verify the retained attempt is
unchanged. Update the test around run_harbor_eval and the resume-job setup
without changing the expected resumed trial assertions.
🪄 Autofix (Beta)
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: 68d2c23b-82d5-4a0a-b169-87bdcb3bb919
⛔ Files ignored due to path filters (1)
sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/agent_eval/runtimes/harbor_runtime.pyis excluded by!sdk/**
📒 Files selected for processing (3)
packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/harbor_runtime.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_harbor_runtime.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_harbor_runtime_e2e.py
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
plugins/nemo-experimentalist/docs/architecture.md (1)
688-728: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the runnable procedure to a HOW-TO page.
This task-specific smoke-test procedure mixes HOW-TO content into the architecture EXPLANATION. Extract it to a dedicated E2E how-to and retain a short cross-link here. As per coding guidelines, “Each documentation page should fit ONE Diataxis quadrant.”
🤖 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-experimentalist/docs/architecture.md` around lines 688 - 728, Move the “Verifying without credentials” runnable procedure and its code sample from the architecture explanation into a dedicated E2E HOW-TO page, preserving the instructions and references there. Replace the removed section in architecture.md with a brief cross-link to the new how-to page, keeping the architecture page focused on explanation.Source: Coding guidelines
🤖 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-experimentalist/docs/architecture.md`:
- Around line 294-296: Update the architecture documentation to remove the claim
that an unavailable SDK falls back to harbor. Document harbor_native as the A/B
baseline and harbor only as a deprecated alias, unless the factory import flow
is changed to implement a genuine fallback.
---
Nitpick comments:
In `@plugins/nemo-experimentalist/docs/architecture.md`:
- Around line 688-728: Move the “Verifying without credentials” runnable
procedure and its code sample from the architecture explanation into a dedicated
E2E HOW-TO page, preserving the instructions and references there. Replace the
removed section in architecture.md with a brief cross-link to the new how-to
page, keeping the architecture page focused on explanation.
🪄 Autofix (Beta)
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: b7122eb6-5abb-4a23-8448-a208c5d8f478
📒 Files selected for processing (21)
plugins/nemo-experimentalist/docs/architecture.mdplugins/nemo-experimentalist/docs/e2e/README.mdplugins/nemo-experimentalist/docs/e2e/experiment-eval-only-sdk.yamlplugins/nemo-experimentalist/docs/e2e/experiment-eval-only.yamlplugins/nemo-experimentalist/docs/e2e/run-eval-only.pyplugins/nemo-experimentalist/examples/hello-harbor-agent/README.mdplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/eval_author/config.yamlplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/eval_author/run.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/base.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/factory.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/harbor.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/harbor_evaluator.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/deps.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/resolve.pyplugins/nemo-experimentalist/tests/experimentalist/test_dataset_staging.pyplugins/nemo-experimentalist/tests/experimentalist/test_evaluator_factory.pyplugins/nemo-experimentalist/tests/experimentalist/test_evaluator_harbor_ab_e2e.pyplugins/nemo-experimentalist/tests/experimentalist/test_evaluator_harbor_evaluator.pyplugins/nemo-experimentalist/tests/experimentalist/test_loop_failure.pyplugins/nemo-experimentalist/tests/experimentalist/test_loop_insight_suite.pyplugins/nemo-experimentalist/tests/test_eval_author_run.py
🚧 Files skipped from review as they are similar to previous changes (2)
- plugins/nemo-experimentalist/docs/e2e/README.md
- plugins/nemo-experimentalist/examples/hello-harbor-agent/README.md
Three findings from CodeRabbit on PR #955. * Scoped agent import: `_install_agent_package` wrote `__path__` unconditionally, so a second scope sharing a package name silently repointed the first, still-open scope at a different directory. Keep the first path instead. CodeRabbit proposed hashing the resolved directory path into the package identity. Rejected: that makes the name location-dependent, so the same agent evaluated from a different path yields a different JobConfig and Harbor refuses to resume -- reintroducing AALGO-430, which this branch exists to fix. A shared package name implies an equal content digest, hence byte-identical trees (the excluded content is not importable), so the already-installed path is exactly as correct and keeping it is the safe resolution. * Resume e2e: deleting the only trial result made "discarded and re-run from scratch" observationally identical to "resumed", so the test passed either way. Now runs two attempts, drops one, and asserts the surviving trial directory keeps its name and its result.json byte-for-byte -- Harbor suffixes trial dirs with a shortuuid, so a discarded job dir returns under a different name. Verified by mutation: forcing force_rerun=True now fails the test. * architecture.md no longer promises `harbor` as an SDK-unavailable fallback -- false since the SDK import moved to module scope. CodeRabbit flagged one paragraph; the adjacent "import optional runtimes lazily inside `_run`" guidance was wrong for the same reason and is corrected too. Uses `harbor_native` per the rename, with `harbor` named only as the deprecated alias. Refs AALGO-430, AALGO-312. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Nick Goncharenko <ngoncharenko@nvidia.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/harbor_runtime.py (1)
299-320: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
beforestamp computed even when caching is disabled.
before = _cache_stamp(self._config, dataset_path, coverage)(and thecoveragewalk feeding it) runs unconditionally whenever a native job is (re)built — including whenself._config.job_name is None. Only theaftercomparison/write is gated onjob_name is not None(line 309). This directly contradicts the stated intent a few lines above ("This keeps the digest I/O off every run of callers that don't pin a job name") — every unpinned run now pays for a full agent/dataset content digest with no use for the result.🔧 Proposed fix
- coverage = _stamp_coverage(dataset_path, tasks, self._task_names) - before = _cache_stamp(self._config, dataset_path, coverage) + before = None + if self._config.job_name is not None: + coverage = _stamp_coverage(dataset_path, tasks, self._task_names) + before = _cache_stamp(self._config, dataset_path, coverage) await run_job() if self._config.job_name is not None: after = _cache_stamp(self._config, dataset_path, coverage)🤖 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/harbor_runtime.py` around lines 299 - 320, Gate the cache fingerprinting flow in the native job execution path on self._config.job_name being set: only compute coverage and before, run the before/after comparison, and write the cache stamp for pinned jobs. Keep unpinned jobs from performing _stamp_coverage or _cache_stamp while preserving the existing Harbor execution behavior.
🤖 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.
Outside diff comments:
In
`@packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/harbor_runtime.py`:
- Around line 299-320: Gate the cache fingerprinting flow in the native job
execution path on self._config.job_name being set: only compute coverage and
before, run the before/after comparison, and write the cache stamp for pinned
jobs. Keep unpinned jobs from performing _stamp_coverage or _cache_stamp while
preserving the existing Harbor execution behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e2947179-02b4-40e3-baec-c2f4fb93cf39
⛔ Files ignored due to path filters (1)
sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/agent_eval/runtimes/harbor_runtime.pyis excluded by!sdk/**
📒 Files selected for processing (4)
packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/harbor_runtime.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_harbor_runtime.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_harbor_runtime_e2e.pyplugins/nemo-experimentalist/docs/architecture.md
🚧 Files skipped from review as they are similar to previous changes (1)
- plugins/nemo-experimentalist/docs/architecture.md
7df6bd9 to
2ac8e19
Compare
SandyChapman
left a comment
There was a problem hiding this comment.
based on our convo in standup the approach is:
- keep the existing experimentalist models; mark them deprecated (or somehow signal that we should be informed if changes are being made)
- merge this and close any feature gaps with the harbor runner
- perform UAT testing with the experimentalist team
- cut over to the new evaluator-harbor runner
- migrate experimentalist to the evaluator models
2ac8e19 to
09ae12e
Compare
4e3acc3 to
a41f3c9
Compare
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Nitpick comments (9)
plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/harbor.py (1)
791-797: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDerive
job_namefrom the resolved path.
agent.nameis taken from the unresolved argument, soPath(".")or a trailing..yields an empty/garbage job name (and hence job dir + cache key), whileagent_pathright above is already normalized.♻️ Suggested change
- job_name=options.job_name or f"{agent.name}-{dataset.id}", + job_name=options.job_name or f"{agent_path.name}-{dataset.id}",🤖 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-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/harbor.py` around lines 791 - 797, Update the job_name fallback in the HarborRunInputs construction to derive the agent identifier from the already-resolved agent_path rather than the unresolved agent.name argument. Preserve options.job_name when provided, and keep the existing dataset-based fallback format unchanged.plugins/nemo-experimentalist/tests/test_deps.py (1)
59-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin the error to
evaluator_type.A bare
ValidationErroralso passes if the agent URI or datasets stop validating, so the test could go green without the alias ever being rejected.♻️ Suggested tightening
- with pytest.raises(ValidationError): + with pytest.raises(ValidationError, match="harbor"):🤖 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-experimentalist/tests/test_deps.py` around lines 59 - 67, Update test_deprecated_evaluator_alias_is_rejected to assert that the ValidationError specifically identifies evaluator_type as the failing field, while preserving the existing invalid alias and valid agent/dataset setup so the test cannot pass due to unrelated validation failures.packages/nemo_evaluator_sdk/tests/agent_eval/test_harbor_runtime.py (1)
331-332: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop
async/asynciofrom these three pure-sync tests.
test_changed_inputs_invalidate_the_cache,test_cosmetic_options_do_not_evict_the_cache, andtest_task_subset_of_a_cached_run_still_hitsnever await; the event-loop plumbing is noise.Also applies to: 355-356, 368-369
🤖 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_harbor_runtime.py` around lines 331 - 332, Remove the async declaration from test_changed_inputs_invalidate_the_cache, test_cosmetic_options_do_not_evict_the_cache, and test_task_subset_of_a_cached_run_still_hits, and remove any corresponding asyncio/event-loop plumbing in these tests. Keep their existing synchronous assertions and behavior unchanged.packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/harbor_runtime.py (1)
503-550: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueUse
_safe_resolvefor the candidate/dataset paths.
resolve()here can raiseOSErroron a symlink that vanishes mid-check, which would kill a run that the rest of the fingerprint path deliberately degrades instead of failing.♻️ Suggested change
- dataset_root = dataset_path.resolve() + dataset_root = _safe_resolve(dataset_path) @@ - candidate_resolved = candidate.resolve() + candidate_resolved = _safe_resolve(candidate)🤖 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/harbor_runtime.py` around lines 503 - 550, Update _task_dirs_for to use the existing _safe_resolve helper for both the stamped candidate path and the active dataset_root instead of calling Path.resolve() directly. Preserve the current directory and relative-to-dataset validation, and ensure transient resolution failures degrade to the existing unresolved/uncacheable behavior rather than propagating OSError.plugins/nemo-experimentalist/examples/tau2-nooa-oo-agent/dataset/template/task_template/environment/runtime-server/server.py (2)
12-12: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
collections.abc.Callableover the deprecatedtyping.Callable.🤖 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-experimentalist/examples/tau2-nooa-oo-agent/dataset/template/task_template/environment/runtime-server/server.py` at line 12, Update the Callable import in server.py to use collections.abc.Callable instead of typing.Callable, while retaining Any from typing.
179-183: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSilent
except Exceptionhides tool-loading failures.If
get_user_toolsfails, the user simulator runs without tools and the task fails in a way that's impossible to diagnose from the state log. Log the exception at minimum.Proposed fix
- user_tools = None try: user_tools = self.environment.get_user_tools(include=self.task.user_tools) or None - except Exception: + except Exception: # noqa: BLE001 - domain envs raise heterogeneous errors + print(f"warning: user tools unavailable for domain {self.domain}", file=sys.stderr) user_tools = None🤖 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-experimentalist/examples/tau2-nooa-oo-agent/dataset/template/task_template/environment/runtime-server/server.py` around lines 179 - 183, Update the get_user_tools error handling in the runtime server to log the caught exception with sufficient context before setting user_tools to None, while preserving the existing fallback behavior.plugins/nemo-experimentalist/examples/tau2-nooa-oo-agent/harbor_wrapper.py (1)
130-141: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove
import asyncioto module scope.Proposed fix
- import asyncio as _asyncio - proc = None for _attempt in range(3): @@ if _attempt < 2: - await _asyncio.sleep(10) + await asyncio.sleep(10)Add
import asyncioat the top of the file.🤖 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-experimentalist/examples/tau2-nooa-oo-agent/harbor_wrapper.py` around lines 130 - 141, Move the asyncio import from inside the setup retry flow to module scope at the top of harbor_wrapper.py, then update the retry loop to use the module-level asyncio reference while preserving the existing sleep behavior.plugins/nemo-experimentalist/examples/tau2-nooa-oo-agent/AGENT-SPEC.md (1)
19-19: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFix markdownlint failures. Add a blank line after the headings on lines 19, 33, 49, 154 (MD022) and end the file with a newline (MD047).
Also applies to: 33-33, 49-49, 154-154, 167-167
🤖 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-experimentalist/examples/tau2-nooa-oo-agent/AGENT-SPEC.md` at line 19, Add blank lines immediately after the headings at the affected locations in AGENT-SPEC.md, including the heading near line 167, and ensure the file ends with a trailing newline. Preserve all existing content and structure.Source: Linters/SAST tools
plugins/nemo-experimentalist/examples/tau2-nooa-oo-agent/agent.py (1)
20-24: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueMissing
INFERENCE_API_KEYfails late.
api_key=Nonesurfaces as an opaque provider auth error mid-run. Fail fast at construction with a clear message.🤖 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-experimentalist/examples/tau2-nooa-oo-agent/agent.py` around lines 20 - 24, Validate that INFERENCE_API_KEY is present before constructing CompletionClient, and fail immediately with a clear configuration error when it is missing. Keep the existing model and INFERENCE_BASE_URL defaults unchanged, and pass the validated key to CompletionClient.
🤖 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-experimentalist/docs/architecture.md`:
- Around line 594-598: Replace NVIDIA_INFERENCE_HUB_KEY with INFERENCE_API_KEY
in the setup commands at plugins/nemo-experimentalist/docs/architecture.md lines
594-598, plugins/nemo-experimentalist/examples/hello-harbor-agent/README.md
lines 84-87, and lines 185-187 so all documented flows provide the credential
consumed by the CLI; alternatively document the required mapping explicitly at
each site.
- Around line 291-296: Update the evaluator-selection documentation to identify
harbor_evaluator as the default SDK-backed evaluator and harbor_native as the
A/B baseline or fallback, preserving the deprecated harbor alias behavior. Apply
this contract correction in plugins/nemo-experimentalist/docs/architecture.md
lines 291-296 and update the evaluator comparison table in
plugins/nemo-experimentalist/examples/hello-harbor-agent/README.md lines
100-103; no runtime changes are needed.
In `@plugins/nemo-experimentalist/docs/e2e/run-eval-only.py`:
- Around line 32-35: Align the evaluator default and documentation with the
SDK-default contract: in plugins/nemo-experimentalist/docs/e2e/run-eval-only.py
lines 32-35, change the evaluator default to harbor_evaluator; in
plugins/nemo-experimentalist/docs/e2e/experiment-eval-only-sdk.yaml lines 21-22,
identify harbor_evaluator as the default SDK arm; in
plugins/nemo-experimentalist/docs/e2e/experiment-eval-only.yaml lines 22-24,
identify harbor_native as the explicit baseline; and in
plugins/nemo-experimentalist/tests/test_e2e_helper_defaults.py lines 13-20,
rename the test and expect harbor_evaluator.
In
`@plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/task-template/tests/test.sh`:
- Around line 41-47: Update the comparison logic in the test script around
EXPECTED and ACTUAL so command substitution does not remove trailing newlines.
Normalize CRLF line endings while preserving each file’s complete contents, then
compare those contents directly so outputs with missing or extra trailing
newlines are scored differently.
In `@plugins/nemo-experimentalist/examples/hello-harbor-agent/harbor_wrapper.py`:
- Around line 62-69: Update the upload loop around AGENT_DIR.iterdir() to
explicitly reject symlink entries before calling entry.is_file() or
entry.is_dir(), and ensure recursive directory uploads also reject symlinks at
every level rather than following them. Preserve the existing EXCLUDE filtering
and upload destinations for regular files and directories.
In `@plugins/nemo-experimentalist/examples/tau2-nooa-oo-agent/AGENT-SPEC.md`:
- Around line 119-121: Correct the grammar in the cabin-change pricing rule by
replacing “the user is should be refunded” with “the user should be refunded,”
while preserving the stated refund behavior.
- Line 1: Add the standard NVIDIA CORPORATION & AFFILIATES Apache-2.0 SPDX
header as an HTML comment at the beginning of AGENT-SPEC.md, matching the format
used by the directory’s README.md.
In `@plugins/nemo-experimentalist/examples/tau2-nooa-oo-agent/agent.py`:
- Line 6: Resolve the unused requests dependency in the example module: either
remove the requests import if it is not needed, or declare requests in the
example manifest so clean environments can import the module successfully.
Update the import or manifest associated with the agent example accordingly.
In
`@plugins/nemo-experimentalist/examples/tau2-nooa-oo-agent/dataset/template/task_template/environment/Dockerfile`:
- Line 1: Prepend the standard NVIDIA copyright and Apache-2.0 SPDX comment
lines to
plugins/nemo-experimentalist/examples/tau2-nooa-oo-agent/dataset/template/task_template/environment/Dockerfile
lines 1-1, runtime-server/Dockerfile lines 1-1, and docker-compose.yaml lines
1-1; place the headers above the existing FROM or services declarations.
- Around line 10-14: Pin the cloned tau2-bench revision in both Dockerfiles: add
a TAU2_BENCH_REV build argument, check out that revision after cloning in the
task environment Dockerfile, and apply the same argument and checkout flow in
the runtime-server Dockerfile so both images use one reproducible version.
In
`@plugins/nemo-experimentalist/examples/tau2-nooa-oo-agent/dataset/template/task_template/environment/runtime-server/Dockerfile`:
- Line 14: Update the fastmcp dependency in the Dockerfile’s pip install command
to require the repository-supported baseline of fastmcp>=3.2.0 and constrain it
below version 4, preserving the existing installation extras and other
dependencies.
In
`@plugins/nemo-experimentalist/examples/tau2-nooa-oo-agent/dataset/template/task_template/environment/runtime-server/server.py`:
- Line 560: Protect the shared Tau3Runtime instance and its mutable state by
adding a synchronization lock around every MCP tool entry point that accesses or
mutates runtime, including the tools near the global initialization and lines
627-632. Ensure each complete tool call, including state-file updates and
step/error accounting, executes single-flight so concurrent requests cannot
interleave.
- Around line 251-266: Update _write_state to serialize the payload to a
temporary file in STATE_LOG_PATH.parent, then atomically replace STATE_LOG_PATH
with os.replace after the write completes. Ensure the temporary file uses the
same directory and is cleaned up if writing or replacement fails.
In
`@plugins/nemo-experimentalist/examples/tau2-nooa-oo-agent/dataset/template/task_template/task.toml`:
- Line 1: Restore the SPDX license and copyright headers across
task_template/task.toml, task_template/instruction.md, both Dockerfiles,
task_template/docker-compose.yaml,
task_template/environment/runtime-server/task_config.json, and
task_template/tests/test.sh. For task_config.json, add the SPDX metadata as
top-level JSON properties rather than comments so json.loads remains valid; use
the repository’s standard header format for each other file.
In `@plugins/nemo-experimentalist/examples/tau2-nooa-oo-agent/optimizer.yaml`:
- Line 17: Update the registry_url in the tau2 optimizer configuration to either
reference a publicly accessible, redistributable registry or clearly document
the NVIDIA-internal access requirement in the surrounding comment. Preserve the
example’s dataset configuration while ensuring users understand or can satisfy
the registry access prerequisite.
In `@plugins/nemo-experimentalist/examples/tau2-nooa-oo-agent/pyproject.toml`:
- Around line 16-17: Update the nooa source entry in the pyproject configuration
to use an immutable commit SHA via rev instead of the movable v0.0.6 tag, and
synchronize the same revision with the main nooa pin, examples/tau3-nooa-agent
pin, and both lockfiles.
In
`@plugins/nemo-experimentalist/tests/experimentalist/test_hello_example_baseline.py`:
- Around line 109-112: Update the baseline test around `_EXPECTED_BASELINE` to
enumerate each split’s on-disk task directory and assert its task IDs exactly
match the expected IDs before constructing `rewards`. Keep the existing
reward-shape assertion, ensuring extra or missing tasks cannot pass unnoticed.
---
Nitpick comments:
In
`@packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/harbor_runtime.py`:
- Around line 503-550: Update _task_dirs_for to use the existing _safe_resolve
helper for both the stamped candidate path and the active dataset_root instead
of calling Path.resolve() directly. Preserve the current directory and
relative-to-dataset validation, and ensure transient resolution failures degrade
to the existing unresolved/uncacheable behavior rather than propagating OSError.
In `@packages/nemo_evaluator_sdk/tests/agent_eval/test_harbor_runtime.py`:
- Around line 331-332: Remove the async declaration from
test_changed_inputs_invalidate_the_cache,
test_cosmetic_options_do_not_evict_the_cache, and
test_task_subset_of_a_cached_run_still_hits, and remove any corresponding
asyncio/event-loop plumbing in these tests. Keep their existing synchronous
assertions and behavior unchanged.
In `@plugins/nemo-experimentalist/examples/tau2-nooa-oo-agent/AGENT-SPEC.md`:
- Line 19: Add blank lines immediately after the headings at the affected
locations in AGENT-SPEC.md, including the heading near line 167, and ensure the
file ends with a trailing newline. Preserve all existing content and structure.
In `@plugins/nemo-experimentalist/examples/tau2-nooa-oo-agent/agent.py`:
- Around line 20-24: Validate that INFERENCE_API_KEY is present before
constructing CompletionClient, and fail immediately with a clear configuration
error when it is missing. Keep the existing model and INFERENCE_BASE_URL
defaults unchanged, and pass the validated key to CompletionClient.
In
`@plugins/nemo-experimentalist/examples/tau2-nooa-oo-agent/dataset/template/task_template/environment/runtime-server/server.py`:
- Line 12: Update the Callable import in server.py to use
collections.abc.Callable instead of typing.Callable, while retaining Any from
typing.
- Around line 179-183: Update the get_user_tools error handling in the runtime
server to log the caught exception with sufficient context before setting
user_tools to None, while preserving the existing fallback behavior.
In `@plugins/nemo-experimentalist/examples/tau2-nooa-oo-agent/harbor_wrapper.py`:
- Around line 130-141: Move the asyncio import from inside the setup retry flow
to module scope at the top of harbor_wrapper.py, then update the retry loop to
use the module-level asyncio reference while preserving the existing sleep
behavior.
In
`@plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/harbor.py`:
- Around line 791-797: Update the job_name fallback in the HarborRunInputs
construction to derive the agent identifier from the already-resolved agent_path
rather than the unresolved agent.name argument. Preserve options.job_name when
provided, and keep the existing dataset-based fallback format unchanged.
In `@plugins/nemo-experimentalist/tests/test_deps.py`:
- Around line 59-67: Update test_deprecated_evaluator_alias_is_rejected to
assert that the ValidationError specifically identifies evaluator_type as the
failing field, while preserving the existing invalid alias and valid
agent/dataset setup so the test cannot pass due to unrelated validation
failures.
🪄 Autofix (Beta)
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: 2353747e-a71e-4e74-8d4e-9d0fdae8f953
⛔ Files ignored due to path filters (1)
sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/agent_eval/runtimes/harbor_runtime.pyis excluded by!sdk/**
📒 Files selected for processing (84)
packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/harbor_runtime.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_harbor_runtime.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_harbor_runtime_e2e.pyplugins/nemo-eval-author/src/nemo_eval_author_plugin/eval_author/config.yamlplugins/nemo-eval-author/src/nemo_eval_author_plugin/eval_author/run.pyplugins/nemo-eval-author/tests/test_eval_author_run.pyplugins/nemo-experimentalist/README.mdplugins/nemo-experimentalist/docs/architecture.mdplugins/nemo-experimentalist/docs/e2e/README.mdplugins/nemo-experimentalist/docs/e2e/experiment-debug-round.yamlplugins/nemo-experimentalist/docs/e2e/experiment-eval-only-sdk.yamlplugins/nemo-experimentalist/docs/e2e/experiment-eval-only.yamlplugins/nemo-experimentalist/docs/e2e/experiment-fast.yamlplugins/nemo-experimentalist/docs/e2e/run-eval-only.pyplugins/nemo-experimentalist/examples/README.mdplugins/nemo-experimentalist/examples/hello-harbor-agent/.env.exampleplugins/nemo-experimentalist/examples/hello-harbor-agent/AGENT-SPEC.mdplugins/nemo-experimentalist/examples/hello-harbor-agent/README.mdplugins/nemo-experimentalist/examples/hello-harbor-agent/agent.pyplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/task-template/README.mdplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/task-template/environment/Dockerfileplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/task-template/instruction.mdplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/task-template/task.tomlplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/task-template/tests/expected.txtplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/task-template/tests/test.shplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/greet-world/environment/Dockerfileplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/greet-world/instruction.mdplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/greet-world/task.tomlplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/greet-world/tests/expected.txtplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/greet-world/tests/test.shplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/sum-two/environment/Dockerfileplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/sum-two/instruction.mdplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/sum-two/task.tomlplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/sum-two/tests/expected.txtplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/sum-two/tests/test.shplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/greet-universe/environment/Dockerfileplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/greet-universe/instruction.mdplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/greet-universe/task.tomlplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/greet-universe/tests/expected.txtplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/greet-universe/tests/test.shplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/sum-three/environment/Dockerfileplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/sum-three/instruction.mdplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/sum-three/task.tomlplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/sum-three/tests/expected.txtplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/sum-three/tests/test.shplugins/nemo-experimentalist/examples/hello-harbor-agent/harbor_wrapper.pyplugins/nemo-experimentalist/examples/hello-harbor-agent/main.pyplugins/nemo-experimentalist/examples/hello-harbor-agent/optimizer.yamlplugins/nemo-experimentalist/examples/hello-harbor-agent/tracing.pyplugins/nemo-experimentalist/examples/tau2-nooa-oo-agent/.env.exampleplugins/nemo-experimentalist/examples/tau2-nooa-oo-agent/AGENT-SPEC.mdplugins/nemo-experimentalist/examples/tau2-nooa-oo-agent/README.mdplugins/nemo-experimentalist/examples/tau2-nooa-oo-agent/agent.pyplugins/nemo-experimentalist/examples/tau2-nooa-oo-agent/dataset/template/task_template/environment/Dockerfileplugins/nemo-experimentalist/examples/tau2-nooa-oo-agent/dataset/template/task_template/environment/docker-compose.yamlplugins/nemo-experimentalist/examples/tau2-nooa-oo-agent/dataset/template/task_template/environment/runtime-server/Dockerfileplugins/nemo-experimentalist/examples/tau2-nooa-oo-agent/dataset/template/task_template/environment/runtime-server/server.pyplugins/nemo-experimentalist/examples/tau2-nooa-oo-agent/dataset/template/task_template/environment/runtime-server/task_config.jsonplugins/nemo-experimentalist/examples/tau2-nooa-oo-agent/dataset/template/task_template/instruction.mdplugins/nemo-experimentalist/examples/tau2-nooa-oo-agent/dataset/template/task_template/task.tomlplugins/nemo-experimentalist/examples/tau2-nooa-oo-agent/dataset/template/task_template/tests/test.shplugins/nemo-experimentalist/examples/tau2-nooa-oo-agent/harbor_wrapper.pyplugins/nemo-experimentalist/examples/tau2-nooa-oo-agent/main.pyplugins/nemo-experimentalist/examples/tau2-nooa-oo-agent/optimizer.yamlplugins/nemo-experimentalist/examples/tau2-nooa-oo-agent/pyproject.tomlplugins/nemo-experimentalist/pyproject.tomlplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/base.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/factory.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/harbor.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/harbor_evaluator.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/deps.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/run.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/preflight.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/resolve.pyplugins/nemo-experimentalist/tests/experimentalist/conftest.pyplugins/nemo-experimentalist/tests/experimentalist/test_dataset_staging_loop.pyplugins/nemo-experimentalist/tests/experimentalist/test_evaluator_factory.pyplugins/nemo-experimentalist/tests/experimentalist/test_evaluator_harbor_ab_e2e.pyplugins/nemo-experimentalist/tests/experimentalist/test_evaluator_harbor_evaluator.pyplugins/nemo-experimentalist/tests/experimentalist/test_hello_example_baseline.pyplugins/nemo-experimentalist/tests/experimentalist/test_loop_failure.pyplugins/nemo-experimentalist/tests/experimentalist/test_loop_insight_suite.pyplugins/nemo-experimentalist/tests/test_deps.pyplugins/nemo-experimentalist/tests/test_e2e_helper_defaults.py
🚧 Files skipped from review as they are similar to previous changes (6)
- plugins/nemo-eval-author/tests/test_eval_author_run.py
- plugins/nemo-eval-author/src/nemo_eval_author_plugin/eval_author/config.yaml
- plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/task-template/README.md
- plugins/nemo-experimentalist/examples/hello-harbor-agent/.env.example
- plugins/nemo-experimentalist/docs/e2e/README.md
- plugins/nemo-experimentalist/tests/experimentalist/test_loop_insight_suite.py
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
plugins/nemo-experimentalist/examples/tau2-nooa-oo-agent/dataset/template/task_template/environment/Dockerfile (1)
1-1: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStill missing SPDX header.
Prior review flagged this same file for a missing SPDX header; it's still absent at line 1.
As per coding guidelines: "Every file must include the SPDX copyright header for NVIDIA CORPORATION & AFFILIATES and the Apache-2.0 license."📝 Proposed fix
+# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 + FROM python:3.12-slim🤖 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-experimentalist/examples/tau2-nooa-oo-agent/dataset/template/task_template/environment/Dockerfile` at line 1, Add the standard SPDX copyright and Apache-2.0 license header at the beginning of the Dockerfile before the existing FROM directive. Use the repository’s established NVIDIA CORPORATION & AFFILIATES header format.Source: Coding guidelines
🧹 Nitpick comments (1)
plugins/nemo-experimentalist/examples/hello-harbor-agent/README.md (1)
96-217: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftSeparate the Diataxis content.
This section mixes HOW-TO commands, evaluator explanations, caching concepts, and configuration reference. Split these into linked pages, or retain one quadrant here and link out. As per coding guidelines, “Each documentation page should fit ONE Diataxis quadrant; do not mix tutorials with reference tables or how-tos with architecture explanations; use cross-links instead.”
🤖 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-experimentalist/examples/hello-harbor-agent/README.md` around lines 96 - 217, Separate the mixed Diataxis content around the “Running the evaluation through the NeMo Evaluator SDK” section: keep this README focused on one quadrant, and move the A/B commands, caching guidance, conceptual evaluator explanation, and configuration reference into appropriately linked pages. Add clear cross-links so readers can find the relocated how-to, explanation, and reference content without duplicating it.Source: Coding guidelines
🤖 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-experimentalist/examples/tau2-nooa-oo-agent/dataset/template/task_template/environment/runtime-server/Dockerfile`:
- Around line 6-9: Add the repository-standard SPDX copyright and license
declarations before the Dockerfile’s FROM instruction, preserving the existing
TAU2_BENCH_REV argument and surrounding comments unchanged.
---
Duplicate comments:
In
`@plugins/nemo-experimentalist/examples/tau2-nooa-oo-agent/dataset/template/task_template/environment/Dockerfile`:
- Line 1: Add the standard SPDX copyright and Apache-2.0 license header at the
beginning of the Dockerfile before the existing FROM directive. Use the
repository’s established NVIDIA CORPORATION & AFFILIATES header format.
---
Nitpick comments:
In `@plugins/nemo-experimentalist/examples/hello-harbor-agent/README.md`:
- Around line 96-217: Separate the mixed Diataxis content around the “Running
the evaluation through the NeMo Evaluator SDK” section: keep this README focused
on one quadrant, and move the A/B commands, caching guidance, conceptual
evaluator explanation, and configuration reference into appropriately linked
pages. Add clear cross-links so readers can find the relocated how-to,
explanation, and reference content without duplicating it.
🪄 Autofix (Beta)
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: dc6bc4c5-9221-4ce1-bec4-21c2f74061f5
⛔ Files ignored due to path filters (1)
sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/agent_eval/runtimes/harbor_runtime.pyis excluded by!sdk/**
📒 Files selected for processing (25)
packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/harbor_runtime.pyplugins/nemo-eval-author/src/nemo_eval_author_plugin/eval_author/config.yamlplugins/nemo-eval-author/src/nemo_eval_author_plugin/eval_author/run.pyplugins/nemo-eval-author/tests/test_eval_author_run.pyplugins/nemo-experimentalist/docs/architecture.mdplugins/nemo-experimentalist/examples/hello-harbor-agent/README.mdplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/task-template/tests/test.shplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/greet-world/tests/test.shplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/sum-two/tests/test.shplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/greet-universe/tests/test.shplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/sum-three/tests/test.shplugins/nemo-experimentalist/examples/hello-harbor-agent/harbor_wrapper.pyplugins/nemo-experimentalist/examples/tau2-nooa-oo-agent/README.mdplugins/nemo-experimentalist/examples/tau2-nooa-oo-agent/agent.pyplugins/nemo-experimentalist/examples/tau2-nooa-oo-agent/dataset/template/task_template/environment/Dockerfileplugins/nemo-experimentalist/examples/tau2-nooa-oo-agent/dataset/template/task_template/environment/docker-compose.yamlplugins/nemo-experimentalist/examples/tau2-nooa-oo-agent/dataset/template/task_template/environment/runtime-server/Dockerfileplugins/nemo-experimentalist/examples/tau2-nooa-oo-agent/dataset/template/task_template/environment/runtime-server/server.pyplugins/nemo-experimentalist/examples/tau2-nooa-oo-agent/dataset/template/task_template/environment/runtime-server/task_config.jsonplugins/nemo-experimentalist/examples/tau2-nooa-oo-agent/dataset/template/task_template/task.tomlplugins/nemo-experimentalist/examples/tau2-nooa-oo-agent/optimizer.yamlplugins/nemo-experimentalist/examples/tau2-nooa-oo-agent/pyproject.tomlplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/harbor.pyplugins/nemo-experimentalist/tests/experimentalist/test_evaluator_harbor_evaluator.pyplugins/nemo-experimentalist/tests/experimentalist/test_tau2_example_contract.py
🚧 Files skipped from review as they are similar to previous changes (8)
- plugins/nemo-experimentalist/examples/tau2-nooa-oo-agent/dataset/template/task_template/environment/runtime-server/task_config.json
- plugins/nemo-experimentalist/examples/tau2-nooa-oo-agent/dataset/template/task_template/environment/docker-compose.yaml
- plugins/nemo-experimentalist/examples/tau2-nooa-oo-agent/optimizer.yaml
- plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/harbor.py
- plugins/nemo-experimentalist/docs/architecture.md
- plugins/nemo-experimentalist/examples/tau2-nooa-oo-agent/README.md
- plugins/nemo-experimentalist/examples/tau2-nooa-oo-agent/dataset/template/task_template/task.toml
- packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/harbor_runtime.py
Signed-off-by: Nick Goncharenko <ngoncharenko@nvidia.com>
`job_name` is the cache identity: it names the Harbor job directory, which is what
the SDK's success-aware cache and Harbor's per-trial resume both key on. It was
built from the caller's spelling of the agent path while the SDK's scoped agent
import derives its package name from the *resolved* directory, so the two
identities could disagree.
Concretely: `Path(".").name` is empty, so every `--agent .` run produced job name
`-<dataset>` regardless of which directory it pointed at, and two different agents
collided on one job dir. With a symlinked agent dir the alias kept its own name
while resolving elsewhere, so flipping the link at a fixed job_name reused one job
dir for a different agent -- caught only by Harbor's config refusal rather than by
design.
`scoped_harbor_agent_import`'s docstring justified its location-sensitivity by
asserting the caller's job_name tracks the agent name too. That was only true once
both derive from the resolved path; the docstring now says so explicitly.
Migration: this changes the cache identity for pinned runs whose agent was passed
as `.` or through a symlink -- one silent re-run each. Old job dirs stay on disk.
Also in this change:
* `_task_dirs_for` uses `_safe_resolve()` for the dataset root and stamped task
paths. Bare `resolve()` raises when a symlink vanishes mid-walk, which would
fail a run over a best-effort cache guard that is designed to degrade.
* Align the Eval Author's `evaluator_type` default with the Experimentalist's
(`harbor_native`). It selects only the dataset adapter -- `run_eval_author`
never calls `build_evaluator`, and both types map to `HarborDataset` -- so this
is inert today, but it stops being inert the day the two types get different
Dataset classes. Guarded by a test so the two cannot drift.
* Assert the deprecated-alias ValidationError originates from `evaluator_type`,
not merely that some field rejected the value.
Refs AALGO-312, AALGO-430.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Nick Goncharenko <ngoncharenko@nvidia.com>
* All five verifier scripts now compare byte-for-byte with `cmp` after end-of-line CRLF normalization, instead of `[ "$ACTUAL" = "$EXPECTED" ]` over command substitution. Command substitution strips *every* trailing newline on both sides, so an agent could append blank lines and still score 1.0. The code under test is LLM-generated and the optimizer is actively maximizing this number, so a comparison that tolerates extra output is a reward-hacking surface -- the same class as the trailing-content hole the script already warned about. Missing, extra, and duplicated trailing newlines all fail now. Verified safe for the baseline: `main.py` writes `answer + "\n"` and every `expected.txt` ends in exactly one newline, so correct answers still score 1.0. End-of-line-only normalization is kept deliberately: `tr -d '\r'` would collapse `sum=4<CR>2` into `sum=42` and score a false 1.0. * `WrappedAgent.setup` rejects symlinks before uploading anything. `upload_dir` follows them, so a link anywhere in a selected subtree would copy host files into the task container -- `.env`, an SSH key, anything the exclude list names but a link bypasses. The whole selection is scanned up front so a rejection cannot leave a half-populated /app. Covers top-level and nested links. * `NVIDIA_INFERENCE_HUB_KEY` -> `INFERENCE_API_KEY` in the example README and architecture doc. The former is not read anywhere; `.env.example` and the credential-defaulting path in cli.py both use the latter. Refs AALGO-312. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Nick Goncharenko <ngoncharenko@nvidia.com>
…ime safe Build reproducibility: * Pin tau2-bench to 363133ada1936491fb5bcec33cd62c3518a99f65 in both images. `git clone --depth=1` cannot check out an arbitrary sha, so it silently tracked the default branch however the ARG was written; replaced with `git init` + `fetch --depth 1 <repo> <rev>` + `checkout --detach FETCH_HEAD`. * Raise the runtime server's FastMCP floor from >=3.0 to >=3.2.0,<4. This was not merely unbounded: root pyproject.toml already overrides FastMCP to >=3.2.0 for GHSA-vv7q-7jx5-f767 (Critical) and GHSA-rww4-4w9c-7733 (High), and pip resolves inside the image where that override cannot reach, so the container was allowed to install a version the repo bans. * Pin nooa to a commit rather than tag v0.0.6 -- tags are mutable. * Drop the unused `requests` import, and fail fast when INFERENCE_API_KEY is absent instead of letting CompletionClient surface it as an auth error mid-trial, which reads as a model failure and costs a container run to diagnose. * Document that the dataset registry_url is NVIDIA-internal and what external users need instead. Runtime state: * Publish the state file atomically via a same-directory temp file and `os.replace`. The Harbor wrapper collects it as a trial artifact while the conversation is still running, so a plain `write_text` let it capture a half-serialized document. * Serialize MCP-exposed runtime calls behind a reentrant lock, including the dynamically registered domain tools. FastMCP dispatches synchronous tools on a thread pool and `runtime` is a module-level singleton driving one conversation, so concurrent calls interleaved mutations of the message history, the counters, and the state file. Serializing is the semantics here, not a throughput cost -- two simultaneous turns on one conversation are already meaningless. The wrapper copies `__signature__` and `__annotations__` explicitly. Python 3.14 swapped `__annotations__` for `__annotate__` in functools.WRAPPER_ASSIGNMENTS (PEP 649), and make_domain_tool_handler assigns `__annotations__` directly, so `wraps` alone produced tools whose schema had no arguments. Verified on 3.12 (the image) and 3.14. * Log user-tool discovery failures with domain context. A genuine tau2 API break was indistinguishable from "this domain has no user tools" and surfaced only as a quietly degraded user simulator. Adds contract tests asserting relationships between files -- both images agree on the revision, neither uses a shallow clone, the FastMCP floor clears the advisory minimum, nooa is a sha -- rather than restating a pin as a literal. Refs AALGO-312. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Nick Goncharenko <ngoncharenko@nvidia.com>
… exemptions
Adds SPDX to the files that are configuration: task.toml and docker-compose.yaml
get comment headers, task_config.json gets top-level metadata fields (JSON has no
comments; Tau3Runtime indexes known keys, so extra ones are inert -- verified the
file still parses and keeps `domain`).
Deliberately NOT applied to `instruction.md` or `AGENT-SPEC.md`, which CodeRabbit
asked for. Both are upstream tau2-bench benchmark content, not source:
* Harbor reads `instruction.md` verbatim into the agent prompt, so a header would
prepend licence boilerplate to every prompt -- the same failure mode CodeRabbit
itself identified for the hello example's `expected.txt` fixture.
* `AGENT-SPEC.md` duplicates the same airline policy and is read by the Coder as a
contract, so the two must stay byte-identical to each other.
For the same reason the flagged grammar ("the user is should be refunded the
difference") is left alone: the sentence appears in *both* files, so it is upstream
policy text. Correcting it would fork the benchmark and make scores incomparable
with published tau2 results, for no behavioural gain.
A README section records this so the next linter pass does not re-open it.
Refs AALGO-312.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Nick Goncharenko <ngoncharenko@nvidia.com>
…aseline task set Follow-up to the CodeRabbit pass on 6a7ef47. * SPDX headers added to both Tau2 Dockerfiles and the task-template `tests/test.sh` (a 0-byte placeholder; two comments still exit 0). These were simply missed when the other infrastructure files were done. * SPDX added to `AGENT-SPEC.md` after all. The earlier commit declined it on the grounds that the file must stay in step with `instruction.md`'s policy text -- but the header is an HTML comment *above* the policy body, and `instruction.md` wraps the same policy in its own `<policy>` block, so nothing desynchronises. The decline was wrong; the header is additive. `instruction.md` itself still has none, and that part stands: Harbor reads it verbatim into the agent prompt, so a header would prepend licence boilerplate to every prompt. * `test_every_split_keeps_one_passing_and_one_failing_task` now asserts the split's on-disk task directories match `_EXPECTED_BASELINE` before scoring. It previously iterated the expectation itself, so adding or removing a dataset task left every assertion passing while the split silently lost the one-pass/one-failure shape the README documents. Refs AALGO-312. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Nick Goncharenko <ngoncharenko@nvidia.com>
Signed-off-by: Nick Goncharenko <ngoncharenko@nvidia.com>
503ca31 to
20813e3
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/harbor_runtime.py`:
- Around line 613-617: Update the task digest construction in the loop over
_task_dirs_for to pass each non-None task_dir through the existing _safe_resolve
helper instead of calling task_dir.resolve() directly. Preserve the
"<unresolved>" value for missing directories and ensure resolution failures
degrade to the safe fallback without raising from cache-stamp generation.
In `@plugins/nemo-experimentalist/docs/architecture.md`:
- Around line 444-446: Update the aggregate_metrics description in the
architecture documentation to state that each key’s mean is computed only over
trials whose status is exactly "completed", replacing the broader "non-failed
trials" wording. Preserve the existing explanation of multi-key Pareto
comparison.
🪄 Autofix (Beta)
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: 1cdc5533-a21e-4e65-92c9-92f461400763
⛔ Files ignored due to path filters (2)
sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/agent_eval/runtimes/harbor_runtime.pyis excluded by!sdk/**uv.lockis excluded by!**/*.lock
📒 Files selected for processing (85)
packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/harbor_runtime.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_harbor_runtime.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_harbor_runtime_e2e.pyplugins/nemo-eval-author/src/nemo_eval_author_plugin/eval_author/config.yamlplugins/nemo-eval-author/src/nemo_eval_author_plugin/eval_author/run.pyplugins/nemo-eval-author/tests/test_eval_author_run.pyplugins/nemo-experimentalist/README.mdplugins/nemo-experimentalist/docs/architecture.mdplugins/nemo-experimentalist/docs/e2e/README.mdplugins/nemo-experimentalist/docs/e2e/experiment-debug-round.yamlplugins/nemo-experimentalist/docs/e2e/experiment-eval-only-sdk.yamlplugins/nemo-experimentalist/docs/e2e/experiment-eval-only.yamlplugins/nemo-experimentalist/docs/e2e/experiment-fast.yamlplugins/nemo-experimentalist/docs/e2e/run-eval-only.pyplugins/nemo-experimentalist/examples/README.mdplugins/nemo-experimentalist/examples/hello-harbor-agent/.env.exampleplugins/nemo-experimentalist/examples/hello-harbor-agent/AGENT-SPEC.mdplugins/nemo-experimentalist/examples/hello-harbor-agent/README.mdplugins/nemo-experimentalist/examples/hello-harbor-agent/agent.pyplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/task-template/README.mdplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/task-template/environment/Dockerfileplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/task-template/instruction.mdplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/task-template/task.tomlplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/task-template/tests/expected.txtplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/task-template/tests/test.shplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/greet-world/environment/Dockerfileplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/greet-world/instruction.mdplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/greet-world/task.tomlplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/greet-world/tests/expected.txtplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/greet-world/tests/test.shplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/sum-two/environment/Dockerfileplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/sum-two/instruction.mdplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/sum-two/task.tomlplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/sum-two/tests/expected.txtplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/sum-two/tests/test.shplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/greet-universe/environment/Dockerfileplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/greet-universe/instruction.mdplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/greet-universe/task.tomlplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/greet-universe/tests/expected.txtplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/greet-universe/tests/test.shplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/sum-three/environment/Dockerfileplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/sum-three/instruction.mdplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/sum-three/task.tomlplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/sum-three/tests/expected.txtplugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/sum-three/tests/test.shplugins/nemo-experimentalist/examples/hello-harbor-agent/harbor_wrapper.pyplugins/nemo-experimentalist/examples/hello-harbor-agent/main.pyplugins/nemo-experimentalist/examples/hello-harbor-agent/optimizer.yamlplugins/nemo-experimentalist/examples/hello-harbor-agent/tracing.pyplugins/nemo-experimentalist/examples/tau2-nooa-oo-agent/.env.exampleplugins/nemo-experimentalist/examples/tau2-nooa-oo-agent/AGENT-SPEC.mdplugins/nemo-experimentalist/examples/tau2-nooa-oo-agent/README.mdplugins/nemo-experimentalist/examples/tau2-nooa-oo-agent/agent.pyplugins/nemo-experimentalist/examples/tau2-nooa-oo-agent/dataset/template/task_template/environment/Dockerfileplugins/nemo-experimentalist/examples/tau2-nooa-oo-agent/dataset/template/task_template/environment/docker-compose.yamlplugins/nemo-experimentalist/examples/tau2-nooa-oo-agent/dataset/template/task_template/environment/runtime-server/Dockerfileplugins/nemo-experimentalist/examples/tau2-nooa-oo-agent/dataset/template/task_template/environment/runtime-server/server.pyplugins/nemo-experimentalist/examples/tau2-nooa-oo-agent/dataset/template/task_template/environment/runtime-server/task_config.jsonplugins/nemo-experimentalist/examples/tau2-nooa-oo-agent/dataset/template/task_template/instruction.mdplugins/nemo-experimentalist/examples/tau2-nooa-oo-agent/dataset/template/task_template/task.tomlplugins/nemo-experimentalist/examples/tau2-nooa-oo-agent/dataset/template/task_template/tests/test.shplugins/nemo-experimentalist/examples/tau2-nooa-oo-agent/harbor_wrapper.pyplugins/nemo-experimentalist/examples/tau2-nooa-oo-agent/main.pyplugins/nemo-experimentalist/examples/tau2-nooa-oo-agent/optimizer.yamlplugins/nemo-experimentalist/examples/tau2-nooa-oo-agent/pyproject.tomlplugins/nemo-experimentalist/pyproject.tomlplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/base.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/factory.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/harbor.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/harbor_evaluator.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/deps.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/run.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/preflight.pyplugins/nemo-experimentalist/src/nemo_experimentalist_plugin/resolve.pyplugins/nemo-experimentalist/tests/experimentalist/conftest.pyplugins/nemo-experimentalist/tests/experimentalist/test_dataset_staging_loop.pyplugins/nemo-experimentalist/tests/experimentalist/test_evaluator_factory.pyplugins/nemo-experimentalist/tests/experimentalist/test_evaluator_harbor_ab_e2e.pyplugins/nemo-experimentalist/tests/experimentalist/test_evaluator_harbor_evaluator.pyplugins/nemo-experimentalist/tests/experimentalist/test_hello_example_baseline.pyplugins/nemo-experimentalist/tests/experimentalist/test_loop_failure.pyplugins/nemo-experimentalist/tests/experimentalist/test_loop_insight_suite.pyplugins/nemo-experimentalist/tests/experimentalist/test_tau2_example_contract.pyplugins/nemo-experimentalist/tests/test_deps.pyplugins/nemo-experimentalist/tests/test_e2e_helper_defaults.py
🚧 Files skipped from review as they are similar to previous changes (60)
- plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/sum-two/tests/expected.txt
- plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/run.py
- plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/preflight.py
- plugins/nemo-experimentalist/pyproject.toml
- plugins/nemo-experimentalist/tests/experimentalist/test_loop_failure.py
- plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/sum-two/task.toml
- plugins/nemo-experimentalist/examples/tau2-nooa-oo-agent/.env.example
- plugins/nemo-experimentalist/examples/tau2-nooa-oo-agent/pyproject.toml
- plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/task-template/README.md
- plugins/nemo-experimentalist/examples/tau2-nooa-oo-agent/main.py
- plugins/nemo-experimentalist/examples/tau2-nooa-oo-agent/dataset/template/task_template/environment/runtime-server/task_config.json
- plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/greet-world/task.toml
- plugins/nemo-experimentalist/tests/experimentalist/test_tau2_example_contract.py
- plugins/nemo-experimentalist/tests/experimentalist/test_dataset_staging_loop.py
- plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/factory.py
- plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/task-template/task.toml
- plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/resolve.py
- plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/sum-two/tests/test.sh
- plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/greet-universe/task.toml
- plugins/nemo-experimentalist/examples/tau2-nooa-oo-agent/dataset/template/task_template/task.toml
- plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/greet-universe/tests/expected.txt
- plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/task-template/tests/expected.txt
- plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/greet-world/tests/test.sh
- plugins/nemo-experimentalist/docs/e2e/experiment-fast.yaml
- plugins/nemo-experimentalist/tests/experimentalist/conftest.py
- plugins/nemo-experimentalist/examples/tau2-nooa-oo-agent/optimizer.yaml
- plugins/nemo-experimentalist/docs/e2e/experiment-eval-only.yaml
- plugins/nemo-experimentalist/examples/tau2-nooa-oo-agent/dataset/template/task_template/tests/test.sh
- plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/task-template/tests/test.sh
- plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/sum-three/tests/expected.txt
- plugins/nemo-experimentalist/README.md
- plugins/nemo-experimentalist/examples/hello-harbor-agent/main.py
- plugins/nemo-experimentalist/examples/hello-harbor-agent/agent.py
- plugins/nemo-eval-author/src/nemo_eval_author_plugin/eval_author/config.yaml
- plugins/nemo-experimentalist/tests/test_deps.py
- plugins/nemo-experimentalist/examples/hello-harbor-agent/optimizer.yaml
- plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/sum-three/task.toml
- plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/train/greet-world/tests/expected.txt
- plugins/nemo-experimentalist/examples/hello-harbor-agent/.env.example
- plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/sum-three/tests/test.sh
- plugins/nemo-experimentalist/tests/experimentalist/test_hello_example_baseline.py
- plugins/nemo-eval-author/tests/test_eval_author_run.py
- plugins/nemo-experimentalist/tests/test_e2e_helper_defaults.py
- packages/nemo_evaluator_sdk/tests/agent_eval/test_harbor_runtime_e2e.py
- plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/deps.py
- plugins/nemo-experimentalist/examples/hello-harbor-agent/harbor_wrapper.py
- plugins/nemo-eval-author/src/nemo_eval_author_plugin/eval_author/run.py
- plugins/nemo-experimentalist/tests/experimentalist/test_loop_insight_suite.py
- plugins/nemo-experimentalist/examples/tau2-nooa-oo-agent/README.md
- plugins/nemo-experimentalist/tests/experimentalist/test_evaluator_factory.py
- plugins/nemo-experimentalist/examples/tau2-nooa-oo-agent/dataset/template/task_template/environment/docker-compose.yaml
- packages/nemo_evaluator_sdk/tests/agent_eval/test_harbor_runtime.py
- plugins/nemo-experimentalist/docs/e2e/experiment-eval-only-sdk.yaml
- plugins/nemo-experimentalist/examples/hello-harbor-agent/AGENT-SPEC.md
- plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/harbor.py
- plugins/nemo-experimentalist/docs/e2e/experiment-debug-round.yaml
- plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/base.py
- plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/harbor_evaluator.py
- plugins/nemo-experimentalist/docs/e2e/README.md
- plugins/nemo-experimentalist/examples/hello-harbor-agent/dataset/validation/greet-universe/tests/test.sh
architecture.md described `aggregate_metrics` as a mean "over non-failed trials", contradicting section 4 two hundred lines earlier, which documents the actual contract: only `status == "completed"` trials are averaged. The wording matters beyond consistency. `base.py` uses the positive predicate deliberately -- `!= "failed"` is equivalent only while TrialStatus is Literal["completed", "failed"], and would silently start averaging any third status someone adds. The doc line was the opt-out phrasing that comment warns against, so a reader adding a status would have read it as scoreable. Refs AALGO-312. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Nick Goncharenko <ngoncharenko@nvidia.com>
…t OSError
CodeRabbit flagged a bare `task_dir.resolve()` in `_cache_stamp` and suggested
routing it through `_safe_resolve` like `_task_dirs_for` does. Investigating that
turned up a larger problem: `_safe_resolve` would not have fixed it.
On CPython 3.12 -- the floor this package targets -- `Path.resolve()` translates
ELOOP into `RuntimeError("Symlink loop from ...")`, which is not an `OSError`.
`_safe_resolve` caught only `OSError`, so a symlink loop propagated straight
through the very helper written to absorb it, killing a run over a best-effort
cache fingerprint. It raises deterministically, not as a race. Verified across
interpreters: 3.12 raises RuntimeError; 3.14 resolves a loop without raising at
all, so the hazard is live only on the supported range.
The loop is reached through `_digest_directory`'s walk, which already routes every
path through `_safe_resolve` -- so this was live regardless of the flagged line.
`_cache_stamp`'s three resolve calls now use `_safe_resolve` too. Being precise
about why: that is consistency, not a demonstrated fix. `_task_dirs_for` filters
candidates with `is_dir()`, which returns False for a loop, so a looping path
never reaches them. Reverting those three lines alone does not fail the new test;
reverting the exception set does.
Regression test uses a self-referential symlink and asserts the stamp still
fingerprints the task. Mutation-checked against CodeRabbit's exact suggestion
(`_safe_resolve` with `except OSError`), which still fails.
Refs AALGO-312.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Nick Goncharenko <ngoncharenko@nvidia.com>
| # Positive predicate on purpose: `!= "failed"` is equivalent while TrialStatus | ||
| # is Literal["completed", "failed"], but it would silently start averaging any | ||
| # third status someone adds. Opt statuses in, do not opt "failed" out. | ||
| completed = [r for r in results if r.status == "completed"] |
There was a problem hiding this comment.
-
This does not change current behavior in main besides making it more defensive. There are two values in the TrialStatus: TrialStatus: TypeAlias = Literal["completed", "failed"] , so status != "failed" to status == "completed" are equivalent currently. If we add a 3rd value in the future to TrialStatus like timeout , that’s when behavior will differ. In my view, we don’t want timeout error, for example, affect the real scores aggregation and drag it down - does not seem right to me.
-
The comment in main was out of date and didn’t reflect actual code behavior:
https://github.com/NVIDIA-NeMo/nemo-platform/blob/main/plugins/nemo-experimentalist/src/nemo_experimentalist_plugin/experimentalist/components/evaluator/base.py#L80 - this is aggregation in main. So it it didn’t count failures towards aggregation
completed = [r for r in results if r.status != "failed"] # failures filtered OUT
....
aggregate_metrics[metric_name] = aggregate_metrics[metric_name] / len(completed) # NOT len(results)…2-wire-eval-into-optimizer Signed-off-by: Alec Khoury <akhoury@nvidia.com>
Retarget the realistic example path to Nico's Tau3 setup, keep hello as the deterministic Harbor harness, and remove the obsolete docs/e2e configs. Signed-off-by: Alec Khoury <akhoury@nvidia.com>
Main changes
HarborAgentTaskRunneras a second evaluatorEvaluatorType = harbor_native | harbor_evaluator. The default staysharbor_nativeuntilharbor_evaluatorreaches full parity.harboris still accepted as a deprecated alias forharbor_nativeand warns once.Linear
Review pointers
packages/nemo_evaluator_sdkare already reviewed and don't require another review fix(evaluator-sdk): make Harbor's per-trial resume reachable with agent_dir set #964evaluator/harbor_evaluator.py— the new evaluatorevaluator/harbor.py—harbor_native+trials_from_job_dirextracted for sharing;HarborEvaluator._runwidened to match the base-class contractdocs/architecture.md§4 - the two namespaces, and how to add a third evaluatorWhat changes
The loop gains a second
evaluator_type. Both run Harbor underneath — what moves is who owns the orchestration:evaluator_typeharbor_evaluatorHarborAgentTaskRunnerbuilds theJobConfig, owns the success-aware job-dir cache, and scopes the agent import.harbor_native(default)JobConfigand drivesJobitself — the original path, kept as the A/B baseline. It stays the default untilharbor_evaluatorreaches full parity.Both read results back through the same on-disk adapter, so the trials the loop sees are equivalent. The SDK's own trial model is deliberately not the contract, but not because it is lossy —
metadata["reward_details"]does carry every verifier reward. The job directory is the shared source of truth for both evaluator types, and that sharing is what keeps them equivalent.Design notes worth reviewing
Entry and exit are both shared.
Two evaluators run Harbor differently, but must mean the same thing by “evaluate this candidate”:
Sharing
resolve_harbor_run_inputsforces both to agree on agent path, dataset path, job dir, and preflight before either starts Harbor.Sharing only the results side would let the two drift on what "the same run" means, and the A/B tests would not catch it — they compare results, not inputs.
Cache staleness is the SDK's job now.
Old (broken) cache key:
Example failure:
Same trap if you change a task’s
test.sh, or a result-affecting option (timeouts, etc.).New: SDK writes a stamp file in the job dir:
Rerun only reuses results if the stamp still matches. Plugin does not invent that logic — it only passes
force_rerun=Truewhen the user wants a wipe.Experimentalist plugin decides when to force a wipe; SDK decides whether previous results are still valid for today’s inputs. Per-task resume (only re-run missing trials) is also SDK-owned (AALGO-427), not the plugin.
Two task-name namespaces
Harbor's local-dataset
task_namesfilter matches the task directory name (sum-two);result.jsonrecords[task].namefromtask.toml(hello/sum-two), which is what the SDK's tasks and cache are keyed by. Mapping is by resolved directory, never name similarity. Passing full names to the Harbor filter — the naive reading of the SDK — makes Harbor match nothing and raise.Same task has two different strings, used in different places:
Config is
extra="forbid".Every
HarborRunnerConfigfield maps 1:1 ontoHarborRuntimeConfig. Options with no unambiguous SDK equivalent (plain Harbor'sretrymodel) are rejected, not ignored.agent_diris absent by design — always derived from the candidate, so a config cannot aim a run at different code.aggregate_results filters
aggregate_resultsfilters onstatus == "completed".** Equivalent to the old!= "failed"whileTrialStatushas two values, but it opts statuses in rather than opting"failed"out, so a third status is not silently averaged.Testing
force_rerun, concurrent candidates), and result parity between the two evaluators.test_evaluator_harbor_ab_e2e.py, 4 tests) — runs the bundled example through both evaluators and asserts they produce equivalent trials, plus cache reuse andforce_rerunbehaviour. Both report{"reward": 0.5, "format_ok": 1.0}.nemo experimentalist runthrough the CLI completes onharbor_evaluator:winner=agent-0, validation_reward={'reward': 0.5, 'format_ok': 1.0}.611 passed, 2 skipped(both skips are pre-existing Eval Author canaries that needRUN_EVAL_AUTHOR_*env vars). Ruff clean;tydiagnostics one below the pre-change baseline.Reproducing the A/B
Needs Docker; no model API key — the evaluator seam makes no LLM calls.
Both print the same aggregate. Full walkthrough — prerequisites, artifact layout, cache behaviour, config reference, and the CLI configs — is in
examples/hello-harbor-agent/README.md.Note
docs/e2e/experiment-eval-only.yamlpinsevaluator_type: harbor_nativeexplicitly. It is the plain-Harbor arm of the A/B; pinning both arms keeps the comparison readable and independent of whatever the default happens to be.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Bug Fixes