Skip to content
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

[DRAFT] Using pyproject.toml and Cmake to build wheels #429

Merged
merged 68 commits into from
Jul 16, 2024

Conversation

DiamonDinoia
Copy link
Collaborator

Hi @lu1and10,

I updated the python build system, changes:

  1. using Cmake for python
  2. using pyproject.toml

Please, have a look!

Marco

@DiamonDinoia DiamonDinoia changed the title Cpython [DRAFT] Using pyproject.toml and Cmake to build wheels Apr 17, 2024
@ahbarnett ahbarnett requested review from janden and lu1and10 April 23, 2024 19:48
Copy link
Collaborator

@janden janden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @DiamonDinoia for putting this together. I've been reading up a bit on skbuild and cmake to try to understand what's going on here, but I still have a few questions.

One major one is how to structure this together with the two Python packages (finufft and cufinufft). Should we move this pyproject.toml into python/finufft and make another one in python/cufinufft. I'm guessing this will mean changing some paths and setting cmake.source_dir = "../.." or am I out of my depth here?

CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated
# Setuptools-style build caching in a local directory
build-dir = "build/{wheel_tag}"
# Build stable ABI wheels for CPython 3.8+
wheel.py-api = "cp38"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this? Shouldn't we just leave it to the user (or the CI pipeline) to specify which ABI they want?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure it is necessary. Maybe for cross platform builds? The wheel is built using python 3.10 but we want it to work on previous versions too

python/finufft/finufft/_finufft.py Outdated Show resolved Hide resolved
python/CMakeLists.txt Outdated Show resolved Hide resolved
pyproject.toml Outdated
minimum-version = "0.4"
cmake.targets = ["finufft"]
cmake.args = ['-DFINUFFT_BUILD_PYTHON=ON', '-DCMAKE_BUILD_TYPE=Release'] # not sure if this is necessary
wheel.packages = ["python/finufft/finufft"]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this know to pick up the libfinufft.so file?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From:

    install(TARGETS finufft LIBRARY DESTINATION finufft)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But doesn't that line tell cmake to install libfinufft.so in the python/finufft directory (not the python/finufft/finufft directory)? I guess it must be doing something else.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So from what I understand, this is what skbuild does, essentially:

  1. Runs cmake and cmake --build
  2. Runs cmake --install to install the targets into some temporary directory (on my system, something like /tmp/tmpn4jdyywf/wheel/platlib).
  3. Copies the package in wheels.packages into that directory
  4. Zips that directory up into a .whl file

So in our case, step 2 will install finufft/libfinufft.so into that temp directory, while step 3 will copy python/finufft/finufft there, which results in all the files living in the right places (so libfinufft.so will be in the same directory as _finufft.py and friends). This means that step 4 will put everything together into the desired wheel.

@DiamonDinoia
Copy link
Collaborator Author

Thanks @DiamonDinoia for putting this together. I've been reading up a bit on skbuild and cmake to try to understand what's going on here, but I still have a few questions.

One major one is how to structure this together with the two Python packages (finufft and cufinufft). Should we move this pyproject.toml into python/finufft and make another one in python/cufinufft. I'm guessing this will mean changing some paths and setting cmake.source_dir = "../.." or am I out of my depth here?

That is a solution. The only alternative I can think of is passing arguments to pip (pip installl . cpu) the only think that worries me of not having pyproject.toml at the top level is that it is not possible to do pip install git+url to install the project. But maybe pip install git+url+folder might work.

Feel free to make changes directly, here.

@janden
Copy link
Collaborator

janden commented May 28, 2024

The only alternative I can think of is passing arguments to pip (pip installl . cpu)

So that would be like an extras-type install (pip install .[cpu] and pip install .[gpu])? Then it's not clear what the default would do (pip install .). Or are you thinking of some other approach here?

the only think that worries me of not having pyproject.toml at the top level is that it is not possible to do pip install git+url to install the project. But maybe pip install git+url+folder might work.

This is an issue with the current setup (pardon the pun) as well. Looks like pip has a way to do this. It would look something like

pip install git+https://github.com/flatironinstitute/finufft#subdirectory=python/finufft

