chore(evaluator): detangle plugin from legacy service - #121
Conversation
1f43442 to
2c8e330
Compare
|
|
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:
📝 WalkthroughWalkthroughThis PR consolidates FilesetRef into the evaluator SDK, adds local dataset utilities for the plugin, removes remote Platform example workflows, establishes a task entrypoint with graceful shutdown, implements lazy SDK exports, updates inferred parameter tests, and refactors dependencies from monolithic to modular packages. ChangesEvaluator SDK and plugin consolidation
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.
🧹 Nitpick comments (1)
plugins/nemo-evaluator/src/nemo_evaluator/jobs/utils.py (1)
48-54: 💤 Low valueUnused
recursiveparameter.Both
download_datasetanddownload_dataset_syncacceptrecursivebut never use it. Either remove it or wire it into the download calls.Also applies to: 92-100
🤖 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/jobs/utils.py` around lines 48 - 54, The `recursive` parameter on download_dataset and download_dataset_sync is unused; either remove it or pass it through to the underlying NeMo SDK download call. Locate the functions download_dataset and download_dataset_sync (they accept sdk: AsyncNeMoPlatform and FilesetRef) and update the call that performs the actual download to include the recursive flag (e.g., forward recursive into the SDK method such as sdk.download_fileset or sdk.download_fileset_sync / whatever method is used to fetch FilesetRef) so the parameter is honored, or if recursion is not supported by the SDK drop the parameter from both function signatures and all callers.
🤖 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 `@plugins/nemo-evaluator/src/nemo_evaluator/jobs/utils.py`:
- Around line 48-54: The `recursive` parameter on download_dataset and
download_dataset_sync is unused; either remove it or pass it through to the
underlying NeMo SDK download call. Locate the functions download_dataset and
download_dataset_sync (they accept sdk: AsyncNeMoPlatform and FilesetRef) and
update the call that performs the actual download to include the recursive flag
(e.g., forward recursive into the SDK method such as sdk.download_fileset or
sdk.download_fileset_sync / whatever method is used to fetch FilesetRef) so the
parameter is honored, or if recursion is not supported by the SDK drop the
parameter from both function signatures and all callers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1580103a-ae5f-41f8-b285-0664a42c3e1d
⛔ Files ignored due to path filters (7)
sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/datasets/loader.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/templates.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/values/common.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/results.pyis excluded by!sdk/**uv.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
packages/nemo_evaluator_sdk/examples/examples.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/datasets/loader.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/execution/README.mdpackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/metrics/aggregation.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/templates.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/values/common.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/values/datasets.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/values/results.pypackages/nemo_evaluator_sdk/tests/test_params.pypackages/nemo_platform/pyproject.tomlplugins/nemo-evaluator/examples/plugin_examples.pyplugins/nemo-evaluator/pyproject.tomlplugins/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/types.pyplugins/nemo-evaluator/src/nemo_evaluator/sdk/values/filesets.pyplugins/nemo-evaluator/src/nemo_evaluator/tasks/evaluate/__main__.pyplugins/nemo-evaluator/tests/test_evaluate_job.pyplugins/nemo-evaluator/tests/test_sdk.pytools/nemo-platform-sdk-tools/src/nemo_platform_sdk_tools/sdk/vendor/vendor_package.pytools/nemo-platform-sdk-tools/tests/sdk/vendor/test_vendor_package.py
💤 Files with no reviewable changes (6)
- packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/templates.py
- packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/metrics/aggregation.py
- packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/values/common.py
- packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/values/results.py
- packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/datasets/loader.py
- packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/values/datasets.py
2c8e330 to
cc34c74
Compare
cc34c74 to
eccfd5e
Compare
c8c3356 to
584cce9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/README.md`:
- Around line 35-36: Replace the vague phrase "plugin package" in the
Evaluator(client=...) description with the concrete package/module name that
contains the platform-specific backend adapters (e.g., the actual backend
adapter package in this repo), and add a markdown link to that package's README
or API docs so users can find the adapters quickly; update the sentence to read
something like "Platform-specific backend adapters are provided by
<package-name> (see <link-to-package-docs>)."
🪄 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: 8d65d6ed-4857-41e4-99f8-bf62f3825cfe
⛔ Files ignored due to path filters (7)
sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/datasets/loader.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/templates.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/values/common.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/results.pyis excluded by!sdk/**uv.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
packages/nemo_evaluator_sdk/examples/examples.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/datasets/loader.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/execution/README.mdpackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/metrics/aggregation.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/templates.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/values/common.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/values/datasets.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/values/results.pypackages/nemo_evaluator_sdk/tests/test_params.pypackages/nemo_platform/pyproject.tomlplugins/nemo-evaluator/examples/plugin_examples.pyplugins/nemo-evaluator/pyproject.tomlplugins/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/types.pyplugins/nemo-evaluator/src/nemo_evaluator/sdk/values/filesets.pyplugins/nemo-evaluator/src/nemo_evaluator/tasks/evaluate.pyplugins/nemo-evaluator/tests/test_evaluate_job.pyplugins/nemo-evaluator/tests/test_sdk.pytools/nemo-platform-sdk-tools/src/nemo_platform_sdk_tools/sdk/vendor/vendor_package.pytools/nemo-platform-sdk-tools/tests/sdk/vendor/test_vendor_package.py
💤 Files with no reviewable changes (6)
- packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/datasets/loader.py
- packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/templates.py
- packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/values/datasets.py
- packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/metrics/aggregation.py
- packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/values/results.py
- packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/values/common.py
🚧 Files skipped from review as they are similar to previous changes (14)
- packages/nemo_platform/pyproject.toml
- plugins/nemo-evaluator/examples/plugin_examples.py
- plugins/nemo-evaluator/pyproject.toml
- plugins/nemo-evaluator/src/nemo_evaluator/sdk/values/filesets.py
- tools/nemo-platform-sdk-tools/tests/sdk/vendor/test_vendor_package.py
- plugins/nemo-evaluator/src/nemo_evaluator/sdk/types.py
- plugins/nemo-evaluator/tests/test_sdk.py
- plugins/nemo-evaluator/src/nemo_evaluator/sdk/init.py
- tools/nemo-platform-sdk-tools/src/nemo_platform_sdk_tools/sdk/vendor/vendor_package.py
- plugins/nemo-evaluator/src/nemo_evaluator/jobs/evaluate.py
- packages/nemo_evaluator_sdk/tests/test_params.py
- plugins/nemo-evaluator/src/nemo_evaluator/sdk/_executor.py
- packages/nemo_evaluator_sdk/examples/examples.py
- plugins/nemo-evaluator/src/nemo_evaluator/jobs/utils.py
584cce9 to
e37968f
Compare
Signed-off-by: Sandy Chapman <schapman@nvidia.com>
e37968f to
618f49b
Compare
Summary
Validation
uv run ruff check packages/nemo_evaluator_sdk plugins/nemo-evaluator tools/nemo-platform-sdk-tools/src/nemo_platform_sdk_tools/sdk/vendor/vendor_package.py tools/nemo-platform-sdk-tools/tests/sdk/vendor/test_vendor_package.pyuv run --frozen --extra cpu ty check ...focused evaluator filesuv run pytest packages/nemo_evaluator_sdk/tests/test_params.py plugins/nemo-evaluator/tests/test_sdk.py plugins/nemo-evaluator/tests/test_evaluate_job.py tools/nemo-platform-sdk-tools/tests/sdk/vendor/test_vendor_package.py -quv run pytest packages/nemo_evaluator_sdk/tests/execution/test_evaluator.py plugins/nemo-evaluator/tests/test_standalone_sdk_backend.py -qtools/lint/lint-all.shSummary by CodeRabbit
Release Notes
New Features
Refactor
Documentation
Chores