feat(evaluator): port metric output protocol by SandyChapman · Pull Request #6 · NVIDIA-NeMo/nemo-platform

SandyChapman · 2026-05-21T14:13:28Z

Summary

Ports the Metric protocol contract from NVIDIA-NeMo/Evaluator#950 into nemo_evaluator_sdk.

This intentionally replaces the SDK runtime metric author contract with:

compute_scores(input: MetricInput) -> MetricResult(outputs=...)
output_spec() -> list[MetricOutputSpec]

The old SDK metric author contract using compute_scores(item, sample), score_names(), and MetricResult(scores=...) is removed.

What Changed

Adds SDK-owned metric protocol types in nemo_evaluator_sdk.metrics.protocol.
Plumbs MetricInput / MetricResult(outputs=...) through local metric execution, benchmark execution, row results, aggregation, exports, and service integration.
Migrates built-in SDK metrics to the new protocol, including deterministic metrics, BLEU/ROUGE/F1, LLM judge, RAGAS, remote metrics, and tool-calling metrics.
Moves shared aggregation helpers into the SDK and updates the Evaluator service to import them directly.
Keeps MetricOutputSpec focused on output contract shape only. LLM judge rubric metrics emit a numeric output plus a companion label output, e.g. quality and quality.label; rubric aggregate metadata is derived separately from metric score config at aggregation time.
Refreshes generated OpenAPI, web SDK, Python SDK, and the vendored beta Evaluator SDK copy.
Fixes the repo-root unit-suite collection path for SDK metric helper imports and tolerates both argparse choice renderings in the platform startup test.

Commit Structure

This PR was originally developed as a layered stack, but has been squashed for review into a single commit:

feat(evaluator): port metric output protocol

Validation

LOG_FORMAT=plain make test-unit-ci
- 9382 passed, 57 skipped, 2 xfailed, 9 subtests passed
uv run --frozen pytest packages/nemo_evaluator_sdk/tests -q
- 791 passed, 1 warning
uv run --frozen pytest plugins/nemo-evaluator/tests -q
- 135 passed, 5 warnings
uv run --frozen pytest services/evaluator/tests/nmp/evaluator/api/v2/metrics/schemas/test_evaluation.py services/evaluator/tests/nmp/evaluator/app/metrics/test_metric_protocol.py -q
- 50 passed
uv run --frozen ruff check
uv run --frozen ruff format --check
Targeted ty check on PR-owned SDK/service/plugin files
bash tools/lint/lint-openapi.sh
bash tools/lint/lint-python-sdk.sh
mise exec [email protected] -- pnpm gen:check

Notes

This is intentionally breaking for SDK metric authors.
Aggregation is derived from numeric/boolean output specs.
Non-score outputs are preserved in row-level results and exports, but excluded from aggregate statistics.
Full-repo ty check still has unrelated baseline diagnostics outside this PR; targeted typecheck on PR-owned files passes.

github-actions · 2026-05-21T14:29:50Z

Suite	Lines Covered	Line Rate	Branch Rate
Unit Tests	18202/24152	75.4%	61.8%
Integration Tests	11645/22934	50.8%	25.9%

github-actions · 2026-05-21T17:55:30Z

Documentation preview is ready

Preview: https://jubilant-adventure-g4rv38m.pages.github.io/pr-preview/pr-6/pr-6/

Built from 5fcb878 in workflow run.

This preview is deployed from this PR branch, updates when docs changes are pushed, and will be removed when the PR closes.

Signed-off-by: Sandy Chapman <[email protected]>

* fix: address open CodeQL alerts in TypeScript code Close 21 open CodeQL alerts on main: Security - LargeFileWorker: remove dead `download` (untrusted-URL fetch) and `upload` actions; only `downloadAsFile` (SDK path-based) is used by callers. Closes #4 (client-side-request-forgery) and #17 (missing-origin-check). - orval/generate.ts: use `fs.mkdtempSync` for the OpenAPI spec temp file instead of a predictable `os.tmpdir()` path. Closes #5 (insecure-temporary-file). Code-quality - Drop redundant `this.page = page` / `this.request = request` in 11 e2e-tests classes — TS parameter properties (`public readonly page: Page`, `private request: APIRequestContext`) already assign the field. Closes #22-#32 (useless-assignment-to-property). - Drop redundant null/undefined checks after narrowing in ReportTraceModal/utils, BenchmarkDetailsPanel, api/intake/utils, ActionMenu, useSubmitICLsFile. Closes #33-#37. - SafeSynthesizerJobReportRoute/util: drop unreachable `else if (score >= 8)` branches and the dead `UNAVAILABLE` fallback; add explicit `Number.isNaN` guard at the top of each grading helper. Closes #20, #21. - WorkspaceDashboardRoute: drop inner `MODEL_COMPARE_ENABLED ? a : b` ternary that always picked `a` (lives inside an outer `MODEL_COMPARE_ENABLED &&` guard); drop now-unused `getWorkspaceBaseModelsRoute` import. Closes #19. Signed-off-by: mschwab <[email protected]> * fix: refactor remaining CodeQL-flagged build scripts to argv form Drop shell interpolation in dev/build scripts so user-supplied branch names, commit hashes, paths, and env values cannot be parsed as shell syntax. Also plug a TOCTOU and add origin allowlists for the two http-to-file fetches. - scripts/cherry-pick.ts: route every git call through execFileSync('git', [...]). Closes #6-#10 (indirect-cmd-line-injection). - scripts/git-utils.ts: openBrowser uses execFile + argv array; status/branch helpers use execFileSync with argv. Removes the brittle " → \" escape and the shell-interpolated browser command. Closes #1 (incomplete-sanitization) and #11 (indirect-cmd-line-injection). - sdk/orval/format-generated.ts: prettier runs via execFileSync. Closes #2 (shell-cmd-injection-from-env) and #13 (indirect-cmd-line-injection). - sdk/orval/generate.ts: orval runs via execFileSync, with its parameters passed in env instead of interpolated into a shell string; remote spec fetches are restricted to an allowlist of github/gitlab hosts; the existsSync+readFileSync TOCTOU in postProcessZodFiles is collapsed into a single try/catch on ENOENT. Closes #3 (file-system-race), #12 (indirect-cmd-line-injection), and #14 (http-to-file-access). - studio/scripts/fetch-styles.ts: validate that the fetch URL hostname matches the configured Kaizen CDN before fetching. Closes #15 (http-to-file-access). Signed-off-by: mschwab <[email protected]> * fix: close remaining CodeQL alerts re-emitted on PR scan - scripts/git-utils.ts openBrowser: parse URL with `new URL()` and require http/https before spawning. Replace the Windows `cmd /c start` shell invocation with `rundll32 url.dll,FileProtocolHandler` so no branch goes through a shell. Pass `--` separator on darwin/linux so a URL starting with `-` cannot be parsed as an option. Closes #3951. - sdk/orval/generate.ts: delete the unused HTTP-fetch branch from `getFile()`. All current `serviceConfigs` reference local YAML paths, so the network->file write CodeQL flagged on line 131 (#14) no longer exists. Throws a clear error if a remote URL is configured. Signed-off-by: mschwab <[email protected]> * fix: drop -- separator for xdg-open xdg-open does not honor -- as an option terminator; passing it as an arg caused openBrowser to fail on Linux. URL is already validated to http(s), so the separator wasn't load-bearing — just drop it on the Linux branch. Codex review on PR #75. Signed-off-by: mschwab <[email protected]> * fix: address CodeRabbit findings on PR #75 - scripts/git-utils.ts: drop `--` from macOS `open` argv too. `open`'s man page does not document `--` as an end-of-options separator. URL is already validated to http(s), so the separator wasn't load-bearing. - sdk/orval/format-generated.ts: on Windows, run prettier through `cmd.exe /c` so the `prettier.cmd` shim resolves. `execFileSync` on Windows cannot launch .cmd shims directly. - sdk/orval/generate.ts: same Windows wrap for `pnpm exec orval`. Signed-off-by: mschwab <[email protected]> * fix: validate format-generated.ts servicePath argv The Windows cmd.exe /c wrap added in ec7aa93 re-opened a CodeQL data-flow finding (#3961, #3962) because generatedPath traces back to process.argv[2]. Validate the argv against a safe-char regex at entry so CodeQL sees it as sanitized before it flows into argv or paths. Signed-off-by: mschwab <[email protected]> * fix: replace regex with hardcoded Set allowlist for servicePath CodeQL did not recognize the regex check as a sanitizer; switching to a hardcoded Set lookup against known serviceConfigs paths so the data flow is reducible to a finite set of literal values. Signed-off-by: mschwab <[email protected]> * fix: use prettier Node API instead of subprocess Replace the prettier CLI invocation with prettier's programmatic format/resolveConfig/getFileInfo API. No subprocess means no cmd.exe wrap, no command-line argument flow, and the CodeQL indirect-command-line-injection / shell-cmd-injection-from-env alerts on format-generated.ts can resolve. Also fixes the Windows .cmd shim resolution problem CR raised, since prettier now runs in-process. The servicePath argv is still validated against a hardcoded Set of known serviceConfigs paths to prevent directory traversal via path.join. Signed-off-by: mschwab <[email protected]> * fix: use readdirSync withFileTypes to avoid statSync TOCTOU CodeQL flagged the statSync -> readFileSync / writeFileSync pair in formatWithPrettier as a file-system-race. Getting Dirent entries from readdirSync(dir, { withFileTypes: true }) lets us check isDirectory / isFile inline without a separate stat round-trip, closing the alert. Signed-off-by: mschwab <[email protected]> * fix: drop remaining statSync usages in format-generated.ts Codex flagged that getTsFiles and splitZodTagFilesIn still used the readdir-string + statSync pattern, leaving two more file-system-race sinks even after formatWithPrettier was converted. Switch both to readdirSync(dir, { withFileTypes: true }) and use Dirent.isFile() / isDirectory() inline. Removes the last statSync from this script. Signed-off-by: mschwab <[email protected]> --------- Signed-off-by: mschwab <[email protected]>