Just tried this locally and it seems to work (although it complains that it can't find libfinufft when compiling the C extension, which is to be expected).

@janden
Copy link
Collaborator

janden commented Jun 13, 2024

Was able to get cibuildwheel to work but needed to rebase on master to get rid of the conflicts (results are here). Was a pretty quick and dirty rebase though, so I want to go through it again here tomorrow more carefully. Just a heads up that you'll have to reset your local branch once I'm done.

@janden
Copy link
Collaborator

janden commented Jun 14, 2024

I've rebased on top of master to remove the conflicts. There's a backup branch called cpython_backup that's on my repo. Let me know if I've broken something here and I'll try to fix it.

@lu1and10
Copy link
Member

I've rebased on top of master to remove the conflicts. There's a backup branch called cpython_backup that's on my repo. Let me know if I've broken something here and I'll try to fix it.

Great, thanks. There is only some change in python/finufft/finufft/_finufft.py breaks the master branch ci for python wheel builder using makefile.
As long as we are retiring the old python wheel builder ci and makes the new python wheel ci work, then we should ignore the old ci failure.

@janden
Copy link
Collaborator

janden commented Jun 18, 2024

As I mentioned during the meeting, the wheel building for windows works… but it currently takes 76 minutes due to the constant rebuilding of FFTW. Some approaches I've tried to reduce this:

  • The official FFTW Windows installation guide. This is what pyfftw uses (see here) wherein prebuilt DLLs are downloaded and then modified using lib.exe that then allows us to link using MSVC. Drawbacks are (i) the prebuilt DLLs are only for FFTW 3.3.5 and (ii) this only includes the vanilla (float, double, and quad) libraries, so no threads or OMP.
  • Installing using MSYS and pacman (as we do in the current Windows wheel building workflow). This gets us FFTW 3.3.10 with threads and OMP. Also scikit-build/cmake seems to be able to find FFTW without a problem, but linking fails with unresolved references to __stack_chk_fail and others. I suspect this has something to do with the DLLs being compiled under mingw/gcc and trying to link to them using MSVC. Maybe there is some way to get these to play nice, but this remains to be seen.

Another option would be to try to compile the latest FFTW from source using MSVC and installing it. This would allow scikit-build/cmake to find it and make use of it without rebuilding for all 10 (5 py versions times {32,64}-bits) wheels. To do this, we need to extract the relevant part of our build pipeline and run it outside (so somehow running cmake/setupFFTW.cmake standalone), which I don't know how to do right now.

@DiamonDinoia
Copy link
Collaborator Author

As I mentioned during the meeting, the wheel building for windows works… but it currently takes 76 minutes due to the constant rebuilding of FFTW. Some approaches I've tried to reduce this:

  • The official FFTW Windows installation guide. This is what pyfftw uses (see here) wherein prebuilt DLLs are downloaded and then modified using lib.exe that then allows us to link using MSVC. Drawbacks are (i) the prebuilt DLLs are only for FFTW 3.3.5 and (ii) this only includes the vanilla (float, double, and quad) libraries, so no threads or OMP.
  • Installing using MSYS and pacman (as we do in the current Windows wheel building workflow). This gets us FFTW 3.3.10 with threads and OMP. Also scikit-build/cmake seems to be able to find FFTW without a problem, but linking fails with unresolved references to __stack_chk_fail and others. I suspect this has something to do with the DLLs being compiled under mingw/gcc and trying to link to them using MSVC. Maybe there is some way to get these to play nice, but this remains to be seen.

Another option would be to try to compile the latest FFTW from source using MSVC and installing it. This would allow scikit-build/cmake to find it and make use of it without rebuilding for all 10 (5 py versions times {32,64}-bits) wheels. To do this, we need to extract the relevant part of our build pipeline and run it outside (so somehow running cmake/setupFFTW.cmake standalone), which I don't know how to do right now.

I could make another Cmakelists in tools that install fftw only and we can make an action that makes that? Alternatively if we do FINUFFT_ENABLE_CPU = FINUFFT_ENABLE_GPU = OFF with everything else off we could add a FINUFFT_INTERNAL_INSTALL_FFTW for developers that just installs fftw. cmake --install would do the trick.

@lu1and10, @janden do you think is sensible? I would personally build FFTW as the pre built might not have all the SIMD instructions enabled.

@lu1and10
Copy link
Member

@janden Actually as the PR you had #403
We may not need to make wheels for all python versions. We can just make one wheel as in PR403 and it should work for all python versions since we only ship libfinufft.so/dylib/dll, it should work cross all python versions.

@janden
Copy link
Collaborator

janden commented Jun 24, 2024

make another Cmakelists in tools that install fftw only and we can make an action that makes that?

Right I think this would make sense. Is it possible to do this in a way that the regular cmake file calls this, however? Would be nice to avoid having this FFTW installation code in two places.

Alternatively if we do FINUFFT_ENABLE_CPU = FINUFFT_ENABLE_GPU = OFF with everything else off we could add a FINUFFT_INTERNAL_INSTALL_FFTW for developers that just installs fftw. ``

Right, but this seems a bit hacky. This would avoid the duplication issue, though.

We may not need to make wheels for all python versions. We can just make one wheel as in PR403 and it should work for all python versions since we only ship libfinufft.so/dylib/dll, it should work cross all python versions.

That's true, but doing the FFTW build in a separate step would also mean that we could cache the result and avoid having to build FFTW each time (similarly to how win and linux just download the library each time instead of building).

@DiamonDinoia
Copy link
Collaborator Author

Right I think this would make sense. Is it possible to do this in a way that the regular cmake file calls this, however? Would be nice to avoid having this FFTW installation code in two places.

What if we install fftw from the original tarball then? Instead of writing our own cmake-lists we could use the one they provide

@janden
Copy link
Collaborator

janden commented Jun 24, 2024

What if we install fftw from the original tarball then? Instead of writing our own cmake-lists we could use the one they provide

Of course. 🤦 This is what CPMAddPackage does under the hood, right? For some reason I was thinking FFTW was using autoconf but now I see they have cmake in there. Will give it a spin.

@DiamonDinoia
Copy link
Collaborator Author

What if we install fftw from the original tarball then? Instead of writing our own cmake-lists we could use the one they provide

Of course. 🤦 This is what CPMAddPackage does under the hood, right? For some reason I was thinking FFTW was using autoconf but now I see they have cmake in there. Will give it a spin.

CPMAddPackage always build from source. One could use CPMFindPackage to use the system one first. I personally build my own way of checking if installed to allow the user to override FindPackage and be able to build it by hand.

@ahbarnett
Copy link
Collaborator

ahbarnett commented Jun 24, 2024 via email

CMakeLists.txt Outdated Show resolved Hide resolved
Invoke-WebRequest -Uri "https://www.fftw.org/fftw-3.3.10.tar.gz" -OutFile "${{ github.workspace }}\fftw.tar.gz"

New-Item -Path "${{ github.workspace }}" -Name "fftw-source" -ItemType Directory
tar --strip-components=1 -C "${{ github.workspace }}\fftw-source" -zxf "${{ github.workspace }}\fftw.tar.gz"
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you could do steps up to here on unix and save the result.

Then you can run cmake install on all OSs and cache the result. Maybe the env variable contains Path/OSs and they can all find the library somewhere.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you could do steps up to here on unix and save the result.

So to have a workflow job that just downloads the FFTW source and saves it somewhere? Sure. But as it is, the FFTW source is only needed by Windows (macOS and Ubuntu can install the library and headers via package managers). Or maybe I'm missing something here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is better to build fftw on all oses by hand as the package manager might ship a not vectorized version. I would like to have the fastest possible fftw bundled in finufft

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. We should be able to copy the recipe here (maybe after we clean up a bit) to ubuntu/macos.

@janden
Copy link
Collaborator

janden commented Jun 24, 2024

what is wrong with cmake/setup_FFTW.cmake ?

This doesn't actually install FFTW, but looks for it on the system and if it doesn't find it downloads the source and “appends” it to the FINUFFT source (if I've understood things correctly). As it is, we can't just run cmake cmake/setupFFTW.cmake as a standalone script.

After some wrangling with non-escaped backslashes, I've been able to get the Windows workflow to run. Although it looks like we have another conflict with master here that's preventing it from running on CI.

@janden
Copy link
Collaborator

janden commented Jun 24, 2024

Tried rebasing to resolve the conflict, but it seems a bit nasty. There's lots of thing happening around line 21 of CMakeLists.txt that needs some care. @DiamonDinoia From what I understand the changes in master are mostly from your PR, so maybe you can look at how to resolve this?

@DiamonDinoia
Copy link
Collaborator Author

Tried rebasing to resolve the conflict, but it seems a bit nasty. There's lots of thing happening around line 21 of CMakeLists.txt that needs some care. @DiamonDinoia From what I understand the changes in master are mostly from your PR, so maybe you can look at how to resolve this?

Have a look. It should be okay now.

@janden
Copy link
Collaborator

janden commented Jun 24, 2024

Have a look. It should be okay now.

Ok that makes sense. Just wanted to be sure.

@janden
Copy link
Collaborator

janden commented Jul 15, 2024

Just did a force push after rebasing on top of latest master.

@janden
Copy link
Collaborator

janden commented Jul 16, 2024

So this PR is now ready for merging. I've moved the pyproject.toml file into python/finufft instead of leaving it in the root. I also deleted the old setup.py that was there along with the old make-based workflows for wheel building. The only thing left is to decide what to do with the python_cmake workflow. Most of it should already be baked in to the python_build_wheels workflow, but perhaps I've missed some thing here.

@DiamonDinoia DiamonDinoia merged commit 82715d4 into flatironinstitute:master Jul 16, 2024
34 checks passed
@DiamonDinoia DiamonDinoia deleted the cpython branch July 16, 2024 21:21
@DiamonDinoia DiamonDinoia mentioned this pull request Jul 17, 2024
8 tasks
@DiamonDinoia DiamonDinoia added this to the 3.0 milestone Jul 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants