refactor(evaluator): clarify SDK ref handling contracts - #128
Merged
Conversation
Contributor
Documentation preview is readyPreview: https://nvidia-nemo.github.io/nemo-platform/pr-preview/pr-128/pr-128/ Built from This preview is deployed from this PR branch, updates when docs changes are pushed, and will be removed when the PR closes. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
|
SandyChapman
changed the title
SandyChapman
force-pushed
the
evaluator-ref-contract-cleanup/SandyChapman
branch
from
June 2, 2026 13:07
64b27d7 to
5c90393
Compare
SandyChapman
marked this pull request as ready for review
June 2, 2026 13:10
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:
📝 WalkthroughWalkthroughRemoves EvaluationRequest wrapper; passes explicit dataset/params/target/prompt/aggregate/hooks through backends and SDK; introduces FilesetRef and download helpers; adds EvaluateInputSpec/to_spec; refactors executors to package specs for local job runs; updates tests and docs; tightens params validation. ChangesBackend contract and configuration parameter resolution
Backend implementation and evaluator entry points
Fileset utilities and job specification
Plugin executor refactoring and SDK resource updates
SDK module reorganization and documentation
Test suite updates
Sequence Diagram(s)No visible sequence diagrams included. Possibly related PRs
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/execution/metric_execution.py (1)
543-565:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestore runtime param validation in
ComputeMetricPipeline.__init__.This now stores raw
paramsinstead ofresolve_params(params, target). Static overloads do not protect runtime callers, so invalid pairs liketarget=Agentwith offline/model params can slip through and run with wrong defaults instead of failing fast.Proposed fix
self.metric = metric self.target = target self.metric_key = metric_key self.prompt_template = prompt_template - self.params = params + self.params = resolve_params(params, target) self.inference_fn = inference_fn🤖 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_evaluator_sdk/src/nemo_evaluator_sdk/execution/metric_execution.py` around lines 543 - 565, The constructor for ComputeMetricPipeline currently assigns the raw params directly, skipping runtime validation; restore runtime validation by calling resolve_params(params, target) inside ComputeMetricPipeline.__init__ and store the resolved value back to self.params (and use the resolved params for any downstream logic), so invalid combinations like target=Agent with offline/model RunConfig are rejected at init time; reference the resolve_params function and the params and target parameters in ComputeMetricPipeline.__init__ when making this change.
🧹 Nitpick comments (1)
packages/nemo_evaluator_sdk/tests/test_api.py (1)
97-105: 💤 Low valueTest name no longer reflects behavior.
This test passes a specific file path (
tmp_path / "train.jsonl"), not a glob pattern. Consider renaming totest_evaluate_ignores_other_files_in_directoryor similar.🤖 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_evaluator_sdk/tests/test_api.py` around lines 97 - 105, Rename the test function test_evaluate_with_glob_path to a name that reflects it passes a specific file path and ignores other files in the same directory (for example test_evaluate_ignores_other_files_in_directory); update the test function declaration and any references to it, keeping the body intact (it calls ExactMatchMetric and uses Evaluator().run_sync with dataset=tmp_path / "train.jsonl") so the name accurately describes that only the provided file is evaluated.
🤖 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 `@docs/evaluator/sdk-resources.md`:
- Line 39: Update the docs tables to match the paragraph: add "local glob paths"
as an accepted form for the dataset argument in both argument tables (the one
for evaluator calls and the one for submit/SDK methods), and update the submit()
target parameter documentation to include ModelRef alongside whatever other
types are listed; ensure the entries referencing dataset, config,
aggregate_fields, target, prompt_template, and submit() are consistent with the
paragraph so the tables document inline rows, local dataset paths, local glob
paths, fileset references (with optional fragment selectors) and that submit()'s
target lists ModelRef.
In `@plugins/nemo-evaluator/src/nemo_evaluator/sdk/_executor.py`:
- Around line 65-73: The ModelRef branch in _submit_params forces callers to
pass RunConfigOnlineModel; instead resolve the submit target first and then call
resolve_params so ModelRef and direct Model behave the same. Replace the
special-case branch in _submit_params (which checks isinstance(target, ModelRef)
and raises TypeError) with logic that first calls _resolve_submit_target(...) on
the provided target, then passes the resolved target to resolve_params(params,
resolved_target) and returns that result; apply the same change to the async
variant so both sync and async submit paths use resolved_target before calling
resolve_params and remove the ModelRef-specific branch.
In `@plugins/nemo-evaluator/src/nemo_evaluator/sdk/standalone_sdk/backend.py`:
- Around line 25-31: The function _reject_unsupported_hooks currently treats
empty tuples as supported because it only checks "is not None"; change the
condition to only raise when hooks actually contain items — e.g., replace the
`is not None` checks for preprocess_hooks and postprocess_hooks with a truthy
check or an explicit length check (e.g., "if preprocess_hooks and
len(preprocess_hooks) > 0" / "if postprocess_hooks and len(postprocess_hooks) >
0") so empty tuples are treated as "no hooks" and only non-empty tuples trigger
the NotImplementedError.
---
Outside diff comments:
In
`@packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/execution/metric_execution.py`:
- Around line 543-565: The constructor for ComputeMetricPipeline currently
assigns the raw params directly, skipping runtime validation; restore runtime
validation by calling resolve_params(params, target) inside
ComputeMetricPipeline.__init__ and store the resolved value back to self.params
(and use the resolved params for any downstream logic), so invalid combinations
like target=Agent with offline/model RunConfig are rejected at init time;
reference the resolve_params function and the params and target parameters in
ComputeMetricPipeline.__init__ when making this change.
---
Nitpick comments:
In `@packages/nemo_evaluator_sdk/tests/test_api.py`:
- Around line 97-105: Rename the test function test_evaluate_with_glob_path to a
name that reflects it passes a specific file path and ignores other files in the
same directory (for example test_evaluate_ignores_other_files_in_directory);
update the test function declaration and any references to it, keeping the body
intact (it calls ExactMatchMetric and uses Evaluator().run_sync with
dataset=tmp_path / "train.jsonl") so the name accurately describes that only the
provided file is evaluated.
🪄 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: a76b615c-cafb-4be3-ac46-edd2b01de2b6
⛔ Files ignored due to path filters (5)
sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/execution/backends/base.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/execution/backends/local/backend.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/execution/config.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/execution/evaluator.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/execution/metric_execution.pyis excluded by!sdk/**
📒 Files selected for processing (25)
docs/evaluator/sdk-resources.mdpackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/execution/README.mdpackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/execution/backends/base.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/execution/backends/local/backend.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/execution/config.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/execution/evaluator.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/execution/metric_execution.pypackages/nemo_evaluator_sdk/tests/execution/backends/local/test_backend.pypackages/nemo_evaluator_sdk/tests/execution/test_config.pypackages/nemo_evaluator_sdk/tests/execution/test_evaluator.pypackages/nemo_evaluator_sdk/tests/execution/test_metric_execution.pypackages/nemo_evaluator_sdk/tests/test_api.pyplugins/nemo-evaluator/src/nemo_evaluator/filesets.pyplugins/nemo-evaluator/src/nemo_evaluator/jobs/evaluate.pyplugins/nemo-evaluator/src/nemo_evaluator/jobs/utils.pyplugins/nemo-evaluator/src/nemo_evaluator/sdk/__init__.pyplugins/nemo-evaluator/src/nemo_evaluator/sdk/_executor.pyplugins/nemo-evaluator/src/nemo_evaluator/sdk/http_utils.pyplugins/nemo-evaluator/src/nemo_evaluator/sdk/resources.pyplugins/nemo-evaluator/src/nemo_evaluator/sdk/standalone_sdk/backend.pyplugins/nemo-evaluator/src/nemo_evaluator/sdk/types.pyplugins/nemo-evaluator/src/nemo_evaluator/sdk/values/filesets.pyplugins/nemo-evaluator/tests/test_evaluate_job.pyplugins/nemo-evaluator/tests/test_sdk.pyplugins/nemo-evaluator/tests/test_standalone_sdk_backend.py
💤 Files with no reviewable changes (1)
- packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/execution/README.md
SandyChapman
force-pushed
the
evaluator-ref-contract-cleanup/SandyChapman
branch
2 times, most recently
from
June 2, 2026 13:28
3fa0923 to
8fa253b
Compare
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/nemo-evaluator/src/nemo_evaluator/sdk/resources.py (1)
99-103:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRequire
prompt_template(non-optional) for model-target overloads
Evaluator/AsyncEvaluatormodel-target overloads still allowprompt_template: ... | None = None, butEvaluateJob.compileraisesValueError("prompt_template is required when EvaluateSpec.target is a model")when it’s missing (tests cover both model + agent online targets). Update the model-target overloads to requireprompt_template(str | dict[...]with no| Noneand no default).Suggested overload fix
def submit( self, *, metric: Metric, dataset: PluginDatasetInput, config: RunConfigOnlineModel, target: Model | ModelRef, - prompt_template: str | dict[str, Any] | None = None, + prompt_template: str | dict[str, Any], metric_bundle_packager: MetricBundlePackager | None = None, ) -> EvaluatorJobResource: ... @@ def run( self, *, metric: Metric, dataset: PluginDatasetInput, config: RunConfigOnlineModel, target: Model, - prompt_template: str | dict[str, Any] | None = None, + prompt_template: str | dict[str, Any], aggregate_fields: tuple[AggregateFieldName, ...] | None = None, ) -> EvaluationResult: ... @@ async def run( self, *, metric: Metric, dataset: PluginDatasetInput, config: RunConfigOnlineModel, target: Model, - prompt_template: str | dict[str, Any] | None = None, + prompt_template: str | dict[str, Any], aggregate_fields: tuple[AggregateFieldName, ...] | None = None, ) -> EvaluationResult: ... @@ async def submit( self, *, metric: Metric, dataset: PluginDatasetInput, config: RunConfigOnlineModel, target: Model | ModelRef, - prompt_template: str | dict[str, Any] | None = None, + prompt_template: str | dict[str, Any], metric_bundle_packager: MetricBundlePackager | None = None, ) -> AsyncEvaluatorJobResource: ...🤖 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 99 - 103, The model-target overloads in Evaluator/AsyncEvaluator should require a prompt_template instead of allowing None; update the signatures (in plugins/nemo-evaluator/src/nemo_evaluator/sdk/resources.py) where target is typed as Model | ModelRef so that prompt_template is typed as str | dict[str, Any] with no " | None" and no default value (remove "= None"), matching EvaluateJob.compile's requirement; ensure you change all corresponding overloads/method declarations for both Evaluator and AsyncEvaluator so callers must pass a prompt_template.
🤖 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.
Outside diff comments:
In `@plugins/nemo-evaluator/src/nemo_evaluator/sdk/resources.py`:
- Around line 99-103: The model-target overloads in Evaluator/AsyncEvaluator
should require a prompt_template instead of allowing None; update the signatures
(in plugins/nemo-evaluator/src/nemo_evaluator/sdk/resources.py) where target is
typed as Model | ModelRef so that prompt_template is typed as str | dict[str,
Any] with no " | None" and no default value (remove "= None"), matching
EvaluateJob.compile's requirement; ensure you change all corresponding
overloads/method declarations for both Evaluator and AsyncEvaluator so callers
must pass a prompt_template.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ebdd0522-4721-49f3-b644-ee35c956af30
📒 Files selected for processing (11)
plugins/nemo-evaluator/src/nemo_evaluator/filesets.pyplugins/nemo-evaluator/src/nemo_evaluator/jobs/evaluate.pyplugins/nemo-evaluator/src/nemo_evaluator/jobs/utils.pyplugins/nemo-evaluator/src/nemo_evaluator/sdk/__init__.pyplugins/nemo-evaluator/src/nemo_evaluator/sdk/_executor.pyplugins/nemo-evaluator/src/nemo_evaluator/sdk/http_utils.pyplugins/nemo-evaluator/src/nemo_evaluator/sdk/resources.pyplugins/nemo-evaluator/src/nemo_evaluator/sdk/types.pyplugins/nemo-evaluator/src/nemo_evaluator/sdk/values/filesets.pyplugins/nemo-evaluator/tests/test_evaluate_job.pyplugins/nemo-evaluator/tests/test_sdk.py
🚧 Files skipped from review as they are similar to previous changes (7)
- plugins/nemo-evaluator/src/nemo_evaluator/sdk/types.py
- plugins/nemo-evaluator/src/nemo_evaluator/sdk/init.py
- plugins/nemo-evaluator/src/nemo_evaluator/sdk/values/filesets.py
- plugins/nemo-evaluator/src/nemo_evaluator/filesets.py
- plugins/nemo-evaluator/src/nemo_evaluator/jobs/utils.py
- plugins/nemo-evaluator/src/nemo_evaluator/sdk/_executor.py
- plugins/nemo-evaluator/tests/test_sdk.py
SandyChapman
force-pushed
the
evaluator-ref-contract-cleanup/SandyChapman
branch
from
June 2, 2026 13:41
8fa253b to
c154160
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/execution/config.py`:
- Around line 29-32: resolve_params() currently allows RunConfigOnlineModel for
Agent targets because RunConfigOnlineModel subclasses RunConfigOnline; update
the Agent branch to reject model-only configs by tightening the type check (for
example require exact type RunConfigOnline or explicitly disallow
isinstance(params, RunConfigOnlineModel)). Specifically, change the check inside
the "if isinstance(target, Agent):" block (the code that now raises
TypeError("agent target requires RunConfigOnline")) so it does not accept
RunConfigOnlineModel—this prevents model-only params from flowing into
_merge_online_hooks(), inference.new_hooks(), generate_online_sample(), and
ultimately make_agent_inference_request().
In `@plugins/nemo-evaluator/src/nemo_evaluator/filesets.py`:
- Around line 138-157: The code joins untrusted values (base_path, pattern,
relative_path, ref) into local Paths before fetching, allowing directory
traversal or absolute writes; fix by resolving and validating each candidate
destination path after constructing file_dest/full path: use
Path(destination)/base_path then for every file_dest compute resolved =
file_dest.resolve() and ensure
resolved.is_relative_to(Path(destination).resolve()) (or compare commonpath) and
reject or raise if not; apply the same validation before calling fs._get_file or
fs._get (including the single-file branch that builds full_remote_path and
file_dest) and also normalize/sanitize inputs (strip leading slashes, collapse
..) before joins.
🪄 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: d1843172-3273-4b6f-ae02-0a3c57fc0c2d
⛔ Files ignored due to path filters (5)
sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/execution/backends/base.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/execution/backends/local/backend.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/execution/config.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/execution/evaluator.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/execution/metric_execution.pyis excluded by!sdk/**
📒 Files selected for processing (25)
docs/evaluator/sdk-resources.mdpackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/execution/README.mdpackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/execution/backends/base.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/execution/backends/local/backend.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/execution/config.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/execution/evaluator.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/execution/metric_execution.pypackages/nemo_evaluator_sdk/tests/execution/backends/local/test_backend.pypackages/nemo_evaluator_sdk/tests/execution/test_config.pypackages/nemo_evaluator_sdk/tests/execution/test_evaluator.pypackages/nemo_evaluator_sdk/tests/execution/test_metric_execution.pypackages/nemo_evaluator_sdk/tests/test_api.pyplugins/nemo-evaluator/src/nemo_evaluator/filesets.pyplugins/nemo-evaluator/src/nemo_evaluator/jobs/evaluate.pyplugins/nemo-evaluator/src/nemo_evaluator/jobs/utils.pyplugins/nemo-evaluator/src/nemo_evaluator/sdk/__init__.pyplugins/nemo-evaluator/src/nemo_evaluator/sdk/_executor.pyplugins/nemo-evaluator/src/nemo_evaluator/sdk/http_utils.pyplugins/nemo-evaluator/src/nemo_evaluator/sdk/resources.pyplugins/nemo-evaluator/src/nemo_evaluator/sdk/standalone_sdk/backend.pyplugins/nemo-evaluator/src/nemo_evaluator/sdk/types.pyplugins/nemo-evaluator/src/nemo_evaluator/sdk/values/filesets.pyplugins/nemo-evaluator/tests/test_evaluate_job.pyplugins/nemo-evaluator/tests/test_sdk.pyplugins/nemo-evaluator/tests/test_standalone_sdk_backend.py
💤 Files with no reviewable changes (1)
- packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/execution/README.md
✅ Files skipped from review due to trivial changes (1)
- plugins/nemo-evaluator/src/nemo_evaluator/sdk/init.py
🚧 Files skipped from review as they are similar to previous changes (21)
- plugins/nemo-evaluator/src/nemo_evaluator/sdk/types.py
- plugins/nemo-evaluator/src/nemo_evaluator/sdk/http_utils.py
- docs/evaluator/sdk-resources.md
- packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/execution/backends/base.py
- plugins/nemo-evaluator/src/nemo_evaluator/jobs/utils.py
- packages/nemo_evaluator_sdk/tests/execution/test_config.py
- packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/execution/backends/local/backend.py
- packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/execution/evaluator.py
- plugins/nemo-evaluator/src/nemo_evaluator/sdk/values/filesets.py
- plugins/nemo-evaluator/src/nemo_evaluator/sdk/standalone_sdk/backend.py
- packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/execution/metric_execution.py
- packages/nemo_evaluator_sdk/tests/execution/test_metric_execution.py
- plugins/nemo-evaluator/tests/test_standalone_sdk_backend.py
- packages/nemo_evaluator_sdk/tests/test_api.py
- plugins/nemo-evaluator/src/nemo_evaluator/sdk/resources.py
- plugins/nemo-evaluator/src/nemo_evaluator/jobs/evaluate.py
- plugins/nemo-evaluator/tests/test_sdk.py
- packages/nemo_evaluator_sdk/tests/execution/backends/local/test_backend.py
- plugins/nemo-evaluator/tests/test_evaluate_job.py
- packages/nemo_evaluator_sdk/tests/execution/test_evaluator.py
- plugins/nemo-evaluator/src/nemo_evaluator/sdk/_executor.py
SandyChapman
force-pushed
the
evaluator-ref-contract-cleanup/SandyChapman
branch
from
June 2, 2026 14:18
c154160 to
4491ecc
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
services/evaluator/src/nmp/evaluator/api/v2/metrics/manager.py (1)
444-450: ⚡ Quick winSource
parallelismandparamsfrom oneRunConfig.This passes two independent defaults into
ComputeMetricPipeline. IfDEFAULT_PARALLELISMandRunConfig().parallelismever diverge, offline evaluation runs with inconsistent config.Suggested change
- pipeline = ComputeMetricPipeline( + run_config = RunConfig(parallelism=DEFAULT_PARALLELISM) + pipeline = ComputeMetricPipeline( rows=rows, - parallelism=DEFAULT_PARALLELISM, + parallelism=run_config.parallelism, metric=metric_impl, target=None, metric_key=metric_type_name(metric_impl), - params=RunConfig(), + params=run_config, )🤖 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 `@services/evaluator/src/nmp/evaluator/api/v2/metrics/manager.py` around lines 444 - 450, Currently DEFAULT_PARALLELISM and a freshly constructed RunConfig() are passed separately into ComputeMetricPipeline causing possible divergence; instead instantiate a single RunConfig (e.g., run_cfg = RunConfig()), use run_cfg.parallelism for the pipeline's parallelism argument and pass run_cfg as the params argument so ComputeMetricPipeline(...) uses one consistent RunConfig for parallelism and other settings (apply this change where ComputeMetricPipeline is constructed with rows, parallelism, metric=metric_impl, metric_key=metric_type_name(metric_impl), params=...).
🤖 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 `@services/evaluator/src/nmp/evaluator/api/v2/metrics/manager.py`:
- Around line 444-450: Currently DEFAULT_PARALLELISM and a freshly constructed
RunConfig() are passed separately into ComputeMetricPipeline causing possible
divergence; instead instantiate a single RunConfig (e.g., run_cfg =
RunConfig()), use run_cfg.parallelism for the pipeline's parallelism argument
and pass run_cfg as the params argument so ComputeMetricPipeline(...) uses one
consistent RunConfig for parallelism and other settings (apply this change where
ComputeMetricPipeline is constructed with rows, parallelism, metric=metric_impl,
metric_key=metric_type_name(metric_impl), params=...).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 6742c317-5a2c-486f-b622-a3655dd0f208
⛔ Files ignored due to path filters (58)
sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/__init__.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/agent_inference.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/dataset_schemas/compatibility.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/dataset_schemas/templates.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/datasets/__init__.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/datasets/loader.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/execution/_protocols.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/execution/backends/base.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/execution/backends/local/backend.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/execution/benchmark_execution.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/execution/config.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/execution/evaluator.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/execution/job_poll.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/execution/metric_execution.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/execution/pipeline.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/execution/runs.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/execution/samples.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/execution/scoring.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/execution/utils.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/inference.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/metrics/aggregation.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/metrics/bleu.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/metrics/exact_match.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/metrics/f1.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/metrics/hooks.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/metrics/llm_judge.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/metrics/llm_judge_defaults.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/metrics/number_check.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/metrics/protocol.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/metrics/ragas/__init__.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/metrics/ragas/base.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/metrics/ragas/imports.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/metrics/ragas/metrics.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/metrics/remote.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/metrics/resolution.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/metrics/rouge.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/metrics/string_check.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/metrics/template_rendering.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/metrics/tool_calling.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/metrics/types.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/resilience/api.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/resilience/classifier.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/resilience/errors.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/resilience/policy.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/resilience/scheduler.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/resilience/types.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/structured_output.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/values/__init__.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/values/agents.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/values/dataset_schemas.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/values/datasets.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/values/metrics.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/values/models.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/values/multi_metric_results.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/values/params.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/values/protocol.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/values/results.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/values/scores.pyis excluded by!sdk/**
📒 Files selected for processing (18)
Makefiledocs/evaluator/sdk-resources.mdpackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/execution/config.pypackages/nemo_evaluator_sdk/tests/execution/test_config.pyplugins/nemo-evaluator/src/nemo_evaluator/filesets.pyplugins/nemo-evaluator/src/nemo_evaluator/jobs/evaluate.pyplugins/nemo-evaluator/src/nemo_evaluator/jobs/utils.pyplugins/nemo-evaluator/src/nemo_evaluator/sdk/__init__.pyplugins/nemo-evaluator/src/nemo_evaluator/sdk/_executor.pyplugins/nemo-evaluator/src/nemo_evaluator/sdk/http_utils.pyplugins/nemo-evaluator/src/nemo_evaluator/sdk/resources.pyplugins/nemo-evaluator/src/nemo_evaluator/sdk/types.pyplugins/nemo-evaluator/src/nemo_evaluator/sdk/values/filesets.pyplugins/nemo-evaluator/tests/test_evaluate_job.pyplugins/nemo-evaluator/tests/test_filesets.pyplugins/nemo-evaluator/tests/test_sdk.pyplugins/nemo-evaluator/tests/test_standalone_sdk_backend.pyservices/evaluator/src/nmp/evaluator/api/v2/metrics/manager.py
✅ Files skipped from review due to trivial changes (2)
- plugins/nemo-evaluator/tests/test_filesets.py
- docs/evaluator/sdk-resources.md
🚧 Files skipped from review as they are similar to previous changes (13)
- plugins/nemo-evaluator/src/nemo_evaluator/sdk/types.py
- plugins/nemo-evaluator/src/nemo_evaluator/sdk/values/filesets.py
- plugins/nemo-evaluator/src/nemo_evaluator/sdk/http_utils.py
- plugins/nemo-evaluator/src/nemo_evaluator/jobs/utils.py
- plugins/nemo-evaluator/src/nemo_evaluator/filesets.py
- packages/nemo_evaluator_sdk/tests/execution/test_config.py
- plugins/nemo-evaluator/tests/test_evaluate_job.py
- plugins/nemo-evaluator/src/nemo_evaluator/sdk/resources.py
- plugins/nemo-evaluator/tests/test_standalone_sdk_backend.py
- packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/execution/config.py
- plugins/nemo-evaluator/src/nemo_evaluator/sdk/_executor.py
- plugins/nemo-evaluator/src/nemo_evaluator/jobs/evaluate.py
- plugins/nemo-evaluator/tests/test_sdk.py
SandyChapman
force-pushed
the
evaluator-ref-contract-cleanup/SandyChapman
branch
from
June 2, 2026 15:00
4491ecc to
fbd5cda
Compare
SandyChapman
force-pushed
the
evaluator-ref-contract-cleanup/SandyChapman
branch
2 times, most recently
from
June 2, 2026 15:34
05f2624 to
59c8666
Compare
SandyChapman
changed the title
ngoncharenko
reviewed
Jun 3, 2026
ngoncharenko
reviewed
Jun 3, 2026
ngoncharenko
approved these changes
Jun 3, 2026
Signed-off-by: Sandy Chapman <schapman@nvidia.com>
SandyChapman
force-pushed
the
evaluator-ref-contract-cleanup/SandyChapman
branch
from
June 3, 2026 11:51
59c8666 to
6564f1b
Compare
Merged
This was referenced Jun 17, 2026
This was referenced Jun 29, 2026
7 tasks
This was referenced Aug 9, 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
EvaluationRequesthandoff in favor of explicit backend fieldsdataset_glob_pattern; dataset paths can now encode local glob selection directlyFilesetRefandModelRefin the plugin layerresolve_paramsand overloadsValidation
uv run --frozen ty check packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/execution/evaluator.py plugins/nemo-evaluator/src/nemo_evaluator/sdk/resources.py plugins/nemo-evaluator/src/nemo_evaluator/jobs/evaluate.py plugins/nemo-evaluator/tests/test_filesets.pyuv run --frozen pytest plugins/nemo-evaluator/tests/test_filesets.py plugins/nemo-evaluator/tests/test_evaluate_job.py -qbash tools/lint/lint-python-style.sh