-
Notifications
You must be signed in to change notification settings - Fork 18
build: windows wheels #340
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
OK - the present setup builds on my mac using the following commands:
Depending on your circumstances you may need to specify I'm gonna leave it at that for now y'all can feel free to poke around on the PR with additions, etc. |
OK - cool so now it's at least compiling (but not finishing the install!) with scikit-build-core. CGAL is on in all the cases, installed via system libs. The errors I'm seeing don't make a lot of sense, so would now definitely appreciate help in getting the CI green again :D |
hmmm it would be nice to be able to browse the packages available in manylinux online. anyway leaving it here for now. more than decent progress. |
|
I'm also not sure what's up with this metadata error in the CI install. This doesn't happen locally. |
Nice, only the macos wheels are failing due to what looks like overlinking?
Still, extremely positive and especially given how much this has simplified the installing and wheel building process. |
@gavinsalam FYI this is what is driving changes in the draft PRs over in fastjet and siscone. Once we get this building wheels in all cases then requirements are satisfied on my side. As you can see it's pretty close! So long as we're careful to reconcile your requirements with the requirements here we shouldn't run into too much trouble. This will also have a side effect of making the new fastjet/siscone/contrib cmake setup fairly robust; cibuildwheel is picky. |
Now that that particularly angry yak is shaved. Let's clean this up and ship it. :-) (after the fastjet/siscone/fastjet-contrib releases, etc.) |
if CGAL is providing too complicated on Windows, it's probably safe to leave it out for now. The switchover to CGAL happens only at relatively high multiplicities. The heuristics are given in Specifically, at large R, the change happens for N=100k (anti-kt) or 40k (kt). For smaller R values the switch to NlnN is at even higher R. Very few users will have that kind of multiplicity, even with full HL-LHC pileup. The C/A algorith has lower thresholds but doesn't need CGAL for its NlnN strategy. |
@gavinsalam Thanks for the info! It's not CGAL that's causing problems here (though it does take a long time to install in windows, I have some ideas to mitigate that), it's weirdly strict standards compliance from MSVC + fastjet-contrib not having been completely vetted. With a little more fiddling it should be working! Only two compilation errors left that I see. The only unfortunate thing is the hour long build time right now, but since I have it working I don't want to now remove CGAL. 😒 |
Cool - windows tests pass. Now to shoehorn it into cibuildwheel. |
@matthewfeickert @henryiii I've got the windows wheel matrix fully up and running. I've added two tasks that I would appreciate help on now that we've got working recipes. Any help you can provide is deeply appreciated in these directions, it'll vastly help with maintainability. |
A quick check of turning off debug info in the windows wheels brought it down to 4.5MB on my home desktop. That's reasonable already. I'll put it in the full wheel matrix if everything works out in the short-form CI. |
looks like things are failing due to gitlab getting angry. I'll start 'em again tomorrow. |
Not too bad, down to ~7 minutes for the windows build now. Will propagate it later to the wheels. |
Nice! The dll build allows us to get to 2.4MB wheels on windows, which is commensurate with the other OS's wheel sizes. I think this is now very well under control. |
OK - there we go now we ship the complete fastjet-contrib uni-library. All plugins except for pxcone are enabled. It's more or less fully featured and ~future proof. |
OK - this latest change is using a top-level only CMake setup for fastjet-contrib. Somehow this has broken sdist, @matthewfeickert @henryiii a look would be appreciated. I should be able to converge on a nice patchset for fastjet-contrib at this point. The unfortunate thing is getting all the variants working does require code changes in fastjet-contrib, but they're relatively minimal. |
@gavinsalam You may find the top-level-only cmake implementation for fastjet-contrib here: Right now it includes the necessary patches for windows. It's not actually required to apply them for unix-likes. |
d86b43e
to
3478841
Compare
Enable windows builds of fastjet wheels, pointing to in-progress MRs for fastjet and fastjet-contrib.
c.f. for in-progress MRs:
We'll merge this once all that is in releases.
Below is historical, documenting progress towards #342
This branch will be where we figure out
pip install .
andcibuildwheel
for scikit-hep/fastjet using the in-progress cmake build branches for fastjet/siscone/fastjet-contrib.I need to get the fastjet-contrib edits into a github repository so we can submodule it correctly.
Right now I have it working locally from the fastjet-contrib svn repository + patches.
At least on my laptop I can get the
scikit-hep/fastjet
extension compiling and linking.It doesn't yet function at runtime, but it's only a matter of time.
I'll send some further notifications when it's ready for a more serious look.
One thing we need to solve:
(instead of 14.0/13.0 which is what gets us all green right now)40m-1hr installing dependencies)16MB4.3MB2.8MB / wheel in windows, ~2.5MB in other OSes)@matthewfeickert @henryiii