feat(python): add RunOutput response contract - #52
Conversation
Signed-off-by: Eric Evans <194135482+ericevans-nv@users.noreply.github.com>
WalkthroughAdds the public ChangesRunOutput model and normalization
Consumer integration and contract tests
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Adapter
participant RunResult
participant RunOutput
participant CodeReviewExample
Adapter->>RunResult: provide adapter output
RunResult->>RunOutput: convert mapping-shaped output
RunOutput-->>RunResult: provide response and extra fields
RunResult-->>CodeReviewExample: expose normalized output
CodeReviewExample->>RunOutput: read response
RunOutput-->>CodeReviewExample: return response or None
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
📖 Fern docs preview: https://nvidia-preview-pull-request-52.docs.buildwithfern.com/nemo/fabric |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@docs/reference/api/python-library-reference/nemo_fabric.types.md`:
- Around line 858-861: Add a blank line immediately after the generated
RunOutput heading in the RunOutput API reference section, ensuring the heading
is surrounded by blank lines and markdownlint MD022 passes.
In `@tests/python/test_sdk_contract.py`:
- Around line 723-767: Add a test alongside the existing RunOutput tests that
calls RunOutput.from_mapping with a non-string response such as {"response":
123} and asserts it raises FabricConfigError using pytest.raises, covering the
validation branch for invalid response values.
🪄 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: f194558e-9541-4744-8a50-465be16505e1
📒 Files selected for processing (7)
docs/reference/api/python-library-reference/index.mddocs/reference/api/python-library-reference/nemo_fabric.types.mdexamples/code_review_agent/__main__.pypython/src/nemo_fabric/__init__.pypython/src/nemo_fabric/types.pytests/python/test_code_review_example.pytests/python/test_sdk_contract.py
📜 Review details
⏰ Context from checks skipped due to timeout. (6)
- GitHub Check: CodeRabbit / Review
- GitHub Check: Build and publish docs
- GitHub Check: Pre-commit
- GitHub Check: Build wheels (x86_64)
- GitHub Check: Test (arm64)
- GitHub Check: Test (x86_64)
🧰 Additional context used
📓 Path-based instructions (6)
**
⚙️ CodeRabbit configuration file
**:Contributing to NeMo Fabric
Thank you for your interest in contributing to NeMo Fabric. This guide covers
the development workflow, coding standards, and pull request process.Development Setup
This section collects the setup steps needed before building, testing, or
contributing changes.Package Installation
NeMo Fabric is not currently available on PyPI. To consume the Python packages,
build wheels from a source checkout:just wheels uv pip install --find-links dist "nemo-fabric[runtime]"Adapters are distributed as optional extras. For example, install the Hermes
SDK adapter with:uv pip install --find-links dist "nemo-fabric[adapters-hermes-sdk]"Refer to the installation guide for the
complete list of adapters and installation options.Source Development
Install these tools before you start:
- Rust (stable toolchain) -- install with rustup
- Python >= 3.11
- uv -- follow the uv installation guide
- just >= 1.50.0 --
cargo install just --lockedClone the repository, create a virtual environment, and build the Rust and
Python packages:git clone https://github.com/NVIDIA/NeMo-Fabric.git cd NeMo-Fabric uv venv --seed .venv --python 3.13 source .venv/bin/activate uv sync --all-groups --all-extras just no_uv=true build-allVerify the checkout by running the test suites described in
Testing Requirements.Release Tagging
Versioned release tags must use raw Rust-compatible SemVer without a leading
v.
- Use
0.1.0for stable releases.- Use
0.1.0-rc.1for prereleases.- Do not create tags such as
v0.1.0orv0.1.0-rc.1.This keeps release tags aligned with Cargo package versions and lets...
Files:
docs/reference/api/python-library-reference/index.mdpython/src/nemo_fabric/__init__.pytests/python/test_code_review_example.pyexamples/code_review_agent/__main__.pytests/python/test_sdk_contract.pypython/src/nemo_fabric/types.pydocs/reference/api/python-library-reference/nemo_fabric.types.md
{docs/**,README.md,AGENTS.md}
⚙️ CodeRabbit configuration file
{docs/**,README.md,AGENTS.md}: Review documentation for technical accuracy against the current API, command correctness, and consistency with generated schemas.
Files:
docs/reference/api/python-library-reference/index.mddocs/reference/api/python-library-reference/nemo_fabric.types.md
python/src/nemo_fabric/**/*
⚙️ CodeRabbit configuration file
python/src/nemo_fabric/**/*: Review Python SDK changes for typed API consistency, import-time dependency neutrality, async/session behavior, and parity with the native extension.
Stubs and runtime implementations should stay aligned.
Files:
python/src/nemo_fabric/__init__.pypython/src/nemo_fabric/types.py
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.pytests/python/test_sdk_contract.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.pytests/python/test_sdk_contract.py
{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/__main__.py
🧠 Learnings (1)
📚 Learning: 2026-06-28T04:03:32.877Z
Learnt from: AjayThorve
Repo: NVIDIA/NeMo-Fabric PR: 26
File: python/tests/smoke_typed_config.py:163-177
Timestamp: 2026-06-28T04:03:32.877Z
Learning: In NVIDIA NeMo Fabric Python SDK serialization of `RuntimeCapabilities` (to satisfy the “parity contract” with Rust core and the CLI), do not emit metadata keys when the corresponding metadata is absent. Instead, omit those fields entirely so the produced JSON matches the Rust/CLI output (e.g., avoid `null`, empty objects, or placeholder metadata). During review, verify the serializer/builders follow this omission rule and that Python outputs/parity tests reflect the same shape.
Applied to files:
python/src/nemo_fabric/__init__.pypython/src/nemo_fabric/types.py
🧬 Code graph analysis (1)
python/src/nemo_fabric/types.py (2)
tests/python/test_sdk_contract.py (1)
RunResult(40-40)python/src/nemo_fabric/__init__.py (1)
RunResult(44-44)
🪛 markdownlint-cli2 (0.22.1)
docs/reference/api/python-library-reference/nemo_fabric.types.md
[warning] 858-858: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🪛 Ruff (0.15.20)
python/src/nemo_fabric/types.py
[warning] 1223-1223: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (8)
python/src/nemo_fabric/types.py (2)
1223-1223: 📐 Maintainability & Code QualityConfirm the TRY003 policy for this exception message.
Ruff reports TRY003 on Line 1223. If this rule is enforced, follow the repository’s exception-message convention or explicitly suppress it consistently for
FabricConfigError.Source: Linters/SAST tools
1208-1222: LGTM!Also applies to: 1224-1233, 1251-1252, 1269-1269, 1294-1294, 1328-1332
python/src/nemo_fabric/__init__.py (1)
41-41: LGTM!Also applies to: 75-75
docs/reference/api/python-library-reference/nemo_fabric.types.md (1)
821-852: LGTM!Also applies to: 864-888, 944-944
docs/reference/api/python-library-reference/index.md (1)
46-46: LGTM!examples/code_review_agent/__main__.py (1)
57-61: LGTM!tests/python/test_code_review_example.py (1)
25-25: LGTM!Also applies to: 126-127
tests/python/test_sdk_contract.py (1)
36-36: LGTM!Also applies to: 697-720, 770-774
Signed-off-by: Eric Evans <194135482+ericevans-nv@users.noreply.github.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/reference/api/python-library-reference/nemo_fabric.types.md (1)
893-899: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake the
RunOutput.from_mappingreturn type concrete.Line 896 advertises
FabricMapping, but the implementation returnscls(mapping), soRunOutput.from_mapping(...)produces aRunOutput. Document the concrete type—or update the source annotation to useSelfand regenerate the reference—so typed consumers can discover.responsecorrectly.🤖 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 `@docs/reference/api/python-library-reference/nemo_fabric.types.md` around lines 893 - 899, Update the documented return type for `RunOutput.from_mapping` to the concrete `RunOutput` type, or change the source annotation for `from_mapping` to use `Self` and regenerate the API reference. Ensure the generated signature accurately exposes `RunOutput` members such as `.response`.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.
Outside diff comments:
In `@docs/reference/api/python-library-reference/nemo_fabric.types.md`:
- Around line 893-899: Update the documented return type for
`RunOutput.from_mapping` to the concrete `RunOutput` type, or change the source
annotation for `from_mapping` to use `Self` and regenerate the API reference.
Ensure the generated signature accurately exposes `RunOutput` members such as
`.response`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 9e345dca-7246-4f35-910d-9292f766201e
📒 Files selected for processing (2)
docs/reference/api/python-library-reference/nemo_fabric.types.mdtests/python/test_sdk_contract.py
📜 Review details
⏰ Context from checks skipped due to timeout. (6)
- GitHub Check: CodeRabbit / Review
- GitHub Check: Build and publish docs
- GitHub Check: Pre-commit
- GitHub Check: Test (arm64)
- GitHub Check: Build wheels (x86_64)
- GitHub Check: Test (x86_64)
🧰 Additional context used
📓 Path-based instructions (4)
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_sdk_contract.py
**
⚙️ CodeRabbit configuration file
**:Contributing to NeMo Fabric
Thank you for your interest in contributing to NeMo Fabric. This guide covers
the development workflow, coding standards, and pull request process.Development Setup
This section collects the setup steps needed before building, testing, or
contributing changes.Package Installation
NeMo Fabric is not currently available on PyPI. To consume the Python packages,
build wheels from a source checkout:just wheels uv pip install --find-links dist "nemo-fabric[runtime]"Adapters are distributed as optional extras. For example, install the Hermes
SDK adapter with:uv pip install --find-links dist "nemo-fabric[adapters-hermes-sdk]"Refer to the installation guide for the
complete list of adapters and installation options.Source Development
Install these tools before you start:
- Rust (stable toolchain) -- install with rustup
- Python >= 3.11
- uv -- follow the uv installation guide
- just >= 1.50.0 --
cargo install just --lockedClone the repository, create a virtual environment, and build the Rust and
Python packages:git clone https://github.com/NVIDIA/NeMo-Fabric.git cd NeMo-Fabric uv venv --seed .venv --python 3.13 source .venv/bin/activate uv sync --all-groups --all-extras just no_uv=true build-allVerify the checkout by running the test suites described in
Testing Requirements.Release Tagging
Versioned release tags must use raw Rust-compatible SemVer without a leading
v.
- Use
0.1.0for stable releases.- Use
0.1.0-rc.1for prereleases.- Do not create tags such as
v0.1.0orv0.1.0-rc.1.This keeps release tags aligned with Cargo package versions and lets...
Files:
tests/python/test_sdk_contract.pydocs/reference/api/python-library-reference/nemo_fabric.types.md
{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_sdk_contract.py
{docs/**,README.md,AGENTS.md}
⚙️ CodeRabbit configuration file
{docs/**,README.md,AGENTS.md}: Review documentation for technical accuracy against the current API, command correctness, and consistency with generated schemas.
Files:
docs/reference/api/python-library-reference/nemo_fabric.types.md
🧬 Code graph analysis (1)
tests/python/test_sdk_contract.py (1)
python/src/nemo_fabric/types.py (1)
RunOutput(1209-1232)
🔇 Additional comments (2)
docs/reference/api/python-library-reference/nemo_fabric.types.md (1)
858-889: LGTM!Also applies to: 904-921, 945-945
tests/python/test_sdk_contract.py (1)
36-36: LGTM!Also applies to: 718-781
There was a problem hiding this comment.
Python now rejects valid Fabric output accepted by the core.
RunOutput requires output.response to be str | None, but the authoritative Rust RunResult.output remains an unrestricted serde_json::Value, and the run-result schema imposes no response contract. Consequently, an adapter can return a core-valid result such as:
{"output": {"response": {"text": "hello"}}}The CLI/core accepts it, while Fabric.run() raises FabricConfigError while decoding the successful result. That introduces cross-surface incompatibility and can turn adapter success into an SDK failure.
Either:
- Make
response: string | nulla real Fabric-wide contract in Rust/schema/adapter validation, with the corresponding compatibility decision; or - Keep
RunOutput.responsepermissive and provide a separate typed accessor for canonical text.
Signed-off-by: Eric Evans <194135482+ericevans-nv@users.noreply.github.com>
Thanks, agreed. I changed this to avoid making Python stricter than the core contract.
I also added SDK contract coverage for structured |
|
/merge |
2 participants
Overview
Adds a Python SDK
RunOutputmapping contract so object-shapedRunResult.outputexposes
responseas a known JSON-valued field while preserving adapter-specificextension fields and existing mapping access.
Details
RunOutputfrom the Python SDK.RunResult.outputvalues asRunOutputwhile preservingnon-object JSON output as-is.
RunOutput.responsealigned with the core Fabric JSON contract insteadof enforcing a string-only adapter convention.
output.responsevalues through attribute access,mapping access, and
to_mapping().--show-outputpath to read the normalizedresponse value safely.
explicit null responses, structured response values, and non-object output
preservation.
RunOutputis documented.Validation
git diff --checkjust --set no_uv true test-python(254 passed, 4 skipped)Where should the reviewer start?
Start with
python/src/nemo_fabric/types.py, specifically the newRunOutputtype and
RunResult._normalize()wrapping logic.Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Relates to: FABRIC-63
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.
Summary by CodeRabbit
RunOutputtype to the Python SDK.RunOutput, includingresponseplus preserved extension fields.RunOutputand revisedRunResult.outputdocumentation.responsefrom normalized output when available.RunOutputbehavior andRunResultwrapping/serialization.