feat(evaluator): persist metrics as entities with CRUD, SDK, and job refs - #379
Merged
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
|
SandyChapman
marked this pull request as ready for review
June 17, 2026 18:51
Contributor
|
Caution Review failedAn error occurred during the review process. Please try again later. 📝 WalkthroughWalkthroughAdds workspace-scoped stored metrics CRUD to the NeMo evaluator: a ChangesStored Metrics CRUD Feature
Sequence Diagram(s)sequenceDiagram
participant SDK as EvaluatorMetricsResource
participant Route as POST /metrics/{name}
participant MetricService
participant EntityClient
participant BundleStorage as metric_storage / Files SDK
SDK->>Route: POST workspace/metrics/{name} (MetricInline JSON)
Route->>MetricService: create_metric(name, MetricInline, workspace)
MetricService->>EntityClient: get entity (duplicate pre-check)
EntityClient-->>MetricService: None
MetricService->>BundleStorage: store_bundle(sdk, workspace, name, bundle)
BundleStorage-->>MetricService: bundle_ref
MetricService->>EntityClient: create MetricBundleEntity
EntityClient-->>MetricService: created entity
MetricService-->>Route: Metric schema
Route-->>SDK: 201 Metric JSON
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 5
🧹 Nitpick comments (1)
plugins/nemo-evaluator/src/nemo_evaluator/api/v2/metrics.py (1)
148-152: 💤 Low valueMissing return type annotation on
delete_metric.async def delete_metric( workspace: str, name: str, service: MetricService = Depends(get_metric_service), -): +) -> 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-evaluator/src/nemo_evaluator/api/v2/metrics.py` around lines 148 - 152, The delete_metric async function is missing a return type annotation on its function signature. Add the appropriate return type annotation after the closing parenthesis of the parameters and before the colon, specifying what type this async function returns. Refer to other similar delete endpoint functions in the same file to determine the correct return type pattern to follow.
🤖 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 590-598: The DELETE /metrics/{name} endpoint specification is
missing a 404 response definition even though the route raises a 404 error when
the requested metric does not exist. Add a 404 response to the responses section
of this endpoint (after the existing 204 and 422 responses) with an appropriate
description indicating the metric was not found, and include the proper schema
reference for the error response.
- Around line 526-538: The POST /metrics/{name} endpoint responses section is
missing documentation for the 409 Conflict response that occurs when attempting
to create a metric that already exists. Add a new response definition for the
409 status code after the 422 Validation Error response in the responses block,
with an appropriate description indicating that the metric name already exists
and a reference to a suitable error schema such as HTTPValidationError or a
conflict-specific schema.
- Around line 558-570: The OpenAPI specification for the GET /metrics/{name}
endpoint is missing a 404 response declaration even though the route
implementation returns a 404 when a metric is not found. Add a 404 response
entry in the responses section of this endpoint (alongside the existing 200 and
422 responses) with an appropriate description like "Metric not found" and
include the content with application/json schema referencing the
HTTPValidationError component schema to match the error handling pattern used in
the 422 response.
In `@plugins/nemo-evaluator/src/nemo_evaluator/metric_storage.py`:
- Around line 68-79: The functions store_bundle, load_bundle, and
delete_bundle_by_ref currently allow raw SDK and Pydantic exceptions to
propagate without catching and normalizing them. Wrap the SDK operations (such
as sdk.files.filesets.create and sdk.files._upload_file in store_bundle) and
Pydantic operations (such as bundle.model_dump_json) in try-except blocks in all
three functions, and re-raise any caught exceptions as MetricBundleStorageError
to maintain a consistent error contract for callers.
In `@plugins/nemo-evaluator/src/nemo_evaluator/sdk/metric_resources.py`:
- Around line 87-107: The retrieve, list, and delete methods are making HTTP
calls without explicit timeouts, which can cause indefinite blocking during
network stalls. Add a timeout parameter to the self._http_client.get() call in
the retrieve method, the self._http_client.get() call in the list method, and
the self._http_client.delete() call in the delete method, using the same timeout
value that is already configured in the create method to ensure consistency
across all HTTP operations.
---
Nitpick comments:
In `@plugins/nemo-evaluator/src/nemo_evaluator/api/v2/metrics.py`:
- Around line 148-152: The delete_metric async function is missing a return type
annotation on its function signature. Add the appropriate return type annotation
after the closing parenthesis of the parameters and before the colon, specifying
what type this async function returns. Refer to other similar delete endpoint
functions in the same file to determine the correct return type pattern to
follow.
🪄 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: bc0f2687-f414-4197-a3e0-8b7cccdb822d
📒 Files selected for processing (19)
plugins/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/metric_service.pyplugins/nemo-evaluator/src/nemo_evaluator/api/v2/metrics.pyplugins/nemo-evaluator/src/nemo_evaluator/entities.pyplugins/nemo-evaluator/src/nemo_evaluator/jobs/evaluate.pyplugins/nemo-evaluator/src/nemo_evaluator/metric_refs.pyplugins/nemo-evaluator/src/nemo_evaluator/metric_storage.pyplugins/nemo-evaluator/src/nemo_evaluator/sdk/_executor.pyplugins/nemo-evaluator/src/nemo_evaluator/sdk/metric_resources.pyplugins/nemo-evaluator/src/nemo_evaluator/sdk/resources.pyplugins/nemo-evaluator/src/nemo_evaluator/service.pyplugins/nemo-evaluator/tests/api/service/test_metric_service.pyplugins/nemo-evaluator/tests/api/v2/test_metrics_routes.pyplugins/nemo-evaluator/tests/sdk/test_metric_sdk_resources.pyplugins/nemo-evaluator/tests/test_metric_entity.pyplugins/nemo-evaluator/tests/test_metric_refs.pyplugins/nemo-evaluator/tests/test_metric_storage.py
SandyChapman
force-pushed
the
metric-persistence/schapman
branch
from
June 17, 2026 19:42
00635ee to
312fc24
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
plugins/nemo-evaluator/src/nemo_evaluator/api/schemas.py (1)
43-45: 💤 Low valueDiscriminated union on single type is a no-op.
Field(discriminator="kind")has no effect when applied to a single concrete type (not aUnion). The discriminator becomes meaningful only when multiple payload types join the union. The code works but the annotation is misleading until a second kind is added.Consider documenting this explicitly or using a type alias without the discriminator until needed:
# Current: discriminator has no effect with single type MetricPayload = CloudpickleMetricPayload # alias until more kinds existNot blocking—the pattern is forward-compatible.
🤖 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/api/schemas.py` around lines 43 - 45, The MetricPayload type alias uses Field(discriminator="kind") on a single concrete type CloudpickleMetricPayload, which makes the discriminator annotation ineffective since discriminators only function with Union types containing multiple options. Simplify the MetricPayload definition by removing the Annotated wrapper and Field(discriminator="kind") to make it a straightforward type alias equal to CloudpickleMetricPayload, then update the comment above it to clarify that the discriminated union pattern will be introduced once additional metric payload kinds are added to the union.
🤖 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 1254-1257: The metadata property in the EvaluateJob schema has
been changed from an inline object type definition to an allOf reference
pointing to the MetricMetadata schema component. This is a breaking change that
will require existing API clients to update their parsing logic. Create or
update the appropriate release notes documentation to clearly communicate this
breaking change, explaining that the metadata property type representation has
changed and provide migration guidance for clients that currently parse
EvaluateJob responses.
In `@plugins/nemo-evaluator/src/nemo_evaluator/api/service/metric_service.py`:
- Around line 194-206: The delete_metric method has a race condition where
EntityNotFoundError is only caught for the initial get call, but not for the
subsequent delete call. If another request deletes the entity between the get
operation and the delete operation, the delete call will raise an unhandled
EntityNotFoundError that violates the method's contract of returning False when
not found. Wrap the entity_client.delete call and the _discard_bundle call in a
try-except block to catch EntityNotFoundError and return False, ensuring the
method handles concurrent deletes gracefully and maintains its documented
behavior.
---
Nitpick comments:
In `@plugins/nemo-evaluator/src/nemo_evaluator/api/schemas.py`:
- Around line 43-45: The MetricPayload type alias uses
Field(discriminator="kind") on a single concrete type CloudpickleMetricPayload,
which makes the discriminator annotation ineffective since discriminators only
function with Union types containing multiple options. Simplify the
MetricPayload definition by removing the Annotated wrapper and
Field(discriminator="kind") to make it a straightforward type alias equal to
CloudpickleMetricPayload, then update the comment above it to clarify that the
discriminated union pattern will be introduced once additional metric payload
kinds are added to the union.
🪄 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: 551edfe3-0a71-4d51-a9a0-db151264703f
📒 Files selected for processing (19)
plugins/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/metric_service.pyplugins/nemo-evaluator/src/nemo_evaluator/api/v2/metrics.pyplugins/nemo-evaluator/src/nemo_evaluator/entities.pyplugins/nemo-evaluator/src/nemo_evaluator/jobs/evaluate.pyplugins/nemo-evaluator/src/nemo_evaluator/metric_refs.pyplugins/nemo-evaluator/src/nemo_evaluator/metric_storage.pyplugins/nemo-evaluator/src/nemo_evaluator/sdk/_executor.pyplugins/nemo-evaluator/src/nemo_evaluator/sdk/metric_resources.pyplugins/nemo-evaluator/src/nemo_evaluator/sdk/resources.pyplugins/nemo-evaluator/src/nemo_evaluator/service.pyplugins/nemo-evaluator/tests/api/service/test_metric_service.pyplugins/nemo-evaluator/tests/api/v2/test_metrics_routes.pyplugins/nemo-evaluator/tests/sdk/test_metric_sdk_resources.pyplugins/nemo-evaluator/tests/test_metric_entity.pyplugins/nemo-evaluator/tests/test_metric_refs.pyplugins/nemo-evaluator/tests/test_metric_storage.py
🚧 Files skipped from review as they are similar to previous changes (15)
- plugins/nemo-evaluator/src/nemo_evaluator/api/dependencies.py
- plugins/nemo-evaluator/src/nemo_evaluator/sdk/_executor.py
- plugins/nemo-evaluator/src/nemo_evaluator/sdk/resources.py
- plugins/nemo-evaluator/tests/test_metric_storage.py
- plugins/nemo-evaluator/tests/api/service/test_metric_service.py
- plugins/nemo-evaluator/src/nemo_evaluator/metric_refs.py
- plugins/nemo-evaluator/tests/sdk/test_metric_sdk_resources.py
- plugins/nemo-evaluator/src/nemo_evaluator/service.py
- plugins/nemo-evaluator/tests/test_metric_refs.py
- plugins/nemo-evaluator/src/nemo_evaluator/metric_storage.py
- plugins/nemo-evaluator/src/nemo_evaluator/sdk/metric_resources.py
- plugins/nemo-evaluator/src/nemo_evaluator/entities.py
- plugins/nemo-evaluator/tests/api/v2/test_metrics_routes.py
- plugins/nemo-evaluator/src/nemo_evaluator/api/v2/metrics.py
- plugins/nemo-evaluator/src/nemo_evaluator/jobs/evaluate.py
SandyChapman
force-pushed
the
metric-persistence/schapman
branch
from
June 17, 2026 20:01
312fc24 to
1f8cb86
Compare
arpitsardhana
approved these changes
Jun 19, 2026
arpitsardhana
left a comment
arpitsardhana
left a comment
Contributor
There was a problem hiding this comment.
Minor nits and suggestions
…refs Add metric persistence to the evaluator plugin so reusable metrics can be stored on the platform and referenced from eval jobs. Restores the metrics surface removed in #230, aligned to its workspace/name conventions. - Entity + storage: MetricBundleEntity (entity_type "metric_bundle") in the entity store; the executable cloudpickle bundle is uploaded to the Files service (one fileset per metric), with the entity holding bundle_ref + digest. - CRUD API (name-in-path): POST/GET/DELETE /apis/evaluator/v2/workspaces/{workspace}/metrics/{name} + list. Metrics are immutable (no update). - SDK: client.evaluator.metrics (create/retrieve/list/delete). - Job integration: EvaluateInputSpec.metrics accepts inline MetricInline and/or MetricRef (workspace/name); refs resolve from the entity store + Files during spec resolution. - Explicit CloudpickleMetricPayload schema (discriminated on kind); regenerated openapi.yaml. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Sandy Chapman <schapman@nvidia.com>
SandyChapman
force-pushed
the
metric-persistence/schapman
branch
from
June 22, 2026 13:19
1f8cb86 to
48cf854
Compare
This was referenced Jun 24, 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.
Summary
Adds metric persistence to the evaluator plugin: store reusable metrics on the platform and reference them from eval jobs. Restores the metrics surface removed in #230, aligned to its
workspace/nameconventions.What's included
MetricBundleEntity(entity_typemetric_bundle) in the entity store; the executable cloudpickle bundle is uploaded to the Files service (one fileset per metric), with the entity holding onlybundle_ref+ integrity digest.POST/GET/DELETE /apis/evaluator/v2/workspaces/{workspace}/metrics/{name}+ workspace list. Metrics are immutable (no update).client.evaluator.metrics(create/retrieve/list/delete).EvaluateInputSpec.metricsaccepts inlineMetricInlineand/orMetricRef(workspace/name); refs are resolved from the entity store + Files during server-side spec resolution (to_spec).CloudpickleMetricPayloadschema (discriminated onkind); regeneratedplugins/nemo-evaluator/openapi/openapi.yaml.Testing
MetricRefat submit time (canonical spec inlined the metric), the job ran to completion, and aggregate scores were returned.ruff,ty, andpyrightclean.Notes / follow-ups
metric-bundle.*fileset, logged) — never a corrupted/broken metric. A GC sweep is tracked as a follow-up.evaluator.metrics.*).files-API SDK drift surfaced when regenerating (tracked separately).🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Tests