finufft
finufft copied to clipboard
flatironinstitute
Remove reliance on extension modules
Following my frustrations at #389, I'm drafting this PR to just remove any extra shared library generation at install time for the python bindings. I'm basing this PR off of #393
already done in this PR
On Tue, Dec 12, 2023 at 4:37 PM Joakim Andén @.***> wrote:
@.**** approved this pull request.
This looks good. Just feels a bit strange to include shared libraries as “data” files but seems to work.
Should we give finufft the same treatment, then?
— Reply to this email directly, view it on GitHub https://github.com/flatironinstitute/finufft/pull/394#pullrequestreview-1778477481, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACY7UWJV74WO7RW72VGUZDYJDFAVAVCNFSM6AAAAABARX3VWSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTONZYGQ3TONBYGE . You are receiving this because you were assigned.Message ID: @.***>
already done in this PR
Weird. Somehow missed it. Anyhow saw it now and it looks good.
Tried making the binary wheels using the docker image for cufinufft. Everything looks good there too. One interesting consequence is that we're now independent of the CPython ABI(?). In other words, we only need to ship a single binary wheel cufinufft-2.2.0.dev0-py3-none-manylinux_2_17_x86_64.manylinux2014_x86_64.whl. So that's a +1 for ctypes!
you were blinded by all of my shiny force pushes
On Tue, Dec 12, 2023 at 5:36 PM Joakim Andén @.***> wrote:
already done in this PR
Weird. Somehow missed it. Anyhow saw it now and it looks good.
Tried making the binary wheels using the docker image for cufinufft. Everything looks good there too. One interesting consequence is that we're now independent of the CPython ABI(?). In other words, we only need to ship a single binary wheel cufinufft-2.2.0.dev0-py3-none-manylinux_2_17_x86_64.manylinux2014_x86_64.whl. So that's a +1 for ctypes!
— Reply to this email directly, view it on GitHub https://github.com/flatironinstitute/finufft/pull/394#issuecomment-1852922085, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACY7UV4PLQLQHOPCNY2R7DYJDL5RAVCNFSM6AAAAABARX3VWSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNJSHEZDEMBYGU . You are receiving this because you were assigned.Message ID: @.***>
This still needs a bit of hardening for macs. In some instances macs get a dylib and others a so. Can't rely solely on the platform.
Maybe related, but the wheel names are the same (finufft-2.2.0.dev0-py3-none-any.whl) for both win and mac, which I assume would cause problems.
Maybe related, but the wheel names are the same (
finufft-2.2.0.dev0-py3-none-any.whl) for both win and mac, which I assume would cause problems.
The python version is not encoded in name either. The master branch ci generates finufft-2.2.0.dev0-cp36-cp36m-manylinux_2_17_x86_64.manylinux2014_x86_64.whl finufft-2.2.0.dev0-cp38-cp38m-manylinux_2_17_x86_64.manylinux2014_x86_64.whl etc after changing, it generates finufft-2.2.0.dev0-py3-none-any.whl for all python versions. https://github.com/flatironinstitute/finufft/actions/runs/7185113178/job/19567673308#step:6:407 Not sure how to fix.
Maybe related, but the wheel names are the same (
finufft-2.2.0.dev0-py3-none-any.whl) for both win and mac, which I assume would cause problems.The python version is not encoded in name either. The master branch ci generates finufft-2.2.0.dev0-cp36-cp36m-manylinux_2_17_x86_64.manylinux2014_x86_64.whl finufft-2.2.0.dev0-cp38-cp38m-manylinux_2_17_x86_64.manylinux2014_x86_64.whl etc after changing, it generates finufft-2.2.0.dev0-py3-none-any.whl for all python versions. https://github.com/flatironinstitute/finufft/actions/runs/7185113178/job/19567673308#step:6:407 Not sure how to fix.
maybe it's fine, building extension module is python version dependent while ship with .so as data does not require python version? the .so, .dylib or .dll file may be os dependent, maybe there is a way to tell package_data, it's os dependent so the wheel name will be named accordingly..
maybe it's fine, building extension module is python version dependent while ship with .so as data does not require python version?
Right. Since we don't depend on the Python ABI, it makes sense that the wheel should be independent of Python version.
the .so, .dylib or .dll file may be os dependent, maybe there is a way to tell
package_data, it's os dependent so the wheel name will be named accordingly..
Yes this is an issue. Right now, Linux has a different tag (manylinux) but macOS and Windows have the same names.
I'm a little worried this may introduce new bugs that we don't completely have a handle on relatively close to a release. At the end of the day, the old way (C extension with rpath and friends) works except for certain corner cases and we expect most people to install the binary wheels anyhow where this wouldn't be an issue. Besides, wouldn't some of these issues (compiler incompatibility, etc.) be resolved once we move to scikit-build (integrating the library building inside the pip install process)?
I'm a little worried this may introduce new bugs that we don't completely have a handle on relatively close to a release. At the end of the day, the old way (C extension with rpath and friends) works except for certain corner cases and we expect most people to install the binary wheels anyhow where this wouldn't be an issue. Besides, wouldn't some of these issues (compiler incompatibility, etc.) be resolved once we move to scikit-build (integrating the library building inside the pip install process)?
I'm not sure I'd recommend scikit-build here necessarily. This solution is showing a fair amount of benefits so far. We also don't need to merge it before release. I was having issues with the ubuntu cython fix, so I opted to try a different method as a trial to completely circumvent it. Currently this PR is more of a playground, but I'm relatively confident as of right now it's a good long-term solution.
That said, I think I can get a handle on the bugs (I can actually test the binaries on a lot of platforms directly). If the wheels work, which is what this is mostly designed to deal with, then I don't see much of an issue with merging it.
closing this 2023 PR since superceded by #429 @janden @blackwer @lu1and10 @DiamonDinoia