feat(nightly): publish aiperf-nightly wheel alongside aiperf#914
Conversation
…rter
AggregateConfidenceJsonExporter was calling importlib.metadata.version("aiperf")
directly with its own try/except, while every other exporter imports the
shared aiperf.__version__ symbol. Align this exporter with the convention
so version lookup happens in one place (src/aiperf/__init__.py).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Signed-off-by: Harrison King Saturley-Hall <hsaturleyhal@nvidia.com>
Users who track development builds need a `pip install aiperf-nightly`
target that doesn't collide with the stable `aiperf` distribution name
on PyPI/Artifactory. The nightly pipeline now produces both wheels from
a single source build: the canonical `aiperf-<dev-version>.whl` and a
renamed `aiperf_nightly-<dev-version>.whl` whose only differences are
the dist name in METADATA, the dist-info directory, RECORD hashes, and
the `version("aiperf")` literal inside `aiperf/__init__.py` (rewritten
in-place so `aiperf.__version__` still resolves under the new dist).
Changes:
* tools/rename_wheel.py (new): stdlib-only utility that repacks a wheel
with a new distribution name. Handles METADATA `Name:` rewrite,
dist-info directory rename, RECORD regeneration with fresh sha256s,
PEP 491 filename construction, and source-patching of
`version("aiperf")` / `get_version("aiperf")` /
`importlib.metadata.version("aiperf")` calls so __version__ lookups
keep working under the renamed dist. Lives in tools/ — outside the
hatchling src/ root, so it is not bundled into the wheel.
* .github/workflows/nightly.yml:
- prepare-nightly: emit a new `nightly_wheel_filename` output
(`aiperf_nightly-<dev-version>-py3-none-any.whl`, PEP 491 escape).
Stored in version-info.env alongside the canonical filename so
retried runs restore it.
- build-container: new "Generate aiperf-nightly wheel variant" step
runs tools/rename_wheel.py against the canonical wheel after the
buildx export.
- build-container: validate step now installs each wheel into its own
venv (.venv-check-orig and .venv-check-nightly) — the two dists
share the `aiperf/` top-level module and cannot coexist. Both must
report `aiperf.__version__ == EXPECTED_VERSION`, which also proves
the rename script's source-patching survived the round trip.
- build-container: S3 upload loops over `aiperf*.whl` (no hyphen) so
both wheels are pushed to s3://${S3_BUCKET}/aiperf/nightly/<ver>/.
- stage-nightly-wheel: pulls and Artifactory-deploys both filenames
in a loop, keeping the existing single-object aws s3 cp pattern.
- trigger-gitlab-security-scan: forwards both WHEEL_FILENAME and
NIGHTLY_WHEEL_FILENAME as pipeline variables. The GitLab side will
need a follow-up to consume the new variable; until then the
canonical wheel continues to be scanned as before.
The container image is unchanged — it still ships the canonical
`aiperf` install. The renamed wheel is a side artifact.
Verified locally:
uv build --wheel
python3 tools/rename_wheel.py dist/aiperf-*.whl --output-dir dist/
-> patched 1 call in aiperf/__init__.py
-> both wheels install into separate venvs and report __version__ correctly
-> importlib.metadata.version("aiperf-nightly") resolves under the renamed dist
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Signed-off-by: Harrison King Saturley-Hall <hsaturleyhal@nvidia.com>
Try out this PRQuick install: pip install --upgrade --force-reinstall git+https://github.com/ai-dynamo/aiperf.git@9b19454f684b92c9a1dcae46752daec9f87e073dRecommended with virtual environment (using uv): uv venv --python 3.12 && source .venv/bin/activate
uv pip install --upgrade --force-reinstall git+https://github.com/ai-dynamo/aiperf.git@9b19454f684b92c9a1dcae46752daec9f87e073dLast updated for commit: |
saturley-hall
requested review from
ajcasagrande and
pvijayakrish
and removed request for
pvijayakrish
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughAdds a CLI to repack wheels with a new distribution name, computes and exposes a nightly wheel filename in the nightly workflow, generates/validates/uploads both canonical and nightly wheels, stages both to Artifactory and forwards filenames to GitLab scanning, and switches version lookups/tests to use ChangesNightly Wheel Variant Build and Deployment
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/aiperf/exporters/aggregate/aggregate_confidence_json_exporter.py (1)
53-53:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd return type hint.
The function is missing a return type annotation. Based on the return statement at line 108, the return type should be
JsonExportData.📝 Proposed fix
- def _aggregate_to_export_data(self): + def _aggregate_to_export_data(self) -> "JsonExportData": """Convert AggregateResult to JsonExportData format.Note: Use forward reference (quoted string) since
JsonExportDatais imported inside the method.As per coding guidelines: "Type hints on ALL functions (params and return)".
🤖 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 `@src/aiperf/exporters/aggregate/aggregate_confidence_json_exporter.py` at line 53, The method _aggregate_to_export_data is missing a return type annotation; update its signature to include the return type "JsonExportData" (use a forward reference string since JsonExportData is imported inside the method), i.e. change def _aggregate_to_export_data(self): to def _aggregate_to_export_data(self) -> "JsonExportData": and keep the existing return value unchanged.
🤖 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 `@tools/rename_wheel.py`:
- Around line 49-54: The function patch_source_version_calls is missing a return
type annotation; update its signature to include the return type dict[str, int]
(i.e., annotate the function as returning dict[str, int]) so the signature reads
patch_source_version_calls(... ) -> dict[str, int]: ensuring the rest of the
function body and return value remain unchanged.
---
Outside diff comments:
In `@src/aiperf/exporters/aggregate/aggregate_confidence_json_exporter.py`:
- Line 53: The method _aggregate_to_export_data is missing a return type
annotation; update its signature to include the return type "JsonExportData"
(use a forward reference string since JsonExportData is imported inside the
method), i.e. change def _aggregate_to_export_data(self): to def
_aggregate_to_export_data(self) -> "JsonExportData": and keep the existing
return value unchanged.
🪄 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: CHILL
Plan: Pro
Run ID: 7a9e7cbc-e8ea-4953-8cee-2613516ff043
📒 Files selected for processing (3)
.github/workflows/nightly.ymlsrc/aiperf/exporters/aggregate/aggregate_confidence_json_exporter.pytools/rename_wheel.py
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
pvijayakrish
left a comment
pvijayakrish
left a comment
There was a problem hiding this comment.
Why are we not using this approach? wheel unpack -> modify metadata safely -> wheel pack
CodeRabbit flagged the function's bare `dict` return annotation. The
function returns `{"files": int, "replacements": int}`, so the more
precise hint is `dict[str, int]`.
Review: #914 (review)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Signed-off-by: Harrison King Saturley-Hall <hsaturleyhal@nvidia.com>
CodeRabbit flagged the missing return annotation. Since the runtime import of JsonExportData is deferred inside the method body, the symbol is exposed at module scope under TYPE_CHECKING so static analysis can resolve the annotation without forcing the runtime import. The annotation itself remains a quoted forward reference per CodeRabbit's suggestion. Aligns with the project standard "Type hints on ALL functions (params and return)" from CLAUDE.md. Review: #914 (review) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Harrison King Saturley-Hall <hsaturleyhal@nvidia.com>
Adds a deeper validation step to the nightly pipeline: install the
renamed `aiperf-nightly` wheel into a fresh venv and run the unit test
suite (`tests/unit/`) against it. Catches wheel-only failure modes the
existing source-tree `unit-tests` job cannot see — missing
Requires-Dist, a module that did not ship in the wheel, or shipped
code that accidentally imports from a dev-only test helper.
The new step lives inline in the `build-container` matrix job between
`Validate wheels (amd64 only)` and `Upload wheels to S3 (amd64 only)`,
amd64-only (the wheel is pure-Python). Failure is hard — it fails
build-container, which blocks `create-multiarch-manifest-ecr` and the
two staging jobs (`stage-nightly-wheel`, `stage-nightly-container`) via
their existing `needs:` chain. The existing source-tree `unit-tests`
job stays advisory exactly as before; this gate sits inside the
publish path, not the test path.
The step:
1. Creates a fresh venv (`.venv-wheel-tests`).
2. Installs the renamed `aiperf_nightly-*.whl`.
3. Installs pytest + the dev-only test deps the wheel does not carry
(pytest-asyncio, pytest-xdist, hypothesis, looptime, trustme).
looptime in particular is non-optional: it powers the
"asyncio.sleep runs instantly" auto-fixture in tests/conftest.py
and ~12 timing tests fail without it. trustme is used by the
TLS-cert fixtures in transport tests.
4. Installs the in-tree `tests/aiperf_mock_server` editable so
`tests/unit/conftest.py` can load — it imports `tests.harness`
which transitively pulls in `aiperf_mock_server` even though
unit tests themselves do not exercise the mock server.
5. Runs `pytest tests/unit -n auto -m 'not performance and not stress'
--tb=short` against the installed wheel. The src/ directory is
not on sys.path, so `import aiperf` resolves to the installed
wheel rather than the source tree.
Also fixes `tests/unit/transports/test_base_transport.py` which
hardcoded `importlib.metadata.version('aiperf')` at module scope.
That literal raises `PackageNotFoundError` under the renamed
`aiperf-nightly` distribution, cascading to ImportErrors in the four
transport test modules and in `test_imports.py`. Replaced with
`from aiperf import __version__ as aiperf_version`, matching the
pattern used by every other consumer of the version string in the
codebase (and consistent with the CodeRabbit-flagged fix to
aggregate_confidence_json_exporter.py earlier in this PR).
Verified locally:
uv build --wheel
python3 tools/rename_wheel.py dist/aiperf-*.whl --output-dir dist/
uv venv .venv-wheel-tests
uv pip install ... aiperf_nightly-*.whl
uv pip install pytest pytest-asyncio pytest-xdist hypothesis looptime trustme
uv pip install -e tests/aiperf_mock_server
pytest tests/unit -n auto -m 'not performance and not stress'
-> 9329 passed, 10 skipped, 0 failed, 87s (exit 0)
Known follow-up: `tests/component_integration/cli/test_cli_help.py:59`
has the same `get_package_version("aiperf")` hardcode. Out of scope for
this PR (component_integration is not in the nightly wheel-install
gate); will need fixing if/when that tier is added.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Signed-off-by: Harrison King Saturley-Hall <hsaturleyhal@nvidia.com>
`tests/component_integration/cli/test_cli_help.py::TestCLIVersion::test_version_flag`
hardcoded `importlib.metadata.version("aiperf")` to compute the expected
version string. That literal raises `PackageNotFoundError` against the
renamed `aiperf-nightly` distribution, so the test cannot run if the
nightly wheel-install gate ever expands to cover component_integration.
Replace with `from aiperf import __version__ as aiperf_version` and
compare against that, matching the pattern already used by every other
consumer of the version in the codebase — including the same fix
applied to tests/unit/transports/test_base_transport.py in the prior
commit on this branch.
Pure renaming; the assertion is unchanged in spirit ("CLI's reported
version matches what the installed package declares"). On the source
tree both lookups return the same string; against the renamed wheel,
`aiperf.__version__` works because rename_wheel.py rewrites the
in-wheel `version("aiperf")` literal to `version("aiperf-nightly")`.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Signed-off-by: Harrison King Saturley-Hall <hsaturleyhal@nvidia.com>
Yes, in roughly descending importance:
If we ever decide to retag (change py3-none-any to something more specific) in addition to renaming, the right move would be to compose |
The wheel-test venv only carried looptime/trustme on top of pytest, but tests/unit/api and tests/unit/server import Starlette/FastAPI TestClient and httpx.AsyncClient at collection time. Without httpx in the venv the new nightly wheel gate would fail during pytest collection in a fresh environment. Signed-off-by: Harrison King Saturley-Hall <hsaturleyhal@nvidia.com>
5 participants
Summary
tools/rename_wheel.pyutility that repacks a wheel under a new distribution name — rewritingMETADATA,dist-info/,RECORDhashes, and theversion("aiperf")literals insideaiperf/__init__.pysoaiperf.__version__keeps resolving under the renamed dist..github/workflows/nightly.ymlto produceaiperf_nightly-<dev-version>-py3-none-any.whlfrom the same source build as the canonical wheel, validate both wheels in separate venvs, upload both to S3, stage both to Artifactory, and forward both filenames to the GitLab security scan trigger.AggregateConfidenceJsonExporterwith the rest of the codebase by using the sharedaiperf.__version__instead of a localimportlib.metadata.version("aiperf")call (sorename_wheel.py's source-patching has one consistent literal to rewrite).Why: users tracking development builds need
pip install aiperf-nightlyto work without colliding with the stableaiperfdistribution name on PyPI/Artifactory. Two wheels from one source build keeps the container image unchanged (it still installs the canonicalaiperf) while making the nightly index installable under its own name.Closes OPS-5191.
Out-of-band coordination
The GitLab security-scan pipeline will need a follow-up to consume the new
NIGHTLY_WHEEL_FILENAMEform variable. Until then the canonical wheel continues to be scanned exactly as before; the renamed wheel is byte-equivalent except for metadata/RECORD, so source coverage is unchanged.Test plan
pre-commit run --files tools/rename_wheel.py .github/workflows/nightly.yml— all green (the localvalidate-plugin-schemasfailure is a pre-existingcrickenv issue, unrelated)uv build --wheel→python3 tools/rename_wheel.py dist/aiperf-*.whl --output-dir dist/→ patched 1 call inaiperf/__init__.py, producedaiperf_nightly-0.8.0-py3-none-any.whlaiperf.__version__ == 0.8.0;importlib.metadata.version("aiperf-nightly")resolves correctly under the renamed distworkflow_dispatchofnightly.ymlwithrelease=falseon this branch to exercise build + validate + S3 onlyworkflow_dispatchwithrelease=trueto exercise full Artifactory staging; confirm both filenames appear at${ARTIFACTORY_URL}/nightly/pip install --extra-index-url ${ARTIFACTORY_PYPI_URL} aiperf-nightlyresolves to the staged version end-to-end🤖 Generated with Claude Code
Summary by CodeRabbit