feat(evaluator-sdk): typed workspace seed sources (inline/path/fileset)#563
Conversation
arpitsardhana
left a comment
There was a problem hiding this comment.
Just ensuring that my understanding is correct: that SDK may have files awareness so that we don't have too many duplicates files between SDK and plugin but SDK won't have any dependency or do resolution of files.
cab7311 to
0928ad6
Compare
7e41506 to
73dc920
Compare
|
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 ignored due to path filters (2)
📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughAdds shared workspace seed parsing/writing, updates Codex runtime to use it, and registers a fileset seed handler plugin with tests. ChangesWorkspace Seed System
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
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/agent_eval/runtimes/codex/runtime.py (1)
108-119: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winOffload workspace seeding from the event loop.
seed_workspace(...)is sync, and the registeredfilesethandler does blocking Files-service I/O viadownload_dataset_sync(), so this coroutine can stall every task sharing the loop.await asyncio.to_thread(seed_workspace, ...)here.🤖 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/agent_eval/runtimes/codex/runtime.py` around lines 108 - 119, The runtime setup in the Codex executor is doing synchronous workspace seeding on the event loop, which can block other tasks. Update the code in the run path around seed_workspace in the runtime class so the seeding work is executed via asyncio.to_thread instead of directly calling the sync function, and keep the existing guarded try block and process creation flow intact.
🧹 Nitpick comments (1)
plugins/nemo-evaluator/src/nemo_evaluator/agent_seeds.py (1)
38-46: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winRef validation allows
..as a segment.
"../name"passes the shape check (splits into["..", "name"], length 2), letting a traversal-like workspace segment reach the Files service unvalidated.🔧 Proposed fix
- if len(parts) != 2: + if len(parts) != 2 or any(part in {".", ".."} for part in parts): raise ValueError(f"fileset ref must be 'workspace/name' or 'workspace/name#path', got {value!r}")🤖 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/agent_seeds.py` around lines 38 - 46, The _validate_ref_shape validator in agent_seeds.py only checks segment count and still allows traversal-like segments such as ".." in ref. Update the ref validation to reject "." and ".." (and any other unsafe path segments) after splitting the base workspace/name part, while keeping the existing workspace/name or workspace/name#path shape check. Use _validate_ref_shape as the fix point and preserve the current ValueError style for invalid refs.
🤖 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/agent_seeds.py`:
- Around line 57-72: The FilesetSeedResolver.resolve path is blocking the Codex
async runtime by calling download_dataset_sync() during seed resolution. Update
resolve() to use the async download flow instead, or offload the download work
to a worker thread before reading the file bytes, so seed_workspace() does not
block the event loop while resolving FilesetSeed entries.
---
Outside diff comments:
In
`@packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/codex/runtime.py`:
- Around line 108-119: The runtime setup in the Codex executor is doing
synchronous workspace seeding on the event loop, which can block other tasks.
Update the code in the run path around seed_workspace in the runtime class so
the seeding work is executed via asyncio.to_thread instead of directly calling
the sync function, and keep the existing guarded try block and process creation
flow intact.
---
Nitpick comments:
In `@plugins/nemo-evaluator/src/nemo_evaluator/agent_seeds.py`:
- Around line 38-46: The _validate_ref_shape validator in agent_seeds.py only
checks segment count and still allows traversal-like segments such as ".." in
ref. Update the ref validation to reject "." and ".." (and any other unsafe path
segments) after splitting the base workspace/name part, while keeping the
existing workspace/name or workspace/name#path shape check. Use
_validate_ref_shape as the fix point and preserve the current ValueError style
for invalid refs.
🪄 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: 9f20d2f0-6661-46ca-aa02-6a857a209f1c
⛔ Files ignored due to path filters (2)
sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/agent_eval/runtimes/codex/runtime.pyis excluded by!sdk/**sdk/python/nemo-platform/src/nemo_platform/beta/evaluator/agent_eval/workspace_seeds.pyis excluded by!sdk/**
📒 Files selected for processing (7)
packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/codex/runtime.pypackages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/workspace_seeds.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_codex_runtime.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_workspace_seeds.pyplugins/nemo-evaluator/src/nemo_evaluator/agent_seeds.pyplugins/nemo-evaluator/src/nemo_evaluator/jobs/agent_evaluate.pyplugins/nemo-evaluator/tests/test_agent_seeds.py
|
73dc920 to
be2593b
Compare
Extends the workspace seeding added for the general Codex runtime. A task's inputs["files"] map now accepts three JSON-serializable seed shapes, chosen by a `kind` discriminator (a bare string stays sugar for inline text): - inline — contents in the task; resolvable anywhere. `encoding="base64"` for binary. - path — a file on the authoring host; local-only. - fileset — a workspace/name#glob reference; platform-resolved. The three differ in *where* they resolve, which is what governs a task's portability. inline + path resolve in the pure-SDK runtime; fileset raises a clear "cannot be resolved in local execution" error (same pattern as metric-ref resolution) and is reserved for the service-side path, where a follow-up will resolve filesets and normalize inline/path seeds into a fileset at submit time. Adds `agent_eval/workspace_seeds.py` (the SeedFile union + `seed_workspace` resolver, with workspace-escape protection and binary support). The Codex runtime delegates to it; AgentEvalTask.inputs stays a generic dict — seeding remains a documented convention, not a core task field. Signed-off-by: Sandy Chapman <schapman@nvidia.com>
be2593b to
6fd4691
Compare
…t) (#563) Extends the workspace seeding added for the general Codex runtime. A task's inputs["files"] map now accepts three JSON-serializable seed shapes, chosen by a `kind` discriminator (a bare string stays sugar for inline text): - inline — contents in the task; resolvable anywhere. `encoding="base64"` for binary. - path — a file on the authoring host; local-only. - fileset — a workspace/name#glob reference; platform-resolved. The three differ in *where* they resolve, which is what governs a task's portability. inline + path resolve in the pure-SDK runtime; fileset raises a clear "cannot be resolved in local execution" error (same pattern as metric-ref resolution) and is reserved for the service-side path, where a follow-up will resolve filesets and normalize inline/path seeds into a fileset at submit time. Adds `agent_eval/workspace_seeds.py` (the SeedFile union + `seed_workspace` resolver, with workspace-escape protection and binary support). The Codex runtime delegates to it; AgentEvalTask.inputs stays a generic dict — seeding remains a documented convention, not a core task field. Signed-off-by: Sandy Chapman <schapman@nvidia.com>
What
Follow-up to the general Codex runtime (#562). A task's
inputs["files"]seed map now accepts three JSON-serializable seed shapes, chosen by akinddiscriminator (a bare string stays sugar for inline text):kindinlineencoding="base64"for binary)pathfilesetworkspace/name#globreferenceThe three differ in where they resolve, which is what governs a task's portability — the reason a bare string alone (which can't tell content from a path from a ref) isn't enough.
Layering
inline+pathresolve in the pure-SDK runtime (no platform dependency).filesetraises a clear "cannot be resolved in local execution" error — the same pattern as metric-reference resolution. It's reserved for the service-side path; a later change will resolve filesets there and normalizeinline/pathseeds into a fileset at the local→remote submit boundary (mirroring inline-metric → derived-metric normalization).Shape
agent_eval/workspace_seeds.py: theSeedFilediscriminated union (InlineSeed/PathSeed/FilesetSeed),parse_seed, and theseed_workspaceresolver — with workspace-escape protection and binary (base64) support.seed_workspace; its private_seed_workspaceis gone.AgentEvalTask.inputsstays a genericdict[str, Any]— seeding remains a documented convention overinputs["files"], not a core task field.Testing
test_workspace_seeds.py(inline/base64/path/fileset/escape/unknown-kind); updated the Codex runtime's escape test (nowWorkspaceSeedError).agent_evalsuite (91) + plugintest_agent_evaluate.pypass;ruff/formatclean; CIlint-python-types.shexit 0.Stacked on #562 — targets
codex-runtime-general-coding/schapman; review/merge after it.Summary by CodeRabbit
filesetseed support to load a single file from a referenced dataset into the workspace.WorkspaceSeedError, and ensured seeding runs off the main event loop.filesethandling, and Codex runtime thread/error behavior.