Repository navigation
Conversation
90578c9 to
647d164
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new Dask interface for the mkl_fft package, providing a drop-in replacement for the dask.fft module that leverages Intel MKL's optimized FFT implementations.
- Implements
mkl_fft.interfaces.dask_fftmodule with all FFT functions wrapped using Dask'sfft_wrap - Adds comprehensive test suite for the new Dask interface functionality
- Updates build configuration and documentation to support the new interface
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pyproject.toml | Adds dask_interface optional dependency and includes dask in test dependencies |
| mkl_fft/interfaces/dask_fft.py | New module implementing Dask FFT interface by wrapping numpy_fft functions |
| mkl_fft/tests/third_party/dask/test_dask_fft.py | Comprehensive test suite for Dask FFT functionality |
| mkl_fft/tests/test_interfaces.py | Updates interface tests to include Dask FFT |
| mkl_fft/interfaces/init.py | Conditionally imports dask_fft module when Dask is available |
| mkl_fft/interfaces/README.md | Adds documentation for the new Dask interface |
| conda-recipe/meta.yaml | Updates conda recipe to include dask test dependency and import test |
| conda-recipe-cf/meta.yaml | Updates conda-forge recipe similarly |
| CHANGELOG.md | Documents the new Dask interface feature |
| .github/workflows/*.yml | Updates CI workflows to include dask as test dependency |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
jharlow-intel
left a comment
There was a problem hiding this comment.
some conflicts need to be resolved.
It's okay that the 3.14 internal CI is broken for now, though
476d4b6 to
d731951
Compare
|
@antonwolfy @jharlow-intel |
d731951 to
0389820
Compare
|
changes in the workflows made it more convenient to put this on top of the conda-forge recipe updates, this can go in after that |
9fea4e6 to
c46a837
Compare
c46a837 to
3426830
Compare
|
@jharlow-intel @antonwolfy ready for review |
jharlow-intel
left a comment
There was a problem hiding this comment.
Thanks for keeping this going, and sorry it sat so long! The interface itself is close: wrapping mkl_fft's NumPy functions with dask's own fft_wrap keeps the chunking checks and signatures identical to dask.array.fft, and the tests adapted from dask's suite cover chunked-axis errors plus n/s/axes/norm. The main thing is that master has moved a lot since April, so it needs another rebase; a few smaller things inline.
- This needs another rebase before it can merge: master has moved about 330 commits since April, and 7 files now conflict (
pyproject.toml,mkl_fft/interfaces/__init__.py, both conda recipes, and the three workflows). When you redointerfaces/__init__.py, it'd be great to follow master's newer pattern there (appending to__all__and deleting the probe import).
| import contextlib | ||
| from itertools import combinations_with_replacement | ||
|
|
||
| import dask |
There was a problem hiding this comment.
Could this use pytest.importorskip("dask.array") instead of a bare import? Right now running the installed suite (pytest --pyargs mkl_fft) without dask fails at collection instead of skipping, and test_dask_fftwrap in test_interfaces.py touches mfi.dask_fft unconditionally, so it fails the same way. It might also be worth avoiding the private dask helpers (da._array_expr_enabled, dask.array.numpy_compat), since those can move between dask releases.
| import mkl_fft.interfaces.dask_fft as dask_fft | ||
|
|
||
| requires_dask_2024_8_2 = pytest.mark.skipif( | ||
| dask.__version__ < "2024.8.2", |
There was a problem hiding this comment.
This compares version strings, so "2024.10.0" < "2024.8.2" is True and the norm tests get skipped on dask 2024.10 through 2024.12. packaging.version.Version(dask.__version__) would fix it. And since norm needs dask >= 2024.8.2, should the dask_interface extra pin dask>=2024.8.2 too?
| except ImportError: | ||
| pass | ||
| else: | ||
| from . import dask_fft |
There was a problem hiding this comment.
Since import mkl_fft imports mkl_fft.interfaces eagerly, anyone with dask installed now pulls in dask.array on every import mkl_fft, and a broken dask install would make import mkl_fft itself fail. Would a lazy import (a module-level __getattr__ for dask_fft) work here? I know scipy_fft is eager today, but dask.array is a heavier import.
|
|
||
| ### Added | ||
| * Enabled support of Python 3.13 [gh-164](https://github.com/IntelPython/mkl_fft/pull/164) | ||
| * Added a new interface for FFT module of Dask accessible through `mkl_fft.interfaces.dask_fft` [gh-214](https://github.com/IntelPython/mkl_fft/pull/214) |
There was a problem hiding this comment.
Heads-up: this entry ended up in an old released section (next to the Python 3.13 entry, gh-164). Could you move it to the current dev section when you rebase?
address #5