feat(evaluator): agent-evaluation plugin job (run_local + submit) - #496
Merged
Conversation
SandyChapman
force-pushed
the
aalgo-297-agent-eval-job/schapman
branch
2 times, most recently
from
June 29, 2026 12:42
24ed157 to
4908e7c
Compare
SandyChapman
marked this pull request as ready for review
June 29, 2026 13:21
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds agent-evaluate contracts, runtime, routing, OpenAPI, and tests. Shared metric resolution, secret environment assembly, task runner, SDK typing, and job-wait helpers are updated. The existing evaluate job is refactored to use shared helpers. ChangesAgent evaluation stack
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/nemo-evaluator/src/nemo_evaluator/jobs/agent_evaluate.py`:
- Around line 174-180: Restrict the header forwarding in agent_evaluate.py so
only the documented identity propagation headers are passed through from
platform.default_headers, instead of forwarding every X-NMP-* key via the
current filter in the identity_headers block. Update the comprehension in the
relevant identity_headers setup to use an explicit allowlist of approved header
names, and keep the same type/str checks when building the dict. Ensure the
change is localized around the platform/default_headers handling in the agent
evaluation path so future SDK-added metadata headers are not propagated to
online targets.
In `@plugins/nemo-evaluator/tests/integration/conftest.py`:
- Around line 51-61: The _wait_for_ready helper currently only polls
/health/ready and can hide an early subprocess exit, so update it to fail fast
when the started process has already terminated. In the waiting loop, check
process.poll() (or equivalent) before retrying the readiness probe and raise
immediately with a clear error if uv run nemo services run has exited. Make the
change in _wait_for_ready and the related startup wait call site referenced by
the same helper logic so a bad config surfaces the real failure instead of
waiting for the full timeout.
- Around line 99-103: The cleanup path in the fixture leaves a zombie if
`process.wait(timeout=20)` times out and `process.kill()` is called without
reaping the child. Update the timeout handling in the `conftest.py` teardown
logic so that after `process.kill()` it waits again for the terminated process
to be collected, keeping the `process.terminate()` / `process.wait()` /
`process.kill()` flow consistent.
In `@plugins/nemo-evaluator/tests/integration/test_agent_evaluate_job.py`:
- Around line 329-356: The xfail in
test_submit_to_docker_backend_runs_agent_eval is still using a
CodexRunnerTarget, which can fail for codex CLI/auth reasons unrelated to the
docker backend or the missing entrypoint. Update the test to use an offline
trials= spec or another container-local target so it only exercises the docker
backend wiring, and keep the existing wait_for_platform_job and submit_remote
flow focused on that path. Refer to
test_submit_to_docker_backend_runs_agent_eval and _codex_eval_input_spec when
making the change.
- Around line 66-70: The requires_codex skip marker currently only checks for
the codex binary via _codex_available, so unauthenticated hosts can still run
these tests. Update the gating logic in _codex_available (or a nearby helper
used by requires_codex) to perform a non-interactive auth check such as codex
login status and return false when the user is not logged in. Keep
requires_codex wired to that helper so the tests only run when both the CLI is
present and auth is valid.
In `@plugins/nemo-evaluator/tests/test_agent_evaluate.py`:
- Around line 206-221: The test in
test_build_evaluator_forwards_platform_identity_headers only checks that X-NMP-*
headers are preserved, so it does not catch accidental forwarding of
Authorization or other transport headers. Update the test setup for
AgentEvalJob._build_evaluator by adding at least one bearer/transport header to
sdk.default_headers, then assert the resulting evaluator.default_headers
contains only the X-NMP-* identity headers and excludes the non-identity
entries.
🪄 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: 4901a199-118b-470b-abd2-9a94f634b573
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (20)
packages/nmp_testing/src/nmp/testing/e2e/jobs.pyplugins/nemo-evaluator/pyproject.tomlplugins/nemo-evaluator/src/nemo_evaluator/jobs/agent_compiler.pyplugins/nemo-evaluator/src/nemo_evaluator/jobs/agent_evaluate.pyplugins/nemo-evaluator/src/nemo_evaluator/jobs/agent_spec.pyplugins/nemo-evaluator/src/nemo_evaluator/jobs/compiler.pyplugins/nemo-evaluator/src/nemo_evaluator/jobs/evaluate.pyplugins/nemo-evaluator/src/nemo_evaluator/jobs/metric_resolution.pyplugins/nemo-evaluator/src/nemo_evaluator/jobs/secret_env.pyplugins/nemo-evaluator/src/nemo_evaluator/metric_refs.pyplugins/nemo-evaluator/src/nemo_evaluator/resolvers.pyplugins/nemo-evaluator/src/nemo_evaluator/service.pyplugins/nemo-evaluator/src/nemo_evaluator/tasks/agent_evaluate.pyplugins/nemo-evaluator/src/nemo_evaluator/tasks/evaluate.pyplugins/nemo-evaluator/src/nemo_evaluator/tasks/runner.pyplugins/nemo-evaluator/tests/integration/conftest.pyplugins/nemo-evaluator/tests/integration/test_agent_evaluate_job.pyplugins/nemo-evaluator/tests/test_agent_evaluate.pyplugins/nemo-evaluator/tests/test_evaluate_job.pyplugins/nemo-evaluator/tests/test_service.py
SandyChapman
force-pushed
the
aalgo-297-agent-eval-job/schapman
branch
from
June 29, 2026 13:43
4908e7c to
ef76e38
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/nemo-evaluator/tests/test_agent_evaluate.py`:
- Around line 422-426: The exclusivity test in
test_spec_requires_exactly_one_of_target_or_trials is using an empty trials
list, which may trigger empty-list validation instead of the intended
mutual-exclusion check. Update the AgentEvalSpec call in this test to pass a
non-empty valid trials payload so it only exercises the “exactly one of target
or trials” contract, while keeping the existing _task_spec, CodexRunnerTarget,
and pytest.raises(ValueError, match="exactly one") 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: 7c39ebfb-a1f6-4d84-8282-ef7a241614e7
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (20)
packages/nmp_testing/src/nmp/testing/e2e/jobs.pyplugins/nemo-evaluator/pyproject.tomlplugins/nemo-evaluator/src/nemo_evaluator/jobs/agent_compiler.pyplugins/nemo-evaluator/src/nemo_evaluator/jobs/agent_evaluate.pyplugins/nemo-evaluator/src/nemo_evaluator/jobs/agent_spec.pyplugins/nemo-evaluator/src/nemo_evaluator/jobs/compiler.pyplugins/nemo-evaluator/src/nemo_evaluator/jobs/evaluate.pyplugins/nemo-evaluator/src/nemo_evaluator/jobs/metric_resolution.pyplugins/nemo-evaluator/src/nemo_evaluator/jobs/secret_env.pyplugins/nemo-evaluator/src/nemo_evaluator/metric_refs.pyplugins/nemo-evaluator/src/nemo_evaluator/resolvers.pyplugins/nemo-evaluator/src/nemo_evaluator/service.pyplugins/nemo-evaluator/src/nemo_evaluator/tasks/agent_evaluate.pyplugins/nemo-evaluator/src/nemo_evaluator/tasks/evaluate.pyplugins/nemo-evaluator/src/nemo_evaluator/tasks/runner.pyplugins/nemo-evaluator/tests/integration/conftest.pyplugins/nemo-evaluator/tests/integration/test_agent_evaluate_job.pyplugins/nemo-evaluator/tests/test_agent_evaluate.pyplugins/nemo-evaluator/tests/test_evaluate_job.pyplugins/nemo-evaluator/tests/test_service.py
✅ Files skipped from review due to trivial changes (1)
- plugins/nemo-evaluator/src/nemo_evaluator/metric_refs.py
🚧 Files skipped from review as they are similar to previous changes (16)
- plugins/nemo-evaluator/tests/test_service.py
- plugins/nemo-evaluator/src/nemo_evaluator/jobs/secret_env.py
- plugins/nemo-evaluator/pyproject.toml
- plugins/nemo-evaluator/src/nemo_evaluator/tasks/runner.py
- plugins/nemo-evaluator/src/nemo_evaluator/tasks/agent_evaluate.py
- packages/nmp_testing/src/nmp/testing/e2e/jobs.py
- plugins/nemo-evaluator/src/nemo_evaluator/jobs/compiler.py
- plugins/nemo-evaluator/src/nemo_evaluator/jobs/agent_spec.py
- plugins/nemo-evaluator/src/nemo_evaluator/jobs/agent_compiler.py
- plugins/nemo-evaluator/tests/test_evaluate_job.py
- plugins/nemo-evaluator/src/nemo_evaluator/resolvers.py
- plugins/nemo-evaluator/src/nemo_evaluator/service.py
- plugins/nemo-evaluator/src/nemo_evaluator/tasks/evaluate.py
- plugins/nemo-evaluator/src/nemo_evaluator/jobs/metric_resolution.py
- plugins/nemo-evaluator/src/nemo_evaluator/jobs/agent_evaluate.py
- plugins/nemo-evaluator/src/nemo_evaluator/jobs/evaluate.py
Contributor
|
SandyChapman
force-pushed
the
aalgo-297-agent-eval-job/schapman
branch
from
June 29, 2026 14:47
ef76e38 to
1833117
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
plugins/nemo-evaluator/tests/integration/test_agent_evaluate_job.py (1)
408-415: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake this xfail strict.
strict=Falseturns the image refresh into a non-failing XPASS, so CI will not force removal of the stale expectation.Suggested change
`@pytest.mark.xfail`( reason="agent-eval can't run under the docker backend until the cpu-tasks image is rebuilt with " "this work: the published image predates the nemo_evaluator.tasks.agent_evaluate entrypoint " "(container exits with ModuleNotFoundError). This submits an offline trials spec (no online " "generation, no codex, no IGW), so the stale image is the only remaining failure cause — the " "xfail flips the moment the image ships the entrypoint. Tracked in AALGO-301.", - strict=False, + strict=True, )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/nemo-evaluator/tests/integration/test_agent_evaluate_job.py` around lines 408 - 415, Make the agent-evaluate docker-backend xfail strict by updating the `@pytest.mark.xfail` on the affected integration test so it uses strict=True instead of strict=False. Keep the existing reason text intact and adjust the test decorator on the agent evaluation job test so any unexpected pass fails CI and forces removal of the stale expectation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@plugins/nemo-evaluator/tests/integration/test_agent_evaluate_job.py`:
- Around line 408-415: Make the agent-evaluate docker-backend xfail strict by
updating the `@pytest.mark.xfail` on the affected integration test so it uses
strict=True instead of strict=False. Keep the existing reason text intact and
adjust the test decorator on the agent evaluation job test so any unexpected
pass fails CI and forces removal of the stale expectation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: fa842ce6-f19b-4304-a496-f41d63b826b1
⛔ Files ignored due to path filters (3)
sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/agent_eval/trials.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/values/evidence.pyis excluded by!sdk/**uv.lockis excluded by!**/*.lock
📒 Files selected for processing (24)
packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/trials.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/values/evidence.pypackages/nemo_platform/pyproject.tomlpackages/nmp_testing/src/nmp/testing/e2e/jobs.pyplugins/nemo-evaluator/openapi/openapi.yamlplugins/nemo-evaluator/pyproject.tomlplugins/nemo-evaluator/src/nemo_evaluator/jobs/agent_compiler.pyplugins/nemo-evaluator/src/nemo_evaluator/jobs/agent_evaluate.pyplugins/nemo-evaluator/src/nemo_evaluator/jobs/agent_spec.pyplugins/nemo-evaluator/src/nemo_evaluator/jobs/compiler.pyplugins/nemo-evaluator/src/nemo_evaluator/jobs/evaluate.pyplugins/nemo-evaluator/src/nemo_evaluator/jobs/metric_resolution.pyplugins/nemo-evaluator/src/nemo_evaluator/jobs/secret_env.pyplugins/nemo-evaluator/src/nemo_evaluator/metric_refs.pyplugins/nemo-evaluator/src/nemo_evaluator/resolvers.pyplugins/nemo-evaluator/src/nemo_evaluator/service.pyplugins/nemo-evaluator/src/nemo_evaluator/tasks/agent_evaluate.pyplugins/nemo-evaluator/src/nemo_evaluator/tasks/evaluate.pyplugins/nemo-evaluator/src/nemo_evaluator/tasks/runner.pyplugins/nemo-evaluator/tests/integration/conftest.pyplugins/nemo-evaluator/tests/integration/test_agent_evaluate_job.pyplugins/nemo-evaluator/tests/test_agent_evaluate.pyplugins/nemo-evaluator/tests/test_evaluate_job.pyplugins/nemo-evaluator/tests/test_service.py
✅ Files skipped from review due to trivial changes (2)
- plugins/nemo-evaluator/src/nemo_evaluator/tasks/agent_evaluate.py
- plugins/nemo-evaluator/src/nemo_evaluator/metric_refs.py
🚧 Files skipped from review as they are similar to previous changes (15)
- plugins/nemo-evaluator/tests/test_service.py
- packages/nmp_testing/src/nmp/testing/e2e/jobs.py
- plugins/nemo-evaluator/src/nemo_evaluator/tasks/runner.py
- plugins/nemo-evaluator/src/nemo_evaluator/jobs/secret_env.py
- plugins/nemo-evaluator/src/nemo_evaluator/jobs/compiler.py
- plugins/nemo-evaluator/pyproject.toml
- plugins/nemo-evaluator/src/nemo_evaluator/resolvers.py
- plugins/nemo-evaluator/src/nemo_evaluator/jobs/metric_resolution.py
- plugins/nemo-evaluator/src/nemo_evaluator/jobs/agent_spec.py
- plugins/nemo-evaluator/src/nemo_evaluator/tasks/evaluate.py
- plugins/nemo-evaluator/src/nemo_evaluator/jobs/agent_compiler.py
- plugins/nemo-evaluator/src/nemo_evaluator/service.py
- plugins/nemo-evaluator/tests/test_evaluate_job.py
- plugins/nemo-evaluator/src/nemo_evaluator/jobs/evaluate.py
- plugins/nemo-evaluator/src/nemo_evaluator/jobs/agent_evaluate.py
SandyChapman
force-pushed
the
aalgo-297-agent-eval-job/schapman
branch
from
June 29, 2026 14:58
1833117 to
181cdc5
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
plugins/nemo-evaluator/tests/integration/test_agent_evaluate_job.py (1)
72-76: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGate Codex tests on auth, not just PATH.
_codex_available()still only checks for the binary. Logged-out hosts will run these cases even though the module docstring says they require a logged-in account. Probe auth non-interactively as part of the skip gate.Proposed fix
import json import shutil +import subprocess import sys import uuid @@ def _codex_available() -> bool: - return shutil.which("codex") is not None + if shutil.which("codex") is None: + return False + return ( + subprocess.run( + ["codex", "login", "status"], + stdout=subprocess.DEVNULL, + stderr=subprocess.DEVNULL, + check=False, + ).returncode + == 0 + ) @@ -requires_codex = pytest.mark.skipif(not _codex_available(), reason="codex CLI not on PATH") +requires_codex = pytest.mark.skipif( + not _codex_available(), + reason="codex CLI missing or not authenticated", +)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/nemo-evaluator/tests/integration/test_agent_evaluate_job.py` around lines 72 - 76, The Codex test skip gate only checks for the CLI binary via _codex_available(), so logged-out machines can still run these tests. Update _codex_available() in test_agent_evaluate_job.py to also verify Codex authentication non-interactively, and keep requires_codex using that combined check so the tests skip unless both the binary is present and the user is logged in.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/nemo-evaluator/openapi/openapi.yaml`:
- Around line 1034-1049: The request schema currently defines `target` and
`trials` as independent optional fields, so the OpenAPI contract does not
enforce the XOR that the runtime expects. Update the relevant eval input schema
that contains `target` and `trials` to encode mutual exclusivity and require
exactly one of them alongside `tasks` (for example by using a oneOf/anyOf
composition or equivalent schema constraints). Apply the same fix in both
affected schema blocks so generated clients and validators can detect the
contract at the OpenAPI level instead of only at runtime.
- Around line 1903-1935: Update the EvidenceDescriptor schema so it actually
enforces the stated constraint that either ref or data must be present. In
openapi.yaml, adjust the EvidenceDescriptor definition to require ref and/or
data appropriately (for example with a oneOf/anyOf or similar schema constraint)
while keeping kind required, and make sure the validation aligns with the
descriptions on ref, data, and the EvidenceDescriptor model.
In `@plugins/nemo-evaluator/tests/integration/conftest.py`:
- Around line 124-126: The mock-provider prefix is coming from two different
sources, which can make `add_mock_provider()` and the spawned platform use
different namespaces when `NMP_INFERENCE_GATEWAY_MOCK_PROVIDER_PREFIX` is
already set. Update the integration fixture setup in `conftest.py` so
`MOCK_PROVIDER_PREFIX_ENVVAR` is treated as the single source of truth: read the
effective prefix from `os.environ[MOCK_PROVIDER_PREFIX_ENVVAR]` instead of
preserving a separate default, and make the platform-spawned configuration in
the relevant helper/setup paths use that same value consistently. Ensure the
fixture logic around `add_mock_provider()` and the platform launch helpers all
reference the same prefix symbol.
---
Duplicate comments:
In `@plugins/nemo-evaluator/tests/integration/test_agent_evaluate_job.py`:
- Around line 72-76: The Codex test skip gate only checks for the CLI binary via
_codex_available(), so logged-out machines can still run these tests. Update
_codex_available() in test_agent_evaluate_job.py to also verify Codex
authentication non-interactively, and keep requires_codex using that combined
check so the tests skip unless both the binary is present and the user is logged
in.
🪄 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: 812eb13e-52c0-4908-b922-7f08c7092310
⛔ Files ignored due to path filters (3)
sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/agent_eval/trials.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/values/evidence.pyis excluded by!sdk/**uv.lockis excluded by!**/*.lock
📒 Files selected for processing (24)
packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/trials.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/values/evidence.pypackages/nemo_platform/pyproject.tomlpackages/nmp_testing/src/nmp/testing/e2e/jobs.pyplugins/nemo-evaluator/openapi/openapi.yamlplugins/nemo-evaluator/pyproject.tomlplugins/nemo-evaluator/src/nemo_evaluator/jobs/agent_compiler.pyplugins/nemo-evaluator/src/nemo_evaluator/jobs/agent_evaluate.pyplugins/nemo-evaluator/src/nemo_evaluator/jobs/agent_spec.pyplugins/nemo-evaluator/src/nemo_evaluator/jobs/compiler.pyplugins/nemo-evaluator/src/nemo_evaluator/jobs/evaluate.pyplugins/nemo-evaluator/src/nemo_evaluator/jobs/metric_resolution.pyplugins/nemo-evaluator/src/nemo_evaluator/jobs/secret_env.pyplugins/nemo-evaluator/src/nemo_evaluator/metric_refs.pyplugins/nemo-evaluator/src/nemo_evaluator/resolvers.pyplugins/nemo-evaluator/src/nemo_evaluator/service.pyplugins/nemo-evaluator/src/nemo_evaluator/tasks/agent_evaluate.pyplugins/nemo-evaluator/src/nemo_evaluator/tasks/evaluate.pyplugins/nemo-evaluator/src/nemo_evaluator/tasks/runner.pyplugins/nemo-evaluator/tests/integration/conftest.pyplugins/nemo-evaluator/tests/integration/test_agent_evaluate_job.pyplugins/nemo-evaluator/tests/test_agent_evaluate.pyplugins/nemo-evaluator/tests/test_evaluate_job.pyplugins/nemo-evaluator/tests/test_service.py
✅ Files skipped from review due to trivial changes (3)
- plugins/nemo-evaluator/src/nemo_evaluator/tasks/agent_evaluate.py
- plugins/nemo-evaluator/src/nemo_evaluator/metric_refs.py
- plugins/nemo-evaluator/src/nemo_evaluator/resolvers.py
🚧 Files skipped from review as they are similar to previous changes (17)
- packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/trials.py
- plugins/nemo-evaluator/tests/test_service.py
- packages/nemo_platform/pyproject.toml
- plugins/nemo-evaluator/src/nemo_evaluator/service.py
- plugins/nemo-evaluator/src/nemo_evaluator/tasks/evaluate.py
- plugins/nemo-evaluator/src/nemo_evaluator/jobs/secret_env.py
- packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/values/evidence.py
- packages/nmp_testing/src/nmp/testing/e2e/jobs.py
- plugins/nemo-evaluator/pyproject.toml
- plugins/nemo-evaluator/src/nemo_evaluator/jobs/agent_spec.py
- plugins/nemo-evaluator/src/nemo_evaluator/tasks/runner.py
- plugins/nemo-evaluator/src/nemo_evaluator/jobs/compiler.py
- plugins/nemo-evaluator/src/nemo_evaluator/jobs/agent_compiler.py
- plugins/nemo-evaluator/src/nemo_evaluator/jobs/metric_resolution.py
- plugins/nemo-evaluator/tests/test_evaluate_job.py
- plugins/nemo-evaluator/src/nemo_evaluator/jobs/agent_evaluate.py
- plugins/nemo-evaluator/src/nemo_evaluator/jobs/evaluate.py
SandyChapman
force-pushed
the
aalgo-297-agent-eval-job/schapman
branch
from
June 29, 2026 15:08
181cdc5 to
9b9b1c1
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
plugins/nemo-evaluator/openapi/openapi.yaml (2)
1032-1079: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winOpenAPI still doesn't enforce the
target/trialsXOR.Both schemas only require
tasks, so clients can send neither source or both and discover the real contract only at runtime. Add a schema-level exclusivity rule that requires exactly one oftargetortrials.Also applies to: 1080-1127
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/nemo-evaluator/openapi/openapi.yaml` around lines 1032 - 1079, The AgentEvalInputSpec schema still allows both target and trials to be omitted or sent together; update the OpenAPI definition for AgentEvalInputSpec to enforce an exact one-of XOR between the target and trials fields. Add the exclusivity rule in the schema itself, using the existing AgentEvalInputSpec, target, and trials properties so clients are validated before runtime, while preserving the current tasks requirement.
1903-1935: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRequire
refordataonEvidenceDescriptor.The description says one of them must be present, but the schema still accepts
{kind: ...}. That leaves generated clients able to submit evidence descriptors that cannot locate any payload.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/nemo-evaluator/openapi/openapi.yaml` around lines 1903 - 1935, Update the EvidenceDescriptor schema so it cannot validate with only kind present; keep EvidenceDescriptor as the target symbol and add a requirement that at least one of ref or data must be supplied, using schema constraints such as oneOf/anyOf or equivalent required rules. Ensure the change preserves the existing properties and description while making the payload location mandatory through either ref or data.plugins/nemo-evaluator/tests/integration/test_agent_evaluate_job.py (1)
72-76: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winGate
requires_codexon auth too.
shutil.which("codex")lets logged-out hosts run these tests even though they need an authenticated CLI session. Make_codex_available()also check a non-interactive auth command before enabling the marker.#!/bin/bash if ! command -v codex >/dev/null 2>&1; then exit 1 fi codex login status >/dev/null 2>&1🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/nemo-evaluator/tests/integration/test_agent_evaluate_job.py` around lines 72 - 76, The requires_codex skip marker is only checking for the codex binary, so logged-out machines can still run auth-dependent tests. Update _codex_available() in test_agent_evaluate_job.py to also verify a non-interactive codex auth status check after shutil.which("codex"), and keep requires_codex based on that combined result so the marker only enables when the CLI is both installed and authenticated.plugins/nemo-evaluator/tests/integration/conftest.py (1)
126-130: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse the effective mock-provider prefix everywhere.
pytest_configure()preserves a caller-suppliedNMP_INFERENCE_GATEWAY_MOCK_PROVIDER_PREFIX, but both platform fixtures still hardcodeigw-mock-. If that env var is overridden,add_mock_provider()and the spawned platform resolve different namespaces.Suggested fix
env_overrides={ "NMP_CONFIG_FILE_PATH": str(config_path), - MOCK_PROVIDER_PREFIX_ENVVAR: MOCK_PROVIDER_PREFIX, + MOCK_PROVIDER_PREFIX_ENVVAR: os.environ[MOCK_PROVIDER_PREFIX_ENVVAR], }, @@ env_overrides={ "NMP_CONFIG_FILE_PATH": str(config_path), - MOCK_PROVIDER_PREFIX_ENVVAR: MOCK_PROVIDER_PREFIX, + MOCK_PROVIDER_PREFIX_ENVVAR: os.environ[MOCK_PROVIDER_PREFIX_ENVVAR], },Also applies to: 189-192, 211-214
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/nemo-evaluator/tests/integration/conftest.py` around lines 126 - 130, Use the effective mock-provider prefix consistently across the integration test setup: `pytest_configure()` already preserves `NMP_INFERENCE_GATEWAY_MOCK_PROVIDER_PREFIX`, but the platform fixtures still hardcode `igw-mock-`, causing `add_mock_provider()` and the spawned platform to use different namespaces when the env var is overridden. Update the relevant fixtures and helpers to read the prefix from the env var (falling back to the default only when unset) so the same resolved prefix is used everywhere, including the code paths referenced by `add_mock_provider()` and the platform startup logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/nemo-evaluator/openapi/openapi.yaml`:
- Around line 991-1019: Update the Agent schema in openapi.yaml so the generic
execution path is enforced structurally, not just described: when Agent.format
is generic (the default), require both body and response_path alongside url and
name. Use the Agent schema’s format/body/response_path definitions to encode
this with conditional validation or equivalent schema constraints so invalid
generic payloads are rejected at validation time instead of failing later.
---
Duplicate comments:
In `@plugins/nemo-evaluator/openapi/openapi.yaml`:
- Around line 1032-1079: The AgentEvalInputSpec schema still allows both target
and trials to be omitted or sent together; update the OpenAPI definition for
AgentEvalInputSpec to enforce an exact one-of XOR between the target and trials
fields. Add the exclusivity rule in the schema itself, using the existing
AgentEvalInputSpec, target, and trials properties so clients are validated
before runtime, while preserving the current tasks requirement.
- Around line 1903-1935: Update the EvidenceDescriptor schema so it cannot
validate with only kind present; keep EvidenceDescriptor as the target symbol
and add a requirement that at least one of ref or data must be supplied, using
schema constraints such as oneOf/anyOf or equivalent required rules. Ensure the
change preserves the existing properties and description while making the
payload location mandatory through either ref or data.
In `@plugins/nemo-evaluator/tests/integration/conftest.py`:
- Around line 126-130: Use the effective mock-provider prefix consistently
across the integration test setup: `pytest_configure()` already preserves
`NMP_INFERENCE_GATEWAY_MOCK_PROVIDER_PREFIX`, but the platform fixtures still
hardcode `igw-mock-`, causing `add_mock_provider()` and the spawned platform to
use different namespaces when the env var is overridden. Update the relevant
fixtures and helpers to read the prefix from the env var (falling back to the
default only when unset) so the same resolved prefix is used everywhere,
including the code paths referenced by `add_mock_provider()` and the platform
startup logic.
In `@plugins/nemo-evaluator/tests/integration/test_agent_evaluate_job.py`:
- Around line 72-76: The requires_codex skip marker is only checking for the
codex binary, so logged-out machines can still run auth-dependent tests. Update
_codex_available() in test_agent_evaluate_job.py to also verify a
non-interactive codex auth status check after shutil.which("codex"), and keep
requires_codex based on that combined result so the marker only enables when the
CLI is both installed and authenticated.
🪄 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: ddd21736-a1d5-456d-9e57-7b64ffa50b0a
⛔ Files ignored due to path filters (3)
sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/agent_eval/trials.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/values/evidence.pyis excluded by!sdk/**uv.lockis excluded by!**/*.lock
📒 Files selected for processing (24)
packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/trials.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/values/evidence.pypackages/nemo_platform/pyproject.tomlpackages/nmp_testing/src/nmp/testing/e2e/jobs.pyplugins/nemo-evaluator/openapi/openapi.yamlplugins/nemo-evaluator/pyproject.tomlplugins/nemo-evaluator/src/nemo_evaluator/jobs/agent_compiler.pyplugins/nemo-evaluator/src/nemo_evaluator/jobs/agent_evaluate.pyplugins/nemo-evaluator/src/nemo_evaluator/jobs/agent_spec.pyplugins/nemo-evaluator/src/nemo_evaluator/jobs/compiler.pyplugins/nemo-evaluator/src/nemo_evaluator/jobs/evaluate.pyplugins/nemo-evaluator/src/nemo_evaluator/jobs/metric_resolution.pyplugins/nemo-evaluator/src/nemo_evaluator/jobs/secret_env.pyplugins/nemo-evaluator/src/nemo_evaluator/metric_refs.pyplugins/nemo-evaluator/src/nemo_evaluator/resolvers.pyplugins/nemo-evaluator/src/nemo_evaluator/service.pyplugins/nemo-evaluator/src/nemo_evaluator/tasks/agent_evaluate.pyplugins/nemo-evaluator/src/nemo_evaluator/tasks/evaluate.pyplugins/nemo-evaluator/src/nemo_evaluator/tasks/runner.pyplugins/nemo-evaluator/tests/integration/conftest.pyplugins/nemo-evaluator/tests/integration/test_agent_evaluate_job.pyplugins/nemo-evaluator/tests/test_agent_evaluate.pyplugins/nemo-evaluator/tests/test_evaluate_job.pyplugins/nemo-evaluator/tests/test_service.py
🚧 Files skipped from review as they are similar to previous changes (20)
- packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/trials.py
- plugins/nemo-evaluator/src/nemo_evaluator/metric_refs.py
- plugins/nemo-evaluator/src/nemo_evaluator/jobs/secret_env.py
- packages/nemo_platform/pyproject.toml
- plugins/nemo-evaluator/src/nemo_evaluator/tasks/agent_evaluate.py
- packages/nmp_testing/src/nmp/testing/e2e/jobs.py
- plugins/nemo-evaluator/tests/test_service.py
- plugins/nemo-evaluator/src/nemo_evaluator/tasks/evaluate.py
- plugins/nemo-evaluator/src/nemo_evaluator/tasks/runner.py
- plugins/nemo-evaluator/src/nemo_evaluator/jobs/compiler.py
- plugins/nemo-evaluator/src/nemo_evaluator/resolvers.py
- plugins/nemo-evaluator/pyproject.toml
- packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/values/evidence.py
- plugins/nemo-evaluator/src/nemo_evaluator/jobs/agent_spec.py
- plugins/nemo-evaluator/src/nemo_evaluator/jobs/agent_compiler.py
- plugins/nemo-evaluator/src/nemo_evaluator/service.py
- plugins/nemo-evaluator/tests/test_evaluate_job.py
- plugins/nemo-evaluator/src/nemo_evaluator/jobs/metric_resolution.py
- plugins/nemo-evaluator/src/nemo_evaluator/jobs/agent_evaluate.py
- plugins/nemo-evaluator/src/nemo_evaluator/jobs/evaluate.py
SandyChapman
force-pushed
the
aalgo-297-agent-eval-job/schapman
branch
from
June 29, 2026 15:39
9b9b1c1 to
12b0b82
Compare
Add AgentEvalJob, a plugin-native job that runs the agent-eval SDK (AgentEvaluator) over a taskset against a model, agent, or agent runner — locally (run_local) or submitted to a backend (subprocess/docker/k8s). Target model is kind-discriminated: ModelTarget | AgentTarget | AgentRunnerTarget (CodexRunnerTarget today), unioned as Target. The declarative *Spec docs keep the prompt where it belongs — on the model target — instead of a job-level prompt_template. A spec must provide exactly one trial source: an online target, or precomputed trials for an offline eval. Online inference forwards the job's platform identity (the task SDK's X-NMP-* headers, e.g. on-behalf-of) to the evaluator so a platform-routed target acts as the job's principal; no bearer is forwarded, so the platform token never leaks to a third-party endpoint. (Bearer auth for auth-enabled deployments is a follow-up.) Shared plumbing extracted while wiring this up: - tasks/runner.py: one container entrypoint (SIGTERM, task SDK, dispatch) shared by both evaluator jobs; per-job modules are thin python -m targets. - secret_env.py: build_task_environment() for metric/endpoint secrets. - metric_resolution.py: stored-ref + model-ref resolution shared by both jobs; refs need a real AsyncNeMoPlatform, model resolution duck-types sync/async via a runtime_checkable ModelResolverSdk protocol. Integration tests cover the matrix: run_local (model/agent/codex via IGW mock + real codex), submit→subprocess, submit with a stored MetricRef, and submit→docker (xfail until the cpu-tasks image ships the task module). nmp-testing is now an explicit evaluator dev dependency. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Sandy Chapman <schapman@nvidia.com>
SandyChapman
force-pushed
the
aalgo-297-agent-eval-job/schapman
branch
from
June 29, 2026 16:07
12b0b82 to
6bd1aa5
Compare
- Security: only forward the job's X-NMP identity headers to a *platform-routed* target (host matches the SDK base URL / IGW); a third-party target the user configured gets none, so the delegated on-behalf-of id/email/groups PII never leaves the platform. A runner (no HTTP endpoint) forwards nothing. - agent-eval spec: rename `parallelism` -> `max_concurrent_tasks` to disambiguate from a target's `params.parallelism` (concurrent inference requests). - target/trials oneOf: exclude an explicit `null` so the OpenAPI XOR matches the runtime validator (which keys off non-null, not mere key presence). - AgentOutput.response: dict -> JsonValue (accept a raw JSON string/array, not only a structured object). - to_spec: narrow async_sdk to AsyncNeMoPlatform | None (the sync arm was unused and unawaitable in an async resolver). - Naming: ModelResolverSdk -> ModelResolverSDK. - agent_compiler: extract image/entrypoint/command to constants. - Drop the "follow-up" editorializing from the module docstring. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Sandy Chapman <schapman@nvidia.com>
Follow-up to fca4215: `ruff format` agent_evaluate.py (only `ruff check` was run), and `make vendor` to sync the vendored SDK copy of agent_eval/trials.py after the AgentOutput.response -> JsonValue change. Fixes lint-python-style / lint-sdk-vendored. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Sandy Chapman <schapman@nvidia.com>
arpitsardhana
approved these changes
Jun 30, 2026
This was referenced Jul 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Adds
AgentEvalJob— a plugin-native job that runs the agent-eval SDK (AgentEvaluator) over a taskset against a model, agent, or agent runner, either locally (run_local) or submitted to a backend (subprocess / docker / k8s). This is the AALGO-297 "agent evaluation as a plugin job" work.Design notes
ModelTarget | AgentTarget | AgentRunnerTarget(CodexRunnerTargettoday), unioned asTarget. The prompt lives where it belongs — on the model target — rather than as a job-levelprompt_template. A spec must provide exactly one trial source: an onlinetarget, or precomputedtrialsfor an offline eval.X-NMP-*headers — includingX-NMP-Principal-Id: service:evaluator) to the evaluator, so a platform-routed target acts as the job's principal. No bearer is forwarded, so the platform token never leaks to a third-party endpoint. Verified against an auth-enabled platform: these service-principal headers are sufficient (the PDP grantsservice:*full permissions) — no bearer is needed. Identity propagation happens at run time (the on-behalf-of principal only exists when the task runs), and rides a genericdefault_headersseam, so the evaluator SDK stays platform-agnostic.Shared plumbing extracted
tasks/runner.py— one container entrypoint (SIGTERM handling, task SDK build, dispatch) shared by both evaluator jobs; per-job modules are thinpython -mtargets.secret_env.py—build_task_environment()for metric/endpoint secrets.metric_resolution.py— stored-ref + model-ref resolution shared by both jobs. Stored refs need a realAsyncNeMoPlatform; model resolution duck-types sync/async clients via aruntime_checkableModelResolverSdkprotocol.Testing
Unit: full evaluator suite green (352 passed). Integration matrix (real platform fixtures):
run_localmodel / agent (IGW mock) / codex (real)MetricRefauth.enabled→ IGW (forwarded identity authenticates online inference end-to-end)cpu-tasksimage ships the task module)Auth-sufficiency probed directly too: a call to the IGW model route with the service-principal headers returns 200, the same call with no auth returns 401 — so the forwarding is both necessary and sufficient under auth.
nmp-testingis now an explicit evaluator dev dependency.Follow-ups (Monday)
/agent-evaluate/jobsroutes.intentleak into the agent prompt).tydiagnostics intest_evaluate_job.py(format="nim"literal,unbundle_metric(MetricInline)) are unrelated to this change.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests