feat(evaluator): add metric-types CLI support - #105
Conversation
5517ab5 to
fb86a80
Compare
|
/nvskills-ci |
|
/nvskills-ci |
|
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:
📝 WalkthroughWalkthroughCLI metric type introspection helpers added to derive available metric names and schemas. Plugin examples refactored to use run/submit execution modes with updated secret resolution. New SDK example script, evaluation spec templates, and documentation covering metric listing, spec structure, and remote job submission with platform secrets. ChangesCLI Metric Type Introspection
SDK Example Execution Model Refactoring
SDK Example, Specs, and Documentation
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 |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/cli.py`:
- Around line 50-55: The _metric_type_models function currently overwrites
entries when multiple model classes returned by
_unwrap_metric_model_classes(Metric) expose the same metric type from
_metric_type_values, so add duplicate detection: while building metric_types in
_metric_type_models, check if metric_type already exists and if so raise a clear
ValueError (or RuntimeError) that includes the conflicting metric_type and both
model class names (the existing model_cls and the one being added) to fail fast
and surface collisions; keep returning the sorted dict only if no duplicates are
found.
In `@skills/nemo-evaluator-plugin/SKILL.md`:
- Line 15: The MD022/MD031 lint errors come from headings and fenced code blocks
lacking surrounding blank lines in SKILL.md; edit the file to ensure a blank
line both before and after each heading and each fenced code block (e.g., the
"### Prerequisites:" heading and all other headings/code fences referenced at
lines 25-26, 28, 33, 44, 55-56, 71, 77, 88, 106) so every heading and ``` fenced
block is separated by an empty line above and below, then re-run markdownlint to
confirm the violations are resolved.
- Around line 32-35: Replace the ambiguous field-path style placeholder in the
CLI example so users understand it's a single metric name: change the command
example `nemo evaluator metric-types <metric_types.name>` to use a concrete CLI
arg placeholder like `nemo evaluator metric-types <metric-name>` (update the
README/ SKILL.md text that displays the command and any related explanation
mentioning `metric_types.name` to use `<metric-name>` instead).
🪄 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: 2c05a00a-7349-4727-809d-746a88ffceab
📒 Files selected for processing (11)
packages/nemo_evaluator_sdk/examples/plugin_examples.pyplugins/nemo-evaluator/src/nemo_evaluator/cli.pyplugins/nemo-evaluator/tests/test_evaluate_job.pyskills/nemo-evaluator-plugin/SKILL.mdskills/nemo-evaluator-plugin/assets/examples/plugin_sdk_examples.pyskills/nemo-evaluator-plugin/assets/examples/test_plugin_sdk_examples.pyskills/nemo-evaluator-plugin/assets/specs/exact_match_benchmark.jsonskills/nemo-evaluator-plugin/assets/specs/exact_match_metric.jsonskills/nemo-evaluator-plugin/assets/specs/llm_as_judge.jsonskills/nemo-evaluator-plugin/resources/api-auth.mdskills/nemo-evaluator-plugin/resources/llm-judge.md
|
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/examples/plugin_examples.py (1)
535-539:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSync client doesn't resolve secrets for submit mode.
When
is_online=Trueandexecution_mode="submit", this uses the globalmodelwith env-var-style secret reference (NVIDIA_API_KEY). The async version at lines 462-466 properly callsmodel_with_valid_secret()to get the hyphenated platform secret name.Current
run_sync_examplesonly calls withis_online=False, so this path isn't exercised. However, the function is public and will fail if invoked withis_online=True, execution_mode="submit".Proposed fix
Add synchronous secret resolution (requires a sync helper):
if is_online: metric = _online_exact_match_metric() config = RunConfigOnlineModel(parallelism=4, limit_samples=limit_samples) - run_kwargs["target"] = model + if execution_mode == "submit": + secret_name = ensure_submit_evaluator_api_key_secret_sync(DEFAULT_WORKSPACE, client) + run_kwargs["target"] = model.model_copy(update={"api_key_secret": SecretRef(root=secret_name)}) + else: + run_kwargs["target"] = model run_kwargs["prompt_template"] = ONLINE_CHAT_PROMPT_TEMPLATEThis requires adding a sync version of
ensure_submit_evaluator_api_key_secret. Alternatively, document thatis_online=True+execution_mode="submit"is unsupported for the sync client.🤖 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/examples/plugin_examples.py` around lines 535 - 539, The sync path in run_sync_examples uses the raw global model (env-var style secret) when is_online=True and execution_mode=="submit", so add a synchronous secret-resolution step: create a sync helper (e.g., ensure_submit_evaluator_api_key_secret_sync or model_with_valid_secret_sync) that mirrors ensure_submit_evaluator_api_key_secret() behavior and call it before setting run_kwargs["target"] in run_sync_examples; replace the direct use of the global model with the resolved model (same hyphenated secret name used by the async model_with_valid_secret()) so submit-mode online runs get a valid secret.
🤖 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/examples/plugin_examples.py`:
- Around line 535-539: The sync path in run_sync_examples uses the raw global
model (env-var style secret) when is_online=True and execution_mode=="submit",
so add a synchronous secret-resolution step: create a sync helper (e.g.,
ensure_submit_evaluator_api_key_secret_sync or model_with_valid_secret_sync)
that mirrors ensure_submit_evaluator_api_key_secret() behavior and call it
before setting run_kwargs["target"] in run_sync_examples; replace the direct use
of the global model with the resolved model (same hyphenated secret name used by
the async model_with_valid_secret()) so submit-mode online runs get a valid
secret.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0c0e4ebe-706d-4d3f-afdd-aba94063da1e
📒 Files selected for processing (2)
plugins/nemo-evaluator/examples/plugin_examples.pyplugins/nemo-evaluator/tests/test_evaluate_job.py
🚧 Files skipped from review as they are similar to previous changes (1)
- plugins/nemo-evaluator/tests/test_evaluate_job.py
bf07bc8 to
610317d
Compare
0dd34e1 to
b7f7043
Compare
e339738 to
dc85a0c
Compare
Signed-off-by: Nick Goncharenko <ngoncharenko@nvidia.com>
f3ca3f1 to
c288524
Compare
Signed-off-by: Nick Goncharenko <ngoncharenko@nvidia.com>
c288524 to
de6f523
Compare
|
/nvskills-ci |
Signed-off-by: nvskills-svc-account <svc-nvskills-signing@nvidia.com>
Summary
metric-typesCLI subcommand to list registered metric schemas and inspect individual metric schema contracts. Current explain command returns the entire schema which fills the agent's context with unrelated to the task data and contradicts the progressive disclosure principlenemo evaluatorworkflow.skills/source-of-truth directories.runandsubmitinstead oflocalandremote.Details
nemo evaluator metric-typeslists available metric types with names and descriptions.nemo evaluator metric-types <name>returns the selected metric schema and reports a clear error for unknown names.plugins/nemo-evaluator/src/nemo_evaluator/skills/evaluator-pluginnow symlinks toskills/nemo-evaluator-pluginso plugin packaging reflects the entire root skill directory, including assets.plugins/nemo-data-designer/src/nemo_data_designer_plugin/skills/data-designernow symlinks toskills/nemo-data-designer-pluginfor the same source-of-truth behavior.nemo evaluator evaluate explainas the schema source of truth and clarify local environment secrets vs submit-time platform secrets.Related
https://github.com/NVIDIA-NeMo/nemo-platform/pull/104/changes
Summary by CodeRabbit
New Features
Documentation
Examples
Tests