feat: add onboarding notebooks for the Python SDK - #58
Conversation
|
📖 Fern docs preview: https://nvidia-preview-pull-request-58.docs.buildwithfern.com/nemo/fabric |
yczhang-nv
added
documentation
|
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:
WalkthroughAdds two guarded Jupyter onboarding notebooks, documentation and launch support, and a Claude adapter variant for the code-review example with configuration, CLI registration, exports, and test coverage. ChangesOnboarding and Claude integration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Notebook
participant Fabric
participant DeepAgents
participant nemo_relay
participant RelayTraceFiles
Notebook->>Fabric: Build config and call plan()
Fabric->>DeepAgents: Execute configured agent request
DeepAgents->>nemo_relay: Emit telemetry when enabled
nemo_relay->>RelayTraceFiles: Write trace files
Notebook->>RelayTraceFiles: Inspect trace files
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@examples/code_review_agent/README.md`:
- Line 64: Update the entrypoint description near the variant table to replace
the stale hard-coded count of four with five, reflecting the added Claude
harness configuration; leave the variant commands and table entries unchanged.
In `@examples/notebooks/02_variations.ipynb`:
- Around line 180-204: Update the default model lookup in the harness loop to
access the ModelConfig attribute directly: use the `.model` property on
`plan.effective_config.config.models["default"]` instead of mapping-style
`["model"]` access.
In `@examples/notebooks/README.md`:
- Around line 29-31: Update the Deep Agents, Codex, Claude credential
description in examples/notebooks/README.md to explicitly include
OPENAI_API_KEY, matching the codex-cli HARNESSES prerequisite and existing Codex
credential wording.
In `@tests/python/test_code_review_example.py`:
- Around line 34-36: Expand the Claude assertions in the test loop and the
Claude-specific checks around the later referenced section to cover the model
identifier, ANTHROPIC_API_KEY, harness settings, artifact paths, and
independence from base, including protection against shallow-copy regressions.
Preserve the existing adapter, provider, and removed-capability assertions while
validating the complete Claude configuration contract.
🪄 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: ASSERTIVE
Plan: Enterprise
Run ID: 2361386f-0bf0-468f-a9e6-35b7233b457c
📒 Files selected for processing (11)
.gitignoreexamples/README.mdexamples/code_review_agent/README.mdexamples/code_review_agent/__init__.pyexamples/code_review_agent/__main__.pyexamples/code_review_agent/config.pyexamples/notebooks/01_quickstart.ipynbexamples/notebooks/02_variations.ipynbexamples/notebooks/README.mdjustfiletests/python/test_code_review_example.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
{adapters/**,examples/**}
⚙️ CodeRabbit configuration file
{adapters/**,examples/**}: Review adapter and example changes for command correctness, config/schema consistency, artifact handling, and compatibility with the public Fabric contracts.
Files:
examples/code_review_agent/README.mdexamples/notebooks/README.mdexamples/code_review_agent/__main__.pyexamples/README.mdexamples/code_review_agent/config.pyexamples/code_review_agent/__init__.pyexamples/notebooks/01_quickstart.ipynbexamples/notebooks/02_variations.ipynb
tests/**/*.py
📄 CodeRabbit inference engine (.agents/skills/python-tests/SKILL.md)
tests/**/*.py: Usepytestto run Python tests.
Do not add@pytest.mark.asyncioto test functions; async tests are detected and run automatically.
Do not add-> Nonereturn type annotations to test functions.
When mocking a class, do not define a new class; useunittest.mock.MagicMockorunittest.mock.AsyncMock, addingspecwhen needed.
Name mocked classes with amockprefix, notfake.
Prefer pytest fixtures over helper methods.
Do not duplicate fixtures across test files; if a fixture is needed in multiple test files, define it inconftest.py.
When creating a fixture, use@pytest.fixture(name="<fixture_name>"[, scope="<scope>"])and define the function as<fixture_name>_fixture() -> <return_type>; only passscopewhen it is notfunction.
Preferpytest.mark.parametrizeover creating separate tests for different input types.
If a fixture is needed for a test but does not return a value, or its value is unused, use@pytest.mark.usefixtures.
When modifying environment variables in a test, useos.environ;tests/conftest.pyprovides an autouserestore_environ_fixturethat restores environment variables after each test, somonkeypatch.setenvis unnecessary.
Files:
tests/python/test_code_review_example.py
{tests/**,python/tests/**}
⚙️ CodeRabbit configuration file
{tests/**,python/tests/**}: Tests should cover the behavior promised by the changed API surface, including error paths, lifecycle cleanup, and SDK/native parity where relevant.
Files:
tests/python/test_code_review_example.py
🧬 Code graph analysis (2)
examples/code_review_agent/config.py (2)
python/src/nemo_fabric/models.py (2)
remove_skill_path(345-352)remove_mcp_server(328-335)crates/fabric-core/src/runtime.rs (1)
adapter_id(1129-1134)
tests/python/test_code_review_example.py (2)
examples/code_review_agent/config.py (3)
hermes_sdk_config(69-98)codex_cli_config(130-158)hermes_cli_config(101-127)python/src/nemo_fabric/types.py (2)
skills(662-665)mcp(652-655)
🪛 Ruff (0.15.21)
examples/notebooks/01_quickstart.ipynb
[warning] 124-128: Use format specifiers instead of percent format
Replace with format specifiers
(UP031)
[error] 132-132: await statement outside of a function
(F704)
[error] 132-132: await should be used within an async function
(PLE1142)
[error] 147-147: await statement outside of a function
(F704)
[error] 147-147: await should be used within an async function
(PLE1142)
[error] 164-168: await should be used within an async function
(PLE1142)
[error] 164-164: await statement outside of a function
(F704)
[error] 164-164: await should be used within an async function
(PLE1142)
[error] 165-165: await statement outside of a function
(F704)
[error] 165-165: await should be used within an async function
(PLE1142)
[error] 166-168: await statement outside of a function
(F704)
[error] 166-168: await should be used within an async function
(PLE1142)
examples/notebooks/02_variations.ipynb
[error] 119-119: await statement outside of a function
(F704)
[error] 119-119: await should be used within an async function
(PLE1142)
[warning] 123-123: Do not catch blind exception: Exception
(BLE001)
[error] 177-177: await statement outside of a function
(F704)
[error] 177-177: await should be used within an async function
(PLE1142)
🔇 Additional comments (11)
examples/code_review_agent/config.py (1)
187-223: LGTM!examples/code_review_agent/__init__.py (1)
9-9: LGTM!Also applies to: 25-25
examples/code_review_agent/__main__.py (1)
17-17: LGTM!Also applies to: 29-29
tests/python/test_code_review_example.py (2)
15-15: LGTM!
87-92: 🩺 Stability & AvailabilityVerify the optional Claude adapter is installed for these tests.
Both the direct planning loop and the subprocess case resolve
nvidia.fabric.claude. Sincenemo-fabric-adapters-claudeis an optional dependency, a base-only test environment will fail before exercising these assertions. Ensure CI installs the Claude extra or applies the suite’s existing dependency gating.Also applies to: 104-104
.gitignore (1)
29-31: LGTM!examples/README.md (1)
10-20: LGTM!Also applies to: 30-30
justfile (1)
328-332: LGTM!examples/notebooks/01_quickstart.ipynb (2)
74-133: LGTM!Top-level
awaitflags (F704/PLE1142) and the UP031 percent-format hint are non-issues here: Jupyter/IPython natively supports top-levelawaitin cells, and the percent-format print is cosmetic-only in a demo notebook.
159-416: LGTM!examples/notebooks/02_variations.ipynb (1)
51-104: LGTM!The
except Exceptionin the harness loop and top-levelawaitflags are expected/intentional for a guarded, multi-harness demo notebook (each harness can fail for a different reason, and Jupyter supports top-levelawaitnatively).Also applies to: 148-170, 230-256, 282-310
389041b to
199c83c
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
examples/notebooks/README.md (1)
29-31: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCodex credential note still omits
OPENAI_API_KEY.This line lists
NVIDIA_API_KEY, an authenticatedcodex, andANTHROPIC_API_KEY, but02_variations.ipynb'scodex-cliharness entry gates on"key": "OPENAI_API_KEY"and itsneedstext explicitly calls it out. A reader relying only on this README line is missing a prerequisite the notebook enforces.📝 Proposed fix
- - **Deep Agents, Codex, Claude** (variations notebook): the matching adapter - installed in the Fabric environment, plus that harness's credentials - (`NVIDIA_API_KEY`, an authenticated `codex`, `ANTHROPIC_API_KEY`). + - **Deep Agents, Codex, Claude** (variations notebook): the matching adapter + installed in the Fabric environment, plus that harness's credentials + (`NVIDIA_API_KEY`, an authenticated `codex` + `OPENAI_API_KEY`, + `ANTHROPIC_API_KEY`).As per path instructions, "Review adapter and example changes for command correctness, config/schema consistency, artifact handling, and compatibility with the public Fabric contracts."
🤖 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 `@examples/notebooks/README.md` around lines 29 - 31, Update the “Deep Agents, Codex, Claude” prerequisite description in the README to include OPENAI_API_KEY alongside the existing NVIDIA_API_KEY, authenticated codex, and ANTHROPIC_API_KEY credentials, matching the codex-cli harness requirements in 02_variations.ipynb.Source: Path instructions
examples/notebooks/02_variations.ipynb (1)
182-189: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDict-style access on
ModelConfigwill raise at runtime.
plan.effective_config.config.models["default"]returns aModelConfigobject, not a mapping;["model"]subscript access will fail. This breaks the harness-comparison loop for every entry (even ones that are otherwise blocked from running), since the print happens unconditionally before theblocker()check.🐛 Proposed fix
- model = plan.effective_config.config.models["default"]["model"] + model = plan.effective_config.config.models["default"].modelAs per path instructions, "Review adapter and example changes for command correctness, config/schema consistency, artifact handling, and compatibility with the public Fabric contracts."
🤖 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 `@examples/notebooks/02_variations.ipynb` around lines 182 - 189, Update the harness-comparison loop around plan.effective_config.config.models["default"] to access the ModelConfig.model attribute instead of using dict-style ["model"] indexing. Preserve the existing model display and unconditional print behavior so every entry can still be reported before blocker() is evaluated.Source: Path instructions
🤖 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.
Duplicate comments:
In `@examples/notebooks/02_variations.ipynb`:
- Around line 182-189: Update the harness-comparison loop around
plan.effective_config.config.models["default"] to access the ModelConfig.model
attribute instead of using dict-style ["model"] indexing. Preserve the existing
model display and unconditional print behavior so every entry can still be
reported before blocker() is evaluated.
In `@examples/notebooks/README.md`:
- Around line 29-31: Update the “Deep Agents, Codex, Claude” prerequisite
description in the README to include OPENAI_API_KEY alongside the existing
NVIDIA_API_KEY, authenticated codex, and ANTHROPIC_API_KEY credentials, matching
the codex-cli harness requirements in 02_variations.ipynb.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: c77cb59e-b4a2-4b0a-ad9c-edc4290a365c
📒 Files selected for processing (11)
.gitignoreexamples/README.mdexamples/code_review_agent/README.mdexamples/code_review_agent/__init__.pyexamples/code_review_agent/__main__.pyexamples/code_review_agent/config.pyexamples/notebooks/01_quickstart.ipynbexamples/notebooks/02_variations.ipynbexamples/notebooks/README.mdjustfiletests/python/test_code_review_example.py
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: Build and publish docs
- GitHub Check: Pre-commit
- GitHub Check: Test (x86_64)
- GitHub Check: Test (arm64)
- GitHub Check: Build wheels (arm64)
🧰 Additional context used
📓 Path-based instructions (3)
{adapters/**,examples/**}
⚙️ CodeRabbit configuration file
{adapters/**,examples/**}: Review adapter and example changes for command correctness, config/schema consistency, artifact handling, and compatibility with the public Fabric contracts.
Files:
examples/code_review_agent/README.mdexamples/code_review_agent/__main__.pyexamples/notebooks/README.mdexamples/README.mdexamples/code_review_agent/__init__.pyexamples/notebooks/01_quickstart.ipynbexamples/code_review_agent/config.pyexamples/notebooks/02_variations.ipynb
tests/**/*.py
📄 CodeRabbit inference engine (.agents/skills/python-tests/SKILL.md)
tests/**/*.py: Usepytestto run Python tests.
Do not add@pytest.mark.asyncioto test functions; async tests are detected and run automatically.
Do not add-> Nonereturn type annotations to test functions.
When mocking a class, do not define a new class; useunittest.mock.MagicMockorunittest.mock.AsyncMock, addingspecwhen needed.
Name mocked classes with amockprefix, notfake.
Prefer pytest fixtures over helper methods.
Do not duplicate fixtures across test files; if a fixture is needed in multiple test files, define it inconftest.py.
When creating a fixture, use@pytest.fixture(name="<fixture_name>"[, scope="<scope>"])and define the function as<fixture_name>_fixture() -> <return_type>; only passscopewhen it is notfunction.
Preferpytest.mark.parametrizeover creating separate tests for different input types.
If a fixture is needed for a test but does not return a value, or its value is unused, use@pytest.mark.usefixtures.
When modifying environment variables in a test, useos.environ;tests/conftest.pyprovides an autouserestore_environ_fixturethat restores environment variables after each test, somonkeypatch.setenvis unnecessary.
Files:
tests/python/test_code_review_example.py
{tests/**,python/tests/**}
⚙️ CodeRabbit configuration file
{tests/**,python/tests/**}: Tests should cover the behavior promised by the changed API surface, including error paths, lifecycle cleanup, and SDK/native parity where relevant.
Files:
tests/python/test_code_review_example.py
🧬 Code graph analysis (2)
tests/python/test_code_review_example.py (2)
crates/fabric-core/src/config.rs (1)
providers(1722-1726)adapters/common/src/nemo_fabric_adapters/common/utils.py (2)
telemetry_plan(112-114)relay_enabled(124-125)
examples/code_review_agent/config.py (1)
python/src/nemo_fabric/models.py (2)
remove_skill_path(467-474)remove_mcp_server(450-457)
🪛 Ruff (0.15.21)
examples/notebooks/01_quickstart.ipynb
[warning] 124-128: Use format specifiers instead of percent format
Replace with format specifiers
(UP031)
[error] 132-132: await statement outside of a function
(F704)
[error] 132-132: await should be used within an async function
(PLE1142)
[error] 147-147: await statement outside of a function
(F704)
[error] 147-147: await should be used within an async function
(PLE1142)
[error] 164-168: await should be used within an async function
(PLE1142)
[error] 164-164: await statement outside of a function
(F704)
[error] 164-164: await should be used within an async function
(PLE1142)
[error] 165-165: await statement outside of a function
(F704)
[error] 165-165: await should be used within an async function
(PLE1142)
[error] 166-168: await statement outside of a function
(F704)
[error] 166-168: await should be used within an async function
(PLE1142)
examples/notebooks/02_variations.ipynb
[error] 119-119: await statement outside of a function
(F704)
[error] 119-119: await should be used within an async function
(PLE1142)
[warning] 123-123: Do not catch blind exception: Exception
(BLE001)
[error] 177-177: await statement outside of a function
(F704)
[error] 177-177: await should be used within an async function
(PLE1142)
🔇 Additional comments (14)
examples/code_review_agent/README.md (2)
56-56: 📐 Maintainability & Code Quality | ⚡ Quick winStale variant count still says "four" after adding Claude.
With the Claude row added at line 64, the entrypoint now exposes five complete harness configs, not four. This was already flagged in a previous review and remains unfixed.
As per path instructions, "review adapter and example changes for command correctness, config/schema consistency, artifact handling, and compatibility with the public Fabric contracts."
✏️ Suggested fix
-The entrypoint exposes the four complete harness configs defined in +The entrypoint exposes the five complete harness configs defined inSource: Path instructions
64-64: LGTM!tests/python/test_code_review_example.py (3)
30-52: 🎯 Functional Correctness | ⚡ Quick winClaude assertions still shallow — unresolved from previous review.
The
claudeconfig is only checked forharness.adapter_id, modelprovider, andmcp/skillsbeingNone. It doesn't assert the model identifier,api_key_env(ANTHROPIC_API_KEY), harnesssettings(system_prompt,permission_mode), runtime/environmentartifactspaths, or independence frombase(e.g.claude.harness is not base.harness). This is the same gap raised in a prior review and is still not addressed.As per path instructions, "tests should cover the behavior promised by the changed API surface, including error paths, lifecycle cleanup, and SDK/native parity where relevant."
🧪 Suggested expanded assertions
assert claude.harness.adapter_id == "nvidia.fabric.claude" assert claude.models["default"].provider == "anthropic" + assert claude.models["default"].model == "anthropic/claude-sonnet-4-5" + assert claude.models["default"].api_key_env == "ANTHROPIC_API_KEY" + assert claude.harness.settings["permission_mode"] == "dontAsk" + assert claude.runtime.artifacts == "./artifacts/claude" + assert claude.environment.artifacts == "./artifacts/claude" + assert claude is not base + assert claude.harness is not base.harness assert claude.mcp is None assert claude.skills is NoneSource: Path instructions
57-97: LGTM!
100-133: LGTM!
--variant claudecase correctly expectsnvidia.fabric.claudeandrelay_enabled=False, consistent withclaude_config().examples/code_review_agent/config.py (2)
187-223: LGTM!
claude_config()follows the same override pattern ascodex_cli_config()(harness/model/runtime/environment replacement, MCP/skill removal), consistent with theremove_mcp_server/remove_skill_pathcontract inpython/src/nemo_fabric/models.py.
204-210: 📐 Maintainability & Code Quality | 💤 Low valueModel alias is valid but not the newest Sonnet generation.
anthropic/claude-sonnet-4-5resolves correctly as a convenience alias to the latest Sonnet 4.5 dated snapshot, but newer generations (Sonnet 4.6, Sonnet 5) are now available. Since this is an onboarding/example config, consider whether it should track a newer model or if pinning to 4.5 is intentional for stability.examples/code_review_agent/__init__.py (1)
6-9: LGTM!Also applies to: 22-25
examples/code_review_agent/__main__.py (1)
17-17: LGTM!Also applies to: 29-29
.gitignore (1)
29-31: LGTM!examples/README.md (1)
10-20: LGTM!Also applies to: 30-30
justfile (1)
328-332: LGTM!examples/notebooks/01_quickstart.ipynb (1)
74-133: LGTM!Also applies to: 159-212, 232-251, 271-285, 307-335, 357-376
examples/notebooks/02_variations.ipynb (1)
50-105: LGTM!Also applies to: 123-171, 224-257, 281-311
199c83c to
ec43da0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
examples/notebooks/README.md (1)
29-31: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCodex credential note still omits
OPENAI_API_KEY.This line lists
NVIDIA_API_KEY, an authenticatedcodex, andANTHROPIC_API_KEY, but the variations notebook'scodex-cliharness entry gates on"key": "OPENAI_API_KEY"and its ownneedsstring explicitly requires+ OPENAI_API_KEY. This README line still doesn't mention it, so a reader following only this doc will miss a real prerequisite.📝 Proposed fix
- - **Deep Agents, Codex, Claude** (variations notebook): the matching adapter - installed in the Fabric environment, plus that harness's credentials - (`NVIDIA_API_KEY`, an authenticated `codex`, `ANTHROPIC_API_KEY`). + - **Deep Agents, Codex, Claude** (variations notebook): the matching adapter + installed in the Fabric environment, plus that harness's credentials + (`NVIDIA_API_KEY`, an authenticated `codex` + `OPENAI_API_KEY`, + `ANTHROPIC_API_KEY`).🤖 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 `@examples/notebooks/README.md` around lines 29 - 31, Update the variations notebook prerequisite description to include OPENAI_API_KEY alongside NVIDIA_API_KEY, the authenticated codex, and ANTHROPIC_API_KEY. Keep the existing adapter and credential requirements unchanged.examples/notebooks/02_variations.ipynb (1)
180-204: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSubscript access on
ModelConfigstill crashes every harness iteration.
plan.effective_config.config.models["default"]["model"]runs unconditionally at the top of the loop, beforeblocker()gates anything.models["default"]is aModelConfig(a Pydantic model), which does not implement__getitem__by default, so["model"]raisesTypeError: 'ModelConfig' object is not subscriptableon the very first iteration — breaking the notebook's core promise that it "always executes top to bottom" regardless of environment, for all four harnesses, not just the unavailable ones.🐛 Proposed fix
- model = plan.effective_config.config.models["default"]["model"] + model = plan.effective_config.config.models["default"].model🤖 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 `@examples/notebooks/02_variations.ipynb` around lines 180 - 204, Update the harness loop around plan.effective_config so model is accessed through the ModelConfig attribute API rather than subscript syntax, and ensure this inspection cannot abort the loop before blocker(entry) runs. Preserve the existing reporting and execution behavior for all four harnesses, including unavailable environments.
🤖 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 `@examples/code_review_agent/config.py`:
- Around line 199-201: Update the system_prompt in the settings configuration
for the Claude entrypoint to use the code-review agent prompt, or inherit the
established base prompt, instead of “You are a concise smoke test assistant.”
Keep the prompt aligned with the entrypoint’s WORKSPACE code-review behavior.
---
Duplicate comments:
In `@examples/notebooks/02_variations.ipynb`:
- Around line 180-204: Update the harness loop around plan.effective_config so
model is accessed through the ModelConfig attribute API rather than subscript
syntax, and ensure this inspection cannot abort the loop before blocker(entry)
runs. Preserve the existing reporting and execution behavior for all four
harnesses, including unavailable environments.
In `@examples/notebooks/README.md`:
- Around line 29-31: Update the variations notebook prerequisite description to
include OPENAI_API_KEY alongside NVIDIA_API_KEY, the authenticated codex, and
ANTHROPIC_API_KEY. Keep the existing adapter and credential requirements
unchanged.
🪄 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: ASSERTIVE
Plan: Enterprise
Run ID: 9fca7577-5945-4cbc-84fc-0fcb4ce2ec6b
📒 Files selected for processing (11)
.gitignoreexamples/README.mdexamples/code_review_agent/README.mdexamples/code_review_agent/__init__.pyexamples/code_review_agent/__main__.pyexamples/code_review_agent/config.pyexamples/notebooks/01_quickstart.ipynbexamples/notebooks/02_variations.ipynbexamples/notebooks/README.mdjustfiletests/python/test_code_review_example.py
📜 Review details
⏰ Context from checks skipped due to timeout. (6)
- GitHub Check: Build and publish docs
- GitHub Check: Pre-commit
- GitHub Check: Test (x86_64)
- GitHub Check: Build wheels (arm64)
- GitHub Check: Test (arm64)
- GitHub Check: Build wheels (x86_64)
🧰 Additional context used
📓 Path-based instructions (3)
{adapters/**,examples/**}
⚙️ CodeRabbit configuration file
{adapters/**,examples/**}: Review adapter and example changes for command correctness, config/schema consistency, artifact handling, and compatibility with the public Fabric contracts.
Files:
examples/notebooks/README.mdexamples/code_review_agent/README.mdexamples/code_review_agent/__init__.pyexamples/code_review_agent/__main__.pyexamples/README.mdexamples/code_review_agent/config.pyexamples/notebooks/01_quickstart.ipynbexamples/notebooks/02_variations.ipynb
tests/**/*.py
📄 CodeRabbit inference engine (.agents/skills/python-tests/SKILL.md)
tests/**/*.py: Usepytestto run Python tests.
Do not add@pytest.mark.asyncioto test functions; async tests are detected and run automatically.
Do not add-> Nonereturn type annotations to test functions.
When mocking a class, do not define a new class; useunittest.mock.MagicMockorunittest.mock.AsyncMock, addingspecwhen needed.
Name mocked classes with amockprefix, notfake.
Prefer pytest fixtures over helper methods.
Do not duplicate fixtures across test files; if a fixture is needed in multiple test files, define it inconftest.py.
When creating a fixture, use@pytest.fixture(name="<fixture_name>"[, scope="<scope>"])and define the function as<fixture_name>_fixture() -> <return_type>; only passscopewhen it is notfunction.
Preferpytest.mark.parametrizeover creating separate tests for different input types.
If a fixture is needed for a test but does not return a value, or its value is unused, use@pytest.mark.usefixtures.
When modifying environment variables in a test, useos.environ;tests/conftest.pyprovides an autouserestore_environ_fixturethat restores environment variables after each test, somonkeypatch.setenvis unnecessary.
Files:
tests/python/test_code_review_example.py
{tests/**,python/tests/**}
⚙️ CodeRabbit configuration file
{tests/**,python/tests/**}: Tests should cover the behavior promised by the changed API surface, including error paths, lifecycle cleanup, and SDK/native parity where relevant.
Files:
tests/python/test_code_review_example.py
🧬 Code graph analysis (2)
examples/code_review_agent/config.py (2)
python/src/nemo_fabric/models.py (2)
remove_skill_path(467-474)remove_mcp_server(450-457)crates/fabric-core/src/runtime.rs (2)
adapter_id(1129-1134)harness(1147-1152)
tests/python/test_code_review_example.py (2)
python/src/nemo_fabric/types.py (2)
skills(638-641)mcp(628-631)crates/fabric-core/src/runtime.rs (2)
adapter_id(1129-1134)harness(1147-1152)
🪛 Ruff (0.15.21)
examples/notebooks/01_quickstart.ipynb
[warning] 124-128: Use format specifiers instead of percent format
Replace with format specifiers
(UP031)
[error] 132-132: await statement outside of a function
(F704)
[error] 132-132: await should be used within an async function
(PLE1142)
[error] 147-147: await statement outside of a function
(F704)
[error] 147-147: await should be used within an async function
(PLE1142)
[error] 164-168: await should be used within an async function
(PLE1142)
[error] 164-164: await statement outside of a function
(F704)
[error] 164-164: await should be used within an async function
(PLE1142)
[error] 165-165: await statement outside of a function
(F704)
[error] 165-165: await should be used within an async function
(PLE1142)
[error] 166-168: await statement outside of a function
(F704)
[error] 166-168: await should be used within an async function
(PLE1142)
examples/notebooks/02_variations.ipynb
[error] 119-119: await statement outside of a function
(F704)
[error] 119-119: await should be used within an async function
(PLE1142)
[warning] 123-123: Do not catch blind exception: Exception
(BLE001)
[error] 177-177: await statement outside of a function
(F704)
[error] 177-177: await should be used within an async function
(PLE1142)
🔇 Additional comments (14)
examples/code_review_agent/README.md (1)
64-64: The variant count is still stale.Line 56 says “four complete harness configs,” but the table now contains five entries after adding Claude. This is the same unresolved issue from the previous review; change “four” to “five” or remove the hard-coded count.
As per path instructions, adapter and example changes must cover command correctness, config/schema consistency, artifact handling, and compatibility with the public Fabric contracts.
Source: Path instructions
tests/python/test_code_review_example.py (3)
35-37: Expand the Claude contract assertions.The test still checks only the adapter ID, provider, and removed MCP/skills. It does not verify the model ID,
ANTHROPIC_API_KEY, harness settings, artifact paths, or independence frombase, so shallow-copy and partial-configuration regressions can pass.As per path instructions, tests should cover the behavior promised by the changed API surface, including error paths, lifecycle cleanup, and SDK/native parity where relevant.
Also applies to: 49-52
Source: Path instructions
88-93: 🩺 Stability & AvailabilityEnsure the test environment installs the Claude adapter.
These cases now unconditionally resolve
nvidia.fabric.claude. Sincenemo-fabric-adapters-claude==0.1.0is supplied through an optionalclaudedependency group, the normal test suite will fail if CI does not install that extra. Verify the test job installs the Claude dependency or gates these cases accordingly.Also applies to: 105-105
15-15: LGTM!examples/code_review_agent/config.py (1)
211-220: 🗄️ Data Integrity & IntegrationVerify that the runtime and environment artifact roots are intentionally shared.
base_config()separatesruntime.artifactsfromenvironment.artifacts, but this variant assigns both to./artifacts/claude. If both writers can emit overlapping filenames, artifacts may be mixed or overwritten. Confirm the Fabric artifact contract; otherwise use separate subdirectories.Source: Path instructions
examples/code_review_agent/__init__.py (1)
9-9: LGTM!Also applies to: 25-25
examples/code_review_agent/__main__.py (1)
17-17: LGTM!Also applies to: 29-29
.gitignore (1)
29-31: LGTM!examples/README.md (1)
10-20: LGTM!Also applies to: 30-30
examples/notebooks/README.md (1)
1-28: LGTM!Also applies to: 32-57
justfile (1)
328-332: LGTM!examples/notebooks/01_quickstart.ipynb (2)
68-134: LGTM!Also applies to: 214-286, 288-336, 338-376, 378-401
160-212: 🗄️ Data Integrity & IntegrationNo issue here.
WORKSPACEpoints at the same fixture used byexamples/code_review_agent/config.py, and the Hermes SDK settings here use the same contract keys (workspace,hermes_home,base_url,max_iterations,max_tokens,reasoning_config,enabled_toolsets).> Likely an incorrect or invalid review comment.examples/notebooks/02_variations.ipynb (1)
44-106: LGTM!Also applies to: 108-172, 206-258, 260-318, 320-343
a9e66da to
860d7f0
Compare
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 `@examples/notebooks/01_quickstart.ipynb`:
- Around line 131-144: Update the prerequisite detection around HAS_ADAPTER and
CAN_RUN_HERMES to require that ADAPTER_PYTHON is set to a non-empty path whose
executable exists, rather than only checking for the environment key. Preserve
the existing environment fallback to _hermes_python, and ensure the readiness
output reflects this validated executable before allowing fabric.run().
In `@examples/notebooks/02_variations.ipynb`:
- Around line 296-309: Update the Deep Agents telemetry branch around
blocker(deep) and fabric.run to verify the adapter is installed before
execution, or catch and report adapter-related run failures using the same
handling as the harness loop. Preserve notebook continuation, command
correctness, and relay_dir cleanup across skipped or failed runs.
- Around line 295-309: Update the notebook’s Relay configuration around
relay_dir and with_relay so Relay writes under the notebook artifact root,
examples/notebooks/artifacts/relay, rather than the code-review agent path.
Ensure the configured output path and the later inspection path both reuse
relay_dir and remain synchronized.
🪄 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: ASSERTIVE
Plan: Enterprise
Run ID: c647ad73-2828-48ec-97e2-2fec643bdd1e
⛔ Files ignored due to path filters (2)
examples/notebooks/img/fabric-contract.svgis excluded by!**/*.svgexamples/notebooks/img/three-axes.svgis excluded by!**/*.svg
📒 Files selected for processing (11)
.gitignoreexamples/README.mdexamples/code_review_agent/README.mdexamples/code_review_agent/__init__.pyexamples/code_review_agent/__main__.pyexamples/code_review_agent/config.pyexamples/notebooks/01_quickstart.ipynbexamples/notebooks/02_variations.ipynbexamples/notebooks/README.mdjustfiletests/python/test_code_review_example.py
📜 Review details
⏰ Context from checks skipped due to timeout. (6)
- GitHub Check: Build and publish docs
- GitHub Check: Pre-commit
- GitHub Check: Test (x86_64)
- GitHub Check: Build wheels (x86_64)
- GitHub Check: Build wheels (arm64)
- GitHub Check: Test (arm64)
🧰 Additional context used
📓 Path-based instructions (3)
{adapters/**,examples/**}
⚙️ CodeRabbit configuration file
{adapters/**,examples/**}: Review adapter and example changes for command correctness, config/schema consistency, artifact handling, and compatibility with the public Fabric contracts.
Files:
examples/code_review_agent/__init__.pyexamples/README.mdexamples/notebooks/README.mdexamples/code_review_agent/__main__.pyexamples/code_review_agent/config.pyexamples/notebooks/01_quickstart.ipynbexamples/code_review_agent/README.mdexamples/notebooks/02_variations.ipynb
tests/**/*.py
📄 CodeRabbit inference engine (.agents/skills/python-tests/SKILL.md)
tests/**/*.py: Usepytestto run Python tests.
Do not add@pytest.mark.asyncioto test functions; async tests are detected and run automatically.
Do not add-> Nonereturn type annotations to test functions.
When mocking a class, do not define a new class; useunittest.mock.MagicMockorunittest.mock.AsyncMock, addingspecwhen needed.
Name mocked classes with amockprefix, notfake.
Prefer pytest fixtures over helper methods.
Do not duplicate fixtures across test files; if a fixture is needed in multiple test files, define it inconftest.py.
When creating a fixture, use@pytest.fixture(name="<fixture_name>"[, scope="<scope>"])and define the function as<fixture_name>_fixture() -> <return_type>; only passscopewhen it is notfunction.
Preferpytest.mark.parametrizeover creating separate tests for different input types.
If a fixture is needed for a test but does not return a value, or its value is unused, use@pytest.mark.usefixtures.
When modifying environment variables in a test, useos.environ;tests/conftest.pyprovides an autouserestore_environ_fixturethat restores environment variables after each test, somonkeypatch.setenvis unnecessary.
Files:
tests/python/test_code_review_example.py
{tests/**,python/tests/**}
⚙️ CodeRabbit configuration file
{tests/**,python/tests/**}: Tests should cover the behavior promised by the changed API surface, including error paths, lifecycle cleanup, and SDK/native parity where relevant.
Files:
tests/python/test_code_review_example.py
🧬 Code graph analysis (2)
examples/code_review_agent/config.py (2)
python/src/nemo_fabric/models.py (2)
remove_skill_path(467-474)remove_mcp_server(450-457)crates/fabric-core/src/runtime.rs (2)
adapter_id(1129-1134)harness(1147-1152)
tests/python/test_code_review_example.py (1)
crates/fabric-core/src/runtime.rs (2)
adapter_id(1129-1134)harness(1147-1152)
🪛 Ruff (0.15.21)
examples/notebooks/01_quickstart.ipynb
[warning] 124-128: Use format specifiers instead of percent format
Replace with format specifiers
(UP031)
[error] 132-132: await statement outside of a function
(F704)
[error] 132-132: await should be used within an async function
(PLE1142)
[error] 147-147: await statement outside of a function
(F704)
[error] 147-147: await should be used within an async function
(PLE1142)
[error] 164-168: await should be used within an async function
(PLE1142)
[error] 164-164: await statement outside of a function
(F704)
[error] 164-164: await should be used within an async function
(PLE1142)
[error] 165-165: await statement outside of a function
(F704)
[error] 165-165: await should be used within an async function
(PLE1142)
[error] 166-168: await statement outside of a function
(F704)
[error] 166-168: await should be used within an async function
(PLE1142)
examples/notebooks/02_variations.ipynb
[error] 119-119: await statement outside of a function
(F704)
[error] 119-119: await should be used within an async function
(PLE1142)
[warning] 123-123: Do not catch blind exception: Exception
(BLE001)
[error] 177-177: await statement outside of a function
(F704)
[error] 177-177: await should be used within an async function
(PLE1142)
🔇 Additional comments (11)
examples/code_review_agent/config.py (1)
199-201: Duplicate: keep the Claude prompt aligned with the code-review example.The Claude variant still uses “smoke test assistant,” although this entrypoint reviews
WORKSPACE. Use the code-review prompt or inherit the base prompt so Claude performs the documented task.examples/code_review_agent/README.md (1)
64-64: Duplicate: update the stale variant count.Line 56 still says “four” variants, but the new Claude row makes five. Change the count to “five” or remove the hard-coded number.
As per path instructions, keep adapter and example documentation consistent with the public Fabric contracts.
Source: Path instructions
tests/python/test_code_review_example.py (2)
35-37: Duplicate: cover the complete Claude configuration contract.The test still checks only the adapter ID, provider, and removed capabilities. Also assert the Claude model ID,
ANTHROPIC_API_KEY, harness settings, runtime/environment artifact paths, and independence frombaseto catch partial or shallow-copy regressions.As per path instructions, tests should cover the behavior promised by the changed API surface.
Also applies to: 49-52
Source: Path instructions
15-15: LGTM!Also applies to: 88-93, 105-105
examples/code_review_agent/__init__.py (1)
9-9: LGTM!Also applies to: 25-25
examples/code_review_agent/__main__.py (1)
17-17: LGTM!Also applies to: 29-29
examples/README.md (1)
10-19: LGTM!Also applies to: 30-30
examples/notebooks/README.md (1)
29-31: Codex prerequisites still omitOPENAI_API_KEY.The notebook requires both an authenticated
codexbinary andOPENAI_API_KEY, but this list mentions only the binary.examples/notebooks/02_variations.ipynb (1)
193-200: Use attribute access for the resolved model.
models["default"]is aModelConfig;["model"]will fail. Use.model..gitignore (1)
29-31: LGTM!justfile (1)
328-331: LGTM!
4844e31 to
cd06c2c
Compare
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
cd06c2c to
862e553
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
examples/notebooks/02_variations.ipynb (1)
196-197: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPossible unresolved subscript-on-
ModelConfigbug — verify whether it was actually fixed.A previous review flagged
plan.effective_config.config.models["default"]["model"]as broken becauseModelConfigis a Pydantic model, not a mapping, and marked it "Addressed in commit ec43da0." However, the code in this snapshot still uses the exact same bracket-subscript pattern. PydanticBaseModelinstances are not subscriptable by default (no__getitem__) unless explicitly overridden, so unlessModelConfig/FabricBaseModelwas given a custom__getitem__, this line will raiseTypeError: 'ModelConfig' object is not subscriptableat runtime and break the harness loop for every entry.#!/bin/bash # Confirm whether ModelConfig (or a common base) implements __getitem__. rg -nP '\bclass\s+ModelConfig\b' -A 40 python/src/nemo_fabric/models.py rg -nP '__getitem__' python/src/nemo_fabric🛡️ Proposed fix if `ModelConfig` is not subscriptable
- model = plan.effective_config.config.models["default"]["model"] + model = plan.effective_config.config.models["default"].model🤖 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 `@examples/notebooks/02_variations.ipynb` around lines 196 - 197, Verify whether ModelConfig or its base class supports __getitem__. If it is not subscriptable, update the notebook’s model lookup around plan.effective_config.config.models to use the model configuration’s attribute access instead of nested mapping subscripts, while preserving selection of the "default" model and its model value.
🤖 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 `@examples/notebooks/01_quickstart.ipynb`:
- Around line 377-389: Update the multi-turn start_runtime block guarded by
CAN_RUN_HERMES to catch and handle failures from both runtime.invoke calls,
including runtime-level errors after prerequisites pass. Preserve the existing
turn output behavior on success while reporting the failure without allowing the
notebook cell to terminate unexpectedly.
- Around line 333-348: Wrap the live execution block under CAN_RUN_HERMES,
including fabric.run and all result access, in the same try/except pattern used
by the 02_variations notebook harness. Catch runtime failures, report the error,
and set result to None so the notebook continues executing top to bottom;
preserve the existing output behavior for successful runs and the current skip
branch when prerequisites are unavailable.
In `@examples/notebooks/02_variations.ipynb`:
- Line 296: Update the notebook’s relay_dir assignment to resolve under the
documented examples/notebooks/artifacts/ root rather than BASE_DIR from
examples.code_review_agent. Preserve the relay subdirectory name and ensure
relay artifacts remain within the notebook artifact location described by the
README.
---
Duplicate comments:
In `@examples/notebooks/02_variations.ipynb`:
- Around line 196-197: Verify whether ModelConfig or its base class supports
__getitem__. If it is not subscriptable, update the notebook’s model lookup
around plan.effective_config.config.models to use the model configuration’s
attribute access instead of nested mapping subscripts, while preserving
selection of the "default" model and its model value.
🪄 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: ASSERTIVE
Plan: Enterprise
Run ID: 6cd5a29d-340c-4ce3-afa8-4024c60e226d
⛔ Files ignored due to path filters (2)
examples/notebooks/img/fabric-contract.svgis excluded by!**/*.svgexamples/notebooks/img/three-axes.svgis excluded by!**/*.svg
📒 Files selected for processing (11)
.gitignoreexamples/README.mdexamples/code_review_agent/README.mdexamples/code_review_agent/__init__.pyexamples/code_review_agent/__main__.pyexamples/code_review_agent/config.pyexamples/notebooks/01_quickstart.ipynbexamples/notebooks/02_variations.ipynbexamples/notebooks/README.mdjustfiletests/python/test_code_review_example.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
{adapters/**,examples/**}
⚙️ CodeRabbit configuration file
{adapters/**,examples/**}: Review adapter and example changes for command correctness, config/schema consistency, artifact handling, and compatibility with the public Fabric contracts.
Files:
examples/code_review_agent/__init__.pyexamples/code_review_agent/__main__.pyexamples/notebooks/README.mdexamples/code_review_agent/config.pyexamples/README.mdexamples/code_review_agent/README.mdexamples/notebooks/01_quickstart.ipynbexamples/notebooks/02_variations.ipynb
tests/**/*.py
📄 CodeRabbit inference engine (.agents/skills/python-tests/SKILL.md)
tests/**/*.py: Usepytestto run Python tests.
Do not add@pytest.mark.asyncioto test functions; async tests are detected and run automatically.
Do not add-> Nonereturn type annotations to test functions.
When mocking a class, do not define a new class; useunittest.mock.MagicMockorunittest.mock.AsyncMock, addingspecwhen needed.
Name mocked classes with amockprefix, notfake.
Prefer pytest fixtures over helper methods.
Do not duplicate fixtures across test files; if a fixture is needed in multiple test files, define it inconftest.py.
When creating a fixture, use@pytest.fixture(name="<fixture_name>"[, scope="<scope>"])and define the function as<fixture_name>_fixture() -> <return_type>; only passscopewhen it is notfunction.
Preferpytest.mark.parametrizeover creating separate tests for different input types.
If a fixture is needed for a test but does not return a value, or its value is unused, use@pytest.mark.usefixtures.
When modifying environment variables in a test, useos.environ;tests/conftest.pyprovides an autouserestore_environ_fixturethat restores environment variables after each test, somonkeypatch.setenvis unnecessary.
Files:
tests/python/test_code_review_example.py
{tests/**,python/tests/**}
⚙️ CodeRabbit configuration file
{tests/**,python/tests/**}: Tests should cover the behavior promised by the changed API surface, including error paths, lifecycle cleanup, and SDK/native parity where relevant.
Files:
tests/python/test_code_review_example.py
🧬 Code graph analysis (2)
examples/code_review_agent/config.py (2)
python/src/nemo_fabric/models.py (2)
remove_skill_path(467-474)remove_mcp_server(450-457)crates/fabric-core/src/runtime.rs (1)
adapter_id(1129-1134)
tests/python/test_code_review_example.py (2)
python/src/nemo_fabric/types.py (2)
skills(638-641)mcp(628-631)crates/fabric-core/src/runtime.rs (2)
adapter_id(1129-1134)harness(1147-1152)
🪛 Ruff (0.15.21)
examples/notebooks/01_quickstart.ipynb
[warning] 125-129: Use format specifiers instead of percent format
Replace with format specifiers
(UP031)
[error] 133-133: await statement outside of a function
(F704)
[error] 133-133: await should be used within an async function
(PLE1142)
[error] 148-148: await statement outside of a function
(F704)
[error] 148-148: await should be used within an async function
(PLE1142)
[error] 165-169: await should be used within an async function
(PLE1142)
[error] 165-165: await statement outside of a function
(F704)
[error] 165-165: await should be used within an async function
(PLE1142)
[error] 166-166: await statement outside of a function
(F704)
[error] 166-166: await should be used within an async function
(PLE1142)
[error] 167-169: await statement outside of a function
(F704)
[error] 167-169: await should be used within an async function
(PLE1142)
examples/notebooks/02_variations.ipynb
[error] 119-119: await statement outside of a function
(F704)
[error] 119-119: await should be used within an async function
(PLE1142)
[warning] 123-123: Do not catch blind exception: Exception
(BLE001)
[error] 178-178: await statement outside of a function
(F704)
[error] 178-178: await should be used within an async function
(PLE1142)
[warning] 179-179: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (16)
examples/code_review_agent/config.py (2)
200-200: Keep Claude’s prompt aligned with the code-review example.The prompt still says “smoke test assistant,” while this variant is exposed as a code-review agent operating on
WORKSPACE. Claude may therefore produce smoke-test behavior instead of reviewing changes. Replace it with the established code-review prompt or inherit the base prompt.
187-199: LGTM!Also applies to: 201-223
tests/python/test_code_review_example.py (2)
49-56: Cover the remaining Claude configuration contract.The test now checks adapter, model, and capability removal, but still omits
system_prompt,permission_mode, both artifact paths, and meaningful deep-copy independence. Those fields can regress while this test remains green.
15-15: LGTM!Also applies to: 35-37, 92-97, 109-109
examples/code_review_agent/__init__.py (1)
9-9: LGTM!Also applies to: 25-25
examples/code_review_agent/__main__.py (1)
17-17: LGTM!Also applies to: 29-29
examples/code_review_agent/README.md (1)
56-64: LGTM!examples/notebooks/README.md (1)
29-32: 📐 Maintainability & Code QualityOPENAI_API_KEY note previously flagged — now correctly included.
The prior review comment about the missing
OPENAI_API_KEYcredential for Codex has been addressed; the line now lists it correctly.examples/notebooks/01_quickstart.ipynb (2)
137-139: 🩺 Stability & AvailabilityADAPTER_PYTHON validation fix confirmed.
Previously flagged issue (validate the executable exists before marking Hermes ready) is now correctly implemented with
Path(_adapter_python).is_file().
133-133: Ruffawait-outside-function hints are false positives for notebooks.Jupyter/IPython supports top-level
awaitin cells via autoawait; these are not real errors.Also applies to: 148-148, 165-169, 291-297
Source: Linters/SAST tools
examples/notebooks/02_variations.ipynb (3)
298-317: 🩺 Stability & AvailabilityDeep Agents telemetry guard confirmed fixed.
The
blocker(deep)check before runningfabric.run()for the telemetry cell, combined with thetry/except, resolves the previously flagged missing-guard issue.
119-119: Ruffawait-outside-function hints are false positives for notebooks.Same as in
01_quickstart.ipynb— top-levelawaitin Jupyter cells is supported and not an error.Also applies to: 178-178
Source: Linters/SAST tools
206-215: LGTM!.gitignore (1)
29-31: LGTM!examples/README.md (1)
10-20: LGTM!Also applies to: 30-30
justfile (1)
328-332: LGTM!
862e553 to
8bb5029
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (3)
examples/notebooks/01_quickstart.ipynb (2)
377-389: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMulti-turn
start_runtime()block has the same unguarded-failure gap.
runtime.invoke()calls are not wrapped intry/except, so a runtime-level failure once prerequisites pass raises uncaught and stops the notebook. Same concern as the one-shot run cell above; flagged previously without an "Addressed" marker and still unresolved.As per path instructions, "Review adapter and example changes for command correctness... and compatibility with the public Fabric contracts."
🤖 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 `@examples/notebooks/01_quickstart.ipynb` around lines 377 - 389, Wrap the multi-turn start_runtime/invoke flow guarded by CAN_RUN_HERMES in try/except so runtime-level failures are handled without terminating the notebook. Preserve the existing two-turn invocation and output behavior on success, and use the same failure-handling approach as the one-shot run cell above.Source: Path instructions
333-348: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
fabric.run()still unguarded despite "every live cell is guarded" claim.
CAN_RUN_HERMESonly checks static prerequisites; a runtime failure insidefabric.run()(network error, model unavailable, etc.) still propagates uncaught here, breaking top-to-bottom execution. The sibling02_variations.ipynbharness loop already wraps the equivalent call intry/exceptfor this exact reason. This was flagged in a prior review without an "Addressed" marker, and the code is unchanged.🛡️ Proposed fix
if CAN_RUN_HERMES: - result = await fabric.run(config, input=REVIEW_INPUT) - response = getattr(result.output, "response", result.output) - print("status :", result.status) - print("error :", result.error) - print("runtime_id :", result.runtime_id) - print("invocation :", result.invocation_id) - print("artifacts :", len(result.artifacts.artifacts), - "file(s) under", result.artifacts.root) - print("\n--- review ---\n") - print(response) + try: + result = await fabric.run(config, input=REVIEW_INPUT) + except Exception as error: + result = None + print(f"Live run failed ({type(error).__name__}: {error}).") + else: + response = getattr(result.output, "response", result.output) + print("status :", result.status) + print("error :", result.error) + print("runtime_id :", result.runtime_id) + print("invocation :", result.invocation_id) + print("artifacts :", len(result.artifacts.artifacts), + "file(s) under", result.artifacts.root) + print("\n--- review ---\n") + print(response) else: result = None print("Skipping the live run - need the native build, NVIDIA_API_KEY,") print("and ADAPTER_PYTHON. See the Hermes SDK quick start in the README.")As per path instructions, "Review adapter and example changes for command correctness, config/schema consistency, artifact handling, and compatibility with the public Fabric contracts."
🤖 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 `@examples/notebooks/01_quickstart.ipynb` around lines 333 - 348, Wrap the live execution block in the notebook around fabric.run, including result processing and artifact printing, with the same try/except handling used by the 02_variations.ipynb harness loop. Ensure runtime failures are caught and reported without aborting top-to-bottom execution, while preserving the existing CAN_RUN_HERMES skip path and successful output behavior.Source: Path instructions
examples/notebooks/02_variations.ipynb (1)
294-334: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRelay artifacts still written outside the documented notebook artifact root.
relay_dir = BASE_DIR / "artifacts" / "relay"resolves under the code-review agent's own directory (BASE_DIRis imported fromexamples.code_review_agent), not underexamples/notebooks/artifacts/asexamples/notebooks/README.mddocuments. This was flagged twice in prior reviews without an "Addressed" marker, and the code is unchanged.📝 Proposed fix
-relay_dir = BASE_DIR / "artifacts" / "relay" +relay_dir = REPO_ROOT / "examples" / "notebooks" / "artifacts" / "relay"As per path instructions, "Review adapter and example changes for command correctness, config/schema consistency, artifact handling, and compatibility with the public Fabric contracts."
🤖 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 `@examples/notebooks/02_variations.ipynb` around lines 294 - 334, Update the relay_dir assignment in the telemetry example so Relay artifacts are created under the documented examples/notebooks/artifacts/relay root rather than BASE_DIR from the code-review agent. Reuse the notebook repository-root or notebook-directory symbol already available in the surrounding setup, and keep the existing cleanup, trace discovery, and relative-path reporting behavior unchanged.Source: Path instructions
🤖 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.
Duplicate comments:
In `@examples/notebooks/01_quickstart.ipynb`:
- Around line 377-389: Wrap the multi-turn start_runtime/invoke flow guarded by
CAN_RUN_HERMES in try/except so runtime-level failures are handled without
terminating the notebook. Preserve the existing two-turn invocation and output
behavior on success, and use the same failure-handling approach as the one-shot
run cell above.
- Around line 333-348: Wrap the live execution block in the notebook around
fabric.run, including result processing and artifact printing, with the same
try/except handling used by the 02_variations.ipynb harness loop. Ensure runtime
failures are caught and reported without aborting top-to-bottom execution, while
preserving the existing CAN_RUN_HERMES skip path and successful output behavior.
In `@examples/notebooks/02_variations.ipynb`:
- Around line 294-334: Update the relay_dir assignment in the telemetry example
so Relay artifacts are created under the documented
examples/notebooks/artifacts/relay root rather than BASE_DIR from the
code-review agent. Reuse the notebook repository-root or notebook-directory
symbol already available in the surrounding setup, and keep the existing
cleanup, trace discovery, and relative-path reporting behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 827e99fc-2029-452e-b5ce-b0a63ed888b8
⛔ Files ignored due to path filters (2)
examples/notebooks/img/fabric-contract.svgis excluded by!**/*.svgexamples/notebooks/img/three-axes.svgis excluded by!**/*.svg
📒 Files selected for processing (11)
.gitignoreexamples/README.mdexamples/code_review_agent/README.mdexamples/code_review_agent/__init__.pyexamples/code_review_agent/__main__.pyexamples/code_review_agent/config.pyexamples/notebooks/01_quickstart.ipynbexamples/notebooks/02_variations.ipynbexamples/notebooks/README.mdjustfiletests/python/test_code_review_example.py
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Test (arm64)
- GitHub Check: Test (x86_64)
🧰 Additional context used
📓 Path-based instructions (3)
{adapters/**,examples/**}
⚙️ CodeRabbit configuration file
{adapters/**,examples/**}: Review adapter and example changes for command correctness, config/schema consistency, artifact handling, and compatibility with the public Fabric contracts.
Files:
examples/code_review_agent/__init__.pyexamples/code_review_agent/config.pyexamples/code_review_agent/__main__.pyexamples/code_review_agent/README.mdexamples/README.mdexamples/notebooks/01_quickstart.ipynbexamples/notebooks/README.mdexamples/notebooks/02_variations.ipynb
tests/**/*.py
📄 CodeRabbit inference engine (.agents/skills/python-tests/SKILL.md)
tests/**/*.py: Usepytestto run Python tests.
Do not add@pytest.mark.asyncioto test functions; async tests are detected and run automatically.
Do not add-> Nonereturn type annotations to test functions.
When mocking a class, do not define a new class; useunittest.mock.MagicMockorunittest.mock.AsyncMock, addingspecwhen needed.
Name mocked classes with amockprefix, notfake.
Prefer pytest fixtures over helper methods.
Do not duplicate fixtures across test files; if a fixture is needed in multiple test files, define it inconftest.py.
When creating a fixture, use@pytest.fixture(name="<fixture_name>"[, scope="<scope>"])and define the function as<fixture_name>_fixture() -> <return_type>; only passscopewhen it is notfunction.
Preferpytest.mark.parametrizeover creating separate tests for different input types.
If a fixture is needed for a test but does not return a value, or its value is unused, use@pytest.mark.usefixtures.
When modifying environment variables in a test, useos.environ;tests/conftest.pyprovides an autouserestore_environ_fixturethat restores environment variables after each test, somonkeypatch.setenvis unnecessary.
Files:
tests/python/test_code_review_example.py
{tests/**,python/tests/**}
⚙️ CodeRabbit configuration file
{tests/**,python/tests/**}: Tests should cover the behavior promised by the changed API surface, including error paths, lifecycle cleanup, and SDK/native parity where relevant.
Files:
tests/python/test_code_review_example.py
🧬 Code graph analysis (2)
examples/code_review_agent/config.py (2)
python/src/nemo_fabric/models.py (2)
remove_skill_path(467-474)remove_mcp_server(450-457)crates/fabric-core/src/runtime.rs (2)
adapter_id(1129-1134)harness(1147-1152)
tests/python/test_code_review_example.py (2)
python/src/nemo_fabric/types.py (2)
mcp(628-631)mcp(634-635)crates/fabric-core/src/runtime.rs (2)
adapter_id(1129-1134)harness(1147-1152)
🪛 Ruff (0.15.21)
examples/notebooks/01_quickstart.ipynb
[warning] 125-129: Use format specifiers instead of percent format
Replace with format specifiers
(UP031)
[error] 133-133: await statement outside of a function
(F704)
[error] 133-133: await should be used within an async function
(PLE1142)
[error] 148-148: await statement outside of a function
(F704)
[error] 148-148: await should be used within an async function
(PLE1142)
[error] 165-169: await should be used within an async function
(PLE1142)
[error] 165-165: await statement outside of a function
(F704)
[error] 165-165: await should be used within an async function
(PLE1142)
[error] 166-166: await statement outside of a function
(F704)
[error] 166-166: await should be used within an async function
(PLE1142)
[error] 167-169: await statement outside of a function
(F704)
[error] 167-169: await should be used within an async function
(PLE1142)
examples/notebooks/02_variations.ipynb
[error] 119-119: await statement outside of a function
(F704)
[error] 119-119: await should be used within an async function
(PLE1142)
[warning] 123-123: Do not catch blind exception: Exception
(BLE001)
[error] 178-178: await statement outside of a function
(F704)
[error] 178-178: await should be used within an async function
(PLE1142)
[warning] 179-179: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (16)
examples/code_review_agent/config.py (2)
199-201: 🎯 Functional Correctness | ⚡ Quick winSystem prompt still mismatched with code-review purpose.
system_promptstill reads "You are a concise smoke test assistant," inconsistent with this being the code-review agent variant (per README/entrypoint). This was flagged in a prior review and remains unaddressed.🐛 Proposed fix
- "system_prompt": "You are a concise smoke test assistant.", + "system_prompt": "You are a concise code review assistant.",
187-223: LGTM!tests/python/test_code_review_example.py (2)
49-56: 🎯 Functional Correctness | ⚡ Quick winClaude assertions still omit harness settings and artifact paths.
Per prior feedback, the Claude config test should also assert
harness.settings(system_prompt,permission_mode) and artifact paths (runtime.artifacts,environment.artifacts) to catch shallow-copy or field-omission regressions; current assertions cover adapter id, model fields, and mcp/skills only.✅ Suggested additions
assert claude.mcp is None assert claude.skills is None + assert claude.harness.settings["permission_mode"] == "dontAsk" + assert claude.runtime.artifacts == "./artifacts/claude" + assert claude.environment.artifacts == "./artifacts/claude"As per path instructions, "Tests should cover the behavior promised by the changed API surface, including error paths, lifecycle cleanup, and SDK/native parity where relevant."
Source: Path instructions
15-15: LGTM!Also applies to: 30-48, 89-101, 104-137
examples/code_review_agent/__init__.py (1)
9-9: LGTM!Also applies to: 22-36
examples/code_review_agent/__main__.py (1)
17-17: LGTM!Also applies to: 25-31
examples/code_review_agent/README.md (2)
56-65: LGTM!
64-64: 🎯 Functional CorrectnessValid link
../../adapters/claude/README.mdexists, so this row is fine.> Likely an incorrect or invalid review comment.examples/notebooks/02_variations.ipynb (2)
192-216: 🎯 Functional CorrectnessVerify
models["default"]["model"]bracket access againstModelConfig's actual type.A prior review concluded
plan.effective_config.config.models["default"]returns aModelConfiginstance (not a nested mapping) and that["model"]would raise at runtime, proposing.modelattribute access instead. That comment is marked "Addressed in commit ec43da0," but this line is unchanged from the flagged pattern. Either the fix was applied elsewhere/reverted, oreffective_config.configactually serializes to a dict here (unlikeresolve(...).configused at Line 244-247, which is accessed via.get(...), hinting these two.configaccessors might return different shapes). Please confirm which is the case; standard PydanticBaseModelinstances do not support bracket access unless a custom__getitem__is defined.#!/bin/bash # Confirm the type returned by plan().effective_config.config and whether ModelConfig supports __getitem__ rg -nP 'class\s+ModelConfig\b' --type=py -A 20 rg -nP 'effective_config' --type=py -B5 -A 15 rg -nP 'def __getitem__' --type=py -B5 -A5
1-191: LGTM!Also applies to: 217-293, 335-375
.gitignore (1)
29-31: LGTM!examples/README.md (1)
10-20: LGTM!Also applies to: 30-30
examples/notebooks/README.md (1)
1-58: LGTM! The previously flagged missingOPENAI_API_KEYcredential note is now included.justfile (1)
328-332: LGTM!examples/notebooks/01_quickstart.ipynb (2)
130-139: LGTM!HAS_ADAPTERnow validates thatADAPTER_PYTHONpoints to an existing file, addressing the earlier gap where merely setting the env var marked Hermes as ready.
1-129: LGTM!Also applies to: 140-332, 390-429
Add a newcomer-facing tour of the Fabric Python SDK under examples/notebooks/: - 01_quickstart.ipynb: the whole lifecycle on one harness. Describe an agent as a typed FabricConfig, inspect it with plan(), diagnose the environment with doctor(), run one request, read the normalized RunResult, and continue the conversation across turns with a stateful runtime. Each section explains the API call it introduces and when to reach for it. - 02_variations.ipynb: the three axes of variation, exercised for real. It runs the same agent on every harness whose prerequisites are present (Hermes, Deep Agents, Codex) and falls back to inspecting the resolved config for the rest, explaining what to provide to run them; adds and removes skills and MCP servers and shows the resolved effect; and turns on NeMo Relay, printing the full trace it emits. Each notebook opens with an SVG diagram: where Fabric provides the contract between your application and the harnesses, and the three axes of variation. Fill the previously missing Claude adapter in the example matrix so the variations notebook can import it: - Add claude_config() to the code-review example and register `--variant claude` in the CLI, keeping the entrypoint and notebook in sync. - Extend the example contract tests with Claude coverage. Wire up notebook tooling and navigation: - Add a `just notebooks` launch target that fetches Jupyter on demand, so the notebook tooling stays out of the project lockfile. - Link the notebooks from examples/README.md as the recommended starting point. - Ignore Jupyter `.ipynb_checkpoints/`. Both notebooks are committed output-free; run artifacts land under the gitignored examples/notebooks/artifacts/. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Yuchen Zhang <yuchenz@nvidia.com>
8bb5029 to
f61d277
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
examples/code_review_agent/config.py (1)
199-201: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winClaude system prompt still says "smoke test assistant."
This mismatch was already flagged in a prior review round and remains unresolved: the prompt describes a smoke-test assistant while the entrypoint/README present this as a code-review variant, so Claude runs may not review
WORKSPACEas intended.🐛 Proposed fix
- "system_prompt": "You are a concise smoke test assistant.", + "system_prompt": "You are a concise code review assistant.",🤖 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 `@examples/code_review_agent/config.py` around lines 199 - 201, Update the system_prompt value in the settings configuration to describe a concise code-review assistant rather than a smoke-test assistant, matching the code-review entrypoint and README so Claude reviews WORKSPACE as intended.
🤖 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.
Duplicate comments:
In `@examples/code_review_agent/config.py`:
- Around line 199-201: Update the system_prompt value in the settings
configuration to describe a concise code-review assistant rather than a
smoke-test assistant, matching the code-review entrypoint and README so Claude
reviews WORKSPACE as intended.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 3aa182ed-00e8-4773-ad7b-990aa489d552
⛔ Files ignored due to path filters (2)
examples/notebooks/img/fabric-contract.svgis excluded by!**/*.svgexamples/notebooks/img/three-axes.svgis excluded by!**/*.svg
📒 Files selected for processing (11)
.gitignoreexamples/README.mdexamples/code_review_agent/README.mdexamples/code_review_agent/__init__.pyexamples/code_review_agent/__main__.pyexamples/code_review_agent/config.pyexamples/notebooks/01_quickstart.ipynbexamples/notebooks/02_variations.ipynbexamples/notebooks/README.mdjustfiletests/python/test_code_review_example.py
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: Test (x86_64)
- GitHub Check: Test (arm64)
- GitHub Check: Pre-commit
🧰 Additional context used
📓 Path-based instructions (3)
{adapters/**,examples/**}
⚙️ CodeRabbit configuration file
{adapters/**,examples/**}: Review adapter and example changes for command correctness, config/schema consistency, artifact handling, and compatibility with the public Fabric contracts.
Files:
examples/README.mdexamples/code_review_agent/README.mdexamples/code_review_agent/__main__.pyexamples/notebooks/README.mdexamples/code_review_agent/__init__.pyexamples/code_review_agent/config.pyexamples/notebooks/01_quickstart.ipynbexamples/notebooks/02_variations.ipynb
tests/**/*.py
📄 CodeRabbit inference engine (.agents/skills/python-tests/SKILL.md)
tests/**/*.py: Usepytestto run Python tests.
Do not add@pytest.mark.asyncioto test functions; async tests are detected and run automatically.
Do not add-> Nonereturn type annotations to test functions.
When mocking a class, do not define a new class; useunittest.mock.MagicMockorunittest.mock.AsyncMock, addingspecwhen needed.
Name mocked classes with amockprefix, notfake.
Prefer pytest fixtures over helper methods.
Do not duplicate fixtures across test files; if a fixture is needed in multiple test files, define it inconftest.py.
When creating a fixture, use@pytest.fixture(name="<fixture_name>"[, scope="<scope>"])and define the function as<fixture_name>_fixture() -> <return_type>; only passscopewhen it is notfunction.
Preferpytest.mark.parametrizeover creating separate tests for different input types.
If a fixture is needed for a test but does not return a value, or its value is unused, use@pytest.mark.usefixtures.
When modifying environment variables in a test, useos.environ;tests/conftest.pyprovides an autouserestore_environ_fixturethat restores environment variables after each test, somonkeypatch.setenvis unnecessary.
Files:
tests/python/test_code_review_example.py
{tests/**,python/tests/**}
⚙️ CodeRabbit configuration file
{tests/**,python/tests/**}: Tests should cover the behavior promised by the changed API surface, including error paths, lifecycle cleanup, and SDK/native parity where relevant.
Files:
tests/python/test_code_review_example.py
🧬 Code graph analysis (2)
examples/code_review_agent/config.py (2)
python/src/nemo_fabric/models.py (2)
remove_skill_path(467-474)remove_mcp_server(450-457)crates/fabric-core/src/runtime.rs (1)
adapter_id(1129-1134)
tests/python/test_code_review_example.py (2)
python/src/nemo_fabric/types.py (2)
skills(638-641)mcp(628-631)crates/fabric-core/src/runtime.rs (2)
adapter_id(1129-1134)harness(1147-1152)
🪛 Ruff (0.15.21)
examples/notebooks/01_quickstart.ipynb
[warning] 125-129: Use format specifiers instead of percent format
Replace with format specifiers
(UP031)
[error] 133-133: await statement outside of a function
(F704)
[error] 133-133: await should be used within an async function
(PLE1142)
[error] 149-149: await statement outside of a function
(F704)
[error] 149-149: await should be used within an async function
(PLE1142)
[warning] 150-150: Do not catch blind exception: Exception
(BLE001)
[error] 171-175: await should be used within an async function
(PLE1142)
[error] 171-171: await statement outside of a function
(F704)
[error] 171-171: await should be used within an async function
(PLE1142)
[error] 172-172: await statement outside of a function
(F704)
[error] 172-172: await should be used within an async function
(PLE1142)
[error] 173-175: await statement outside of a function
(F704)
[error] 173-175: await should be used within an async function
(PLE1142)
[warning] 176-176: Do not catch blind exception: Exception
(BLE001)
examples/notebooks/02_variations.ipynb
[error] 119-119: await statement outside of a function
(F704)
[error] 119-119: await should be used within an async function
(PLE1142)
[warning] 123-123: Do not catch blind exception: Exception
(BLE001)
[error] 178-178: await statement outside of a function
(F704)
[error] 178-178: await should be used within an async function
(PLE1142)
[warning] 179-179: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (13)
examples/notebooks/02_variations.ipynb (2)
197-197: Use attribute access forModelConfig.This remains
models["default"]["model"]; usemodels["default"].modelto avoid a runtime failure.
1-196: LGTM!Also applies to: 198-374
.gitignore (1)
29-31: LGTM!examples/README.md (1)
10-20: LGTM!Also applies to: 30-30
examples/notebooks/README.md (1)
1-59: LGTM!justfile (1)
328-332: LGTM!examples/notebooks/01_quickstart.ipynb (1)
1-438: LGTM!examples/code_review_agent/config.py (2)
187-198: LGTM!Also applies to: 202-210, 221-223
211-220: 🗄️ Data Integrity & IntegrationNo collision risk here.
runtime.artifactsis the manifest root for this flow, andenvironment.artifactsis only a fallback/default path, so using./artifacts/claudefor both does not create competing writers.> Likely an incorrect or invalid review comment.examples/code_review_agent/__init__.py (1)
9-9: LGTM!Also applies to: 25-25
examples/code_review_agent/__main__.py (1)
17-17: LGTM!Also applies to: 29-29
examples/code_review_agent/README.md (1)
56-65: LGTM!tests/python/test_code_review_example.py (1)
15-15: LGTM!Also applies to: 35-37, 49-56, 92-97, 109-109
Remove the "Composed per run" and "Toggle on / off" pills and align the remaining capability/telemetry pills with the harness column. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Yuchen Zhang <yuchenz@nvidia.com>
Drop "— add / remove" from the Skills and MCP servers pills in the three-axes diagram. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Yuchen Zhang <yuchenz@nvidia.com>
The Claude variant sets a workspace and keeps the adapter's default tools, so "smoke test assistant" was a mismatch. Use a concise code-review prompt matching the example's code-review framing. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Yuchen Zhang <yuchenz@nvidia.com>
Redirect the variations notebook's Relay output to examples/notebooks/artifacts/relay (overriding the example builder's default), so telemetry lands under the documented notebook artifact location, and update the README accordingly. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Yuchen Zhang <yuchenz@nvidia.com>
AnuradhaKaruppiah
left a comment
AnuradhaKaruppiah
left a comment
There was a problem hiding this comment.
Overall, this is directionally good and the notebooks provide a useful introduction to Fabric. I have a few comments about the conceptual organization and terminology:
-
Could we organize the variations notebook into two groups rather than three axes?
- Harness variation: run the same logical agent across Hermes, Codex, Claude, and Deep Agents.
- Agent configuration variation: vary skills, MCP servers, models, telemetry, and other FabricConfig fields. Telemetry is another configuration variation rather than a separate peer axis.
-
Running the same agent across harnesses is Fabric’s primary purpose, but the current harness variants also change the model, system prompt, skills/MCP configuration, and input schema. This makes it unclear what “same agent” means and prevents this from being a controlled harness comparison. Could the notebook keep the portable agent configuration constant and isolate the necessary harness-specific settings? If some differences are unavoidable, they should be called out explicitly.
-
Please distinguish harnesses, adapters, and the Fabric runtime consistently:
- Hermes, Codex, Claude, and Deep Agents are agent harnesses.
- Adapters connect Fabric to those harnesses.
- Runtime should refer to Fabric’s execution/invocation lifecycle. Phrases such as “compare runtimes,” “when the runtime changes,” and “which runtime to use” should use “harness” where that is what is actually changing.
-
Please use “Hermes” consistently in the user-facing narrative rather than “NVIDIA Hermes” or “Hermes SDK.” Hermes CLI is going away, so the SDK qualifier creates an unnecessary distinction. Technical identifiers such as adapter IDs and internal builder names can remain implementation-specific.
-
with_relay() is an example-local helper, but in an onboarding notebook it looks like part of the Fabric API and hides the actual configuration change. Could the notebook show the public FabricConfig mechanism directly—for example, clone the config and call enable_relay(...)—so readers can see how telemetry is configured?
Reorganize 02_variations around two kinds of variation instead of three axes: harness variation (run the same agent on Hermes, Codex, Claude, Deep Agents) and agent configuration variation (skills, MCP servers, models, telemetry). - Make the harness comparison controlled: build every variant from one shared portable config and change only the harness, calling out the unavoidable differences (model provider, Codex input schema, instruction delivery). - Show telemetry through the public FabricConfig.enable_relay() rather than the example-local with_relay() helper, so the config change is visible. - Use "harness" (not "runtime") and "Hermes" (not "Hermes SDK") in the narrative. - Replace the three-axes diagram with a two-group diagram. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Yuchen Zhang <yuchenz@nvidia.com>
Reserve "runtime" for Fabric's execution lifecycle and use "harness" where the
harness is what changes ("compare harnesses", "when the harness changes",
"which harness runs it"). Use "Hermes" in the narrative instead of "NVIDIA
Hermes" / "Hermes SDK", keeping technical identifiers unchanged. Update the
contract diagram's harness box to "Hermes".
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Yuchen Zhang <yuchenz@nvidia.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
examples/notebooks/02_variations.ipynb (1)
241-264: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winRegression:
ModelConfigsubscript access will crash Group 1 for every harness.Line 247 still does
plan.effective_config.config.models["default"]["model"].models["default"]is aModelConfigobject, not a mapping — subscripting it with["model"]raises at runtime. This exact defect was previously flagged and marked "Addressed," yet it persists in the current code (and directly contradicts the correct.modelattribute access used two cells later insummarize()at line 291). Since this line executes unconditionally for all four harnesses before theblocker()check, it will break the notebook on the very first loop iteration.🐛 Proposed fix
- model = plan.effective_config.config.models["default"]["model"] + model = plan.effective_config.config.models["default"].modelAs per path instructions, "Review adapter and example changes for command correctness, config/schema consistency... and compatibility with the public Fabric contracts."
🤖 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 `@examples/notebooks/02_variations.ipynb` around lines 241 - 264, Update the model lookup in the harness loop to access the `ModelConfig` object’s `.model` attribute instead of subscripting it with `["model"]`; keep it consistent with the existing `.model` access in `summarize()` so the unconditional setup succeeds for every harness.Source: Path instructions
🤖 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.
Duplicate comments:
In `@examples/notebooks/02_variations.ipynb`:
- Around line 241-264: Update the model lookup in the harness loop to access the
`ModelConfig` object’s `.model` attribute instead of subscripting it with
`["model"]`; keep it consistent with the existing `.model` access in
`summarize()` so the unconditional setup succeeds for every harness.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 349dd8f3-36ec-4f1d-830c-06cc1edd728b
⛔ Files ignored due to path filters (1)
examples/notebooks/img/variations.svgis excluded by!**/*.svg
📒 Files selected for processing (1)
examples/notebooks/02_variations.ipynb
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: Test (arm64)
- GitHub Check: Test (x86_64)
- GitHub Check: Pre-commit
- GitHub Check: Build and publish docs
🧰 Additional context used
📓 Path-based instructions (1)
{adapters/**,examples/**}
⚙️ CodeRabbit configuration file
{adapters/**,examples/**}: Review adapter and example changes for command correctness, config/schema consistency, artifact handling, and compatibility with the public Fabric contracts.
Files:
examples/notebooks/02_variations.ipynb
🔇 Additional comments (5)
examples/notebooks/02_variations.ipynb (5)
1-129: LGTM!
132-239: LGTM!
266-311: LGTM!
313-382: LGTM!Relay artifact path now correctly resolves under
examples/notebooks/artifacts/, and the telemetry run is properly guarded with try/except matching the harness-loop pattern, addressing both prior concerns.
384-403: LGTM!
…example Reframe 02_variations as the advanced notebook that builds on the maintained code-review example: it imports the example's base_config() as the portable agent and composes on it, linking to the example. The quickstart stays fully self-contained. - Derive the controlled harness comparison from base_config (holding the base constant; drop the skill/MCP the example carries, since not every harness accepts them, and call that out). - Compose skills, MCP servers, and models back onto base_config copies in the configuration-variation group. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Yuchen Zhang <yuchenz@nvidia.com>
State the intent of each notebook: the quickstart is fully self-contained (every agent built inline); the variations notebook is advanced composition built on the maintained code-review example. Link the quickstart's recap and the README to that example, and fix a lingering "one runtime" -> "one harness". Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Yuchen Zhang <yuchenz@nvidia.com>
Lead 01_quickstart with the run path (run(), start_runtime()) and move the optional inspection calls (plan(), doctor()) into a clearly-labeled "Inspect and diagnose (optional)" section, so it no longer reads like a required configure -> plan -> doctor -> run pipeline. Add an up-front callout and a run-vs-inspect table, frame doctor() as recommended-not-required, and regroup the contract diagram to match. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Yuchen Zhang <yuchenz@nvidia.com>
Color plan() and doctor() the same green as run()/start_runtime() instead of muted gray, which read as deprecated. The run-vs-inspect grouping is still carried by the section labels and the divider. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Yuchen Zhang <yuchenz@nvidia.com>
Address review feedback: - ADAPTER_PYTHON is no longer required. When Hermes and its adapter are installed in the notebook's own environment (e.g. `uv sync --all-groups --all-extras`) the quickstart runs without one; otherwise it defaults to a dedicated Hermes venv. This also removes the is_file() check that skipped a relative ADAPTER_PYTHON. - Drop the hard-coded harness-config count from the code-review README to match main. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Yuchen Zhang <yuchenz@nvidia.com>
Adopt the Hermes adapter rename (#59): hermes_config / nvidia.fabric.hermes and the removal of hermes_cli, plus the Claude Relay (#62) and max_iterations (#64) changes. Keep the Claude example variant and the onboarding notebooks; update the notebooks and docs to the renamed adapter id and the #quick-start-hermes-agent anchor; reconcile the example contract tests.
Adopt the suggested pattern: keep ADAPTER_PYTHON optional (default to this interpreter when unset), but when it is set, resolve a relative value against the repo root before is_file() and normalize it to absolute, so a relative ADAPTER_PYTHON (e.g. `ADAPTER_PYTHON=.tmp/hermes-venv/bin/python just notebooks`) validates and launches correctly instead of being skipped. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Yuchen Zhang <yuchenz@nvidia.com>
|
/merge |
3 participants
Overview
Adds a newcomer-facing tour of the NeMo Fabric Python SDK as two Jupyter notebooks under
examples/notebooks/:01_quickstart.ipynb— the full lifecycle on one harness: describe an agent as a typedFabricConfig, inspect it withplan(), diagnose the environment withdoctor(),run()one request, read the normalizedRunResult, and continue across turns with a stateful runtime. Each section explains the API call it introduces and when to reach for it.02_variations.ipynb— the three axes of variation, exercised for real: it runs the same agent on every harness whose prerequisites are present (Hermes, Deep Agents, Codex) and falls back to inspecting the resolved config for the rest (explaining what to provide to run them); adds and removes skills and MCP servers and shows the resolved effect; and turns on NeMo Relay to emit trace files. It reuses the maintained code-review builders so the tour never drifts from runnable code.Supporting changes:
claude_config()builder to the code-review example and register--variant claudein the CLI — this fills the previously missing Claude adapter in the example matrix so the variations notebook can import it — and extend the example contract tests with Claude coverage.just notebookslaunch target that fetches Jupyter on demand, so the notebook tooling stays out of the project lockfile; link the notebooks fromexamples/README.mdas the recommended starting point; ignore Jupyter.ipynb_checkpoints/.Both notebooks are committed output-free and were executed end to end before commit; run artifacts land under the gitignored
examples/notebooks/artifacts/.Where should the reviewer start?
.ipynb):examples/notebooks/01_quickstart.ipynbthenexamples/notebooks/02_variations.ipynb.claude_config()builder inexamples/code_review_agent/config.py(adapter id, model id, and settings).Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
N/A — no associated public GitHub issue.
I confirm this contribution is my own work, or I have the right to submit it under this project's license.
I searched existing issues and open pull requests, and this does not duplicate existing work.
🤖 Generated with Claude Code
Summary by CodeRabbit
notebooksrecipe to launch the onboarding notebooks.