feat(evaluator): persist eval results as queryable entities - #515
Merged
Conversation
SandyChapman
force-pushed
the
eval-results-persistence/schapman
branch
2 times, most recently
from
June 30, 2026 15:17
5984400 to
60e6cba
Compare
SandyChapman
marked this pull request as ready for review
June 30, 2026 17:52
SandyChapman
force-pushed
the
eval-results-persistence/schapman
branch
from
June 30, 2026 18:11
60e6cba to
dcef462
Compare
Contributor
|
arpitsardhana
approved these changes
Jun 30, 2026
arpitsardhana
left a comment
arpitsardhana
left a comment
Contributor
There was a problem hiding this comment.
LGTM with minor nits
Re-add queryable persistence of eval-job results (removed with the legacy service in #231), now on the plugin-job API. - AgentEvalResultEntity / EvaluateResultEntity store aggregated scores plus filterable target/dataset traits; the full bundle stays in the run's fileset (bundle_ref). Jobs persist best-effort in run() (a store error never fails the eval). - AgentEvalResult / EvaluateResult API DTOs (mapped from the entities) back the read routes, so id/created_at round-trip cleanly on the wire and in the SDK. - /agent-eval-results and /eval-results list/get/delete routes with trait filtering: a DataFilter base translates custom fields to data.* (MetricFilter adopts it too, fixing metric_type filtering). - client.evaluator.{agent_eval_results,eval_results} SDK resources, plus metric-type filtering on client.evaluator.metrics.list. - get_async_task_sdk: async counterpart of get_task_sdk so a sync job run() can drive the async entity-store write with the full on-behalf-of identity. Verified end-to-end on a live platform (agent-eval submit under auth, row-eval submit, metric-type filtering) plus unit coverage. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Sandy Chapman <schapman@nvidia.com>
SandyChapman
force-pushed
the
eval-results-persistence/schapman
branch
from
July 2, 2026 13:07
dcef462 to
cfc423d
Compare
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds async task SDK support, persisted evaluator result records, result read/delete APIs and SDK resources, metric filtering updates, and tests for persistence, routing, SDK encoding, and integration behavior. ChangesEvaluator Result Persistence and API
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/nemo_platform_plugin/src/nemo_platform_plugin/sdk_provider.py (1)
189-211: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate header-building logic with
get_task_sdk.Lines 168-187 and 189-211 are near-identical (principal lookup, warning, header dict). Docstring at Line 354 explains why this shouldn't wrap
get_async_platform_sdk, but a private helper local to this class (e.g._task_headers(service_name)) could still de-duplicate without going through that wrapper, reducing drift risk the parity test was written to guard against.♻️ Suggested extraction
+ `@staticmethod` + def _task_headers(service_name: str, *, async_variant: bool) -> dict[str, str]: + headers: dict[str, str] = { + "X-NMP-Principal-Id": f"service:{service_name}", + _INTERNAL_REQUEST_HEADER: "true", + } + principal = _read_principal_from_env() + if principal is not None: + headers.update(_on_behalf_of_headers(principal)) + else: + kind = "async task" if async_variant else "task" + logger.warning( + "%s not set; %s SDK will authenticate as service:%s without on-behalf-of delegation", + _NMP_PRINCIPAL_ENVVAR, kind, service_name, + ) + return headers + def get_task_sdk(self, service_name: str) -> NeMoPlatform: - headers: dict[str, str] = {...} - ... - return NeMoPlatform(base_url=self._base_url(), default_headers=headers) + return NeMoPlatform(base_url=self._base_url(), default_headers=self._task_headers(service_name, async_variant=False)) def get_async_task_sdk(self, service_name: str) -> AsyncNeMoPlatform: - headers: dict[str, str] = {...} - ... - return AsyncNeMoPlatform(base_url=self._base_url(), default_headers=headers) + return AsyncNeMoPlatform(base_url=self._base_url(), default_headers=self._task_headers(service_name, async_variant=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 `@packages/nemo_platform_plugin/src/nemo_platform_plugin/sdk_provider.py` around lines 189 - 211, The header-building logic in get_async_task_sdk is duplicated from get_task_sdk and should be extracted into a private helper on the same class, such as _task_headers(service_name), that returns the shared headers and warning behavior. Update get_task_sdk and get_async_task_sdk to both call this helper so the service principal, internal marker, and on-behalf-of delegation stay in sync without routing through get_async_platform_sdk.packages/nmp_common/src/nmp/common/sdk_factory.py (1)
194-221: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor duplication with
get_task_sdk(principal lookup + warning).Both functions repeat the
principal_from_env()/ warning-log pattern before delegating to their respective platform-sdk factory. Could extract a small shared helper, but low priority since behavior is correct and delegation targets differ (sync vs async).🤖 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/nmp_common/src/nmp/common/sdk_factory.py` around lines 194 - 221, Both get_task_sdk and get_async_task_sdk duplicate the principal_from_env lookup and missing-principal warning logic before delegating to their SDK factories. Refactor this repeated pattern into a small shared helper used by get_task_sdk and get_async_task_sdk, keeping the existing warning text and on_behalf_of behavior intact while leaving the sync/async delegation to get_platform_sdk and get_async_platform_sdk unchanged.plugins/nemo-evaluator/tests/api/v2/test_results_routes.py (1)
26-92: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate fixture scaffolding across test files.
_FakeEntityClient,_agent_entity, and_eval_entityhere are near-identical copies of the ones intest_result_service.py. Consider hoisting into a shared conftest/fixture module for theapitest tree to avoid drift between the two.🤖 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/api/v2/test_results_routes.py` around lines 26 - 92, The test helpers `_FakeEntityClient`, `_agent_entity`, and `_eval_entity` are duplicated in multiple test files and should be centralized. Move these shared fixtures into a common `conftest` or shared test utility module for the `api` test tree, then update `test_results_routes` and `test_result_service` to import and reuse the same symbols so their behavior stays in sync.plugins/nemo-evaluator/src/nemo_evaluator/sdk/result_resources.py (1)
30-38: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate
_query_paramswithmetric_resources.py's_list_params.Both build
{page, page_size, sort?, filter[...]}query dicts;metric_resources.py:29-38has the same shape for a single filter. A shared helper inhttp_utils(accepting a filters dict) would serve both call sites.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/nemo-evaluator/src/nemo_evaluator/sdk/result_resources.py` around lines 30 - 38, Consolidate the duplicated query-dict building logic in _query_params by extracting a shared helper in http_utils that builds {page, page_size, sort?, filter[...]} from a filters dict; then update _query_params in result_resources.py and _list_params in metric_resources.py to call the shared helper instead of maintaining separate implementations.
🤖 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/result_persistence.py`:
- Around line 47-61: Sanitize the target URL fields before persisting result
records, because _target_fields and _row_target_fields currently use
str(target.*.url) and may store sensitive userinfo or query tokens. Update the
URL handling in these helpers to redact credentials and sensitive query values,
or omit the URL entirely when it cannot be safely normalized. Keep the existing
tuple shape returned by _target_fields and _row_target_fields so downstream
persistence and read APIs continue to work.
---
Nitpick comments:
In `@packages/nemo_platform_plugin/src/nemo_platform_plugin/sdk_provider.py`:
- Around line 189-211: The header-building logic in get_async_task_sdk is
duplicated from get_task_sdk and should be extracted into a private helper on
the same class, such as _task_headers(service_name), that returns the shared
headers and warning behavior. Update get_task_sdk and get_async_task_sdk to both
call this helper so the service principal, internal marker, and on-behalf-of
delegation stay in sync without routing through get_async_platform_sdk.
In `@packages/nmp_common/src/nmp/common/sdk_factory.py`:
- Around line 194-221: Both get_task_sdk and get_async_task_sdk duplicate the
principal_from_env lookup and missing-principal warning logic before delegating
to their SDK factories. Refactor this repeated pattern into a small shared
helper used by get_task_sdk and get_async_task_sdk, keeping the existing warning
text and on_behalf_of behavior intact while leaving the sync/async delegation to
get_platform_sdk and get_async_platform_sdk unchanged.
In `@plugins/nemo-evaluator/src/nemo_evaluator/sdk/result_resources.py`:
- Around line 30-38: Consolidate the duplicated query-dict building logic in
_query_params by extracting a shared helper in http_utils that builds {page,
page_size, sort?, filter[...]} from a filters dict; then update _query_params in
result_resources.py and _list_params in metric_resources.py to call the shared
helper instead of maintaining separate implementations.
In `@plugins/nemo-evaluator/tests/api/v2/test_results_routes.py`:
- Around line 26-92: The test helpers `_FakeEntityClient`, `_agent_entity`, and
`_eval_entity` are duplicated in multiple test files and should be centralized.
Move these shared fixtures into a common `conftest` or shared test utility
module for the `api` test tree, then update `test_results_routes` and
`test_result_service` to import and reuse the same symbols so their behavior
stays in sync.
🪄 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: d0434803-5508-4b34-a4e3-0c3e32ea343e
📒 Files selected for processing (29)
packages/nemo_platform_plugin/src/nemo_platform_plugin/sdk_provider.pypackages/nemo_platform_plugin/tests/test_sdk_provider.pypackages/nmp_common/src/nmp/common/sdk_factory.pyplugins/nemo-evaluator/openapi/openapi.yamlplugins/nemo-evaluator/src/nemo_evaluator/api/dependencies.pyplugins/nemo-evaluator/src/nemo_evaluator/api/schemas.pyplugins/nemo-evaluator/src/nemo_evaluator/api/service/result_service.pyplugins/nemo-evaluator/src/nemo_evaluator/api/v2/results.pyplugins/nemo-evaluator/src/nemo_evaluator/entities.pyplugins/nemo-evaluator/src/nemo_evaluator/jobs/agent_evaluate.pyplugins/nemo-evaluator/src/nemo_evaluator/jobs/evaluate.pyplugins/nemo-evaluator/src/nemo_evaluator/jobs/result_persistence.pyplugins/nemo-evaluator/src/nemo_evaluator/sdk/metric_resources.pyplugins/nemo-evaluator/src/nemo_evaluator/sdk/resources.pyplugins/nemo-evaluator/src/nemo_evaluator/sdk/result_resources.pyplugins/nemo-evaluator/src/nemo_evaluator/service.pyplugins/nemo-evaluator/src/nemo_evaluator/tasks/runner.pyplugins/nemo-evaluator/tests/api/service/test_result_service.pyplugins/nemo-evaluator/tests/api/v2/test_metrics_routes.pyplugins/nemo-evaluator/tests/api/v2/test_results_routes.pyplugins/nemo-evaluator/tests/integration/test_agent_evaluate_job.pyplugins/nemo-evaluator/tests/integration/test_evaluate_job.pyplugins/nemo-evaluator/tests/integration/test_metric_filtering.pyplugins/nemo-evaluator/tests/sdk/test_metric_sdk_resources.pyplugins/nemo-evaluator/tests/sdk/test_result_sdk_resources.pyplugins/nemo-evaluator/tests/test_agent_evaluate.pyplugins/nemo-evaluator/tests/test_evaluate_job.pyplugins/nemo-evaluator/tests/test_result_entity.pyplugins/nemo-evaluator/tests/test_result_persistence.py
- Best-effort result persistence: wrap persist_agent_eval_result and persist_evaluate_result in try/except with a logged warning. The authoritative output (bundle/result artifacts) is already saved, so a persistence failure no longer fails an otherwise-successful eval job. Regression tests cover both jobs. - Results GET routes now set response_model_exclude_none=True to match the list routes' serialization. - Wrap results GET and DELETE handlers in try/except -> 500 (re-raising HTTPException so the 404 is preserved), matching the metrics routes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Sandy Chapman <schapman@nvidia.com>
target_url is stored on the result entity and returned by the read APIs, so a target endpoint carrying userinfo or a token query param would leak. Route both target-field helpers through a new _safe_target_url() that strips userinfo, redacts sensitive query values, and omits the URL when it has no host. Tuple shape is unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Sandy Chapman <schapman@nvidia.com>
…ession The negative test suppressed with `# type: ignore[call-arg]` (a mypy code ty doesn't recognize), so ty's `missing-argument` leaked through and failed the lint-python-types CI check. Use `# ty: ignore[missing-argument]`. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Sandy Chapman <schapman@nvidia.com>
SandyChapman
force-pushed
the
eval-results-persistence/schapman
branch
from
July 2, 2026 14:09
06b33ed to
042c652
Compare
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Jul 2, 2026
This was referenced Jul 6, 2026
arpitsardhana
pushed a commit
that referenced
this pull request
Jul 9, 2026
* feat(evaluator): persist eval results as queryable entities Re-add queryable persistence of eval-job results (removed with the legacy service in #231), now on the plugin-job API. - AgentEvalResultEntity / EvaluateResultEntity store aggregated scores plus filterable target/dataset traits; the full bundle stays in the run's fileset (bundle_ref). Jobs persist best-effort in run() (a store error never fails the eval). - AgentEvalResult / EvaluateResult API DTOs (mapped from the entities) back the read routes, so id/created_at round-trip cleanly on the wire and in the SDK. - /agent-eval-results and /eval-results list/get/delete routes with trait filtering: a DataFilter base translates custom fields to data.* (MetricFilter adopts it too, fixing metric_type filtering). - client.evaluator.{agent_eval_results,eval_results} SDK resources, plus metric-type filtering on client.evaluator.metrics.list. - get_async_task_sdk: async counterpart of get_task_sdk so a sync job run() can drive the async entity-store write with the full on-behalf-of identity. Verified end-to-end on a live platform (agent-eval submit under auth, row-eval submit, metric-type filtering) plus unit coverage. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Sandy Chapman <schapman@nvidia.com> * fix(evaluator): address review feedback on eval-results persistence - Best-effort result persistence: wrap persist_agent_eval_result and persist_evaluate_result in try/except with a logged warning. The authoritative output (bundle/result artifacts) is already saved, so a persistence failure no longer fails an otherwise-successful eval job. Regression tests cover both jobs. - Results GET routes now set response_model_exclude_none=True to match the list routes' serialization. - Wrap results GET and DELETE handlers in try/except -> 500 (re-raising HTTPException so the 404 is preserved), matching the metrics routes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Sandy Chapman <schapman@nvidia.com> * fix(evaluator): redact credentials from persisted target URLs target_url is stored on the result entity and returned by the read APIs, so a target endpoint carrying userinfo or a token query param would leak. Route both target-field helpers through a new _safe_target_url() that strips userinfo, redacts sensitive query values, and omits the URL when it has no host. Tuple shape is unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Sandy Chapman <schapman@nvidia.com> * fix(evaluator): use ty rule name in result-entity negative-test suppression The negative test suppressed with `# type: ignore[call-arg]` (a mypy code ty doesn't recognize), so ty's `missing-argument` leaked through and failed the lint-python-types CI check. Use `# ty: ignore[missing-argument]`. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Sandy Chapman <schapman@nvidia.com> --------- Signed-off-by: Sandy Chapman <schapman@nvidia.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Re-adds queryable persistence of eval-job results (removed with the legacy service in #231), now on the plugin-job API. Results become first-class, filterable entities again — without resurrecting the old service.
What's included
AgentEvalResultEntity/EvaluateResultEntitystore the aggregated scores plus the filterable traits (target, dataset); the full per-trial/per-row bundle stays in the run's fileset, referenced bybundle_ref. Jobs persist best-effort inrun()— a transient entity-store error never fails an eval that already succeeded.AgentEvalResult/EvaluateResult(mapped from the entities, likeMetric↔MetricBundleEntity) back the read routes, soid/created_at/updated_atround-trip cleanly on the wire and in the SDK (anEntityBasedoes not)./agent-eval-resultsand/eval-resultslist/get/delete, with trait filtering. A smallDataFilterbase translates custom fields todata.*for the entity store;MetricFilteradopts it too, which also fixesmetric_typefiltering on the existing/metricsroute (same latent bug).client.evaluator.{agent_eval_results,eval_results}resources (retrieve/list/delete, typed DTOs, trait filters), plusmetric_typefiltering onclient.evaluator.metrics.list.get_async_task_sdk— async counterpart ofget_task_sdk(protocol + both providers) so a synchronous jobrun()can drive the async entity-store write with the full on-behalf-of identity (id + email + groups).Verification
RUN_AGENT_EVAL_INTEGRATION=1): agent-eval submit under auth (proves the delegated identity authorizes the entity write), row-eval submit, and metric-type filtering. These caught three real bugs unit tests couldn't (responseexclude_noneround-trip, target wiring, custom-field filter 500).Notes
🤖 Generated with Claude Code
Summary by CodeRabbit