refactor(evaluator-sdk): migrate Fabric agent-eval runtime to the updated Fabric SDK#648
Merged
Merged
Conversation
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:
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 (1)
📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughChangesFabric Config-First Runtime
Sequence Diagram(s)sequenceDiagram
participant FabricAgentRuntime
participant Workspace
participant Fabric
FabricAgentRuntime->>Workspace: seed task workspace
FabricAgentRuntime->>Fabric: run request with composed task config
Fabric-->>FabricAgentRuntime: return task result
FabricAgentRuntime->>FabricAgentRuntime: normalize RunOutput
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 (1)
packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/runtime.py (1)
47-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTYPE_CHECKING-only imports of Fabric SDK types.
Fabric,FabricConfig,FabricProfileConfig,RunOutputare imported only underTYPE_CHECKINGand then used as concrete annotations (client: Fabric,agent_config: FabricConfig,output: RunOutput | JsonValue, etc.) throughout the file. The comment justifies this via the optional-dependency constraint, but the coding guideline explicitly prohibits TYPE_CHECKING-only imports for these types.As per coding guidelines, "In Python code, prefer concrete type hints over string-based type hints, and do not import those types only under
TYPE_CHECKING; import them normally when possible."♻️ Alternative that keeps the optional-dependency guard while importing normally
-if TYPE_CHECKING: - # Annotations use nemo_fabric's real types (single source of truth). nemo_fabric is an optional - # native package not yet in our locked dependency set, so it is imported for typing only and - # loaded lazily at runtime (see ``run_tasks``). Drop the ty:ignore once nemo-fabric is a - # resolvable dependency and the checker can see it. - from nemo_fabric import ( # ty: ignore[unresolved-import] - Fabric, - FabricConfig, - FabricProfileConfig, - RunOutput, - ) +try: + from nemo_fabric import Fabric, FabricConfig, FabricProfileConfig, RunOutput # ty: ignore[unresolved-import] +except ImportError: # nemo_fabric is an optional native dependency, not yet locked + Fabric = FabricConfig = FabricProfileConfig = RunOutput = Any # type: ignore[assignment,misc]🤖 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/fabric/runtime.py` around lines 47 - 56, Replace the TYPE_CHECKING-only import block for Fabric, FabricConfig, FabricProfileConfig, and RunOutput with normal imports, while preserving the existing lazy runtime behavior for the optional nemo_fabric dependency as required by run_tasks. Keep the concrete annotations on client, agent_config, output, and related symbols intact, and remove the obsolete justification and ignore directive.Source: Coding guidelines
🤖 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_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/runtime.py`:
- Around line 47-56: Replace the TYPE_CHECKING-only import block for Fabric,
FabricConfig, FabricProfileConfig, and RunOutput with normal imports, while
preserving the existing lazy runtime behavior for the optional nemo_fabric
dependency as required by run_tasks. Keep the concrete annotations on client,
agent_config, output, and related symbols intact, and remove the obsolete
justification and ignore directive.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 25aa81c3-0351-43c9-8a30-32a76687cf09
📒 Files selected for processing (4)
packages/nemo_evaluator_sdk/src/nemo_evaluator_sdk/agent_eval/runtimes/fabric/runtime.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_fabric_integration.pypackages/nemo_evaluator_sdk/tests/agent_eval/test_fabric_runtime.pyscript/dev-install-fabric.sh
SandyChapman
force-pushed
the
fabric-client-to-fabric/schapman
branch
from
1edeee6 to
caa6876
Compare
Contributor
|
SandyChapman
force-pushed
the
fabric-client-to-fabric/schapman
branch
2 times, most recently
from
99ac2d1 to
0235501
Compare
ngoncharenko
approved these changes
Jul 14, 2026
…ated Fabric SDK Align the host FabricAgentRuntime with the current NeMo Fabric Python SDK: - Rename FabricClient -> Fabric (a reusable facade, not an async context manager); fold per-invocation input and request id into RunRequest. - Compose each task's workspace/model/trajectory settings directly onto a copied FabricConfig (config-first: model_copy + environment + enable_relay) instead of layering profile overlays; drop the profile-builder helpers. - Handle the RunOutput response contract: RunResult.output is now a RunOutput Mapping, so normalize it to a plain dict for the JsonValue trial response. - Keep nemo_fabric an optional lazy import (still not a locked dependency); re-import the two runtime types locally where used rather than threading them through helper signatures. - Fix the dev-install-fabric.sh smoke check (FabricClient -> Fabric). Verified: hermetic tests (fake nemo_fabric) pass, and a live codex end-to-end run (test_fabric_codex_live_eval_captures_atif_trajectory) captures an ATIF trajectory through the migrated runtime. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Sandy Chapman <schapman@nvidia.com>
SandyChapman
force-pushed
the
fabric-client-to-fabric/schapman
branch
from
0235501 to
44720ca
Compare
SandyChapman
added a commit
that referenced
this pull request
Jul 14, 2026
…ic agent-eval Add optional agent-skill injection to FabricAgentRuntime for A/B skill evals: inject an agentskills.io bundle into every task — native harnesses via a per-task `skills` profile overlay pointing at the staged bundle; codex via workspace `.agents/skills/<name>/` self-injection — stamp skill provenance (name + content hash + mode) on each trial, and add `with_skill()` to derive baseline vs. skilled runtimes from one instance. Per-run evidence is isolated under a generated run id so baseline and skilled runs sharing a work_root don't collide. SkillUsedMetric scores whether an expected skill was present and used, so a skill-required task fails when the agent ignores the skill. Ported onto the config-first Fabric runtime (stacked on #648); adds skills.py, tests, and a runnable example. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Sandy Chapman <schapman@nvidia.com>
SandyChapman
added a commit
that referenced
this pull request
Jul 14, 2026
…ic agent-eval Add optional agent-skill injection to FabricAgentRuntime for A/B skill evals: inject an agentskills.io bundle into every task — native harnesses via a per-task `skills` profile overlay pointing at the staged bundle; codex via workspace `.agents/skills/<name>/` self-injection — stamp skill provenance (name + content hash + mode) on each trial, and add `with_skill()` to derive baseline vs. skilled runtimes from one instance. Per-run evidence is isolated under a generated run id so baseline and skilled runs sharing a work_root don't collide. SkillUsedMetric scores whether an expected skill was present and used, so a skill-required task fails when the agent ignores the skill. Ported onto the config-first Fabric runtime (stacked on #648); adds skills.py, tests, and a runnable example. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> 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 & why
Aligns the host
FabricAgentRuntime(agent_eval/runtimes/fabric/) with the current NeMo Fabric Python SDK. The SDK renamed its client, moved to a config-first composition model, and introduced theRunOutputresponse contract — this branch migrates our runtime to match and keeps everything importable/testable without the (still optional, unlocked)nemo-fabricnative package.Key changes
FabricClient→Fabric.Fabricis a plain reusable facade (not an async context manager); per-invocationinput+request_idnow go throughRunRequest.FabricConfig(model_copy+environment+enable_relay) instead of layered profile overlays. The old profile-builder helpers are removed; caller-suppliedprofiles=still pass through.RunOutputresponse contract (chore(release): bump platform version to 0.1.1 #52).RunResult.outputis now aRunOutput(aMapping), not a raw JSON value._normalize_outputcopies it to a plain dict so it round-trips through the trial'sJsonValueresponse field.nemo_fabricstays an optional lazy import (guarded inrun_tasks); helpers re-import the two runtime types locally where used rather than threadingtype[...]params through signatures. Annotations name the real SDK types via theTYPE_CHECKINGimport.script/dev-install-fabric.shsmoke check updatedFabricClient→Fabric.Verification
nemo_fabric, run in CI): green — runner → evaluator → metric → evidence chain.test_fabric_codex_live_eval_captures_atif_trajectorypassed — a real codex agent run through the migrated runtime → relay → ATIF trajectory → metric.ruff(check + format), copyright headers, merge-conflict, andty(CI config) clean for all changed files.Notes
nemo-fabricremains an optional lazy import because there is no published, cross-platform, indexed wheel to lock yet (its CI builds Linux-only artifacts, unpublished). CI stays on thety: ignore[unresolved-import]path; local dev usesscript/dev-install-fabric.sh.container_runtime.py, feat(evaluator-sdk): sandboxed containerized Fabric runner (AALGO-321) #604) is a separate, unmerged branch and still needs the same migration — out of scope here.🤖 Generated with Claude Code
Summary by CodeRabbit