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

Compile binary Python wheels for Python 3.12 #399

Merged
merged 9 commits into from
Dec 20, 2023

Conversation

janden
Copy link
Collaborator

@janden janden commented Dec 18, 2023

Updates the code to use importlib instead of imp to make the library compatible with Python 3.12. Also updates the wheel building scripts to build wheels for Python 3.12 and test them.

@janden janden force-pushed the build_py312_wheels branch 2 times, most recently from 977c210 to a0fca04 Compare December 18, 2023 21:04
@janden janden requested a review from lu1and10 December 18, 2023 22:17
Copy link
Member

@lu1and10 lu1and10 left a comment

Choose a reason for hiding this comment

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

@janden Thanks, look great! Just have one question, I saw you committed a dummy extension in cufinufft setup.py and removed that dummy file extension .c file. Does it mean for cufinufft setup.py you don't have this issue commented in the following link?

# For certain platforms (e.g. Ubuntu 20.04), we need to create a dummy source

@janden
Copy link
Collaborator Author

janden commented Dec 19, 2023

I saw you committed a dummy extension in cufinufft setup.py and removed that dummy file extension .c file.

Yes I tried this approach in the other PR (#393) but gave up for now. The reason is that since we include CUDA headers (in order to access the cuFloatComplex and cuDoubleComplex types) in cufinufft.h, we need to make sure those headers are available to Python, which I wanted to avoid. The solution I tried was to convert the CUDA pointers to regular pointers (float complex and double complex in C99 or std::complex<float> and std::complex<double> in C++) but this then meant that I had to perform type casts in other places. So this ended up being a bigger task than I expected. May prepare this in a different PR.

Does it mean for cufinufft setup.py you don't have this issue commented in the following link?

So it does have the same issue. I haven't tried building the wheel on Ubuntu 20.04, but I did encounter it trying to build it on the FI cluster, which from what I understand runs Rocky 8. Installing the binary wheels there should be fine, it's just building them that's problematic due to the linking issue.

At the end of the day, this probably needs a longer discussion in light of #394. Maybe ctypes is not the right fit for us here and we should start considering alternatives.

Use `importlib` instead.
Since these are hardcoded, we have to update them once in a while.
Since `distutils` is removed from the standard library in Python 3.12,
we need to switch to using the vendored version of `distutils` found in
`setuptools._distutils`.

Also, we make sure `setuptools` and friends are upgraded before patching
and separate this from the library installation.
Running some of these commands (notably `pytest`) from `python/finufft`
confuses the interpreter as it tries to look for `finufft.finufftc` in
the source directory (`python/finufft/finufft`) instead of
`site-packages`. This behavior seems to be related to replacing `imp`
with `importlib`. At any rate, the solution is to stay in the repo root
and run all the commands from there, prefixing with `python/finufft`
when necessary.
@janden janden force-pushed the build_py312_wheels branch from fa15e74 to 6716185 Compare December 19, 2023 22:56
@janden janden merged commit 9ba0355 into flatironinstitute:master Dec 20, 2023
8 checks passed
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.

2 participants