Add PocketFFT support #60976
Closed
Add PocketFFT support #60976
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
💊 CI failures summary and remediationsAs of commit 94b89e3 (more details on the Dr. CI page and at hud.pytorch.org/pr/60976):
🕵️ 1 new failure recognized by patternsThe following CI failures do not appear to be due to upstream breakages:
|
|
@malfet has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
e2efbc5 to
59ed84f
Compare
Codecov Report
@@ Coverage Diff @@
## master #60976 +/- ##
==========================================
- Coverage 80.60% 75.72% -4.88%
==========================================
Files 1879 2062 +183
Lines 202892 209076 +6184
==========================================
- Hits 163543 158333 -5210
- Misses 39349 50743 +11394 |
peterbell10
requested changes
Jun 30, 2021
Needed on platforms, that do not have MKL, such as aarch64 and M1 - Add `AT_POCKETFFT_ENABLED()` to Config.h.in - Introduce torch._C.has_spectral that is true if PyTorch was compiled with either MKL or PocketFFT - Modify spectral test to use @skipCPUIfNoFFT instead of @skipCPUIfNoMKL
72780df to
94b89e3
Compare
|
Rebase PR on top of #60313 |
|
@malfet has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
peterbell10
approved these changes
Jun 30, 2021
Comment on lines
+222
to
+225
| find_path(POCKETFFT_INCLUDE_DIR NAMES pocketfft_hdronly.h | ||
| PATHS /usr/local/include | ||
| PATHS $ENV{POCKETFFT_HOME} | ||
| ) |
There was a problem hiding this comment.
Curious, why not just include in third_party?
There was a problem hiding this comment.
Because it is not hosted on github, which might be hard to sync up if used frequently.
But might create a mirror on github and add to 3rd party later on
|
|
||
| } // anonymous namespace | ||
|
|
||
| Tensor _fft_c2r_mkl(const Tensor& self, IntArrayRef dim, int64_t normalization, int64_t last_dim_size) { |
There was a problem hiding this comment.
Is it worth changing these to _cpu instead of _mkl?
There was a problem hiding this comment.
Plan to do it in followup PR (and actually move parts of the code away from mkl/SpectralOps.cpp folder)
mruberry
reviewed
Jun 30, 2021
| REGISTER_NO_CPU_DISPATCH(fft_fill_with_conjugate_symmetry_stub, fft_fill_with_conjugate_symmetry_fn); | ||
|
|
||
| Tensor _fft_c2r_mkl(const Tensor& self, IntArrayRef dim, int64_t normalization, int64_t last_dim_size) { | ||
| AT_ERROR("fft: ATen not compiled with FFT support"); |
There was a problem hiding this comment.
Note for a follow-up: AT_ERROR is deprecated use TORCH_CHECK(false, ...)
mruberry
reviewed
Jun 30, 2021
| @@ -1094,6 +1094,11 @@ def skipCPUIfNoLapack(fn): | |||
| return skipCPUIf(not torch._C.has_lapack, "PyTorch compiled without Lapack")(fn) | |||
|
|
|||
|
|
|||
| # Skips a test on CPU if FFT is not available. | |||
| def skipCPUIfNoFFT(fn): | |||
There was a problem hiding this comment.
This name is a little odd because "NoFFT" suggests there's no fft available on any device type. It would more accurately be "skipCPUIfNoCPUFFT" or "skipCPUIfNoMKLorPocket" or something
mruberry
approved these changes
Jun 30, 2021
facebook-github-bot
pushed a commit
that referenced
this pull request
Jul 1, 2021
Summary: The new tag should fix the "Missing <omp.h>" error message on clang-tidy runs. Pull Request resolved: #61115 Test Plan: Ran the clang-tidy job using the diff from #60976. Expected Output: There should be no clang diagnostic errors. Reviewed By: walterddr Differential Revision: D29516845 Pulled By: 1ntEgr8 fbshipit-source-id: 554229904db67eb7a7b93b3def434b30de6a43b0
|
Hey there! I just tried to use this functionality by running a PyTorch lite model that uses However running the model gives: Is PocketFFT available in the nightly builds? Or is some further configuration needed? |
|
So it looks to me that I need to build from source with |
|
Update: got it working! A gamechanger for us, many thanks! Getting |
HI @erksch, do you have any pointers on getting this working? Facing the exact same error at the moment. Have tried
To generate the AAR after that I'm running: I'm still crashing in Android on: So obviously I'm not disabling MKL correctly, but I'm not sure where I'm going wrong. |
|
@another-dave What helped me was to put some print statements in # --- [ PocketFFT
set(AT_POCKETFFT_ENABLED 0)
message(STATUS "Checking POCKETFFT")
if(NOT MKL_FOUND)
message(STATUS "POCKETFFT No MKL")
find_path(POCKETFFT_INCLUDE_DIR NAMES pocketfft_hdronly.h
PATHS /usr/local/include
PATHS $ENV{POCKETFFT_HOME}
)
set(POCKETFFT_INCLUDE_DIR /usr/local/include)
message(STATUS "POCKETFFT (${POCKETFFT_INCLUDE_DIR})")
if(POCKETFFT_INCLUDE_DIR)
message(STATUS "Enabling POCKETFFT")
set(AT_POCKETFFT_ENABLED 1)
endif()
endif()Then, when running CMake you should see if not there is still something wrong And also as you may see I added an explicit to the location where I had the header file. The |
Hi @erksch, thanks very much for this!! Got it sorted now 🎉 Realised from following when you said that it was still detecting MKL for some reason. I'm sure there's a more elegant way to flag that off, but if it helps anyone else, I just commented-out lines 381-394 of And I needed to adjust my After that I ran: And got the AAR built correctly. (As per this comment, I had to downgrade Android NDK to Thanks for the pointers & for the speedy reply! best |
|
Awesome! @another-dave |
|
Do you think the PocketFFT configuration should be standard for mobile? In that case, would be really helpful if the nightly mobile builds would already include this (as you already did for MacOS M1). |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Needed on platforms, that do not have MKL, such as aarch64 and M1
AT_POCKETFFT_ENABLED()to Config.h.inShare implementation of
_outfunctions as well as fft_fill_with_conjugate_symmetry_stub between MKL and PocketFFT implementationsFixes #41592