fix: align Deep Agents with Relay 0.6 - #107
Conversation
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
WalkthroughUpdated Relay dependency constraints and attribution versions, and changed observability translation to emit version 2 with sink-based ATOF configuration. Legacy ATOF fields and endpoints are converted into file and stream sinks, with tests covering the transformed output. ChangesRelay v2 migration
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant PluginConfig
participant relay_api_plugin_config
participant _relay_api_atof_config
participant ObservabilityConfig
PluginConfig->>relay_api_plugin_config: Provide plugin configuration
relay_api_plugin_config->>_relay_api_atof_config: Translate ATOF configuration
_relay_api_atof_config->>_relay_api_atof_config: Build file and stream sinks
_relay_api_atof_config-->>relay_api_plugin_config: Return sink-based AtofConfig
relay_api_plugin_config->>ObservabilityConfig: Emit version 2 configuration
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.com>
![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 `@adapters/common/src/nemo_fabric_adapters/common/utils.py`:
- Around line 338-364: Update the sink-processing loop in the ATOF configuration
parser to handle every unrecognized or malformed entry in value["sinks"] through
the existing unsupported-value policy, or raise a validation error when no such
policy applies. Do not silently continue for non-dict entries or unknown sink
types; preserve the existing handling for valid "file" and "stream" sinks.
In `@tests/adapters/test_adapaters_common_utils.py`:
- Around line 380-383: Update
test_relay_api_plugin_config_translates_flat_atof_to_relay_v06_sinks to set
TOKEN through os.environ instead of monkeypatch.setenv, relying on the autouse
restore_environ_fixture for cleanup. Remove the now-unused monkeypatch parameter
if no other code in the test uses it.
🪄 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: 2ea9a6a9-8f0b-4d85-9a10-28979418e484
📒 Files selected for processing (3)
ATTRIBUTIONS-Python.mdadapters/common/src/nemo_fabric_adapters/common/utils.pytests/adapters/test_adapaters_common_utils.py
📜 Review details
⏰ Context from checks skipped due to timeout. (17)
- GitHub Check: Test (Python 3.12, windows-amd64)
- GitHub Check: Test (Python 3.12, linux-amd64)
- GitHub Check: Test (Python 3.14, windows-amd64)
- GitHub Check: Test (Python 3.11, windows-amd64)
- GitHub Check: Test (Python 3.14, linux-amd64)
- GitHub Check: Test (Python 3.13, linux-amd64)
- GitHub Check: Test (Python 3.11, linux-amd64)
- GitHub Check: Test (Python 3.11, linux-arm64)
- GitHub Check: Test (Python 3.12, macos-arm64)
- GitHub Check: Test (Python 3.13, windows-amd64)
- GitHub Check: Test (Python 3.14, macos-arm64)
- GitHub Check: Test (Python 3.12, linux-arm64)
- GitHub Check: Test (Python 3.13, macos-arm64)
- GitHub Check: Test (Python 3.14, linux-arm64)
- GitHub Check: Test (Python 3.13, linux-arm64)
- GitHub Check: Test (Python 3.11, macos-arm64)
- GitHub Check: Pre-commit
🧰 Additional context used
📓 Path-based instructions (21)
**/*.{rs,py,pyi,json,yaml,yml}
📄 CodeRabbit inference engine (.agents/skills/contribute-api/SKILL.md)
Determine and update every affected public surface, including the CLI, PyO3 bindings, Python SDK, type stubs, schemas, and adapter contract, so they remain in parity.
Files:
tests/adapters/test_adapaters_common_utils.pyadapters/common/src/nemo_fabric_adapters/common/utils.py
**/*
📄 CodeRabbit inference engine (.agents/skills/karpathy-guidelines/SKILL.md)
**/*: Before implementing, explicitly state assumptions, surface ambiguity and tradeoffs, present multiple interpretations when relevant, and ask for clarification rather than silently deciding or proceeding when requirements are unclear.
Prefer the minimum code needed to solve the requested problem: avoid speculative features, unnecessary abstractions, unrequested flexibility, and handling of impossible scenarios; simplify overcomplicated solutions.
When editing existing code, make surgical changes only: do not modify unrelated code, comments, formatting, or pre-existing dead code; match the existing style, and remove only unused imports, variables, or functions introduced by your changes.
Define verifiable success criteria for each task, such as writing regression tests for bugs and invalid-input tests for validation, then verify the implementation against those criteria. For multi-step work, state a brief plan with a verification check for each step.
**/*: Always spellNVIDIAin all caps; do not useNvidia,nvidia,nVidia,nVIDIA, orNV.
Usean NVIDIAbefore a noun, because the name begins with an “en” sound.
Do not add a registered trademark symbol afterNVIDIAwhen referring to the company; use trademark symbols with product names only when required by the document type or legal guidance.
Verify official capitalization, spacing, hyphenation, and spelling for NVIDIA and third-party product names; do not rewrite official product names for grammar or title-case rules.
Precede NVIDIA product names withNVIDIAon first mention when natural and accurate, and link the first mention when the destination helps the reader.
On first use, include the company name and full model qualifier when it helps identify the model; preserve official capitalization and punctuation, and use shorter family names only after establishing the full name.
For learning-oriented and developer content, do not force trademark symbols unless explicitly required; for press, ...
Files:
tests/adapters/test_adapaters_common_utils.pyATTRIBUTIONS-Python.mdadapters/common/src/nemo_fabric_adapters/common/utils.py
**/*.{rs,py}
📄 CodeRabbit inference engine (.agents/skills/prepare-pr/SKILL.md)
For native binding changes, run
cargo check -p fabric-python --locked.
Files:
tests/adapters/test_adapaters_common_utils.pyadapters/common/src/nemo_fabric_adapters/common/utils.py
**/*.{py,pyi}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
If Python code or a Python-facing adapter changes, run
just test-python.Use type annotations for public Python APIs and keep native Python binding declarations synchronized with their Rust implementations.
Files:
tests/adapters/test_adapaters_common_utils.pyadapters/common/src/nemo_fabric_adapters/common/utils.py
**/*.{rs,py,pyi}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
**/*.{rs,py,pyi}: If public configuration types change, confirm schema snapshot tests injust test-rustpass and review generated schema diffs.
For schema or public contract changes, run both language suites and review changes underschemas/and generated API references.
**/*.{rs,py,pyi}: Usesnake_casefor Rust and Python functions and variables; usePascalCasefor Rust types and Python classes.
Keep native Python binding declarations synchronized with their Rust implementations when public contracts change.
Files:
tests/adapters/test_adapaters_common_utils.pyadapters/common/src/nemo_fabric_adapters/common/utils.py
tests/adapters/**/*.py
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
tests/adapters/**/*.py: If an adapter or integration changes, run its focused tests.
For adapter behavior changes, run focused adapter tests undertests/adapters, then runjust test-python.
Files:
tests/adapters/test_adapaters_common_utils.py
**/*.{py,pyi,rs}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
For Python SDK or PyO3 binding changes, use
python-tests, run focused pytest tests first, thenjust test-python; rebuild withjust build-pythonwhen native code or packaging changes.
Files:
tests/adapters/test_adapaters_common_utils.pyadapters/common/src/nemo_fabric_adapters/common/utils.py
tests/**/*.{rs,py}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
When adding functionality, include tests in the corresponding Rust crate or the relevant area under
tests/.
Files:
tests/adapters/test_adapaters_common_utils.py
tests/**/*.py
📄 CodeRabbit inference engine (.agents/skills/python-tests/SKILL.md)
tests/**/*.py: Use Pytest to run Python tests.
Do not add@pytest.mark.asyncioto tests; async tests are automatically detected and run by the async runner.
Do not add-> Nonereturn type annotations to test functions.
When mocking a class, useunittest.mock.MagicMockorunittest.mock.AsyncMock, using thespecargument when necessary, rather than defining a new class.
Prefix mocked class names withmock, notfake.
Prefer pytest fixtures over helper methods.
Do not repeat fixtures; place fixtures needed by multiple test files inconftest.py.
Define fixtures using@pytest.fixture(name="<fixture_name>"[, scope="<scope>"])and a function named<fixture_name>_fixture; specifyscopeonly when it is notfunction.
Preferpytest.mark.parametrizeover separate tests for different input types.
Use@pytest.mark.usefixtureswhen a fixture is needed but its return value is unused or it does not return a value.
Use the autouserestore_environ_fixturefromtests/conftest.pyto restore environment variables; modify variables withos.environand do not usemonkeypatch.setenv.
Avoid defensive programming in tests; access expected data directly so missing data raises a clear failure, such as usingresults["data"]instead ofresults.get("data").
Files:
tests/adapters/test_adapaters_common_utils.py
**/*.{rs,py,toml}
📄 CodeRabbit inference engine (.agents/skills/update-project-version/SKILL.md)
When editing version helpers, verify every
nemo-fabric-*workspace package through Cargo metadata and reject a static version inpython/pyproject.toml.
Files:
tests/adapters/test_adapaters_common_utils.pyadapters/common/src/nemo_fabric_adapters/common/utils.py
**/*.{toml,rs,py}
📄 CodeRabbit inference engine (.agents/skills/update-project-version/SKILL.md)
Avoid blind repository-wide replacement of version-like strings; distinguish package-version references from examples and unrelated dependency versions.
Files:
tests/adapters/test_adapaters_common_utils.pyadapters/common/src/nemo_fabric_adapters/common/utils.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/adapters/test_adapaters_common_utils.py
**/*.{md,rst}
📄 CodeRabbit inference engine (.agents/skills/contribute-api/SKILL.md)
Update documentation and examples in the same branch as the public API change.
Files:
ATTRIBUTIONS-Python.md
**/*.{md,mdx,rst}
📄 CodeRabbit inference engine (.agents/skills/review-doc-style/assets/nvidia-style-guide.md)
**/*.{md,mdx,rst}: For NeMo Fabric documentation, verify technical claims against the current repository, public API, or documented command before reviewing style.
Always spellNVIDIAin all caps; do not useNvidia,nvidia, orNV.
Format commands, code elements, expressions, package names, file names, and paths as inline code.
Use descriptive link text; avoid raw URLs and weak anchors such ashereorread more.
Use title case consistently for technical documentation headings.
Introduce code blocks, lists, tables, and images with complete sentences.
Write procedures as imperative, parallel steps; split long procedures into smaller tasks.
Prefer active voice, present tense, short sentences, contractions, and plain English while preserving necessary technical precision.
Usecanfor possibility and reservemayfor permission.
Useafterfor temporal relationships instead ofonce, and preferrefer tooverseewhen directing readers to another resource.
Avoid culture-specific idioms, unnecessary Latinisms, jokes, and marketing exaggeration in technical documentation.
Spell out months in body text, avoid ordinal dates, and use clear time zones.
Spell out whole numbers from zero through nine unless they are technical values, parameters, versions, or UI values; use numerals for 10 or greater and commas in thousands.
Do not add trademark symbols to learning-oriented documentation unless the source, platform, or legal guidance explicitly requires them.
Do not replace precise technical terms with simpler words when doing so would lose precision.
Do not flag passive voice when the actor is unknown or the action is the important part.
Do not rewrite API names, package names, command flags, or code literals for style.
**/*.{md,mdx,rst}: Use consistent title case for technical-document headings and table headers; avoid quotation marks, ampersands, and exclamation marks in headings, while preserving official product, event, research, and whitepaper title ...
Files:
ATTRIBUTIONS-Python.md
**/*.{md,rst,txt,adoc}
📄 CodeRabbit inference engine (.agents/skills/review-doc-style/assets/nvidia-style-language-mechanics.md)
**/*.{md,rst,txt,adoc}: For technical documentation, use professional, active, conversational, engaging, precise, and plain-English prose. Prefer active voice, present tense, short sentences, and scannable paragraphs. Avoid casual or imprecise language, swearing, threats, insults, jokes, puns, culture-specific idioms, marketing exaggeration, and unsupported third-party comparisons.
Usecanfor possibility and reservemayfor permission; useafterfor temporal order; userefer tofor cross-references; prefer short direct sentences and specific verbs; avoid unnecessarypleasein technical documentation.
Prefer active voice when the actor matters. Passive voice is acceptable when the actor is unknown or irrelevant, when the action or result is the focus, or in programmer documentation.
Use natural contractions in conversational technical prose, but do not force them in formal legal copy, API references, or generated text.
Prefer simpler English over Latinisms: usefor exampleorsuch asinstead ofe.g.,and so oninstead ofetc.,that isinstead ofi.e.,compared toinstead ofvs., andby,through, orusinginstead ofvia. Use industry-standard terms such as in silico, in vitro, and in vivo when appropriate, and italicize them in running text.
Usethatwithout commas for essential clauses, andwhichwith commas for nonessential clauses.
Format dates and times clearly: spell out months in body text; use forms such asJune 12, 2025; avoid numeric or ordinal dates; capitalize days; use 12-hour time when appropriate; include a space beforea.m.orp.m.; useETandPTfor needed time zones; avoid24/7; and preferfrom 12:30 to 1:00 p.m.for prose ranges.
Format numbers consistently: spell out zero through nine in body text, use numerals for 10 or greater and for technical values, use commas in thousands, do not begin a sentence with a numeral, spell out ordinals, and use numerals consistently within a category wh...
Files:
ATTRIBUTIONS-Python.md
**/ATTRIBUTIONS-*.md
📄 CodeRabbit inference engine (.agents/skills/prepare-pr/SKILL.md)
Regenerate and include changed
ATTRIBUTIONS-*.mdfiles.
Files:
ATTRIBUTIONS-Python.md
**/*.{md,mdx}
📄 CodeRabbit inference engine (.agents/skills/review-doc-style/SKILL.md)
**/*.{md,mdx}: Use the full product nameNVIDIA NeMo Fabricon its first usage, typically in the title or H1; useNeMo Fabricthereafter.
Usefabricby itself only when referring to the CLI tool, and surround those references with backticks.
CapitalizeNVIDIAcorrectly in public documentation.
Format commands, code elements, expressions, file names, paths, and filenames as inline code where needed.
Use title case consistently for headings in technical documentation.
Introduce code blocks, tables, and lists with complete lead-in sentences.
Use descriptive anchor text instead of raw URLs or generic link text such ashere.
Prefer active voice, present tense, short sentences, and plain English.
Use consistent terminology for the same concept throughout a document.
Write procedures as imperative, parallel, easy-to-scan steps, and split long sequences into smaller tasks.
Useafterinstead ofoncewhen expressing temporal sequence.
Usecaninstead ofmaywhen the intended meaning is possibility rather than permission.
Avoid ambiguous numeric dates and ordinal dates in body text.
For learning-oriented documentation, do not force trademark symbols unless the source document explicitly requires them.
Introduce examples' code blocks with full sentences and ensure examples match current APIs and build commands.For documentation-site changes, run
just docsto regenerate Python and Rust API references and validate Fern configuration.
Files:
ATTRIBUTIONS-Python.md
**/{README.md,*.md,*.mdx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Update relevant SDK, API, adapter, example, integration, and embedded documentation when public behavior or the corresponding surface changes.
Files:
ATTRIBUTIONS-Python.md
**/*.{html,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
HTML and Markdown files must use the specified HTML comment form for SPDX license headers.
Files:
ATTRIBUTIONS-Python.md
{*.md,**/*.md,**/*.mdx,**/*.ipynb}
⚙️ CodeRabbit configuration file
{*.md,**/*.md,**/*.mdx,**/*.ipynb}: Enforce the product name in user-facing prose: use "NVIDIA NeMo Fabric" on first use and "NeMo Fabric" thereafter. Flag standalone capitalized "Fabric" when it refers to the product. Do not flag the lowercasefabricCLI command, package/import/crate names, code identifiers, API symbols, configuration keys, file paths, or unrelated generic uses of the word.
Files:
ATTRIBUTIONS-Python.md
{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 NeMo Fabric contracts.
Files:
adapters/common/src/nemo_fabric_adapters/common/utils.py
🪛 ast-grep (0.44.1)
tests/adapters/test_adapaters_common_utils.py
[info] 393-393: Do not hardcode temporary file or directory names
Context: "/tmp/atof"
Note: [CWE-377] Insecure Temporary File.
(hardcoded-tmp-file)
[info] 420-420: Do not hardcode temporary file or directory names
Context: "/tmp/atof"
Note: [CWE-377] Insecure Temporary File.
(hardcoded-tmp-file)
🪛 Ruff (0.15.21)
tests/adapters/test_adapaters_common_utils.py
[error] 394-394: Probable insecure usage of temporary file or directory: "/tmp/atof"
(S108)
[error] 421-421: Probable insecure usage of temporary file or directory: "/tmp/atof"
(S108)
adapters/common/src/nemo_fabric_adapters/common/utils.py
[warning] 359-359: Use list.extend to create a transformed list
(PERF401)
🔇 Additional comments (3)
ATTRIBUTIONS-Python.md (1)
1804-1813: LGTM!Also applies to: 4763-4766, 5463-5466
adapters/common/src/nemo_fabric_adapters/common/utils.py (1)
288-291: LGTM!tests/adapters/test_adapaters_common_utils.py (1)
384-436: LGTM!
| from nemo_relay.observability import AtofFileSinkConfig | ||
| from nemo_relay.observability import AtofStreamSinkConfig | ||
|
|
||
| sinks: list[AtofFileSinkConfig | AtofStreamSinkConfig] = [] | ||
| has_explicit_file_sink = False | ||
| for sink in value.get("sinks") or []: | ||
| if not isinstance(sink, dict): | ||
| continue | ||
| if sink.get("type") == "file": | ||
| has_explicit_file_sink = True | ||
| sinks.append(_relay_api_atof_file_sink_config(sink)) | ||
| elif sink.get("type") == "stream": | ||
| sinks.append(_relay_api_atof_stream_sink_config(sink)) | ||
|
|
||
| if not has_explicit_file_sink and any( | ||
| key in value for key in ("output_directory", "filename", "mode") | ||
| ): | ||
| sinks.append(_relay_api_atof_file_sink_config(value)) | ||
|
|
||
| for endpoint in value.get("endpoints") or []: | ||
| if isinstance(endpoint, dict): | ||
| sinks.append(_relay_api_atof_stream_sink_config(endpoint)) | ||
|
|
||
| return AtofConfig( | ||
| enabled=bool(value.get("enabled", False)), | ||
| sinks=sinks, | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm nemo-relay's Python bindings for AtofConfig/sinks and whether
# unknown sink "type" values are expected to be rejected during validation.
python - <<'EOF'
import inspect
try:
from nemo_relay.observability import AtofConfig, AtofFileSinkConfig, AtofStreamSinkConfig
print(inspect.signature(AtofConfig))
print(inspect.signature(AtofFileSinkConfig))
print(inspect.signature(AtofStreamSinkConfig))
except ImportError as e:
print("nemo_relay not installed in this sandbox:", e)
EOFRepository: NVIDIA/NeMo-Fabric
Length of output: 227
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant converter and nearby validation helpers.
FILE='adapters/common/src/nemo_fabric_adapters/common/utils.py'
grep -n -A80 -B40 'def .*relay_api.*atof\|def relay_api_plugin_config\|ConfigPolicy\.unsupported_value\|sinks' "$FILE" | sed -n '1,240p'
printf '\n--- tests mentioning Atof or sink kinds ---\n'
rg -n "Atof|unsupported_value|sink.*type|output_directory|filename|mode|endpoints" adapters/common -g '*test*' -g '*py'
printf '\n--- definitions of ConfigPolicy ---\n'
rg -n "class ConfigPolicy|unsupported_value" adapters/common -g '*.py'Repository: NVIDIA/NeMo-Fabric
Length of output: 18155
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Narrow read of the converter to understand control flow and error handling.
FILE='adapters/common/src/nemo_fabric_adapters/common/utils.py'
python3 - <<'PY'
from pathlib import Path
path = Path('adapters/common/src/nemo_fabric_adapters/common/utils.py')
lines = path.read_text().splitlines()
for start in (300, 330, 360):
end = min(len(lines), start + 80)
print(f"\n### lines {start}-{end}")
for i in range(start, end):
print(f"{i+1:4d}: {lines[i]}")
PYRepository: NVIDIA/NeMo-Fabric
Length of output: 10461
Handle unknown ATOF sink types
Unrecognized or malformed sinks[] entries are skipped here, so a typo or future sink kind disappears without any diagnostic. Route them through the same unsupported-value policy or raise an error instead of dropping them.
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 359-359: Use list.extend to create a transformed list
(PERF401)
🤖 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 `@adapters/common/src/nemo_fabric_adapters/common/utils.py` around lines 338 -
364, Update the sink-processing loop in the ATOF configuration parser to handle
every unrecognized or malformed entry in value["sinks"] through the existing
unsupported-value policy, or raise a validation error when no such policy
applies. Do not silently continue for non-dict entries or unknown sink types;
preserve the existing handling for valid "file" and "stream" sinks.
There was a problem hiding this comment.
only relevant with the compatibility parser. See if we need this after getting peer feedback
There was a problem hiding this comment.
@bbednarski9, agreed. This is only relevant if the flat/v1 compatibility parser remains. I’ll leave the finding open pending the configuration decision; if the parser is removed in favor of typed v2 sinks, it no longer applies.
There was a problem hiding this comment.
Follow-up opened in #114. Since NeMo Fabric has not been released, I removed the compatibility parser instead of adding more handling around it. Relay 0.6 v2 file and stream sinks are now the canonical Rust/Python config, and the adapters consume that sink shape directly. This finding therefore no longer applies.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
|
Reviewer input requested on the ATOF configuration boundary: Relay 0.6 now models ATOF destinations as typed file and stream sinks. This PR currently keeps the older flat Fabric fields and translates them internally so the dependency upgrade remains backward compatible. Because Fabric is still pre-first-release, should we instead make the Relay 0.6 v2 sink structure the canonical typed Fabric configuration now and remove the flat representation before it becomes a public compatibility contract? My current lean is to migrate now unless we already have external configurations that we explicitly want to preserve. I can either:
Please weigh in on which contract you want Fabric to establish. |
Signed-off-by: Bryan Bednarski <bbednarski@nvidia.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)
tests/adapters/test_adapaters_common_utils.py (1)
380-433: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the remaining promised translation paths.
This test verifies only legacy flat ATOF input. Add, or point to existing tests for, structured v2 sink preservation and invalid configuration/error handling so the migration contract is not protected by a single happy-path case.
🤖 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 `@tests/adapters/test_adapaters_common_utils.py` around lines 380 - 433, Extend coverage around test_relay_api_plugin_config_translates_flat_atof_to_relay_v06_sinks by adding tests for preserving structured v2 ATOF sinks and rejecting invalid configurations with the expected error. Reuse existing fixtures or helpers where available, and keep the current legacy flat-input assertions 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.
Outside diff comments:
In `@tests/adapters/test_adapaters_common_utils.py`:
- Around line 380-433: Extend coverage around
test_relay_api_plugin_config_translates_flat_atof_to_relay_v06_sinks by adding
tests for preserving structured v2 ATOF sinks and rejecting invalid
configurations with the expected error. Reuse existing fixtures or helpers where
available, and keep the current legacy flat-input assertions unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 3f22f8b2-6f94-4ed2-b1f3-dfbf4622a3c6
📒 Files selected for processing (1)
tests/adapters/test_adapaters_common_utils.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (12)
**/*.{rs,py,pyi,json,yaml,yml}
📄 CodeRabbit inference engine (.agents/skills/contribute-api/SKILL.md)
Determine and update every affected public surface, including the CLI, PyO3 bindings, Python SDK, type stubs, schemas, and adapter contract, so they remain in parity.
Files:
tests/adapters/test_adapaters_common_utils.py
**/*
📄 CodeRabbit inference engine (.agents/skills/karpathy-guidelines/SKILL.md)
**/*: Before implementing, explicitly state assumptions, surface ambiguity and tradeoffs, present multiple interpretations when relevant, and ask for clarification rather than silently deciding or proceeding when requirements are unclear.
Prefer the minimum code needed to solve the requested problem: avoid speculative features, unnecessary abstractions, unrequested flexibility, and handling of impossible scenarios; simplify overcomplicated solutions.
When editing existing code, make surgical changes only: do not modify unrelated code, comments, formatting, or pre-existing dead code; match the existing style, and remove only unused imports, variables, or functions introduced by your changes.
Define verifiable success criteria for each task, such as writing regression tests for bugs and invalid-input tests for validation, then verify the implementation against those criteria. For multi-step work, state a brief plan with a verification check for each step.
**/*: Always spellNVIDIAin all caps; do not useNvidia,nvidia,nVidia,nVIDIA, orNV.
Usean NVIDIAbefore a noun, because the name begins with an “en” sound.
Do not add a registered trademark symbol afterNVIDIAwhen referring to the company; use trademark symbols with product names only when required by the document type or legal guidance.
Verify official capitalization, spacing, hyphenation, and spelling for NVIDIA and third-party product names; do not rewrite official product names for grammar or title-case rules.
Precede NVIDIA product names withNVIDIAon first mention when natural and accurate, and link the first mention when the destination helps the reader.
On first use, include the company name and full model qualifier when it helps identify the model; preserve official capitalization and punctuation, and use shorter family names only after establishing the full name.
For learning-oriented and developer content, do not force trademark symbols unless explicitly required; for press, ...
Files:
tests/adapters/test_adapaters_common_utils.py
**/*.{rs,py}
📄 CodeRabbit inference engine (.agents/skills/prepare-pr/SKILL.md)
For native binding changes, run
cargo check -p fabric-python --locked.
Files:
tests/adapters/test_adapaters_common_utils.py
**/*.{py,pyi}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
If Python code or a Python-facing adapter changes, run
just test-python.Use type annotations for public Python APIs and keep native Python binding declarations synchronized with their Rust implementations.
Files:
tests/adapters/test_adapaters_common_utils.py
**/*.{rs,py,pyi}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
**/*.{rs,py,pyi}: If public configuration types change, confirm schema snapshot tests injust test-rustpass and review generated schema diffs.
For schema or public contract changes, run both language suites and review changes underschemas/and generated API references.
**/*.{rs,py,pyi}: Usesnake_casefor Rust and Python functions and variables; usePascalCasefor Rust types and Python classes.
Keep native Python binding declarations synchronized with their Rust implementations when public contracts change.
Files:
tests/adapters/test_adapaters_common_utils.py
tests/adapters/**/*.py
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
tests/adapters/**/*.py: If an adapter or integration changes, run its focused tests.
For adapter behavior changes, run focused adapter tests undertests/adapters, then runjust test-python.
Files:
tests/adapters/test_adapaters_common_utils.py
**/*.{py,pyi,rs}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
For Python SDK or PyO3 binding changes, use
python-tests, run focused pytest tests first, thenjust test-python; rebuild withjust build-pythonwhen native code or packaging changes.
Files:
tests/adapters/test_adapaters_common_utils.py
tests/**/*.{rs,py}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
When adding functionality, include tests in the corresponding Rust crate or the relevant area under
tests/.
Files:
tests/adapters/test_adapaters_common_utils.py
tests/**/*.py
📄 CodeRabbit inference engine (.agents/skills/python-tests/SKILL.md)
tests/**/*.py: Use Pytest to run Python tests.
Do not add@pytest.mark.asyncioto tests; async tests are automatically detected and run by the async runner.
Do not add-> Nonereturn type annotations to test functions.
When mocking a class, useunittest.mock.MagicMockorunittest.mock.AsyncMock, using thespecargument when necessary, rather than defining a new class.
Prefix mocked class names withmock, notfake.
Prefer pytest fixtures over helper methods.
Do not repeat fixtures; place fixtures needed by multiple test files inconftest.py.
Define fixtures using@pytest.fixture(name="<fixture_name>"[, scope="<scope>"])and a function named<fixture_name>_fixture; specifyscopeonly when it is notfunction.
Preferpytest.mark.parametrizeover separate tests for different input types.
Use@pytest.mark.usefixtureswhen a fixture is needed but its return value is unused or it does not return a value.
Use the autouserestore_environ_fixturefromtests/conftest.pyto restore environment variables; modify variables withos.environand do not usemonkeypatch.setenv.
Avoid defensive programming in tests; access expected data directly so missing data raises a clear failure, such as usingresults["data"]instead ofresults.get("data").
Files:
tests/adapters/test_adapaters_common_utils.py
**/*.{rs,py,toml}
📄 CodeRabbit inference engine (.agents/skills/update-project-version/SKILL.md)
When editing version helpers, verify every
nemo-fabric-*workspace package through Cargo metadata and reject a static version inpython/pyproject.toml.
Files:
tests/adapters/test_adapaters_common_utils.py
**/*.{toml,rs,py}
📄 CodeRabbit inference engine (.agents/skills/update-project-version/SKILL.md)
Avoid blind repository-wide replacement of version-like strings; distinguish package-version references from examples and unrelated dependency versions.
Files:
tests/adapters/test_adapaters_common_utils.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/adapters/test_adapaters_common_utils.py
🪛 Ruff (0.15.21)
tests/adapters/test_adapaters_common_utils.py
[error] 381-381: Possible hardcoded password assigned to: "TOKEN"
(S105)
🔇 Additional comments (1)
tests/adapters/test_adapaters_common_utils.py (1)
5-5: LGTM!
|
/merge |
#### Overview Make the Relay 0.6 observability v2 sink model canonical in NeMo Fabric: - replace flat ATOF file fields and endpoint models with typed `file` and `stream` sinks in Rust and Python - pass v2 sink configuration directly to the Relay API and CLI paths - normalize and collect artifacts only for local file sinks, leaving stream sinks untouched - update schemas, generated API references, examples, integration guidance, and tests Validation: - `cargo test --workspace --locked` - focused Python SDK, adapter, schema-alignment, and API-reference tests - Ruff and `cargo fmt` checks - Fern config and strict broken-link checks #### Where should the reviewer start? Start with `python/src/nemo_fabric/models.py` and `crates/fabric-core/src/config.rs` for the public sink contract, then `adapters/common/src/nemo_fabric_adapters/common/utils.py` for the direct Relay 0.6 adapter path. #### Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to) - Relates to #107 - [x] I confirm this contribution is my own work, or I have the right to submit it under this project's license. - [x] I searched existing issues and open pull requests, and this does not duplicate existing work. ## Summary by CodeRabbit * **New Features** * Relay observability now defaults to version 2. * ATOF output configuration is now sink-based via `sinks`, supporting multiple **file** and **stream** sinks (HTTP POST, WebSocket, NDJSON). * **Documentation** * Updated SDK guidance, API references, examples, and notebooks to use the new sink-based Relay ATOF models. * **Bug Fixes** * Artifact collection is tightened to pull ATOF/ATIF artifacts only from enabled, correctly configured sink output directories (and skips when outputs are missing). * **Tests** * Expanded/updated coverage for sink-based v2 behavior and artifact collection rules. Authors: - Anuradha Karuppiah (https://github.com/AnuradhaKaruppiah) Approvers: - Ajay Thorve (https://github.com/AjayThorve) URL: #114
2 participants
Summary
deepagents0.5.x to the 0.6.x lineWhy
The previous constraints could not resolve Relay 0.6 together with Deep Agents 0.6. Once those constraints were corrected, CI exposed a second incompatibility at runtime: Relay 0.6 removed the flat
AtofConfig(output_directory=..., filename=..., mode=..., endpoints=...)constructor in favor of an observability v2 model containing typed sinks.Without adapting that boundary, every Deep Agents invocation with Relay enabled failed before the agent ran:
ATOF compatibility behavior
This PR currently preserves the existing Fabric configuration shape and translates it into Relay 0.6 objects internally:
output_directory,filename, andmodebecome anAtofFileSinkConfigendpointsbecomes anAtofStreamSinkConfigheaders,header_env,timeout_millis,field_name_policy, andnameare preservedsinksentries are also acceptedThis keeps the dependency upgrade independently usable without forcing a coordinated configuration migration.
However, Fabric has not had its first release yet. This may be the least expensive point to make the Relay 0.6 v2 sink model the canonical typed Fabric configuration and remove the legacy flat representation instead of establishing a compatibility contract that must later be deprecated. Reviewer guidance on that choice is requested below.
Impact
nemo-relay: 0.5.0 -> 0.6.0deepagents: 0.5.9 -> 0.6.12This PR does not change Hermes behavior, add a Relay CLI/gateway process, or install the separately distributed
nemo-relayCLI executable.Validation
tests/adapters/test_adapaters_common_utils.pyandtests/adapters/test_deepagents.py: 79 passedtests/e2e/test_deepagents.py::test_deepagents_persistent_host_with_relay_and_mock_model: passedUV_CACHE_DIR=/private/tmp/nemo-fabric-pr0-uv-cache uv lock --checkgit diff --checkSummary by CodeRabbit
New Features
Bug Fixes
Tests