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

Update Python interface to fall back on ctypes.util.find_library #435

Closed
wants to merge 1 commit into from

Conversation

dfm
Copy link
Contributor

@dfm dfm commented May 2, 2024

Since the default dynamic library suffix is platform dependent, loading libfinufft.so directly with ctypes isn't very portable. This PR uses ctypes.util.find_library as a fallback to discover system libraries if libfinufft.so isn't found. This won't change the behavior for most users, but does allow users on macOS (for example) to use an installed version of libfinufft.

@ahbarnett ahbarnett requested a review from janden May 7, 2024 20:25
@DiamonDinoia
Copy link
Collaborator

I'll have a look

@ahbarnett
Copy link
Collaborator

@DiamonDinoia

@ahbarnett
Copy link
Collaborator

Thanks! @DiamonDinoia will incorporate this into #429

@ahbarnett
Copy link
Collaborator

maybe this should be brought in before we make any bigger shift of python wrapping, @janden ?

@DiamonDinoia
Copy link
Collaborator

This should be fixed in master after #429 was merged.

@ahbarnett
Copy link
Collaborator

ahbarnett commented Jul 17, 2024 via email

@janden
Copy link
Collaborator

janden commented Jul 23, 2024

So this is actually incorporated for the FINUFFT (it was merged as part of #429). See

libname = find_library('finufft')
.

However, we should discuss today whether this behavior still makes sense. When a user does pip install for (even from source), the library will be compiled and the .so/.dll/.dylib will be installed along with the Python source. So under no circumstances should the pre-packaged library be missing. As such, there is not really a place for a system fallback.

@dfm
Copy link
Contributor Author

dfm commented Jul 23, 2024 via email

@DiamonDinoia
Copy link
Collaborator

DiamonDinoia commented Jul 23, 2024

For conda-forge (conda-forge/finufft-split-feedstock) the library and Python bindings are built separately so this is crucial for that. We can keep patching if strictly necessary.

On Tue, Jul 23, 2024 at 12:02 PM Joakim Andén @.> wrote: So this is actually incorporated for the FINUFFT (it was merged as part of #429 <#429>). See

libname = find_library('finufft')
. However, we should discuss today whether this behavior still makes sense. When a user does pip install for (even from source), the library will be compiled and the .so/.dll/.dylib will be installed along with the Python source. So under no circumstances should the pre-packaged library be missing. As such, there is not really a place for a system fallback. — Reply to this email directly, view it on GitHub <#435 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACVQSQWW5TX4AALRAPCTZDZNZ5B3AVCNFSM6AAAAABHEMAC5GVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENBVGY2DGNBUGE . You are receiving this because you authored the thread.Message ID: @.>

#429 does the following:

  1. search for a .so inside the _finufft.py directory, if found use that
  2. otherwise search for finufft.so in the system LIBRARY_PATH and if found use it
  3. fail with a meaningful error.

I think this works with conda-forge?

@dfm
Copy link
Contributor Author

dfm commented Jul 23, 2024

@DiamonDinoia Yes that's all good. @janden was suggesting step 2 should be removed, and I was just asking that it not be.

@DiamonDinoia
Copy link
Collaborator

Hi, thanks for pointing it out. We do not plan on removing step 2.

@janden
Copy link
Collaborator

janden commented Jul 23, 2024

Ok we can keep that then. Out of curiosity, do you manually remove the included .so from the built wheel then in conda-forge? Otherwise I don't see how we would ever hit this fallback.

@dfm
Copy link
Contributor Author

dfm commented Jul 23, 2024

On conda-forge the build is patched so that the Python extension never compiles the core libraries since those are packaged separately.

@janden
Copy link
Collaborator

janden commented Jul 23, 2024

Ok got it.

At any rate, since this is already included in #429, I'm closing this one.

@janden janden closed this Jul 23, 2024
@DiamonDinoia
Copy link
Collaborator

DiamonDinoia commented Aug 14, 2024

@dfm v2.3.0-rc1 is out with these fixes. Could you give us feedback on conda-forge? We switched to pyproject.toml which may cause problems.

@dfm
Copy link
Contributor Author

dfm commented Aug 17, 2024

Thanks for the heads up @DiamonDinoia! I probably won't have a chance to update the conda-forge recipe until after the final version is out, but I'm sure it'll be no problem to get it running!

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.

4 participants