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

[BUG] Build dependency that is NOT a runtime compulsory dependency #6189

Open
gabrielfougeron opened this issue May 8, 2024 · 7 comments
Open

Comments

@gabrielfougeron
Copy link
Contributor

Describe the bug

Hi,

I'm developing a package with a cython module fir scientific computation. A large part of the cpu time is spend doind ffts, so I've included the possibility to use pyfftw directly instead of numpy or scipy.
Let me stress this point: pyfftw is a compulsory build dependency, but only an optional runtime dependency of this project.

in the *.pyx file, the corresponding imports are protected by a try block:

try:
    import pyfftw as p_pyfftw   
    cimport pyfftw
    PYFFTW_AVAILABLE = True
except:
    PYFFTW_AVAILABLE = False

Compilation and runtime tests when pyfftw is installed go as expected.
When it is not, I get a runtime error: ModuleNotFoundError: No module named 'pyfftw'.

Code to reproduce the behaviour:

# example code

Expected behaviour

No ModuleNotFoundError if the module is not available at runtime.

OS

WSL2 Ubuntu 20.04

Python version

3.11.2

Cython version

3.0.10

Additional context

I doubt this is relevant, but I'm using a forked version of pyfftw that is not yet merged in the main project.
The point of the PR is to allow calls to fftw from cython in a nogil environment.
Cf pyFFTW/pyFFTW#375 for the pull request.
This version can be installed with the following command:

pip install pyfftw@git+ssh://git@github.com/gabrielfougeron/pyFFTW.git@84e58bdff277bb654b77df1fd20febfbbd1e5dd2
@da-woods
Copy link
Contributor

da-woods commented May 9, 2024

This would be pretty hard the way we currently handle cimports.

I don't think it would be completely impossible - ultimately what a cimport does is initializes a bunch of pointers to functions and objects - but it'd be a fairly big change. There's currently a lot of assumptions that we never have to check these pointers, and that cimports always happen first.

So it's currently expected behaviour that this doesn't work, and that probably isn't likely to change soon.

@gabrielfougeron
Copy link
Contributor Author

OK, got it 👍.

Do you then know of a way I could setup the build process to compile two different versions. One where the optional module is included, one where it its not.

@da-woods
Copy link
Contributor

da-woods commented May 9, 2024

Do you then know of a way I could setup the build process to compile two different versions. One where the optional module is included, one where it its not.

  1. Use the simple "tempita" templating system that's built into Cython. It's largely designed for our own use although we've been meaning to expose more publicly someday - but Scipy and Pandas both use it for this sort of purpose. You'll have to do something custom to get them into your build process though but Pandas is probably a good place to copy. A few links to relevant files there:
    https://github.com/pandas-dev/pandas/blob/1df457fb530924e474f3c4e50c7659dc062a9ae3/generate_pxi.py#L7,
    https://github.com/pandas-dev/pandas/blob/1df457fb530924e474f3c4e50c7659dc062a9ae3/meson.build#L17,
    https://github.com/pandas-dev/pandas/blob/1df457fb530924e474f3c4e50c7659dc062a9ae3/setup.py#L93
  2. You could use the "Conditional Compilation" feature of the Cython language (https://cython.readthedocs.io/en/latest/src/userguide/language_basics.html#conditional-compilation). That's deprecated in that we intend to remove it at some point in the future, but not before we've got a well-tested and functional replacement (which isn't yet).

Other different approaches to the problem that you could consider:

  • See if you can isolate use of PyFFTW to a single Cython module with a public interface consisting only of Python importables (e.g. not having to call any cdef functions. Then you could just import that module and you wouldn't have to cimport it.
  • Inheritance, where you have a cdef class that implements the public interface (and optionally the fallback implementation), and a derived cdef class in a separate file that implements the PyFFTW version. Since you only use it through the base class interface you only need to cimport the base file, and you can plain import the derived one.
  • Something involving getting access to the FFTW functions via PyCapsules carrying function pointers. I don't really recommend this, but again, it lets you pass them across a purely Python interface.

@gabrielfougeron
Copy link
Contributor Author

Thank you so much @da-woods. I'll look into your suggestions :-). Should I close the issue ?

@da-woods
Copy link
Contributor

da-woods commented May 9, 2024

Should I close the issue ?

Leave it - I think there's a valid feature request there. I suspect it's unlikely but it's not something I'd given any thought to before so maybe there's a better approach I've not considered.

@scoder
Copy link
Contributor

scoder commented May 10, 2024

I second the proposal to extract the optional part into a separate module. You mentioned that you can use NumPy and SciPy instead, so there's apparently some functionality that you could extract and call into in both variants. You can then still try-import the two modules from your main module and expose the preferred main entry function there, so that users don't see the difference and don't need to care about the internal structure themselves.

@gabrielfougeron
Copy link
Contributor Author

In the end, I was a bit shy about using Tempita option because of the lack of documentation. I opted for a homemade (ugly) code generation option in my setup.py that looks like this:

# Special rule for the optional run dependency PyFFTW, disabled in pyodide
cython_extnames.append("choreo.cython.optional_pyfftw")
cython_safemath_needed.append(False)

include_pyfftw = not("PYODIDE" in os.environ)
cython_filenames.append(f"choreo.cython.optional_pyfftw_{include_pyfftw}".replace('.','/') + src_ext)

if include_pyfftw:
    pyfftw_pxd_str = """
cimport pyfftw
import pyfftw as p_pyfftw   
cdef bint PYFFTW_AVAILABLE
    """
else:
    pyfftw_pxd_str = """
cimport choreo.cython.pyfftw_fake as pyfftw
import choreo.cython.pyfftw_fake as p_pyfftw   
cdef bint PYFFTW_AVAILABLE
    """
with open("choreo/cython/optional_pyfftw.pxd", "w") as text_file:
    text_file.write(pyfftw_pxd_str)

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

No branches or pull requests

3 participants