ci: add release preparation and wheel verification#634
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📜 Recent review details⏰ Context from checks skipped due to timeout. (7)
WalkthroughChangesRelease and wheel validation
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
added
area:dev-ex
0485f51 to
3cc4ddf
Compare
532c7f3 to
704b01f
Compare
3cc4ddf to
c20669d
Compare
704b01f to
dcc537a
Compare
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
a378b11 to
2567181
Compare
Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
|
||
| COPY dist/*.whl /tmp/dist/ | ||
|
|
||
| RUN --mount=type=cache,target=/root/.cache/uv \ |
There was a problem hiding this comment.
I wish Dockerfiles in github had syntax highlighting for these
my nit would be to put this section into a script since it's a bit rough to read here
There was a problem hiding this comment.
Addressed in 25671818. The CI-facing orchestration now lives in mise run release:verify-wheel; the actual install and smoke checks remain in the wheel-install stage so the Docker build itself verifies the clean end-user environment.
| from pathlib import Path | ||
| from types import ModuleType | ||
|
|
||
| REPO_ROOT = Path(__file__).parents[2] |
There was a problem hiding this comment.
my nit- would be to prefer a git rev-parse --show-toplevel or gitpy equivalent, but maybe not worth the git hit
There was a problem hiding this comment.
Addressed in d52cd440. The test now uses pytest's pytestconfig.rootpath fixture, so it follows pytest's discovered repository root without shelling out to Git.
|
|
||
|
|
||
| def _list_local_tags(*, cwd: Path) -> list[str]: | ||
| output = _run_git(["tag", "--list"], cwd=cwd) |
There was a problem hiding this comment.
does this also fetch first or is that done somewhere else?
There was a problem hiding this comment.
No, intentionally. The helper only inspects local tags and has no network side effects. CONTRIBUTING.md now explicitly says to fetch the tags you want considered before running it.
Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
coderabbitai
Bot
added
docs
Greptile SummaryThis PR adds release preparation and built-wheel verification. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (2): Last reviewed commit: "fix(release): address review nits" | Re-trigger Greptile |
![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
🧹 Nitpick comments (3)
tests/tools/test_release_version.py (2)
48-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFixture missing the
fixture_prefix.
release_toolshould be namedfixture_release_toolper project convention (and its usage as a parameter name at line 57 updated accordingly).As per path instructions, "Prefix fixtures with
fixture_, use function scope by default, and document each fixture with a one-line docstring."Source: Path instructions
60-70: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUses
tempfile.TemporaryDirectory()instead oftmp_path.Path instructions call for
tmp_pathfor filesystem operations in tests. Since setup/teardown are xunit-style methods (not fixtures),tmp_pathcan't be injected directly here without restructuring to a fixture-based pattern.As per path instructions, "Use
tmp_pathfor filesystem writes and avoid shared mutable state."Source: Path instructions
.github/workflows/README.md (1)
233-236: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse em-dash for asides, per markdown style guideline.
"Build wheel - Builds the production wheel"and"Publish to PyPI - Uploads to PyPI"use a hyphen as an aside separator.As per coding guidelines, "Use
--(em-dash) instead of-(hyphen) for asides in markdown."🔧 Proposed fix
-1. Build wheel - Builds the production wheel +1. Build wheel -- Builds the production wheel 2. Verify the wheel installs in a clean end-user container 3. Push to test PyPI -4. Publish to PyPI - Uploads to PyPI +4. Publish to PyPI -- Uploads to PyPISource: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 435b4157-f034-4f33-829c-931bf75422c2
📒 Files selected for processing (13)
.dockerignore.github/actions/detect-changes/action.yml.github/workflows/README.md.github/workflows/ci-checks.yml.github/workflows/container-build.yml.github/workflows/release.yml.mise/tasks/release/prepare.mise/tasks/release/verify-wheelCONTRIBUTING.mdcontainers/Dockerfile.test_cicontainers/README.mdtests/tools/test_release_version.pytools/release_version.py
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: Greptile Review
- GitHub Check: Unit Tests (3.11)
- GitHub Check: Unit Tests (3.12)
- GitHub Check: Unit Tests (3.13)
🧰 Additional context used
📓 Path-based instructions (14)
.github/workflows/*.yml
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Preserve CI workflow requirements, including the repository's quality checks and documented release and documentation deployment behavior.
Files:
.github/workflows/container-build.yml.github/workflows/release.yml.github/workflows/ci-checks.yml
.github/**
⚙️ CodeRabbit configuration file
Review GitHub configuration for branch protection expectations, CODEOWNERS alignment, least privilege permissions, pinned actions where practical, and consistency with CONTRIBUTING.md.
Files:
.github/workflows/container-build.yml.github/actions/detect-changes/action.yml.github/workflows/README.md.github/workflows/release.yml.github/workflows/ci-checks.yml
**/*.{md,markdown,py}
📄 CodeRabbit inference engine (.cursor/rules/agent-markdown-style.mdc)
**/*.{md,markdown,py}: Avoid decorative bold (**text**) in list items, body text, and docstrings; use structural cues (headers, list markers, colons, backticks) for emphasis instead
Use backticks for code identifiers, paths, and CLI commands in markdown and docstrings
Files:
containers/README.mdtests/tools/test_release_version.pyCONTRIBUTING.mdtools/release_version.py
**/*.{md,markdown}
📄 CodeRabbit inference engine (.cursor/rules/agent-markdown-style.mdc)
**/*.{md,markdown}: Bold is acceptable only in markdown tables where it's the conventional way to mark header-like cells in the body
Use##headers to segment markdown sections instead of bold text
Use--(em-dash) instead of-(hyphen) for asides in markdown
Files:
containers/README.mdCONTRIBUTING.md
**/*.md
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
Do not use decorative bold in Markdown body text, list items, or docstrings; use single backticks for code identifiers, paths, and commands.
Use the repository's Markdown style and include SPDX copyright headers.
Files:
containers/README.mdCONTRIBUTING.md
**/*
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
**/*: Every source file requires the SPDX copyright and license header appropriate to its file format.
End files with a newline, remove trailing whitespace, use one space between sentences, and keep code, comments, and docstrings within 120 characters.
**/*: Before submitting a pull request, runmise run format,mise run check, andmise run test; use pinned mise tasks rather than invokingruffortydirectly.
All commits merged tomainmust use Conventional Commits format:<type>(<scope>): <description>or<type>: <description>, with a lowercase valid type and a description of at most 100 characters.
All contributions must include a DCOSigned-off-bytrailer, and commits must also have a verified cryptographic signature.
Never move a published release tag; if release code changes, create and validate the next release candidate instead.
Stable release tags must point to the exact tested commit SHA, and artifacts, containers, documentation, and installation must be verified before announcing a release.
Usemise run <task>for project tasks; the Makefile only bootstraps mise and provides deprecated compatibility messages.
When testing NSS changes in NMP, use the NMP Makefile'suse-nss-localtarget for local wheels and always runuse-nss-artifactorybefore committing.
Files:
containers/README.mdcontainers/Dockerfile.test_citests/tools/test_release_version.pyCONTRIBUTING.mdtools/release_version.py
⚙️ CodeRabbit configuration file
**/*: Review as a senior maintainer for NeMo Safe Synthesizer. Prioritize issues that can change behavior, break user workflows, weaken privacy guarantees, hide failures, make tests unreliable, or create maintenance risk. Avoid generic style commentary unless it points to a concrete project convention that automated tools will not catch.
Comment only when the finding is actionable and tied to changed code. For each finding, state the impact, the condition that triggers it, and the smallest practical fix. Prefer one precise comment over broad advice. Do not ask for refactors outside the PR scope unless the changed code creates the problem.
Review type guidance: - Potential issue: use for correctness bugs, data loss, privacy leaks,
security risks, broken public APIs, invalid config behavior, missing
validation, hidden failures, nondeterministic tests, or CI breakage.
- Refactor suggestion: use for local maintainability problems introduced
by the diff when they have clear future cost, such as duplicated setup,
unclear boundaries, over-mocking, avoidable complexity, or opaque test
helpers.- Nitpick: avoid in chill mode. Do not emit formatting, import-order,
wording, or style-only comments unless automated tools cannot catch the
issue and it affects maintainability.Severity guidance: - Critical: security/privacy leaks, data loss, training/test/holdout
contamination, or broken release/package/core pipeline execution.
- Major: incorrect generation/training/evaluation behavior, broken
CLI/SDK public API, invalid config defaults or validators, or GPU/vLLM
cleanup and process-isolation bugs likely to fail CI or production
runs.- Minor: localized bugs, missing focused tests for changed behavior, or
bad test patterns that weaken regression coverage.- Trivial: small cleanup with no behavior impact. Usually suppress in
chill mode.- Info: context only. Avoid unless it helps reviewers understand risk.
Safe-Synthesizer-specific review focus: - Data ...
Files:
containers/README.mdcontainers/Dockerfile.test_citests/tools/test_release_version.pyCONTRIBUTING.mdtools/release_version.py
**/*.{py,sh,yaml,yml,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Add SPDX copyright headers to all Python, shell, YAML, YML, and Markdown source files; honor exclusions in
.copyrightignore.
Files:
containers/README.mdtests/tools/test_release_version.pyCONTRIBUTING.mdtools/release_version.py
containers/**
⚙️ CodeRabbit configuration file
Review container changes against STYLE_GUIDE.md and containers/README.md. Check cache mounts, uv usage, non-root runtime behavior, CUDA settings, and copy order.
Files:
containers/README.mdcontainers/Dockerfile.test_ci
containers/Dockerfile*
📄 CodeRabbit inference engine (STYLE_GUIDE.md)
Use multi-stage production builds, cache mounts,
--no-install-recommends, non-rootappuser, and proper PID 1 handling.
Files:
containers/Dockerfile.test_ci
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Place durable implementation guidance in function and class docstrings for public contracts and source comments for local invariants
Target Python 3.11–3.13 with modern syntax (X | Y,list[str],Self). Python 3.14+ is not supported
**/*.py: Use American English spelling in Python code, documentation, and messages.
UseField(description=...)for every Pydantic model field.
Use assignment-styleField()by default; useAnnotatedonly for additional metadata such as validators, constrained aliases, or discriminated unions.
Use@dataclass(frozen=True)for immutable value objects and validators; use mutable dataclasses only for builders, accumulators, and pipeline state.
Usefield(default_factory=list)instead of mutable list defaults.
UseStrEnumfor string-valued configuration or serialization enums and plainEnumfor internal constants.
Obtain loggers withobservability.get_logger(__name__); do not calllogging.getLogger()orstructlog.get_logger()directly.
Use.runtime,.user, and.systemcategory loggers appropriately.
Do not useprint()for operational library output; use the approved logger,click.echo(), orsys.stdout.write()where appropriate.
Useextra={}for machine-queryable logging data and f-strings only for human-readable context.
Raise errors from the custom Safe Synthesizer error hierarchy, using the documented dual inheritance for user and internal errors.
Keep shared package code compatible with Python 3.11; do not use Python 3.12-only syntax such as PEP 695 type statements or bracketed generic parameters.
PreferX | Y, built-in collection generics, andSelfoverOptional,Union, and legacy typing collections.
Use collection ABCs for function arguments and concrete collection types for return values.
UseProtocolfor structural subtyping and avoidAnywhenobject, generics, or protocols are suitable.
UseTYPE_CHECKINGguards for heavy imports such as pandas, torch, and transformers.
...
Files:
tests/tools/test_release_version.pytools/release_version.py
**/test_*.py
📄 CodeRabbit inference engine (AGENTS.md)
Use the
unitmarker instead of the deprecatedunit_testmarker for test identification
Files:
tests/tools/test_release_version.py
tests/**
📄 CodeRabbit inference engine (.cursor/rules/repo-navigation.mdc)
tests/**: Mirrorsrc/directory structure intests/directory for test organization
Auto-mark tests by directory:tests/e2e/→e2e,tests/smoke/→smoke, otherwise default tounitMirror source code directory structure in tests directory (e.g.,
tests/training/,tests/generation/parallel to source structure)
Files:
tests/tools/test_release_version.py
tests/**/*.py
📄 CodeRabbit inference engine (tests/TESTING.md)
tests/**/*.py: Auto-mark tests based on file path: tests under/e2e/gete2emarker, tests under/smoke/getsmokemarker, all others getunitmarker (only if no category marker already present)
Every test should have exactly one category marker:unit,smoke, ore2e
Usepytest.mark.requires_gpumodifier on tests that need CUDA hardware
Usepytest.mark.vllmon tests using vLLM generation backend and ensure each vLLM test file runs in its own process for GPU memory isolation
Usepytest.mark.slowon long-running tests
Usepytest.mark.smollm2for SmolLM2 Hub download tests to enable process isolation
Usepytest.mark.noautouseto skip autouse fixtures for specific tests
Useload_test_dataset(filename)helper to load test datasets fromtests/stub_datasets/as HuggingFaceDatasetobjects
Useload_test_dataframe(filename)helper to load test data files fromtests/stub_datasets/as pandas DataFrames
Convert pandas columns to nullable dtypes (pd.Int64Dtype(),pd.BooleanDtype()) before assigningnp.nanvalues
Usefake.seed_instance(seed)andrandom.seed(seed)together for Faker-based test data reproducibility
When sharing methods across multiple test files, define them inconftest.pyand import them using relative imports (e.g.,from .conftest import train_with_sdk); note that importing from other test files liketests/cli/helpers.pydoes not work
Usefixture_mock_processororfixture_mock_processor_without_valid_recordsfor mocking ParsedResponse objects withvalid_records,invalid_records,errors, andprompt_numberfields
Usepytest.importorskipto gate tests on optional dependencies that require specific extras (e.g.,sentence_transformers,vllm)
Run vLLM tests with separate pytest invocations (one per file) using-n 0(single process) for GPU memory isolation, or use staged mise tasks for CI visibility
Print statements are allowed in tests (ruffT201is suppressed fortests/directory) and should...
Files:
tests/tools/test_release_version.py
⚙️ CodeRabbit configuration file
Review tests against tests/TESTING.md. Check marker usage, fixture naming, tmp_path usage, determinism, and GPU/vLLM process-isolation requirements. Flag slop tests that only check that code runs, assert result is not None when stronger invariants exist, over-mock internal implementation details, patch around the bug instead of reproducing it, or add broad snapshot/golden churn without a clear contract. Flag change detector tests that fail on harmless refactors, formatting, record ordering, incidental wording, or private implementation details without demonstrating a behavior regression. Prefer existing fixtures or focused new fixtures for repeated setup; keep tests DRY when reasonable without making the behavior under test opaque. print() is allowed in tests.
Files:
tests/tools/test_release_version.py
tools/**
⚙️ CodeRabbit configuration file
Review tools as developer and CI infrastructure. Check that scripts use uv or Makefile wrappers instead of ad hoc python/pip commands, preserve read-only behavior for check targets, fail with clear messages, avoid hidden network or filesystem side effects, and stay consistent with STYLE_GUIDE.md and CONTRIBUTING.md. Tooling may use print() when it is a standalone script or intentional CLI output.
Files:
tools/release_version.py
🧠 Learnings (1)
📚 Learning: 2026-05-27T22:20:37.354Z
Learnt from: kendrickb-nvidia
Repo: NVIDIA-NeMo/Safe-Synthesizer PR: 520
File: tests/generation/test_vllm_backend.py:556-587
Timestamp: 2026-05-27T22:20:37.354Z
Learning: In NVIDIA-NeMo/Safe-Synthesizer, `tests/conftest.py`’s `pytest_collection_modifyitems` hook applies pytest category markers automatically based on each test file’s path: tests under `/e2e/` get `pytest.mark.e2e`, tests under `/smoke/` get `pytest.mark.smoke`, and all other tests get `pytest.mark.unit`. Therefore, when reviewing pytest tests outside `tests/e2e/` and `tests/smoke/`, do not flag missing explicit `pytest.mark.unit` decorators on test classes/functions as an issue (the hook will add them during collection). If a new test directory/category is introduced, ensure the hook is updated so it’s categorized correctly.
Applied to files:
tests/tools/test_release_version.py
🪛 ast-grep (0.44.1)
tests/tools/test_release_version.py
[error] 71-71: Use of unsanitized data to create processes
Context: subprocess.run(args, cwd=self.repo, check=False, capture_output=True, text=True)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(os-system-unsanitized-data)
[error] 71-71: Command coming from incoming request
Context: subprocess.run(args, cwd=self.repo, check=False, capture_output=True, text=True)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
tools/release_version.py
[info] 317-317: use jsonify instead of json.dumps for JSON output
Context: json.dumps({"error": str(error)}, sort_keys=True)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 323-323: use jsonify instead of json.dumps for JSON output
Context: json.dumps(asdict(plan), sort_keys=True)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[error] 130-136: Command coming from incoming request
Context: subprocess.run(
["git", *args],
cwd=cwd,
check=False,
capture_output=True,
text=True,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
🪛 LanguageTool
CONTRIBUTING.md
[uncategorized] ~688-~688: The official name of this software platform is spelled with a capital “H”.
Context: ...inal releases, including post-releases. container-build.yml publishe...
(GITHUB)
[style] ~839-~839: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...elease is not marked as a prerelease. - Confirm versioned documentation is available at...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🪛 Ruff (0.15.21)
tests/tools/test_release_version.py
[error] 72-72: subprocess call: check for execution of untrusted input
(S603)
[warning] 192-192: Pattern passed to match= contains metacharacters but is neither escaped nor raw
(RUF043)
[warning] 230-230: Pattern passed to match= contains metacharacters but is neither escaped nor raw
(RUF043)
tools/release_version.py
[error] 131-131: subprocess call: check for execution of untrusted input
(S603)
[error] 132-132: Starting a process with a partial executable path
(S607)
🔇 Additional comments (17)
.dockerignore (1)
27-28: LGTM!.mise/tasks/release/verify-wheel (1)
6-9: LGTM!containers/Dockerfile.test_ci (2)
49-59: 🩺 Stability & Availability | ⚡ Quick winCommit-pinned vLLM wheel index is a fragile external dependency for CI.
The cu129 dependency-resolution check depends on
https://wheels.vllm.ai/ee0da84ab9e04ac7610e28580af62c365e898389/cu129, a wheel index tied to one specific upstream commit. If that index is ever pruned or rotated upstream, this dependency-resolution step (and thus wheel verification / release / PR CI) will start failing for reasons unrelated to this repository's changes.Confirm this pin is intentionally synced with
pyproject.toml's vLLM/CUDA extras and that there's a process to bump it when the pinned commit rotates.
27-61: LGTM!containers/README.md (1)
126-152: LGTM!.github/actions/detect-changes/action.yml (1)
88-90: LGTM!.github/workflows/ci-checks.yml (2)
258-266: LGTM!
54-59: 🗄️ Data Integrity & IntegrationNo issue:
depsis already exposed by.github/actions/detect-changes/action.yml, so thewheel-installandsmoke-testgates can useneeds.changes.outputs.depsas intended.> Likely an incorrect or invalid review comment..github/workflows/release.yml (1)
71-73: LGTM!.github/workflows/container-build.yml (1)
88-89: LGTM!tools/release_version.py (2)
144-166: Solid, well-tested canonical-tag parser.Cross-checked every documented valid/invalid tag example (rc/post dash variants, alpha prereleases, leading zeros,
+devNlocals) against this logic and the parametrized tests — all resolve as expected.
1-12: LGTM!Also applies to: 24-141, 168-332
tests/tools/test_release_version.py (1)
18-46: LGTM! Thorough, precise assertions and good parametrized coverage of the planning/CLI contract.Also applies to: 55-55, 71-231
.mise/tasks/release/prepare (2)
1-5: LGTM!Also applies to: 11-13
6-10: 📐 Maintainability & Code QualityNo change needed for
MISE_CONFIG_ROOThere. File tasks run under mise withMISE_CONFIG_ROOTset, and this task is meant to be invoked viamise run release:prepare; the same source pattern is used elsewhere under.mise/tasks/**.> Likely an incorrect or invalid review comment..github/workflows/README.md (1)
218-232: LGTM!Also applies to: 237-238
CONTRIBUTING.md (1)
688-847: LGTM! Documentation accurately reflectstools/release_version.py's behavior (validity table, helper options, rejection conditions) and is internally consistent.
Signed-off-by: Aaron Gonzales <aagonzales@nvidia.com>
2 participants
Summary
release:preparetask that previews the next release's initialrc0tag without mutating GitStack
This PR is stacked on #633. Its own diff contains only the semver helper, release-tool documentation, wheel verification tooling/tests, and workflow integration. Retarget it to
mainafter #633 merges; the PR workflow only runs automatically for pull requests whose base ismain.Validation
mise run validate: 1,648 tests and 3 subtests passedmise run release:prepare -- --ref origin/main --jsonproducedv0.1.7rc0without creating a tag or syncing the projectmise run format-check, type checking, lock checking, andgit diff --checkpassedSummary by CodeRabbit