feat(evaluator): bundle metrics for plugin execution - #46
Merged
SandyChapman merged 2 commits intoJun 1, 2026
Conversation
SandyChapman
force-pushed
the
metric-bundling-cloudpickle-on-resolvers/schapman
branch
4 times, most recently
from
May 27, 2026 13:56
420568a to
78018ea
Compare
SandyChapman
force-pushed
the
4050-evaluator-metric-resolvers/schapman
branch
3 times, most recently
from
May 28, 2026 15:09
ea313db to
210323a
Compare
SandyChapman
force-pushed
the
metric-bundling-cloudpickle-on-resolvers/schapman
branch
from
May 28, 2026 15:15
78018ea to
859c50a
Compare
SandyChapman
force-pushed
the
4050-evaluator-metric-resolvers/schapman
branch
from
May 28, 2026 15:22
210323a to
2315857
Compare
SandyChapman
force-pushed
the
metric-bundling-cloudpickle-on-resolvers/schapman
branch
from
May 28, 2026 15:28
859c50a to
9c070c9
Compare
SandyChapman
marked this pull request as ready for review
May 28, 2026 15:33
SandyChapman
force-pushed
the
4050-evaluator-metric-resolvers/schapman
branch
5 times, most recently
from
May 29, 2026 12:22
5145619 to
9dcbc66
Compare
Make evaluator plugin job specs bundle-native so backend execution receives MetricBundle payloads, hydrates runtime metrics dynamically, and resolves platform model refs at execution time. Add cloudpickle bundle primitives, plugin compiler/runtime wiring, and local/remote smoke coverage. Signed-off-by: Sandy Chapman <schapman@nvidia.com>
SandyChapman
force-pushed
the
metric-bundling-cloudpickle-on-resolvers/schapman
branch
from
May 29, 2026 13:38
9c070c9 to
50cfeba
Compare
SandyChapman
changed the base branch from
4050-evaluator-metric-resolvers/schapman
to
main
May 29, 2026 13:38
arpitsardhana
approved these changes
May 29, 2026
arpitsardhana
left a comment
arpitsardhana
left a comment
Contributor
There was a problem hiding this comment.
Can you add how input spec will look like from user when submitting a custom metric
Contributor
Author
|
Replying to the review note: "Can you add how input spec will look like from user when submitting a custom metric" Added a small custom metric example that shows the user-facing flow: define a metric object, generate the cloudpickle bundle from code with |
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 (12)
🚧 Files skipped from review as they are similar to previous changes (9)
📝 WalkthroughWalkthroughThis PR adds a backend-neutral metric bundling system (registry, envelope, helpers), a Cloudpickle packager, changes EvaluateSpec to accept bundled metrics, adds compile_evaluate_job, updates executors to require/forward packagers and resolve FilesetRef datasets, adds a task entrypoint, updates examples, pyproject deps, license, and extensive tests to the bundled-metric flow. ChangesMetric Bundle Infrastructure and Packaging
Job Schema and Compilation Refactoring
Executor Integration and Dataset Resolution
Public API and Adapter Integration
Example Usage and Dependency Updates
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 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
plugins/nemo-evaluator/src/nemo_evaluator/sdk/standalone_sdk/backend.py (1)
33-49:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemote backend mode should reject missing
metric_bundle_packagerbefore delegating.With
execution_mode="remote"and defaultmetric_bundle_packager=None, calls fail later in executor policy checks. Validate this in backend adapter methods and raise a direct configuration error.Proposed fix
def evaluate( @@ _reject_unsupported_hooks(request) if self.execution_mode == "remote": + if self.metric_bundle_packager is None: + raise ValueError( + "metric_bundle_packager is required when execution_mode='remote'." + ) return self.resource._executor.evaluate_remote( @@ async def evaluate( @@ _reject_unsupported_hooks(request) if self.execution_mode == "remote": + if self.metric_bundle_packager is None: + raise ValueError( + "metric_bundle_packager is required when execution_mode='remote'." + ) return await self.resource._executor.evaluate_remote(Also applies to: 83-99
🤖 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/standalone_sdk/backend.py` around lines 33 - 49, The backend must validate that metric_bundle_packager is provided when execution_mode == "remote"; in the evaluate method (and the other backend adapter methods such as the batch evaluation variant around the same area) check self.execution_mode == "remote" and if so raise a clear configuration error if self.metric_bundle_packager is None instead of delegating to self.resource._executor.evaluate_remote (and the corresponding remote call in the other adapter), so callers get an immediate, descriptive exception about the missing metric_bundle_packager.plugins/nemo-evaluator/src/nemo_evaluator/sdk/resources.py (1)
79-99:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
submit()advertises an optional packager, but remote submit requires it.Both sync and async
submitdefaultmetric_bundle_packager=None, yet the downstream remote-spec path requires a concrete packager and throws at runtime. Make this fail fast at this API boundary (or make the parameter required in the signature) to avoid misleading callers.Proposed fix (fail fast at public API boundary)
def submit( @@ - metric_bundle_packager: MetricBundlePackager | None = None, + metric_bundle_packager: MetricBundlePackager | None = None, ) -> EvaluatorJobResource: @@ + if metric_bundle_packager is None: + raise ValueError( + "metric_bundle_packager is required for submit(); " + "pass CloudpickleMetricBundlePackager() to enable metric bundling." + ) return self._executor.submit( @@ async def submit( @@ - metric_bundle_packager: MetricBundlePackager | None = None, + metric_bundle_packager: MetricBundlePackager | None = None, ) -> AsyncEvaluatorJobResource: @@ + if metric_bundle_packager is None: + raise ValueError( + "metric_bundle_packager is required for submit(); " + "pass CloudpickleMetricBundlePackager() to enable metric bundling." + ) return await self._executor.submit(Also applies to: 186-206
🤖 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/resources.py` around lines 79 - 99, The submit() API in nemo_evaluator.sdk.resources currently allows metric_bundle_packager=None but downstream _executor.submit requires a concrete MetricBundlePackager and fails at runtime; update the public API to fail fast by validating metric_bundle_packager at the start of submit (and the async counterpart, same-named method around the 186-206 region) and raise a clear ValueError if it's None, or alternatively make metric_bundle_packager a required non-optional parameter in the method signature; ensure the check references the submit method and _executor.submit call so callers get an immediate, descriptive error instead of a downstream exception.
🤖 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/compiler.py`:
- Around line 48-52: The helper _add_secret_ref currently allows bundle secret
env names to collide with reserved job env vars (e.g.
PERSISTENT_JOB_STORAGE_PATH_ENVVAR); update _add_secret_ref to explicitly check
if env_name equals any reserved env var constants and raise a ValueError with a
clear message if so, then proceed with the existing conflict check and
assignment; apply the same reserved-name check to the analogous helper handling
bundle secret refs in the 63-73 region to prevent duplicate/reserved env var
collisions.
- Around line 85-86: The container task currently invokes Python directly via
entrypoint=["python","-m"] and command=["nemo_evaluator.tasks.evaluate"]; update
the entrypoint to use uv run so it becomes entrypoint=["uv","run","python","-m"]
(leave command as ["nemo_evaluator.tasks.evaluate"]) so the module is executed
through uv run; modify the entrypoint value wherever it is defined in the
compiler job (look for entrypoint and command in the code around the
container/task spec).
In `@plugins/nemo-evaluator/src/nemo_evaluator/shared/metric_bundles/bundles.py`:
- Around line 109-121: register_metric_bundle_kind currently overwrites any
existing entry in _BUNDLE_REGISTRY for the same kind; update it to prevent
silent re-registration by first checking if kind exists in _BUNDLE_REGISTRY:
retrieve existing = _BUNDLE_REGISTRY.get(kind) and if existing is None, proceed
to insert the new _MetricBundleRegistration(payload_type=payload_type,
packager_factory=packager_factory); if existing is present, compare
existing.payload_type and existing.packager_factory to the incoming payload_type
and packager_factory and if they are identical do nothing (idempotent),
otherwise raise a ValueError describing the conflicting registration for that
kind so callers cannot accidentally override implementations.
- Around line 207-212: The current check only compares output names
(output_names vs bundled_output_names) and misses structural differences; change
the validation in the bundling logic so that you compare the full hydrated
output specs returned by metric.output_spec() against bundle.outputs (not just
.name), e.g. validate equality of each output object's schema/description/fields
with the corresponding bundle.outputs entry, and raise MetricBundlingError with
the same message if any structural mismatch occurs; keep the existing metric
type check (validate_metric_type(metric) vs bundle.metric_type) intact.
---
Outside diff comments:
In `@plugins/nemo-evaluator/src/nemo_evaluator/sdk/resources.py`:
- Around line 79-99: The submit() API in nemo_evaluator.sdk.resources currently
allows metric_bundle_packager=None but downstream _executor.submit requires a
concrete MetricBundlePackager and fails at runtime; update the public API to
fail fast by validating metric_bundle_packager at the start of submit (and the
async counterpart, same-named method around the 186-206 region) and raise a
clear ValueError if it's None, or alternatively make metric_bundle_packager a
required non-optional parameter in the method signature; ensure the check
references the submit method and _executor.submit call so callers get an
immediate, descriptive error instead of a downstream exception.
In `@plugins/nemo-evaluator/src/nemo_evaluator/sdk/standalone_sdk/backend.py`:
- Around line 33-49: The backend must validate that metric_bundle_packager is
provided when execution_mode == "remote"; in the evaluate method (and the other
backend adapter methods such as the batch evaluation variant around the same
area) check self.execution_mode == "remote" and if so raise a clear
configuration error if self.metric_bundle_packager is None instead of delegating
to self.resource._executor.evaluate_remote (and the corresponding remote call in
the other adapter), so callers get an immediate, descriptive exception about the
missing metric_bundle_packager.
🪄 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: 488c6c84-830f-49a7-a104-1e6a15715d7d
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
packages/nemo_evaluator_sdk/examples/plugin_examples.pypackages/nemo_platform/pyproject.tomlplugins/nemo-evaluator/pyproject.tomlplugins/nemo-evaluator/src/nemo_evaluator/jobs/compiler.pyplugins/nemo-evaluator/src/nemo_evaluator/jobs/evaluate.pyplugins/nemo-evaluator/src/nemo_evaluator/jobs/utils.pyplugins/nemo-evaluator/src/nemo_evaluator/sdk/_executor.pyplugins/nemo-evaluator/src/nemo_evaluator/sdk/resources.pyplugins/nemo-evaluator/src/nemo_evaluator/sdk/standalone_sdk/backend.pyplugins/nemo-evaluator/src/nemo_evaluator/shared/metric_bundles/bundles.pyplugins/nemo-evaluator/src/nemo_evaluator/shared/metric_bundles/cloudpickle.pyplugins/nemo-evaluator/src/nemo_evaluator/tasks/evaluate.pyplugins/nemo-evaluator/tests/shared/metric_bundles/test_cloudpickle.pyplugins/nemo-evaluator/tests/test_evaluate_job.pyplugins/nemo-evaluator/tests/test_sdk.pyplugins/nemo-evaluator/tests/test_sdk_job_resources.pyplugins/nemo-evaluator/tests/test_standalone_sdk_backend.pythird_party/licenses.jsonl
Contributor
|
Signed-off-by: Sandy Chapman <schapman@nvidia.com>
SandyChapman
force-pushed
the
metric-bundling-cloudpickle-on-resolvers/schapman
branch
from
June 1, 2026 15:24
8e76a89 to
4e5b80f
Compare
Contributor
Author
|
Addressed the two CodeRabbit outside-diff notes in 4e5b80f as well: sync/async |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| from __future__ import annotations | ||
|
|
||
| from nemo_evaluator.jobs.evaluate import EvaluateSpec |
| from nemo_evaluator.jobs.utils import resolve_run_dataset | ||
| from nemo_evaluator.resolvers import PlatformModelResolver | ||
| from nemo_evaluator.shared.metric_bundles.bundles import MetricBundle, unbundle_metric | ||
| from nemo_evaluator.shared.metric_bundles.cloudpickle import CloudpickleMetricPayload # noqa: F401 |
| ) | ||
| """Compile canonical spec to a plugin-native evaluator job.""" | ||
| del workspace, entity_client, job_name, async_sdk, options | ||
| from nemo_evaluator.jobs.compiler import compile_evaluate_job |
| @abstractmethod | ||
| def kind(self) -> str: | ||
| """Payload discriminator used to select the packager implementation.""" | ||
| ... |
| @abstractmethod | ||
| def digest(self) -> str: | ||
| """Format-specific digest for the payload contents.""" | ||
| ... |
|
|
||
| def package(self, metric: Metric) -> MetricBundlePayload: | ||
| """Package a runtime metric object into a format-specific payload.""" | ||
| ... |
|
|
||
| def load(self, payload: MetricBundlePayload) -> Metric: | ||
| """Hydrate an executable metric from a bundle payload.""" | ||
| ... |
| import sys | ||
| from typing import Annotated, Literal | ||
|
|
||
| import cloudpickle |
| ], | ||
| "dataset": [{"expected": "a", "output": "a"}], | ||
| } | ||
| _LEGACY_EXACT_MATCH_SPEC = { |
SandyChapman
deleted the
metric-bundling-cloudpickle-on-resolvers/schapman
branch
June 2, 2026 11:58
This was referenced Jun 2, 2026
This was referenced Jun 17, 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
Note: rebased onto main after the resolver branch landed in #38. Remote result-registration fix will be split into a separate main-based PR.
Summary by CodeRabbit
New Features
Chores