fix: discover installed adapter descriptors - #110
Conversation
Signed-off-by: Zhongxuan Wang <daniewang@nvidia.com>
WalkthroughThe change adds caller-supplied adapter descriptor paths to core plan resolution, exposes them through Python bindings, discovers descriptors from installed distributions, and validates installed adapter discovery in tests and the wheel-building workflow. ChangesInstalled adapter descriptor resolution
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant FabricPlan as Fabric.plan()
participant Metadata as Installed distributions
participant Native as Native bindings
participant Core as Core adapter registry
FabricPlan->>Metadata: Discover fabric-adapter.json paths
Metadata-->>FabricPlan: Return resolved descriptor paths
FabricPlan->>Native: Pass adapter_descriptors
Native->>Core: Resolve run plan with descriptor paths
Core-->>Native: Resolve configured adapter
Native-->>FabricPlan: Return plan
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
Fern docs preview: https://nvidia-preview-pull-request-110.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: 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 @.github/workflows/ci_python.yml:
- Around line 156-187: Set shell: bash on the “Test installed adapter discovery”
workflow step so its Bash-specific commands run correctly on the windows-2022
matrix job, while preserving the existing smoke test commands.
In `@tests/python/test_runtime.py`:
- Around line 98-100: Update the lambda assigned to
mock_native.plan_config.side_effect so all three callback parameters use
underscore-prefixed names to satisfy Ruff ARG005, while retaining the required
three-argument signature and existing return behavior.
In `@tests/python/test_sdk_contract.py`:
- Around line 1149-1180: Add analogous coverage to
test_plan_registers_installed_adapter_descriptors for Fabric.doctor(): reuse the
installed descriptor setup and NativeRecorder, invoke client.doctor(), and
assert native.adapter_descriptor_calls contains the resolved descriptor path,
confirming doctor forwards the same descriptor list as plan.
🪄 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: 078ace12-08f1-48ca-9559-4bfd6f6db9e0
📒 Files selected for processing (7)
.github/workflows/ci_python.ymlcrates/fabric-core/src/config.rscrates/fabric-python/src/lib.rspython/src/nemo_fabric/_native.pyipython/src/nemo_fabric/client.pytests/python/test_runtime.pytests/python/test_sdk_contract.py
📜 Review details
⏰ Context from checks skipped due to timeout. (13)
- GitHub Check: Preview docs
- GitHub Check: Test (Python 3.14, linux-amd64)
- GitHub Check: Test (Python 3.13, linux-arm64)
- GitHub Check: Test (Python 3.13, linux-amd64)
- GitHub Check: Test (Python 3.14, windows-amd64)
- GitHub Check: Test (Python 3.12, windows-amd64)
- GitHub Check: Test (Python 3.12, macos-arm64)
- GitHub Check: Test (Python 3.12, linux-arm64)
- GitHub Check: Test (Python 3.13, macos-arm64)
- GitHub Check: Test (Python 3.13, windows-amd64)
- GitHub Check: Test (Python 3.11, windows-amd64)
- GitHub Check: Test (Python 3.14, macos-arm64)
- GitHub Check: Pre-commit
🧰 Additional context used
📓 Path-based instructions (25)
**/.github/workflows/*.{yml,yaml}
📄 CodeRabbit inference engine (.agents/skills/maintain-ci/SKILL.md)
**/.github/workflows/*.{yml,yaml}: Definepermissions:per job that needs token access, using the minimum required scopes; avoid workflow-level permissions unless centralized inheritance is intentionally documented.
Pin every third-party GitHub Action to a full commit SHA and retain a readable version comment after the SHA.
Use action-native or ecosystem-native caching instead of genericactions/cachewhen available.
Tie cache invalidation to lockfiles, dependency manifests, or explicit tool versions; avoid caching generated outputs unless deliberate repository behavior requires it.
Useastral-sh/setup-uvcache support withcache-dependency-globanchored touv.lock.
UseSwatinem/rust-cachewith explicitshared-keyandworkspacesinstead of ad hoc target-directory caching.
Keep deploy and publish permissions isolated to only the jobs that require them; restrictpages: writeandid-token: writeto Pages deployment jobs and their reusable-workflow callers.
Usecontents: readas the minimum permission for checkout-based build, test, documentation, and packaging jobs.
Grantpull-requests: readonly to jobs that perform pull-request metadata lookups.
When usingworkflow_call, inspect both caller and callee, and ensure the caller grants every permission required by the called jobs because the callee cannot elevate permissions.
Keep local workflow commands aligned with equivalentjustfilerecipes, and keep Python, Rust, and documentation jobs aligned with their lockfiles.
Preserve intended concurrency settings, branch filters, and documentation publish guards when modifying workflows.
Before editing workflows, inspect workflow relationships and settings with searches coveringuses:,permissions:,secrets:,concurrency:,cache, andjust.
Files:
.github/workflows/ci_python.yml
{docs/package.json,docs/package-lock.json,.github/workflows/*.yml,justfile}
📄 CodeRabbit inference engine (.agents/skills/maintain-packaging/SKILL.md)
Keep documentation tooling, CI workflows, install commands, build/test/clean recipes, and examples consistent with current package names and commands.
Files:
.github/workflows/ci_python.yml
{pyproject.toml,python/pyproject.toml,Cargo.toml,docs/package.json,.github/workflows/*.yml,justfile}
📄 CodeRabbit inference engine (.agents/skills/maintain-packaging/SKILL.md)
Reflect public packaging changes in release-facing documentation and ensure CI references the same package names and commands used by local workflows.
Files:
.github/workflows/ci_python.yml
**/*.{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/python/test_runtime.pypython/src/nemo_fabric/client.pypython/src/nemo_fabric/_native.pyitests/python/test_sdk_contract.pycrates/fabric-python/src/lib.rscrates/fabric-core/src/config.rs
**/*
📄 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/python/test_runtime.pypython/src/nemo_fabric/client.pypython/src/nemo_fabric/_native.pyitests/python/test_sdk_contract.pycrates/fabric-python/src/lib.rscrates/fabric-core/src/config.rs
**/*.{rs,py}
📄 CodeRabbit inference engine (.agents/skills/prepare-pr/SKILL.md)
For native binding changes, run
cargo check -p fabric-python --locked.
Files:
tests/python/test_runtime.pypython/src/nemo_fabric/client.pytests/python/test_sdk_contract.pycrates/fabric-python/src/lib.rscrates/fabric-core/src/config.rs
**/*.{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/python/test_runtime.pypython/src/nemo_fabric/client.pypython/src/nemo_fabric/_native.pyitests/python/test_sdk_contract.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/python/test_runtime.pypython/src/nemo_fabric/client.pypython/src/nemo_fabric/_native.pyitests/python/test_sdk_contract.pycrates/fabric-python/src/lib.rscrates/fabric-core/src/config.rs
**/*.{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/python/test_runtime.pypython/src/nemo_fabric/client.pypython/src/nemo_fabric/_native.pyitests/python/test_sdk_contract.pycrates/fabric-python/src/lib.rscrates/fabric-core/src/config.rs
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/python/test_runtime.pytests/python/test_sdk_contract.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/python/test_runtime.pytests/python/test_sdk_contract.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/python/test_runtime.pypython/src/nemo_fabric/client.pytests/python/test_sdk_contract.pycrates/fabric-python/src/lib.rscrates/fabric-core/src/config.rs
**/*.{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/python/test_runtime.pypython/src/nemo_fabric/client.pytests/python/test_sdk_contract.pycrates/fabric-python/src/lib.rscrates/fabric-core/src/config.rs
{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_runtime.pytests/python/test_sdk_contract.py
python/src/nemo_fabric/**
📄 CodeRabbit inference engine (.agents/skills/maintain-packaging/SKILL.md)
Ensure the editable maturin build continues to produce the native extension at
nemo_fabric._native, with generated artifacts placed where downstream consumers expect.
Files:
python/src/nemo_fabric/client.pypython/src/nemo_fabric/_native.pyi
python/src/nemo_fabric/**/*.py
📄 CodeRabbit inference engine (.agents/skills/review-doc-style/SKILL.md)
For Python API reference changes, update source docstrings under
python/src/nemo_fabric/instead of editing generated reference output.
Files:
python/src/nemo_fabric/client.py
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/client.pypython/src/nemo_fabric/_native.pyi
**/*.rs
📄 CodeRabbit inference engine (.agents/skills/contribute-api/SKILL.md)
Implement new runtime or binding behavior in the shared Rust core first.
Files:
crates/fabric-python/src/lib.rscrates/fabric-core/src/config.rs
**/*.{rs,toml}
📄 CodeRabbit inference engine (.agents/skills/prepare-pr/SKILL.md)
For any Rust change, run
just test-rustandcargo fmt --all -- --check.For Rust core, CLI, or shared runtime semantic changes, run Rust formatting and tests, and add Python tests when behavior is exposed through the SDK.
Use Rust stable tooling; format Rust code with
cargo fmt --all, check formatting withcargo fmt --all -- --check, and compile withcargo check --workspace --locked.
Files:
crates/fabric-python/src/lib.rscrates/fabric-core/src/config.rs
**/*.{rs,rmeta}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
If Rust code changes, run
cargo fmt --all -- --checkandjust test-rust.
Files:
crates/fabric-python/src/lib.rscrates/fabric-core/src/config.rs
crates/fabric-python/**/*.{rs,toml}
📄 CodeRabbit inference engine (.agents/skills/validate-change/SKILL.md)
If the PyO3 bridge or package metadata changes, run
just build-pythonandcargo check -p fabric-python --locked.
Files:
crates/fabric-python/src/lib.rs
crates/fabric-python/**/*
⚙️ CodeRabbit configuration file
crates/fabric-python/**/*: Treat native binding changes as public API changes. Check JSON/type conversion, error propagation, GIL/thread behavior, and parity with the Python SDK.
Files:
crates/fabric-python/src/lib.rs
crates/fabric-core/**/*.{rs,py}
📄 CodeRabbit inference engine (.agents/skills/prepare-pr/SKILL.md)
Changes under
crates/fabric-coremust run both the Rust and Python test suites.
Files:
crates/fabric-core/src/config.rs
crates/fabric-core/**/*.rs
📄 CodeRabbit inference engine (.agents/skills/review-doc-style/SKILL.md)
For Rust API reference changes, update Rust documentation comments under
crates/fabric-core/instead of editing generated reference output.If
crates/fabric-corechanges in a way exposed through Python, run both the Rust and Python suites.
Files:
crates/fabric-core/src/config.rs
crates/fabric-core/src/**/*.rs
⚙️ CodeRabbit configuration file
crates/fabric-core/src/**/*.rs: Review the Rust core for runtime lifecycle correctness, handle validation, capability routing accuracy, schema stability, and error semantics.
Public API changes should match committed schemas, tests, and documentation.
Files:
crates/fabric-core/src/config.rs
🧠 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/client.py
🪛 ast-grep (0.44.1)
tests/python/test_runtime.py
[info] 98-98: use jsonify instead of json.dumps for JSON output
Context: json.dumps(_plan())
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 100-100: use jsonify instead of json.dumps for JSON output
Context: json.dumps(_runtime())
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
tests/python/test_sdk_contract.py
[info] 644-644: use jsonify instead of json.dumps for JSON output
Context: json.dumps(_plan())
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🪛 Ruff (0.15.21)
tests/python/test_runtime.py
[warning] 99-99: Unused lambda argument: config_json
(ARG005)
[warning] 99-99: Unused lambda argument: base_dir
(ARG005)
[warning] 99-99: Unused lambda argument: adapter_descriptors
(ARG005)
🔇 Additional comments (8)
crates/fabric-core/src/config.rs (3)
167-179: LGTM!Also applies to: 214-226
1004-1026: LGTM!Also applies to: 1043-1048, 1071-1077
1746-1764: LGTM!crates/fabric-python/src/lib.rs (2)
8-10: LGTM!Also applies to: 23-69
160-175: LGTM!python/src/nemo_fabric/_native.pyi (1)
5-23: LGTM!python/src/nemo_fabric/client.py (1)
10-14: LGTM!Also applies to: 88-92, 127-131, 280-289
tests/python/test_sdk_contract.py (1)
13-16: LGTM!Also applies to: 632-645
| - name: Test installed adapter discovery | ||
| run: | | ||
| set -euo pipefail | ||
| uv venv .wheel-test-venv | ||
| if [[ -x .wheel-test-venv/bin/python ]]; then | ||
| test_python="$PWD/.wheel-test-venv/bin/python" | ||
| else | ||
| test_python="$PWD/.wheel-test-venv/Scripts/python.exe" | ||
| fi | ||
| uv pip install \ | ||
| --python "$test_python" \ | ||
| --find-links dist \ | ||
| "nemo-fabric[hermes]" | ||
|
|
||
| source_adapters="$GITHUB_WORKSPACE/adapters" | ||
| staged_source_adapters="$RUNNER_TEMP/nemo-fabric-source-adapters" | ||
| mv "$source_adapters" "$staged_source_adapters" | ||
| trap 'mv "$staged_source_adapters" "$source_adapters"' EXIT | ||
|
|
||
| smoke_dir="$(mktemp -d)" | ||
| cd "$smoke_dir" | ||
| "$test_python" - <<'PY' | ||
| from nemo_fabric import Fabric, FabricConfig, HarnessConfig, MetadataConfig | ||
|
|
||
| config = FabricConfig( | ||
| metadata=MetadataConfig(name="installed-adapter-smoke"), | ||
| harness=HarnessConfig(adapter_id="nvidia.fabric.hermes"), | ||
| ) | ||
| plan = Fabric().plan(config) | ||
| assert plan.adapter.adapter_id == "nvidia.fabric.hermes" | ||
| PY | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Run the smoke script with Bash.
The matrix includes windows-2022, where run defaults to PowerShell. Lines 158-186 use Bash syntax, so that wheel job fails before testing. Set shell: bash on this step.
Proposed fix
- name: Test installed adapter discovery
+ shell: bash
run: |📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Test installed adapter discovery | |
| run: | | |
| set -euo pipefail | |
| uv venv .wheel-test-venv | |
| if [[ -x .wheel-test-venv/bin/python ]]; then | |
| test_python="$PWD/.wheel-test-venv/bin/python" | |
| else | |
| test_python="$PWD/.wheel-test-venv/Scripts/python.exe" | |
| fi | |
| uv pip install \ | |
| --python "$test_python" \ | |
| --find-links dist \ | |
| "nemo-fabric[hermes]" | |
| source_adapters="$GITHUB_WORKSPACE/adapters" | |
| staged_source_adapters="$RUNNER_TEMP/nemo-fabric-source-adapters" | |
| mv "$source_adapters" "$staged_source_adapters" | |
| trap 'mv "$staged_source_adapters" "$source_adapters"' EXIT | |
| smoke_dir="$(mktemp -d)" | |
| cd "$smoke_dir" | |
| "$test_python" - <<'PY' | |
| from nemo_fabric import Fabric, FabricConfig, HarnessConfig, MetadataConfig | |
| config = FabricConfig( | |
| metadata=MetadataConfig(name="installed-adapter-smoke"), | |
| harness=HarnessConfig(adapter_id="nvidia.fabric.hermes"), | |
| ) | |
| plan = Fabric().plan(config) | |
| assert plan.adapter.adapter_id == "nvidia.fabric.hermes" | |
| PY | |
| - name: Test installed adapter discovery | |
| shell: bash | |
| run: | | |
| set -euo pipefail | |
| uv venv .wheel-test-venv | |
| if [[ -x .wheel-test-venv/bin/python ]]; then | |
| test_python="$PWD/.wheel-test-venv/bin/python" | |
| else | |
| test_python="$PWD/.wheel-test-venv/Scripts/python.exe" | |
| fi | |
| uv pip install \ | |
| --python "$test_python" \ | |
| --find-links dist \ | |
| "nemo-fabric[hermes]" | |
| source_adapters="$GITHUB_WORKSPACE/adapters" | |
| staged_source_adapters="$RUNNER_TEMP/nemo-fabric-source-adapters" | |
| mv "$source_adapters" "$staged_source_adapters" | |
| trap 'mv "$staged_source_adapters" "$source_adapters"' EXIT | |
| smoke_dir="$(mktemp -d)" | |
| cd "$smoke_dir" | |
| "$test_python" - <<'PY' | |
| from nemo_fabric import Fabric, FabricConfig, HarnessConfig, MetadataConfig | |
| config = FabricConfig( | |
| metadata=MetadataConfig(name="installed-adapter-smoke"), | |
| harness=HarnessConfig(adapter_id="nvidia.fabric.hermes"), | |
| ) | |
| plan = Fabric().plan(config) | |
| assert plan.adapter.adapter_id == "nvidia.fabric.hermes" | |
| PY |
🤖 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 @.github/workflows/ci_python.yml around lines 156 - 187, Set shell: bash on
the “Test installed adapter discovery” workflow step so its Bash-specific
commands run correctly on the windows-2022 matrix job, while preserving the
existing smoke test commands.
| mock_native.plan_config.side_effect = ( | ||
| lambda config_json, base_dir, adapter_descriptors: json.dumps(_plan()) | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Avoid the new Ruff violations.
Line 99 reports ARG005 for all callback parameters. Prefix them with _ while retaining the fixed three-argument mock signature.
Proposed fix
- lambda config_json, base_dir, adapter_descriptors: json.dumps(_plan())
+ lambda _config_json, _base_dir, _adapter_descriptors: json.dumps(_plan())📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| mock_native.plan_config.side_effect = ( | |
| lambda config_json, base_dir, adapter_descriptors: json.dumps(_plan()) | |
| ) | |
| mock_native.plan_config.side_effect = ( | |
| lambda _config_json, _base_dir, _adapter_descriptors: json.dumps(_plan()) | |
| ) |
🧰 Tools
🪛 ast-grep (0.44.1)
[info] 98-98: use jsonify instead of json.dumps for JSON output
Context: json.dumps(_plan())
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 100-100: use jsonify instead of json.dumps for JSON output
Context: json.dumps(_runtime())
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🪛 Ruff (0.15.21)
[warning] 99-99: Unused lambda argument: config_json
(ARG005)
[warning] 99-99: Unused lambda argument: base_dir
(ARG005)
[warning] 99-99: Unused lambda argument: adapter_descriptors
(ARG005)
🤖 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/python/test_runtime.py` around lines 98 - 100, Update the lambda
assigned to mock_native.plan_config.side_effect so all three callback parameters
use underscore-prefixed names to satisfy Ruff ARG005, while retaining the
required three-argument signature and existing return behavior.
Source: Linters/SAST tools
| def test_plan_registers_installed_adapter_descriptors( | ||
| monkeypatch: pytest.MonkeyPatch, | ||
| tmp_path: Path, | ||
| ): | ||
| descriptor = ( | ||
| tmp_path | ||
| / "share" | ||
| / "nemo-fabric" | ||
| / "adapters" | ||
| / "shim" | ||
| / "fabric-adapter.json" | ||
| ) | ||
| descriptor.parent.mkdir(parents=True) | ||
| descriptor.write_text("{}") | ||
|
|
||
| mock_distribution = MagicMock() | ||
| mock_distribution.files = [ | ||
| Path("share/nemo-fabric/adapters/shim/fabric-adapter.json") | ||
| ] | ||
| mock_distribution.locate_file.side_effect = lambda file: tmp_path / file | ||
|
|
||
| monkeypatch.setattr( | ||
| client_mod.importlib.metadata, | ||
| "distributions", | ||
| lambda: [mock_distribution], | ||
| ) | ||
| native = NativeRecorder() | ||
| client = NativeClient(native) | ||
|
|
||
| client.plan(_fabric_config()) | ||
|
|
||
| assert native.adapter_descriptor_calls == [[str(descriptor.resolve())]] |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add SDK doctor forwarding coverage.
This verifies Fabric.plan() only, although Fabric.doctor() now forwards the same descriptor list. Add an analogous doctor assertion using the recorder.
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.”
🤖 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/python/test_sdk_contract.py` around lines 1149 - 1180, Add analogous
coverage to test_plan_registers_installed_adapter_descriptors for
Fabric.doctor(): reuse the installed descriptor setup and NativeRecorder, invoke
client.doctor(), and assert native.adapter_descriptor_calls contains the
resolved descriptor path, confirming doctor forwards the same descriptor list as
plan.
Source: Path instructions
1 participant
Overview
Fix installed Python adapter packages not being visible to the native adapter registry. This is the cause of
unknown adapter 'nvidia.fabric.hermes'; available adapters: []when the SDK and Hermes adapter are installed from wheels outside a NeMo Fabric source checkout.Details
fabric-adapter.jsonfiles through Python distribution metadata and pass their resolved paths into the native planner and doctor.base_dir/adaptersdiscovery paths.adapters/directory, preventing the compile-time checkout fallback from masking packaging regressions.Scope
This fix discovers adapter distributions installed in the same Python environment as
nemo_fabric, which covers the published-wheel notebook failure. It intentionally does not inspect a separate interpreter selected throughADAPTER_PYTHON; cross-interpreter discovery remains outside this minimal fix.Validation
cargo fmt --all -- --checkjust --fmt --checkcargo check -p fabric-python --lockedjust build-pythonjust test-rust(39 passed)just test-python(401 passed, 44 skipped)just wheelsadapters/directory hidden; Hermes resolved successfully from installed distribution metadataWhere should the reviewer start?
Start with
_installed_adapter_descriptor_paths()inpython/src/nemo_fabric/client.py, then follow the optional descriptor list throughcrates/fabric-python/src/lib.rsintocrates/fabric-core/src/config.rs. The regression boundary is exercised byTest installed adapter discoveryin.github/workflows/ci_python.yml.Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Relates to docs: Reafactor the
README.md#103I 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
New Features
Bug Fixes
Tests