wheels: avoid vendoring libnvJitLink - #1741
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
/ok to test |
| --exclude "libcusparse.so.*" | ||
| --exclude "libnccl.so.*" | ||
| --exclude "libnvJitLink*" | ||
| --exclude "libnvJitLink.so*" |
There was a problem hiding this comment.
The .so was dropped from this line in #507, I think because of a misunderstanding of the order things happened in these builds.
auditwheel repair will take a dependency on libnvJitLink.so.13.3.33 and vendor in a library renamed like cuopt_cu13.libs/libnvJitLink-3ba1e744.so.13.3.33.
But this --exclude is matched against the DT_NEEDED entries in the .so files in the wheel, not against the names auditwheel creates afterwards.
So I think we really do want .so here, to avoid accidentally matching an (unlikely, but possible) other file named like libnvJitLink-cuopt-wrapper.so or something.
CI Test Summary✅ All 22 test job(s) passed. (1 skipped) |
|
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 (3)
Included review availability: Your plan includes up to 12 reviews per rolling hour; 11 remain after this review. 📝 WalkthroughWalkthroughChangesWheel packaging
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This change removes an unnecessary vendored library from the wheels and tightens size limits to prevent similar packaging regressions; no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/merge |
Description
Noticed while reviewing #1731 and looking through logs...
cuopt-cu{12,13}wheels are vendoring a copy oflibnvJitLink.so.That's unnecessary in 2 ways:
cuoptdoesn't directly use nvJitLink (auditwheelis probably picking that up as a transitive dependency coming from cusparse or similar)nvidia-nvjitlink-cu{12,13}wheels, not vendor oneThis proposes removing that file from
cuoptwheels and tightening the wheel-size limits to help prevent things like this from slipping through again.Issue
N/A
Notes for Reviewers
How I tested this
Check the
pydistcheckoutput in CI logs from builds.Saw wheel-testing CI pass.
Checklist