fix(cuda_cccl): return the actual merged wheel instead of an arbitrart glob hit - #10230
fix(cuda_cccl): return the actual merged wheel instead of an arbitrart glob hit#10230andrewwhitecdw wants to merge 5 commits into
Conversation
…y glob hit
## Summary
After repacking the merged multi-CUDA wheel, `merge_cuda_wheels.py`
"finds" the output with `output_dir.glob("*.whl")[0]`. If the output
directory already contains any other wheel (a previous run, input wheels,
or unrelated artifacts), the glob may return one of those instead, so the
script reports — and downstream CI steps consume — an unmerged or
unrelated wheel while printing "Successfully merged wheel".
## Root cause
```python
output_wheels = list(output_dir.glob("*.whl"))
if not output_wheels:
raise RuntimeError("Failed to create merged wheel")
merged_wheel = output_wheels[0]
```
`glob` order is filesystem-dependent and the result is not filtered to
the wheel that `wheel pack` just produced. Repro: place any pre-existing
`*.whl` in the output dir and call `merge_wheels()` — the returned path
can be the pre-existing wheel (missing the `compute/cu13` payload) even
though the merge itself succeeded.
## Fix
Derive the expected output name from the base wheel's `.dist-info`
directory and `WHEEL` tags — exactly how `wheel pack` names its output —
and verify that file exists:
```python
dist_name, dist_version = dist_infos[0].name[:-len(".dist-info")].split("-")
merged_wheel = output_dir / f"{dist_name}-{dist_version}-{wheel_tags[0]}.whl"
if not merged_wheel.exists():
raise RuntimeError("Failed to create merged wheel")
```
This is deterministic, immune to unrelated wheels in the output dir, and
still works when re-running a merge into the same directory (where
`wheel pack` overwrites the same-named output).
## Testing
Standalone repro (`uv run --with wheel python repro.py`) that builds two
fake cu12/cu13 wheels with `wheel pack`, seeds the output dir with 21
pre-existing decoy wheels, and calls `merge_wheels()` twice:
- pre-fix (stash): `AssertionError: merge_wheels returned
zz_decoy_18-1.0-py3-none-any.whl which is NOT the merged wheel
(cu12=True, cu13=False)` — wrong wheel returned, 5/5 runs.
- post-fix: returns `cuda_cccl-1.0.0-py3-none-any.whl` containing both
`compute/cu12` and `compute/cu13` payloads on both runs (second run
overwrites cleanly).
⚠️ The real wheel merge requires CUDA toolkit builds and could not be run
locally; please rely on CI for end-to-end validation.
## Why existing tests missed it
There are no tests for `merge_cuda_wheels.py`, and CI always merges into
a freshly created empty `wheelhouse_merged` directory, where the glob
happens to contain exactly one wheel.
|
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:
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughChanges
Wheel merge output resolution
Suggested reviewers: 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: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1e87f8ee-83eb-4b97-ab52-204c2025fb44
📒 Files selected for processing (1)
python/cuda_cccl/merge_cuda_wheels.py
merge_wheels used output_dir.glob("*.whl")[0], so a pre-existing decoy
wheel in the output directory could be returned instead of the merged wheel.
Red-green verification (python 3.12.3, pytest 9.0.3):
- with fix: PYTHONPATH=python/cuda_cccl python -m pytest python/cuda_cccl/tests/test_merge_cuda_wheels.py -v -> 1 passed
- pre-fix: git show HEAD~1:python/cuda_cccl/merge_cuda_wheels.py > python/cuda_cccl/merge_cuda_wheels.py
PYTHONPATH=python/cuda_cccl python -m pytest python/cuda_cccl/tests/test_merge_cuda_wheels.py -v
-> AssertionError: got /tmp/.../aa_decoy_4-...whl instead of cuda_cccl-1.0.0-py3-none-any.whl
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
python/cuda_cccl/tests/test_merge_cuda_wheels.py (1)
73-110: 📐 Maintainability & Code Quality | 🔵 Trivialimportant: Run the targeted merge-wheel regression test and the package
pre-commitchecks before merging:python -m pytest -q python/cuda_cccl/tests/test_merge_cuda_wheels.pyandpre-commit run --files python/cuda_cccl/merge_cuda_wheels.py python/cuda_cccl/tests/test_merge_cuda_wheels.py.Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5f6b2605-33be-49f8-9e2b-ce5749c8c11e
📒 Files selected for processing (1)
python/cuda_cccl/tests/test_merge_cuda_wheels.py
|
Addressed code review findings: Major (merge_cuda_wheels.py): The merged wheel filename is now reconstructed from the complete wheel metadata — all Minor (test_merge_cuda_wheels.py): Added a deterministic regression test that monkeypatches All 3 tests pass: |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
python/cuda_cccl/merge_cuda_wheels.py (1)
148-171: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCorrect reconstruction, but duplicates wheel's own algorithm.
This reimplements
wheel.cli.pack.compute_taglineline-for-line (verified against upstream source). Fine functionally, but any future change to wheel's tagline/build-number logic will silently diverge from this copy since there's nothing tying the two together.♻️ Prefer importing the helper instead of duplicating it
+from wheel.cli.pack import compute_tagline + ... - wheel_tags = [ - line.split(":", 1)[1].strip() - for line in wheel_text.splitlines() - if line.startswith("Tag:") - ] - if not wheel_tags: - raise RuntimeError(f"No Tag found in {dist_infos[0] / 'WHEEL'}") - impls = sorted({tag.split("-")[0] for tag in wheel_tags}) - abivers = sorted({tag.split("-")[1] for tag in wheel_tags}) - platforms = sorted({tag.split("-")[2] for tag in wheel_tags}) - tagline = "-".join([".".join(impls), ".".join(abivers), ".".join(platforms)]) + wheel_tags = [ + line.split(":", 1)[1].strip() + for line in wheel_text.splitlines() + if line.startswith("Tag:") + ] + if not wheel_tags: + raise RuntimeError(f"No Tag found in {dist_infos[0] / 'WHEEL'}") + tagline = compute_tagline(wheel_tags)
wheel.cli.packis a CLI-internal module, so pin/verify compatibility with the installedwheelversion before relying on it as an import.python/cuda_cccl/tests/test_merge_cuda_wheels.py (1)
113-151: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMonkeypatch never fires under the current implementation.
merge_wheels()no longer callsoutput_dir.glob("*.whl")— it now builds the expected filename exactly and checks.exists(). Thedecoy_first_globbranch guardingself == output_dir and pattern == "*.whl"is therefore dead code in this test: it never intercepts anything, so the test passes the same with or without the monkeypatch. The docstring's claim ("forcing a decoy first... ensuring the fix is not filesystem-order dependent") doesn't hold anymore — that guarantee now comes from the deterministic filename construction itself, already covered bytest_merge_wheels_returns_expected_wheel_despite_decoys.Either drop this test as redundant, or repurpose the monkeypatch to guard a call path still present in
merge_wheels()(e.g.base_wheel.glob("*.dist-info")) so it actually exercises order-independence.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1b43b8dc-b1cc-4e01-9ba4-81ffa0d85ddd
📒 Files selected for processing (2)
python/cuda_cccl/merge_cuda_wheels.pypython/cuda_cccl/tests/test_merge_cuda_wheels.py
… tag Address code review findings: - merge_cuda_wheels.py now computes the tagline from every Tag entry (sorted unique impls, abivers, platforms) and includes the Build tag when present, exactly matching how wheel pack names its output. - Add deterministic regression test that monkeypatches Path.glob to return a decoy first, proving the fix is not filesystem-order dependent. - Add multi-tag and build-tag regression coverage.
d49854e to
14e0672
Compare
Auditor: The dist-info name parsing uses `.split('-')`, which fails for normalized distribution names containing hyphens (e.g. `cuda-cccl-1.0.0.dist-info` would unpack into 3 parts and raise ValueError). Use `.rsplit('-', 1)` instead.
1 participant
Summary
After repacking the merged multi-CUDA wheel,
merge_cuda_wheels.py"finds" the output withoutput_dir.glob("*.whl")[0]. If the output directory already contains any other wheel (a previous run, input wheels, or unrelated artifacts), the glob may return one of those instead, so the script reports — and downstream CI steps consume — an unmerged or unrelated wheel while printing "Successfully merged wheel".Root cause
globorder is filesystem-dependent and the result is not filtered to the wheel thatwheel packjust produced. Repro: place any pre-existing*.whlin the output dir and callmerge_wheels()— the returned path can be the pre-existing wheel (missing thecompute/cu13payload) even though the merge itself succeeded.Fix
Derive the expected output name from the base wheel's
.dist-infodirectory andWHEELtags — exactly howwheel packnames its output — and verify that file exists:This is deterministic, immune to unrelated wheels in the output dir, and still works when re-running a merge into the same directory (where
wheel packoverwrites the same-named output).Testing
Standalone repro (
uv run --with wheel python repro.py) that builds two fake cu12/cu13 wheels withwheel pack, seeds the output dir with 21 pre-existing decoy wheels, and callsmerge_wheels()twice:AssertionError: merge_wheels returned zz_decoy_18-1.0-py3-none-any.whl which is NOT the merged wheel (cu12=True, cu13=False)— wrong wheel returned, 5/5 runs.cuda_cccl-1.0.0-py3-none-any.whlcontaining bothcompute/cu12andcompute/cu13payloads on both runs (second run overwrites cleanly).Why existing tests missed it
There are no tests for
merge_cuda_wheels.py, and CI always merges into a freshly created emptywheelhouse_mergeddirectory, where the glob happens to contain exactly one wheel.Description
closes
Checklist