chore: Ensure Rust and Python package versions are always in sync - #41
Conversation
… there was no equivelant in Cargo 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>
…abric/integrations/harbor/__init__.py 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>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📜 Recent review details⏰ Context from checks skipped due to timeout. (1)
WalkthroughThis PR migrates Rust and Python packages from dynamic setuptools-scm/Cargo-derived versioning to statically pinned versions (0.1.0), adds a justfile-based version synchronization workflow with skill documentation, and updates the runtime version lookup to use ChangesStatic Version Pinning
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Developer
participant Justfile
participant CargoToml
participant PyprojectFiles
Developer->>Justfile: just set-version version
Justfile->>Justfile: resolve version from arg or ref_name
Justfile->>CargoToml: update workspace and fabric-core version
Justfile->>PyprojectFiles: update version and internal pins
Justfile->>Justfile: run just lock-python
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
dagardner-nv
changed the title
|
📖 Fern docs preview: https://nvidia-preview-pull-request-41.docs.buildwithfern.com/nemo/fabric |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@justfile`:
- Around line 192-215: The version update logic in the justfile is too broad
because the regex in the pyproject.toml loop is not scoped to the [project]
table. Update the version replacement in the project_paths processing block so
it tracks the current TOML section, like set_cargo_workspace_version does for
Cargo.toml, and only rewrites the version field when inside [project]. Keep the
internal pin update via pin_pattern unchanged, but ensure the version
substitution is structurally constrained to the intended section rather than any
matching line in the file.
- Around line 25-63: The SemVer validation pattern is duplicated between
semver_to_pep440 and set_cargo_workspace_version, so update the Justfile to
define one shared regex/pattern source and reuse it in both helpers. Refactor
the validation logic so both semver_to_pep440 and set_cargo_workspace_version
reference the same accepted version format (release plus optional alpha/beta/rc
prerelease and optional build metadata) instead of keeping separate inline
copies. Keep the shared definition easy to locate and update so future format
changes only need one edit.
🪄 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: 8731b914-1ed8-4c1f-b672-4839f736ad0e
⛔ Files ignored due to path filters (5)
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!**/*.lockuv.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
.agents/skills/update-project-version/SKILL.mdadapters/codex-cli/pyproject.tomladapters/common/pyproject.tomladapters/hermes-cli/pyproject.tomladapters/hermes-sdk/pyproject.tomljustfilepyproject.tomlpython/src/nemo_fabric/integrations/harbor/__init__.py
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Test
🧰 Additional context used
📓 Path-based instructions (3)
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/integrations/harbor/__init__.py
adapters/**/pyproject.toml
📄 CodeRabbit inference engine (.agents/skills/update-project-version/SKILL.md)
Keep each adapter’s
nemo-fabric-adapters-common == <version>dependency pin aligned with the release version.
Files:
adapters/hermes-cli/pyproject.tomladapters/common/pyproject.tomladapters/codex-cli/pyproject.tomladapters/hermes-sdk/pyproject.toml
{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/common/pyproject.tomladapters/codex-cli/pyproject.tomladapters/hermes-sdk/pyproject.toml
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: NVIDIA/NeMo-Fabric
Timestamp: 2026-07-08T23:15:46.253Z
Learning: Use the same `X.Y.Z` version string everywhere for a normal release; for prerelease or build-metadata versions, use valid Cargo SemVer in `Cargo.toml` and the equivalent PEP 440 version in explicit Python metadata, ensuring the Rust-built and setuptools-built package versions are equivalent.
Learnt from: CR
Repo: NVIDIA/NeMo-Fabric
Timestamp: 2026-07-08T23:15:46.253Z
Learning: Keep changes scoped, surface assumptions, and define focused validation before editing; audit old-version references carefully and distinguish package-version surfaces from examples or unrelated dependency versions.
Learnt from: CR
Repo: NVIDIA/NeMo-Fabric
Timestamp: 2026-07-08T23:15:46.253Z
Learning: If editing the helper implementation, keep `set_project_version`, `set_cargo_workspace_version`, `set_python_project_versions`, and the `set-version` recipe aligned with the documented version-update contracts.
📚 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/integrations/harbor/__init__.py
🪛 LanguageTool
.agents/skills/update-project-version/SKILL.md
[uncategorized] ~40-~40: Do not mix variants of the same word (‘prerelease’ and ‘pre-release’) within a single text.
Context: ...e same X.Y.Z string everywhere. For a prerelease or build-metadata version, use valid Ca...
(EN_WORD_COHERENCY)
[uncategorized] ~51-~51: Do not mix variants of the same word (‘prerelease’ and ‘pre-release’) within a single text.
Context: ...cipe converts supported Cargo SemVer prereleases to PEP 440 and updates: - `Cargo.tom...
(EN_WORD_COHERENCY)
[style] ~86-~86: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...all Python package metadata resolves. - Run just test-python when the integration...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~89-~89: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ing validation of every Python wheel. - Run git diff --check. ## Avoid - Updati...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🔇 Additional comments (11)
python/src/nemo_fabric/integrations/harbor/__init__.py (2)
8-8: LGTM!
56-60: 🗄️ Data Integrity & IntegrationKeep the
Nonefallback or document the contract.python/pyproject.tomlalready declaresname = "nemo-fabric-runtime", so the metadata key is correct.FabricAgent.version()can now returnNonewhen metadata is missing; make sure callers handle that explicitly..agents/skills/update-project-version/SKILL.md (1)
1-110: LGTM!justfile (3)
25-63: 🎯 Functional CorrectnessVerify custom SemVer→PEP440 conversion matches Maturin's own conversion.
semver_to_pep440computes the PEP 440 string used to update thenemo-fabric-runtime == <version>pin inpyproject.toml, but the actualnemo-fabric-runtimewheel version is produced independently by Maturin fromCargo.toml's SemVer version. Maturin converts SemVer to PEP 440 itself. If Maturin's conversion diverges from this custom implementation for any supported prerelease or build-metadata format, the pin would again mismatch the built package — the exact class of bug this PR states it's fixing ("previously caused thenemo-fabric-runtimepackage to get a slightly different version").Recommend adding a validation step (e.g. in CI or
just wheels) that builds a prerelease/build-metadata version through both paths and diffs the resulting version strings, rather than relying on two independent implementations staying in sync by inspection.Also applies to: 196-236
10-13: LGTM!Also applies to: 238-243, 288-295
76-77: 🎯 Functional Correctness
set-versiondoes not receive a tag-derivedref_namehere. The fallback uses the local Just variableref_name := "", and this repository does not pass av-prefixed tag into it, so thev-prefix failure path is not reachable from this codebase.> Likely an incorrect or invalid review comment.pyproject.toml (1)
12-15: LGTM!Also applies to: 20-59, 98-101
adapters/common/pyproject.toml (1)
4-18: LGTM!adapters/codex-cli/pyproject.toml (1)
12-19: LGTM!Also applies to: 28-29
adapters/hermes-cli/pyproject.toml (1)
12-20: LGTM!Also applies to: 29-30
adapters/hermes-sdk/pyproject.toml (1)
12-19: LGTM!Also applies to: 28-29
…on-syncs-b2 Signed-off-by: David Gardner <dagardner@nvidia.com>
2 participants
Overview
setuptools-scmandsetuptools_dynamic_dependenciesfrom the setuptools-based Python packages. The reason is that Cargo and Maturin don't have the ability to infer the version from a git tag. This caused thenemo-fabric-runtimepackage to have a slightly different versionset-versionJust recipe from Relay to automate setting/updating version stringsWhere should the reviewer start?
Start with the
set-versionrecipe and its helper functions injustfile, then review.agents/skills/update-project-version/SKILL.mdfor the intended release workflow.Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Closes FABRIC-57
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