Each adapter is an independent Python package - #38
Conversation
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
…ic and unsupported by maturin Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
|
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:
WalkthroughThis PR updates packaging for the root, runtime, and adapter Python projects, replaces sys.path-based adapter/test bootstrapping with direct imports, and adds wheel-building support to CI and the task runner. ChangesPackaging and build system migration
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ 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 @.github/workflows/ci_python.yml:
- Around line 72-100: The wheel build job’s Checkout step is using a shallow
clone, which can break setuptools-scm version discovery in the Build wheels
flow. Update the checkout configuration in the build-wheels job to fetch full
git history/tags so setuptools-scm can resolve the correct package version
reliably. Use the existing Checkout action block in the workflow as the place to
apply the fix.
In `@pyproject.toml`:
- Around line 18-23: The base install is metadata-only because `packages` is
empty and `tool.setuptools_dynamic_dependencies.dependencies` has no runtime
requirement, so `pip install nemo-fabric` does not provide the expected runtime.
Update the packaging configuration in `pyproject.toml` so the core runtime is
included in the default install path (or, if the empty base install is
intentional, explicitly change the public contract/docs to say runtime is only
available via the `runtime` extra). Use the `tool.setuptools` and
`tool.setuptools_dynamic_dependencies` sections to make the intended behavior
unambiguous.
In `@python/pyproject.toml`:
- Around line 13-16: The runtime wheel version is currently sourced differently
from the root package, which can cause the nemo-fabric-runtime dependency pin in
the Python package to drift. Update the versioning in python/pyproject.toml so
it uses the same source as the root package version, keeping the maturin-built
runtime and the setuptools-scm-based package aligned; use the pyproject/maturin
configuration and the root package versioning setup as the key places to adjust.
In `@tests/adapters/test_adapaters_common_hermes.py`:
- Line 106: Remove the redundant `-> None` annotations from the affected test
functions so they match the project style and the updated patterns in
`test_adapaters_common_utils.py`. Update each listed test signature directly in
`test_validate_hermes_telemetry_provider_rejects_native`,
`test_build_hermes_config_maps_fabric_config_to_hermes_config`,
`test_hermes_config_variation_matrix_surfaces_supported_capabilities`,
`test_write_hermes_config_writes_file`,
`test_hermes_mcp_server_config_rejects_unsupported_mappings`,
`test_without_none`, `test_summarize_hermes_config`,
`test_configure_hermes_relay_sets_hermes_plugin_environment`, and
`test_configure_hermes_relay_returns_none_when_disabled`.
🪄 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: b42da070-b0e6-4d94-a639-cc2eb3bcbd5b
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
.github/workflows/ci_python.yml.gitignoreREADME.mdadapters/codex-cli/pyproject.tomladapters/codex-cli/src/nemo_fabric_adapters/codex_cli/adapter.pyadapters/common/pyproject.tomladapters/hermes-cli/pyproject.tomladapters/hermes-cli/src/nemo_fabric_adapters/hermes_cli/adapter.pyadapters/hermes-sdk/pyproject.tomladapters/hermes-sdk/src/nemo_fabric_adapters/hermes_sdk/adapter.pyjustfilepyproject.tomlpython/pyproject.tomltests/adapters/test_adapaters_common_hermes.pytests/adapters/test_adapaters_common_utils.pytests/adapters/test_codex_cli.pytests/conftest.pytests/e2e/test_hermes_config_mapping.pytests/python/test_consumer_neutral.py
💤 Files with no reviewable changes (2)
- .gitignore
- tests/conftest.py
📜 Review details
⚠️ CI failures not shown inline (11)
GitHub Actions: Fern Docs / 0_Build and publish docs.txt: Add jsut recipe for building wheels, and a CI stage
Conclusion: failure
##[group]Run bail() {
�[36;1mbail() {�[0m
�[36;1m printf '::error::install-action: %s\n' "$*"�[0m
GitHub Actions: Fern Docs / Build and publish docs: Add jsut recipe for building wheels, and a CI stage
Conclusion: failure
##[group]Run just docs
�[36;1mjust docs�[0m
shell: /usr/bin/bash -e {0}
env:
UV_PYTHON_INSTALL_DIR: /home/runner/work/_temp/uv-python-dir
UV_CACHE_DIR: /home/runner/work/_temp/setup-uv-cache
CARGO_INCREMENTAL: 0
CARGO_PROFILE_DEV_DEBUG: 0
CARGO_TERM_COLOR: always
RUST_BACKTRACE: short
RUSTFLAGS: -D warnings
CARGO_UNSTABLE_SPARSE_REGISTRY: true
CARGO_REGISTRIES_CRATES_IO_PROTOCOL: sparse
##[endgroup]
Using CPython 3.12.3 interpreter at: /usr/bin/python3
Creating virtual environment at: .venv
Resolved 144 packages in 5.04s
error: Extra `docs` is not defined in the project's `optional-dependencies` table
error: Recipe `docs` failed with exit code 2
##[error]Process completed with exit code 2.
GitHub Actions: Fern Docs / Build and publish docs: Add jsut recipe for building wheels, and a CI stage
Conclusion: failure
##[group]Run bail() {
�[36;1mbail() {�[0m
�[36;1m printf '::error::install-action: %s\n' "$*"�[0m
GitHub Actions: Rust / Test: Each adapter is an independent Python package
Conclusion: failure
##[group]Run just test-rust
�[36;1mjust test-rust�[0m
shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
env:
CARGO_INCREMENTAL: 0
CARGO_PROFILE_DEV_DEBUG: 0
CARGO_TERM_COLOR: always
RUST_BACKTRACE: short
RUSTFLAGS: -D warnings
CARGO_UNSTABLE_SPARSE_REGISTRY: true
CARGO_REGISTRIES_CRATES_IO_PROTOCOL: sparse
CACHE_ON_FAILURE: false
##[endgroup]
cargo test --workspace --locked
�[1m�[92m Compiling�[0m fabric-core v0.1.0 (/home/runner/work/NeMo-Fabric/NeMo-Fabric/crates/fabric-core)
�[1m�[92m Compiling�[0m fabric-cli v0.1.0 (/home/runner/work/NeMo-Fabric/NeMo-Fabric/crates/fabric-cli)
�[1m�[92m Finished�[0m `test` profile [unoptimized] target(s) in 5.92s
�[1m�[92m Running�[0m unittests src/main.rs (target/debug/deps/fabric-c3572a4d481471cf)
running 0 tests
test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s
�[1m�[92m Running�[0m unittests src/lib.rs (target/debug/deps/fabric_core-9b081566c1d472c1)
running 45 tests
test config::tests::errors_for_unknown_manifest_profile ... ok
GitHub Actions: Rust / Test: Each adapter is an independent Python package
Conclusion: failure
##[group]Run bail() {
�[36;1mbail() {�[0m
�[36;1m printf '::error::install-action: %s\n' "$*"�[0m
GitHub Actions: Rust / 0_Test.txt: Each adapter is an independent Python package
Conclusion: failure
##[group]Run bail() {
�[36;1mbail() {�[0m
�[36;1m printf '::error::install-action: %s\n' "$*"�[0m
GitHub Actions: Python / Build wheels: Each adapter is an independent Python package
Conclusion: failure
##[group]Run bail() {
�[36;1mbail() {�[0m
�[36;1m printf '::error::install-action: %s\n' "$*"�[0m
GitHub Actions: Python / Test: Each adapter is an independent Python package
Conclusion: failure
##[group]Run bail() {
�[36;1mbail() {�[0m
�[36;1m printf '::error::install-action: %s\n' "$*"�[0m
GitHub Actions: Python / Test: Each adapter is an independent Python package
Conclusion: failure
##[group]Run uv venv --python 3.12 .venv
�[36;1muv venv --python 3.12 .venv�[0m
�[36;1muv sync --group test --no-group dev --extra harbor --extra hermes --extra relay�[0m
shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
env:
CARGO_INCREMENTAL: 0
CARGO_PROFILE_DEV_DEBUG: 0
CARGO_TERM_COLOR: always
RUST_BACKTRACE: short
RUSTFLAGS: -D warnings
CARGO_UNSTABLE_SPARSE_REGISTRY: true
CARGO_REGISTRIES_CRATES_IO_PROTOCOL: sparse
CACHE_ON_FAILURE: false
UV_PYTHON_INSTALL_DIR: /home/runner/work/_temp/uv-python-dir
UV_CACHE_DIR: /home/runner/work/_temp/setup-uv-cache
##[endgroup]
Using CPython 3.12.3 interpreter at: /usr/bin/python3.12
Creating virtual environment at: .venv
Activate with: .venv/bin/activate
Resolved 144 packages in 4.97s
Building nemo-fabric @ file:///home/runner/work/NeMo-Fabric/NeMo-Fabric
Building nemo-fabric-adapters-hermes-cli @ file:///home/runner/work/NeMo-Fabric/NeMo-Fabric/adapters/hermes-cli
Building nemo-fabric-adapters-hermes-sdk @ file:///home/runner/work/NeMo-Fabric/NeMo-Fabric/adapters/hermes-sdk
Building nemo-fabric-adapters-common @ file:///home/runner/work/NeMo-Fabric/NeMo-Fabric/adapters/common
Downloading tiktoken (1.1MiB)
Downloading openai (1.1MiB)
Downloading pandas (10.4MiB)
Downloading numpy (15.9MiB)
Downloading pydantic-core (2.0MiB)
Downloading pygments (1.2MiB)
Downloading hf-xet (4.3MiB)
Downloading cryptography (4.5MiB)
Downloading aiohttp (1.7MiB)
Downloading tokenizers (3.2MiB)
Downloading uvloop (4.2MiB)
Downloading litellm (14.8MiB)
Downloading pyarrow (46.6MiB)
Downloading pillow (6.8MiB)
Downloading nemo-relay (6.0MiB)
Downloading harbor (1.5MiB)
Downloading hermes-agent (8.2MiB)
× Failed to build `nemo-fabric-adapters-hermes-cli @
│ file:///home/runner/work/NeMo-Fabric/NeMo-Fabric/adapters/hermes-cli`
╰─▶ Package metadata version `0.1.dev1+g7bdeb6361` does not match
`0.1.dev1+g7bdeb6361.d20260707` from the wheel filename
hi...
GitHub Actions: Python / 0_Build wheels.txt: Each adapter is an independent Python package
Conclusion: failure
##[group]Run bail() {
�[36;1mbail() {�[0m
�[36;1m printf '::error::install-action: %s\n' "$*"�[0m
GitHub Actions: Python / 1_Test.txt: Each adapter is an independent Python package
Conclusion: failure
##[group]Run bail() {
�[36;1mbail() {�[0m
�[36;1m printf '::error::install-action: %s\n' "$*"�[0m
🧰 Additional context used
📓 Path-based instructions (4)
{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:
adapters/common/pyproject.tomladapters/hermes-cli/pyproject.tomladapters/codex-cli/pyproject.tomladapters/hermes-sdk/pyproject.tomladapters/hermes-sdk/src/nemo_fabric_adapters/hermes_sdk/adapter.pyadapters/codex-cli/src/nemo_fabric_adapters/codex_cli/adapter.pyadapters/hermes-cli/src/nemo_fabric_adapters/hermes_cli/adapter.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:
README.md
tests/**/*.py
📄 CodeRabbit inference engine (.agents/skills/python-tests/SKILL.md)
tests/**/*.py: Use Pytest to run and write tests.
Do not add@pytest.mark.asyncioto test functions; async tests are detected automatically by the async runner.
Do not add a-> Nonereturn type annotation to test functions.
When mocking a class, do not define a new class; useunittest.mock.MagicMockorunittest.mock.AsyncMock, with thespecargument when necessary.
Name mocked classes with themockprefix, notfake.
Prefer pytest fixtures over helper methods.
Do not duplicate fixtures across test files; if a fixture is needed in multiple test files, place 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 specifyscopewhen it is notfunction.
Preferpytest.mark.parametrizeover creating individual 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.
If you need to modify 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_consumer_neutral.pytests/e2e/test_hermes_config_mapping.pytests/adapters/test_codex_cli.pytests/adapters/test_adapaters_common_utils.pytests/adapters/test_adapaters_common_hermes.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_consumer_neutral.pytests/e2e/test_hermes_config_mapping.pytests/adapters/test_codex_cli.pytests/adapters/test_adapaters_common_utils.pytests/adapters/test_adapaters_common_hermes.py
🔇 Additional comments (19)
README.md (1)
58-59: LGTM!justfile (2)
33-46: LGTM!Reinstalling both
nemo-fabricandnemo-fabric-runtimeon theuv syncpath correctly aligns with the runtime package's new maturin-based build.
80-96: LGTM!.github/workflows/ci_python.yml (2)
24-25: LGTM!
65-100: LGTM!Job-scoped permissions, toolchain setup, and artifact upload flow (with
if-no-files-found: error) look correct.pyproject.toml (1)
5-17: LGTM!Also applies to: 25-60, 66-97
python/pyproject.toml (1)
5-12: LGTM!Also applies to: 17-19
adapters/hermes-cli/pyproject.toml (1)
26-31: Same exact-pin concern ascodex-cli.Duplicate concern regarding the
nemo-fabric-adapters-common == {version}pin fragility across independently-built adapter packages; see comment onadapters/codex-cli/pyproject.toml.adapters/hermes-sdk/pyproject.toml (1)
26-30: Same exact-pin concern ascodex-cli.Duplicate concern regarding the
nemo-fabric-adapters-common == {version}pin fragility across independently-built adapter packages; see comment onadapters/codex-cli/pyproject.toml.adapters/common/pyproject.toml (1)
4-25: LGTM!adapters/codex-cli/pyproject.toml (2)
28-28: 🎯 Functional Correctness | 🏗️ Heavy liftExact-pin on
nemo-fabric-adapters-commonmay be brittle for local/dirty dev builds.
nemo-fabric-adapters-common == {version}pins the adapter to the exact setuptools-scm-derived version of the common package. In a monorepo, dirty working trees produce distinct local version segments (+dTIMESTAMP/.dNNN) for each package built independently; ifcodex-cliandcommonare built at slightly different times/commits during local dev or CI matrix builds, the pin can fail to resolve even though the source is otherwise compatible. Confirm this has been exercised in local editable-install and CI wheel-build workflows referenced elsewhere in this PR (justfilewheelstarget, CIbuild-wheelsjob).
26-30: 🎯 Functional Correctness
[tool.setuptools_dynamic_dependencies]is the correct table name
setuptools-dynamic-dependenciesexpects this section; thesetuptools_dynamic_requiresspelling is the documentation typo.> Likely an incorrect or invalid review comment.tests/e2e/test_hermes_config_mapping.py (1)
10-14: LGTM!tests/python/test_consumer_neutral.py (1)
30-32: LGTM!adapters/codex-cli/src/nemo_fabric_adapters/codex_cli/adapter.py (1)
16-25: LGTM!adapters/hermes-cli/src/nemo_fabric_adapters/hermes_cli/adapter.py (1)
16-20: LGTM!adapters/hermes-sdk/src/nemo_fabric_adapters/hermes_sdk/adapter.py (1)
23-24: LGTM!tests/adapters/test_adapaters_common_utils.py (1)
7-15: LGTM!Also applies to: 49-57, 60-63, 77-87, 112-119, 190-202, 232-249
tests/adapters/test_codex_cli.py (1)
8-8: LGTM!
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@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)
justfile (1)
96-112: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate project list between
lock-pythonandwheels.The
projectsarray is duplicated verbatim in bothlock-python(Lines 55-62) andwheels(Lines 101-108). If a new adapter package is added later, it's easy to update one target and forget the other, leaving lock/wheel builds silently out of sync.Consider extracting the shared list into a single recipe-level variable or a helper function that both recipes reference.
🤖 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 `@justfile` around lines 96 - 112, The Python project list is duplicated between the lock-python and wheels recipes, so update the justfile to define the project paths once and have both recipes reference that shared source. Use a single recipe-level variable or helper in justfile so lock-python and wheels stay in sync when projects are added or removed, and keep the existing behavior of iterating over the same set of project directories in both targets.
🤖 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 `@justfile`:
- Around line 96-112: The Python project list is duplicated between the
lock-python and wheels recipes, so update the justfile to define the project
paths once and have both recipes reference that shared source. Use a single
recipe-level variable or helper in justfile so lock-python and wheels stay in
sync when projects are added or removed, and keep the existing behavior of
iterating over the same set of project directories in both targets.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: a596ad5c-b198-4f87-aacf-8b22384413e5
⛔ Files ignored due to path filters (6)
adapters/codex-cli/uv.lockis excluded by!**/*.lockadapters/common/uv.lockis excluded by!**/*.lockadapters/hermes-cli/uv.lockis excluded by!**/*.lockadapters/hermes-sdk/uv.lockis excluded by!**/*.lockpython/uv.lockis excluded by!**/*.lockuv.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
adapters/codex-cli/pyproject.tomladapters/hermes-cli/pyproject.tomladapters/hermes-sdk/pyproject.tomljustfile
📜 Review details
🧰 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:
adapters/hermes-cli/pyproject.tomladapters/hermes-sdk/pyproject.tomladapters/codex-cli/pyproject.toml
🔇 Additional comments (8)
adapters/hermes-cli/pyproject.toml (2)
23-24: 🗄️ Data Integrity & Integration | ⚡ Quick winSame
data-filesartifact-handling concern asadapters/codex-cli/pyproject.toml.
fabric-adapter.jsoninstalled viadata-fileslands undersys.prefix, not inside the package — same portability/discoverability risk noted for the codex-cli adapter.Source: Path instructions
26-34: LGTM!adapters/hermes-sdk/pyproject.toml (2)
23-24: 🗄️ Data Integrity & Integration | ⚡ Quick winSame
data-filesartifact-handling concern as the other adapters.
fabric-adapter.jsoninstalled viadata-filesundersys.prefixrather than as package data.Source: Path instructions
26-33: LGTM!adapters/codex-cli/pyproject.toml (2)
26-33: LGTM! Usingsetuptools_dynamic_dependencieswith== {version}pin for the siblingnemo-fabric-adapters-commonpackage plus atool.uv.sourceslocal path override is the documented pattern for monorepo core/backend lockstep versioning.
23-24: 🗄️ Data Integrity & IntegrationConfirm the runtime lookup path for
fabric-adapter.json. Determine whether the adapter loader expects the manifest undershare/nemo-fabric/adapters/...or as a package resource before changing packaging.justfile (2)
35-46: LGTM!
51-66: LGTM!
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
|
📖 Fern docs preview: https://nvidia-preview-pull-request-38.docs.buildwithfern.com/nemo/fabric |
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)
justfile (1)
51-66: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated project list between
lock-pythonandwheels.Both recipes appear to iterate over the same fixed set of project directories (root,
python, adapter packages). Maintaining this list in two places risks drift when a new adapter package is added (e.g., forgetting to update one of the two lists).Consider extracting the shared project list into a single
justvariable or a small array sourced by both recipes:♻️ Suggested pattern to avoid duplication
+PYTHON_PROJECTS := ". python adapters/common adapters/codex-cli adapters/hermes-cli adapters/hermes-sdk" + lock-python: #!/usr/bin/env bash set -euo pipefail - for project in . python adapters/common adapters/codex-cli adapters/hermes-cli adapters/hermes-sdk; do + for project in {{ PYTHON_PROJECTS }}; do uv lock --project "$project" done wheels: #!/usr/bin/env bash set -euo pipefail - for project in . python adapters/common adapters/codex-cli adapters/hermes-cli adapters/hermes-sdk; do + for project in {{ PYTHON_PROJECTS }}; do ... done#!/bin/bash # Description: Confirm whether the project lists actually match between the two recipes. sed -n '51,112p' justfileAlso applies to: 96-112
🤖 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 `@justfile` around lines 51 - 66, The `lock-python` and `wheels` recipes are duplicating the same fixed Python project list, which can drift when packages change. Refactor the shared project directories into one reusable `just` variable or helper list, then have both recipes consume it instead of maintaining separate inline arrays; update the logic in `lock-python` and `wheels` so the project set is defined only once.
🤖 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 `@justfile`:
- Around line 51-66: The `lock-python` and `wheels` recipes are duplicating the
same fixed Python project list, which can drift when packages change. Refactor
the shared project directories into one reusable `just` variable or helper list,
then have both recipes consume it instead of maintaining separate inline arrays;
update the logic in `lock-python` and `wheels` so the project set is defined
only once.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: c0468137-408e-4fb2-a6ec-175d39f4eb72
📒 Files selected for processing (1)
justfile
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Test
- GitHub Check: Build and publish docs
⚠️ CI failures not shown inline (3)
GitHub Actions: Rust / Test: Each adapter is an independent Python package
Conclusion: failure
##[group]Run bail() {
�[36;1mbail() {�[0m
�[36;1m printf '::error::install-action: %s\n' "$*"�[0m
GitHub Actions: Rust / Test: Each adapter is an independent Python package
Conclusion: failure
##[group]Run just test-rust
�[36;1mjust test-rust�[0m
shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
env:
CARGO_INCREMENTAL: 0
CARGO_PROFILE_DEV_DEBUG: 0
CARGO_TERM_COLOR: always
RUST_BACKTRACE: short
RUSTFLAGS: -D warnings
CARGO_UNSTABLE_SPARSE_REGISTRY: true
CARGO_REGISTRIES_CRATES_IO_PROTOCOL: sparse
CACHE_ON_FAILURE: false
##[endgroup]
cargo test --workspace --locked
�[1m�[92m Compiling�[0m fabric-core v0.1.0 (/home/runner/work/NeMo-Fabric/NeMo-Fabric/crates/fabric-core)
�[1m�[92m Compiling�[0m fabric-cli v0.1.0 (/home/runner/work/NeMo-Fabric/NeMo-Fabric/crates/fabric-cli)
�[1m�[92m Finished�[0m `test` profile [unoptimized] target(s) in 6.28s
�[1m�[92m Running�[0m unittests src/main.rs (target/debug/deps/fabric-c3572a4d481471cf)
running 0 tests
test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s
�[1m�[92m Running�[0m unittests src/lib.rs (target/debug/deps/fabric_core-9b081566c1d472c1)
running 45 tests
test config::tests::errors_for_unknown_manifest_profile ... ok
GitHub Actions: Rust / 0_Test.txt: Each adapter is an independent Python package
Conclusion: failure
##[group]Run bail() {
�[36;1mbail() {�[0m
�[36;1m printf '::error::install-action: %s\n' "$*"�[0m
🔇 Additional comments (3)
justfile (3)
86-86: Added--extra runtimeto test sync — LGTM given visible context.Consistent with the runtime package now being separately installable; test env needs the extra to exercise runtime-dependent tests.
33-47: 🎯 Functional Correctness
build-pythonmatches the package split. Reinstalling bothnemo-fabricandnemo-fabric-runtimein the uv path is consistent with the root package acting as the meta-package.
69-79: 🎯 Functional Correctness
docsis already a dependency group.uv sync --group docsmatches the currentpyproject.toml; no change needed.> Likely an incorrect or invalid review comment.
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
Signed-off-by: David Gardner <dagardner@nvidia.com>
…n -m <module>' Signed-off-by: David Gardner <dagardner@nvidia.com>
2 participants
Overview
python -m <module>rather thanpython <script>, this has two benefits:python/pyproject.tomlis responsible for handling the Rust bindings and builds thenemo-fabric-runtimepackagepyproject.tomlbecomes thenemo-fabricmeta package.uv.lockfiles in gitsys.pathmanipulation for bootstrappingnemo_fabric_adapters.commonWhere should the reviewer start?
pyproject.tomlRelated Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Closes FABRIC-34
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
dist/*.whl.