* fix: address open CodeQL alerts in TypeScript code Close 21 open CodeQL alerts on main: Security - LargeFileWorker: remove dead `download` (untrusted-URL fetch) and `upload` actions; only `downloadAsFile` (SDK path-based) is used by callers. Closes #4 (client-side-request-forgery) and #17 (missing-origin-check). - orval/generate.ts: use `fs.mkdtempSync` for the OpenAPI spec temp file instead of a predictable `os.tmpdir()` path. Closes #5 (insecure-temporary-file). Code-quality - Drop redundant `this.page = page` / `this.request = request` in 11 e2e-tests classes — TS parameter properties (`public readonly page: Page`, `private request: APIRequestContext`) already assign the field. Closes #22-#32 (useless-assignment-to-property). - Drop redundant null/undefined checks after narrowing in ReportTraceModal/utils, BenchmarkDetailsPanel, api/intake/utils, ActionMenu, useSubmitICLsFile. Closes #33-#37. - SafeSynthesizerJobReportRoute/util: drop unreachable `else if (score >= 8)` branches and the dead `UNAVAILABLE` fallback; add explicit `Number.isNaN` guard at the top of each grading helper. Closes #20, #21. - WorkspaceDashboardRoute: drop inner `MODEL_COMPARE_ENABLED ? a : b` ternary that always picked `a` (lives inside an outer `MODEL_COMPARE_ENABLED &&` guard); drop now-unused `getWorkspaceBaseModelsRoute` import. Closes #19. Signed-off-by: mschwab <[email protected]> * fix: refactor remaining CodeQL-flagged build scripts to argv form Drop shell interpolation in dev/build scripts so user-supplied branch names, commit hashes, paths, and env values cannot be parsed as shell syntax. Also plug a TOCTOU and add origin allowlists for the two http-to-file fetches. - scripts/cherry-pick.ts: route every git call through execFileSync('git', [...]). Closes #6-#10 (indirect-cmd-line-injection). - scripts/git-utils.ts: openBrowser uses execFile + argv array; status/branch helpers use execFileSync with argv. Removes the brittle " → \" escape and the shell-interpolated browser command. Closes #1 (incomplete-sanitization) and #11 (indirect-cmd-line-injection). - sdk/orval/format-generated.ts: prettier runs via execFileSync. Closes #2 (shell-cmd-injection-from-env) and #13 (indirect-cmd-line-injection). - sdk/orval/generate.ts: orval runs via execFileSync, with its parameters passed in env instead of interpolated into a shell string; remote spec fetches are restricted to an allowlist of github/gitlab hosts; the existsSync+readFileSync TOCTOU in postProcessZodFiles is collapsed into a single try/catch on ENOENT. Closes #3 (file-system-race), #12 (indirect-cmd-line-injection), and #14 (http-to-file-access). - studio/scripts/fetch-styles.ts: validate that the fetch URL hostname matches the configured Kaizen CDN before fetching. Closes #15 (http-to-file-access). Signed-off-by: mschwab <[email protected]> * fix: close remaining CodeQL alerts re-emitted on PR scan - scripts/git-utils.ts openBrowser: parse URL with `new URL()` and require http/https before spawning. Replace the Windows `cmd /c start` shell invocation with `rundll32 url.dll,FileProtocolHandler` so no branch goes through a shell. Pass `--` separator on darwin/linux so a URL starting with `-` cannot be parsed as an option. Closes #3951. - sdk/orval/generate.ts: delete the unused HTTP-fetch branch from `getFile()`. All current `serviceConfigs` reference local YAML paths, so the network->file write CodeQL flagged on line 131 (#14) no longer exists. Throws a clear error if a remote URL is configured. Signed-off-by: mschwab <[email protected]> * fix: drop -- separator for xdg-open xdg-open does not honor -- as an option terminator; passing it as an arg caused openBrowser to fail on Linux. URL is already validated to http(s), so the separator wasn't load-bearing — just drop it on the Linux branch. Codex review on PR #75. Signed-off-by: mschwab <[email protected]> * fix: address CodeRabbit findings on PR #75 - scripts/git-utils.ts: drop `--` from macOS `open` argv too. `open`'s man page does not document `--` as an end-of-options separator. URL is already validated to http(s), so the separator wasn't load-bearing. - sdk/orval/format-generated.ts: on Windows, run prettier through `cmd.exe /c` so the `prettier.cmd` shim resolves. `execFileSync` on Windows cannot launch .cmd shims directly. - sdk/orval/generate.ts: same Windows wrap for `pnpm exec orval`. Signed-off-by: mschwab <[email protected]> * fix: validate format-generated.ts servicePath argv The Windows cmd.exe /c wrap added in ec7aa93 re-opened a CodeQL data-flow finding (#3961, #3962) because generatedPath traces back to process.argv[2]. Validate the argv against a safe-char regex at entry so CodeQL sees it as sanitized before it flows into argv or paths. Signed-off-by: mschwab <[email protected]> * fix: replace regex with hardcoded Set allowlist for servicePath CodeQL did not recognize the regex check as a sanitizer; switching to a hardcoded Set lookup against known serviceConfigs paths so the data flow is reducible to a finite set of literal values. Signed-off-by: mschwab <[email protected]> * fix: use prettier Node API instead of subprocess Replace the prettier CLI invocation with prettier's programmatic format/resolveConfig/getFileInfo API. No subprocess means no cmd.exe wrap, no command-line argument flow, and the CodeQL indirect-command-line-injection / shell-cmd-injection-from-env alerts on format-generated.ts can resolve. Also fixes the Windows .cmd shim resolution problem CR raised, since prettier now runs in-process. The servicePath argv is still validated against a hardcoded Set of known serviceConfigs paths to prevent directory traversal via path.join. Signed-off-by: mschwab <[email protected]> * fix: use readdirSync withFileTypes to avoid statSync TOCTOU CodeQL flagged the statSync -> readFileSync / writeFileSync pair in formatWithPrettier as a file-system-race. Getting Dirent entries from readdirSync(dir, { withFileTypes: true }) lets us check isDirectory / isFile inline without a separate stat round-trip, closing the alert. Signed-off-by: mschwab <[email protected]> * fix: drop remaining statSync usages in format-generated.ts Codex flagged that getTsFiles and splitZodTagFilesIn still used the readdir-string + statSync pattern, leaving two more file-system-race sinks even after formatWithPrettier was converted. Switch both to readdirSync(dir, { withFileTypes: true }) and use Dirent.isFile() / isDirectory() inline. Removes the last statSync from this script. Signed-off-by: mschwab <[email protected]> --------- Signed-off-by: mschwab <[email protected]> Signed-off-by: Alex Ray <[email protected]>

