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.

Sign up for GitHub

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

Jump to bottom

Remove reliance on extension modules #394

Closed
wants to merge 9 commits into from
Closed

Remove reliance on extension modules #394

wants to merge 9 commits into from

Conversation

blackwer
Copy link
Member

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

janden and others added 5 commits December 12, 2023 06:23
@janden
Fix CUDA library search in cufinufft setup
25e9e1f 
Need to specify the path to the built library since the user may not
have specified `LD_LIBRARY_PATH`.
@janden
Craete dummy source when building cufinufft C ext
b314ad8 
Similarly to what we do for finufft. The issue is that the default gcc
config on certain OSes (Ubuntu 20.04, Rocky 8), libraries are not linked
unless their symbols are explicitly referenced in the source. Since we
have no source, no libraries are linked. The solution is to create a
dummy source that just calls a single function
`cufinufft_default_opts`.
@janden
Fix URL for cufinufft package
2489a02
@janden
Move cufinufftc C extension into cufinufft package
b723d7f 
Instead of living in the site-packages root, we put this in the
`cufinufft` package, similarly to `finufft.
@blackwer
cufinufft: remove setup.py reliance on ext_modules
c4fd0ec 
Just copies the shared object directly into the python package
@blackwer blackwer requested a review from janden December 12, 2023 16:22
@blackwer blackwer assigned blackwer and unassigned blackwer Dec 12, 2023
@blackwer blackwer marked this pull request as draft December 12, 2023 16:22
@blackwer blackwer force-pushed the no-ext-modules branch 2 times, most recently from 95cd95a to ab69c11 Compare December 12, 2023 17:18
@blackwer blackwer marked this pull request as ready for review December 12, 2023 17:25
@blackwer blackwer mentioned this pull request Dec 12, 2023
Merged
janden
janden approved these changes Dec 12, 2023
View reviewed changes
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.

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?

@blackwer
Copy link
Member Author

blackwer commented Dec 12, 2023 via email

@janden
Copy link
Collaborator

janden commented Dec 12, 2023

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!

@blackwer
Copy link
Member Author

blackwer commented Dec 12, 2023 via email

@blackwer
Copy link
Member Author

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.

@janden
Copy link
Collaborator

janden commented Dec 13, 2023

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.

@lu1and10
Copy link
Member

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.

@lu1and10
Copy link
Member

lu1and10 commented Dec 13, 2023 •
edited
Loading

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..

@janden
Copy link
Collaborator

janden commented Dec 13, 2023

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.

@blackwer blackwer marked this pull request as draft December 13, 2023 20:11
@janden
Copy link
Collaborator

janden commented Dec 13, 2023

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)?

@blackwer
Copy link
Member Author

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.

@janden janden mentioned this pull request Dec 19, 2023
Merged
@lu1and10 lu1and10 mentioned this pull request Dec 22, 2023
Closed
@ahbarnett
Copy link
Collaborator

This will get taken care of by #429

@ahbarnett
Copy link
Collaborator

closing this 2023 PR since superceded by #429 @janden @blackwer @lu1and10 @DiamonDinoia

@ahbarnett ahbarnett closed this Jun 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@janden janden janden approved these changes

Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@blackwer @janden @lu1and10 @ahbarnett