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 4 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.
📝 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
… 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.
|
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
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