Composition over inheritance for the k8s reconcilers (review #2/#3): * Extract StatusProjector (pod-status projection, crash-loop/pending-timeout error builders, host URL) and ResourceDeleter (idempotent 404-tolerant delete) as standalone collaborators. * Reconciler is now a pure interface (the 5 verbs); NimOperatorReconciler and K8sReconciler compose the projector + deleter instead of inheriting them. The backend builds both collaborators in init() and injects them. Thread the reconcile context through the backend interface (review #19): * create/update/get_model_deployment_status now take a single ctx: ModelContext instead of (deployment, config, model_entity); applied across the ServiceBackend ABC and the docker / none / k8s backends, the deployment reconciler call sites, and the test mocks. delete stays (workspace, name). Fixes + nits: * Harden NIMService status read against a null status/state (review #15): (nim_status.get("state") or "").lower() can no longer raise. * Convert nim_operator logging to structured extra={} (review #13); avoid the reserved LogRecord 'name' key (use resource_name / deployment_name). * Flatten the Files-service create/update branches into a guard-clause helper (review #14). * compile_puller_job: rename args -> container_args (review #17). * Reconciler nits: import the vllm_k8s_compiler module under its full name (review #7), reflow the P3 (a)/(b) comment (review #8), quote values in the model-source error (review #9), drop the _ = image_pull_secrets dance (review #12), name the event-message cap MAX_EVENT_MESSAGE_CHARS (review #6), and document the _select_reconciler None contract (review #16). Signed-off-by: Ben McCown <[email protected]>

* feat(evaluator): add publish_to_intake entrypoint (D9) publish_to_intake(result, *, platform, experiment_id, workspace=None, ...) is the explicit, post-run consumer of AgentEvalResult. Per trial it POSTs the ATIF trajectory, resolves the root span via the traces query-back (§3.5 option 1), then POSTs one evaluator-result per metric output. All request shapes come from the D8 mapping module; wire calls go through the generated SDK intake resources. - Async, bounded-concurrent across trials (asyncio.gather + semaphore). - References an existing Experiment; never creates one (caller pre-creates via the Experiments SDK). Agent identity is taken as arguments since it lives on the run target, which AgentEvalResult does not carry (§3.9 NVIDIA-NeMo#6). - Returns a PublishReport (per-trial session/span ids + result counts). - Raises on HTTP/validation failure; the local bundle is never touched. Unit-tested with a fake async client (round-trip, multi-trial, score-less trial, workspace resolution, span-resolution failure, ingest-failure propagation). Manually validated end-to-end against live Intake + ClickHouse. Refs: AALGO-290 Co-Authored-By: Claude Opus 4.8 <[email protected]> Signed-off-by: Sandy Chapman <[email protected]> * test(evaluator): add live Intake integration test for publish_to_intake (D9) Stands up ClickHouse (Docker) + the platform (auth,entities,intake) via session fixtures, creates an Experiment, runs publish_to_intake, and asserts the full round-trip back through the public Intake API: experiment_context propagation (experiment_id + test_case_id), root-span resolution, and every evaluator-result field/data_type (NUMERIC/BOOLEAN/TEXT, including false->0.0). Marked 'integration' (registered in the plugin pyproject), so it runs under make test-integration / -m integration and is excluded from the unit suite; it skips cleanly when Docker is unavailable, mirroring the intake conftest's _docker_available() gate. Verified end-to-end locally (~18s). Refs: AALGO-290 Co-Authored-By: Claude Opus 4.8 <[email protected]> Signed-off-by: Sandy Chapman <[email protected]> --------- Signed-off-by: Sandy Chapman <[email protected]> Co-authored-by: Claude Opus 4.8 <[email protected]>

#3: add is_external_agent(agent) (backend, mirrors is_container_deployment_mode) and isExternalAgent() (frontend helper); replace the 4 backend + 6 frontend inline `source == "external"` / `source === 'external'` checks so the next AgentSource value only touches one predicate. #6: extract the external-agent logs notice into a reusable ExternalAgentNotice component + a shared EXTERNAL_AGENT_HEADING constant, so the "runs outside NeMo Platform" phrasing lives in one place. Signed-off-by: mschwab <[email protected]>

SandyChapman self-assigned this May 21, 2026

SandyChapman force-pushed the metric-protocol-contract/schapman branch 7 times, most recently from 745bf02 to 3ca97f2 Compare May 21, 2026 17:53

SandyChapman force-pushed the metric-protocol-contract/schapman branch from 3ca97f2 to 671b64a Compare May 21, 2026 18:12

SandyChapman marked this pull request as ready for review May 21, 2026 18:14

SandyChapman requested review from a team as code owners May 21, 2026 18:14

SandyChapman requested review from arpitsardhana and ngoncharenko May 21, 2026 18:51