feat(evaluator): add Taskset entity for grouping tasks (AALGO-307) - #561
Merged
Conversation
Contributor
|
SandyChapman
force-pushed
the
aalgo-307-taskset-entity/schapman
branch
from
July 6, 2026 15:40
150a615 to
3b97f97
Compare
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 (20)
🚧 Files skipped from review as they are similar to previous changes (19)
📝 WalkthroughWalkthroughAdds taskset CRUD across schemas, persistence, service, REST routes, SDK, OpenAPI, and tests. Also extracts a shared ChangesTaskset CRUD feature
Shared log sanitization
Sequence Diagram(s)sequenceDiagram
participant Client
participant TasksetsRouter
participant TasksetService
participant TaskService
participant EntityClient
Client->>TasksetsRouter: POST /tasksets/{name}
TasksetsRouter->>TasksetService: create_taskset(name, input, workspace)
TasksetService->>TaskService: get_task(workspace, name) per TaskRef
TaskService-->>TasksetService: exists / None
TasksetService->>EntityClient: create(TasksetEntity)
EntityClient-->>TasksetService: entity or conflict
TasksetService-->>TasksetsRouter: Taskset DTO
TasksetsRouter-->>Client: 201 / 409 / 422
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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.
🧹 Nitpick comments (2)
packages/nemo_platform_plugin/src/nemo_platform_plugin/log_utils.py (1)
9-11: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winOnly strips
\r/\n; misses other log-injection vectors.ANSI escape sequences and unicode line separators (
\u2028/\u2029) can still forge log entries or manipulate terminal output. Since this is now the single shared sanitizer for all callers, hardening here benefits every site at once.🛡️ Broader sanitization
+import re + +_CONTROL_CHARS = re.compile(r"[\r\n\x00-\x1f\x7f\u2028\u2029]") + + def sanitize_for_log(value: object) -> str: """Strip line-break/control characters from a value before logging (prevents log injection).""" - return str(value).replace("\r", "").replace("\n", "") + return _CONTROL_CHARS.sub("", str(value))🤖 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_platform_plugin/src/nemo_platform_plugin/log_utils.py` around lines 9 - 11, The shared sanitize_for_log helper only removes carriage returns and newlines, so it still allows ANSI escape sequences and unicode line separators that can be used for log injection. Harden sanitize_for_log in log_utils.py to also strip or neutralize ANSI control/escape sequences and unicode separators such as U+2028 and U+2029, while preserving the existing caller-facing string conversion behavior. Keep the fix localized to sanitize_for_log so every caller gets the stronger protection automatically.plugins/nemo-evaluator/src/nemo_evaluator/api/service/taskset_service.py (1)
93-111: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSequential per-ref existence checks (N+1).
Each task ref triggers a separate awaited
get_taskcall inside the loop. For tasksets with many members this serializes N round-trips. Resolve/dedupe first, then batch the existence checks concurrently.⚡ Proposed fix using asyncio.gather
+import asyncio + async def _validate_tasks_exist(self, tasks: list[TaskRef], *, workspace: str) -> None: seen: set[tuple[str, str]] = set() + resolved_refs: list[tuple[str, tuple[str, str]]] = [] for ref in tasks: resolved = parse_entity_ref(ref.root, workspace) if resolved in seen: raise DuplicateTaskRefError( f"Task reference '{ref.root}' resolves to '{resolved[0]}/{resolved[1]}', already in this taskset" ) seen.add(resolved) - if await self.task_service.get_task(*resolved) is None: - raise TaskRefNotFoundError(f"Task reference '{ref.root}' not found in workspace '{resolved[0]}'") + resolved_refs.append((ref.root, resolved)) + results = await asyncio.gather(*(self.task_service.get_task(*r) for _, r in resolved_refs)) + for (ref_root, resolved), task in zip(resolved_refs, results): + if task is None: + raise TaskRefNotFoundError(f"Task reference '{ref_root}' not found in workspace '{resolved[0]}'")🤖 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/service/taskset_service.py` around lines 93 - 111, The _validate_tasks_exist method in TasksetService is doing one awaited get_task lookup per TaskRef inside the loop, which creates a serial N+1 pattern. First resolve each ref with parse_entity_ref and keep the deduped resolved tuples, then check existence for all unique tasks concurrently with asyncio.gather against task_service.get_task. Preserve the current DuplicateTaskRefError and TaskRefNotFoundError behavior by mapping results back to the original refs when reporting missing tasks.
🤖 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 `@packages/nemo_platform_plugin/src/nemo_platform_plugin/log_utils.py`:
- Around line 9-11: The shared sanitize_for_log helper only removes carriage
returns and newlines, so it still allows ANSI escape sequences and unicode line
separators that can be used for log injection. Harden sanitize_for_log in
log_utils.py to also strip or neutralize ANSI control/escape sequences and
unicode separators such as U+2028 and U+2029, while preserving the existing
caller-facing string conversion behavior. Keep the fix localized to
sanitize_for_log so every caller gets the stronger protection automatically.
In `@plugins/nemo-evaluator/src/nemo_evaluator/api/service/taskset_service.py`:
- Around line 93-111: The _validate_tasks_exist method in TasksetService is
doing one awaited get_task lookup per TaskRef inside the loop, which creates a
serial N+1 pattern. First resolve each ref with parse_entity_ref and keep the
deduped resolved tuples, then check existence for all unique tasks concurrently
with asyncio.gather against task_service.get_task. Preserve the current
DuplicateTaskRefError and TaskRefNotFoundError behavior by mapping results back
to the original refs when reporting missing tasks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1048ca83-aee3-4aa6-96d4-2127230d0078
📒 Files selected for processing (20)
packages/nemo_platform_plugin/src/nemo_platform_plugin/log_utils.pyplugins/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/service/task_service.pyplugins/nemo-evaluator/src/nemo_evaluator/api/service/taskset_service.pyplugins/nemo-evaluator/src/nemo_evaluator/api/v2/metrics.pyplugins/nemo-evaluator/src/nemo_evaluator/api/v2/results.pyplugins/nemo-evaluator/src/nemo_evaluator/api/v2/tasks.pyplugins/nemo-evaluator/src/nemo_evaluator/api/v2/tasksets.pyplugins/nemo-evaluator/src/nemo_evaluator/entities.pyplugins/nemo-evaluator/src/nemo_evaluator/metric_refs.pyplugins/nemo-evaluator/src/nemo_evaluator/sdk/resources.pyplugins/nemo-evaluator/src/nemo_evaluator/sdk/taskset_resources.pyplugins/nemo-evaluator/src/nemo_evaluator/service.pyplugins/nemo-evaluator/tests/api/service/test_taskset_service.pyplugins/nemo-evaluator/tests/api/v2/test_tasksets_routes.pyplugins/nemo-evaluator/tests/sdk/test_taskset_sdk_resources.pyplugins/nemo-evaluator/tests/test_taskset_entity.py
Adds the Taskset half of the AALGO-307 entity work: a flexible grouping of stored tasks with metadata, mirroring the Task entity stack end-to-end. - TasksetEntity (entity_type "taskset") + Taskset/TasksetInput DTOs - TasksetService with create/get/list/delete - /tasksets CRUD routes with decorator authz via TasksetPerms - client.evaluator.tasksets SDK resource (sync + async) - mounted in service.py and sdk/resources.py Members are referenced by workspace/name (no inline tasks) with set semantics: order is not significant and duplicate references are rejected both byte-identically (field validator) and by resolved (workspace, name) in the service. Referenced tasks are validated to exist at create time. Missing/duplicate refs surface as typed errors mapped to 422. Factors the shared workspace/name ref parser into schemas.parse_entity_ref, reused by metric_refs.parse_metric_ref. Signed-off-by: Sandy Chapman <schapman@nvidia.com>
SandyChapman
force-pushed
the
aalgo-307-taskset-entity/schapman
branch
from
July 6, 2026 16:22
3b97f97 to
244922c
Compare
This was referenced Jul 6, 2026
Merged
arpitsardhana
approved these changes
Jul 6, 2026
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Jul 7, 2026
arpitsardhana
pushed a commit
that referenced
this pull request
Jul 9, 2026
) Adds the Taskset half of the AALGO-307 entity work: a flexible grouping of stored tasks with metadata, mirroring the Task entity stack end-to-end. - TasksetEntity (entity_type "taskset") + Taskset/TasksetInput DTOs - TasksetService with create/get/list/delete - /tasksets CRUD routes with decorator authz via TasksetPerms - client.evaluator.tasksets SDK resource (sync + async) - mounted in service.py and sdk/resources.py Members are referenced by workspace/name (no inline tasks) with set semantics: order is not significant and duplicate references are rejected both byte-identically (field validator) and by resolved (workspace, name) in the service. Referenced tasks are validated to exist at create time. Missing/duplicate refs surface as typed errors mapped to 422. Factors the shared workspace/name ref parser into schemas.parse_entity_ref, reused by metric_refs.parse_metric_ref. Signed-off-by: Sandy Chapman <schapman@nvidia.com>
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
2 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.
What
Adds the Taskset half of the AALGO-307 entity work: a flexible grouping of stored tasks with metadata, mirroring the merged Task entity stack (#527) end-to-end.
TasksetEntity(entity_type="taskset") +Taskset/TasksetInputDTOsTasksetService(create/get/list/delete)/tasksetsCRUD routes with decorator authz viaTasksetPermsclient.evaluator.tasksetsSDK resource (sync + async)service.pyandsdk/resources.pyDesign
workspace/name(no inline tasks). Metadata reusesMetadataItem/TaskMetadataList; the grouping carries an optionaldescription.(workspace, name)in the service, sotask-aanddefault/task-acan't both slip in.TaskRefNotFoundError/DuplicateTaskRefError) mapped to 422, name collisions to 409.workspace/nameref parser intoschemas.parse_entity_ref, now reused bymetric_refs.parse_metric_ref.Not covered (follow-ups)
task_refson the agent-eval submit path (run an eval over a stored taskset) — separate follow-up.Testing
ruff check/format, CIlint-python-types.sh(exit 0),lint-all.shall pass.make refresh-openapi: onlyplugins/nemo-evaluator/openapi/openapi.yamlchanged.Summary by CodeRabbit