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

ENH: uarray based backend support for scipy.fft #10383

Merged
merged 30 commits into from Jul 17, 2019

Conversation

@peterbell10
Copy link
Member

commented Jul 1, 2019

Related to #10204

This uses uarray to implement a backend system for the transform functions in scipy.fft.

A simple example:

import scipy.fft
scipy.fft.fft([1])  # Calls _pocketfft.fft

from scipy.fft._debug_backends import NumPyBackend
scipy.fft.set_global_backend(NumPyBackend)
scipy.fft.fft([1])  # Calls np.fft.fft

with scipy.fft.set_backend('scipy'):
    scipy.fft.fft([1])  # Calls _pocketfft.fft

Currently there is a ~4-5 us overhead per function call (on my machine). That means a length 200 fft takes about twice as long. I've been working to bring that down in Quansight-Labs/uarray#170 where it's down to ~700 ns.

Quansight-Labs/uarray#170 has been merged meaning the protocol is implemented entirely in the C API. The overhead is down to ~700ns as measured by me with similar timings observed by @grlee in Quansight-Labs/uarray#156 (comment).

To put this into perspective, a length 256 FFT takes ~15% more time with this.

@peterbell10 peterbell10 force-pushed the peterbell10:uarray-backend branch from 195f0d6 to f66111f Jul 1, 2019

@andyfaff

This comment has been minimized.

Copy link
Contributor

commented Jul 1, 2019

Perhaps the vendored code would be better off in scipy._lib, that's usually where it goes.

@hameerabbasi

This comment has been minimized.

Copy link
Contributor

commented Jul 2, 2019

I must state here: uarray, in order to manage thread-specific context, uses the contextvars package. This is in the standard library on Py37+ and needs a PyPI package for Py35-Py36.

In order to circumvent this, we either need to use a similar construct in C++ (further bringing down the overhead), or accept this, or use Thread-local storage, which is finicky in Python, but good in C++. https://stackoverflow.com/questions/33088614/best-practices-syntax-for-implementing-a-contextmanager-in-c

@peterbell10

This comment has been minimized.

Copy link
Member Author

commented Jul 2, 2019

I'm working on a solution that uses the PyContextVar c-api for python 3.7 and thread_local for earlier versions.

@hameerabbasi

This comment has been minimized.

Copy link
Contributor

commented Jul 2, 2019

It might be better to use a graph structure plus C++11 thread-local storage for optimal efficiency. 😄

I can take a stab at that, if you're okay with it.

@peterbell10

This comment has been minimized.

Copy link
Member Author

commented Jul 2, 2019

Is thread_local good enough? I assumed there's some edge cases where they don't work since the python c api includes both.

@hameerabbasi

This comment has been minimized.

Copy link
Contributor

commented Jul 2, 2019

The problem with thread_local (I'm not too sure about contextvars) is that it's initialized/copied at the time of access, not at the time of thread creation. C++ has the same issue, except that the standard doesn't specify.

So, in practice, to implement contextvars, you'd need to do a wait on the main thread, and a join on the started thread after creating a thread to be bug-free.

This is highly unlikely to happen with uarray (someone spawning a thread and setting the backend inside, and setting a backend in the main thread after it's told to spawn the child), so in practice I think it should be fine, in either language. My desire of going down to C++ was to minimize overhead.

@peterbell10

This comment has been minimized.

Copy link
Member Author

commented Jul 2, 2019

I though the issue was something to do with coroutines or async. Something not stack-based.

someone spawning a thread and setting the backend inside, and setting a backend in the main thread after it's told to spawn the child),

In this scenario is the child supposed to inherit the contextvar's value from the parent?

@hameerabbasi

This comment has been minimized.

Copy link
Contributor

commented Jul 2, 2019

The child is supposed to inherit it at thread spawn and access its backend first, before the main thread has the opportunity to change the backend. We can push this burden down to downstream libraries, it isn't that important, though it is a data race. 😄

P.S. The possibility of it coming up is remote.

@grlee77

This comment has been minimized.

Copy link
Contributor

commented Jul 6, 2019

One comment from a user perspective:

If I set scipy.fft.set_global_backend(PyfftwBackend) when pyFFTW is not installed, the error when calling scipy.fft.fft is not very user-friendly:

BackendNotImplementedError: ('No selected backends had an implementation for this function.', [])

The text of that error sounds like pyFFTW doesn't have an implementation for fft, but the actual issue is that the pyFFTW module itself is not installed. We should indicate this to the user so they will know that this backend requires a third-party module that they need to install.

@rgommers

This comment has been minimized.

Copy link
Member

commented Jul 6, 2019

If I set scipy.fft.set_global_backend(PyfftwBackend) when pyFFTW is not installed, the error when calling scipy.fft.fft is not very user-friendly:

PyfftwBackend should be part of PyFFTW right, so then it's not possible to hit this situation?

@hameerabbasi

This comment has been minimized.

Copy link
Contributor

commented Jul 6, 2019

This should at least be an ImportError.

Show resolved Hide resolved scipy/fft/_basic.py Outdated
Show resolved Hide resolved scipy/fft/_basic.py Outdated

@peterbell10 peterbell10 force-pushed the peterbell10:uarray-backend branch from 162ec3f to a77841f Jul 6, 2019

@peterbell10

This comment has been minimized.

Copy link
Member Author

commented Jul 6, 2019

If I set scipy.fft.set_global_backend(PyfftwBackend) when pyFFTW is not installed, the error when calling scipy.fft.fft is not very user-friendly:

PyfftwBackend should be part of PyFFTW right, so then it's not possible to hit this situation?

Everything in _debug_backends.py is just for testing purposes. I've intentionally kepy everything in there private. Ultimately, the backend should be in pyfftw.

If we want to provide our own __ua_function__ for pyfftw then I think it would be better to allow set_backend('pyfftw') instead of exposing the class directly. That way we could detect pyfftw's support and use that when available. This could be the place to raise the ImportError as well.

@rgommers

This comment has been minimized.

Copy link
Member

commented Jul 6, 2019

Everything in _debug_backends.py is just for testing purposes.

makes sense.

If we want to provide our own __ua_function__ for pyfftw

I'd prefer to not have that. Both design-wise and license-wise it makes more sense to have the whole backend in pyfftw itself (and same for other backends).

@peterbell10 peterbell10 force-pushed the peterbell10:uarray-backend branch 2 times, most recently from 21788a0 to 06ac656 Jul 9, 2019

@peterbell10 peterbell10 changed the title ENH: Add uarray based backend support ENH: uarray based backend support for scipy.fft Jul 9, 2019

@peterbell10 peterbell10 force-pushed the peterbell10:uarray-backend branch from 06ac656 to 27c0be4 Jul 9, 2019

@hameerabbasi

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2019

It seems:

a. I'm using f-strings which aren't compatible with Python 3.5
b. The pickling error still exists, but we got a bit farther.

@peterbell10

This comment has been minimized.

Copy link
Member Author

commented Jul 9, 2019

From what I've read, the issue is that the function decorator replaces the argument extractor with the multimethod object. That means that the unpickler can't access the argument extractor to rebuild the multimethod.

@hameerabbasi

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2019

It seems you should just store the multimethod name and retrieve it from the module. Like the following:

I think the issue is that the constructed multimethod is a different object to the multimethod in the other process. This will allow it to be the same object.

def __setstate__(self, state):
    return getattr(sys.modules[state["__module__"]], state["__name__"])

def __getstate__(self):
    return {"__module__": self.__module__, "__name__": self.__name__}

https://stackoverflow.com/questions/8966685/picklingerror-cant-pickle-class-its-not-the-same-object-as-in-gae

@peterbell10

This comment has been minimized.

Copy link
Member Author

commented Jul 9, 2019

I think the issue is that the constructed multimethod is a different object to the multimethod in the other process.

The issue is that it tries to pickle the function scipy.fft.fft (the argument extractor) but instead finds the multimethod which is "not the same object". The solutions are either to just store the qualified name as you suggest, or to not overwrite the argument extractor.

@hameerabbasi

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2019

The decorator replaces the original object in Python, so if you applied the decorator to scipy.fft.fft, it's completely replaced, for all intents and purposes, by the thing returned by the decorator, unless it's using old versions.

You can probably keep your original code as a backup in case of someone directly calling generate_multimethod or similar.

@hameerabbasi

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2019

I think the issue here is one version of the Function objects is created at import time and another at unpickling time, and since their id isn't the same, they aren't exactly the same object.

@larsoner

This comment has been minimized.

Copy link
Member

commented Jul 15, 2019

I can seperate the scipy changes out more cleanly if you prefer though.

Please do. Any time we can simply copy-paste a file to update a vendored version of a library it makes maintenance easier.

@peterbell10 peterbell10 force-pushed the peterbell10:uarray-backend branch from 7165b99 to c52e824 Jul 15, 2019

@larsoner
Copy link
Member

left a comment

@hameerabbasi can you take another look?

If you're happy I'll aim to merge tomorrow

@hameerabbasi
Copy link
Contributor

left a comment

This one change and we should be ready to merge. 😄

"""
The default backend for fft calculations
"""
__ua_domain__ = "scipy.fft"

This comment has been minimized.

Copy link
@hameerabbasi

hameerabbasi Jul 15, 2019

Contributor

This should be numpy.scipy.fft, as discussed in the hierarchical structure by @peterbell10, in case we update uarray to support that.

This comment has been minimized.

Copy link
@hameerabbasi

hameerabbasi Jul 15, 2019

Contributor
Suggested change
__ua_domain__ = "scipy.fft"
__ua_domain__ = "numpy.scipy.fft"

This comment has been minimized.

Copy link
@peterbell10

peterbell10 Jul 15, 2019

Author Member

Is their any issue with the fact that numpy and scipy share some functions with the same name but may be slightly incompatible?

This comment has been minimized.

Copy link
@hameerabbasi

hameerabbasi Jul 15, 2019

Contributor

Then NumPy would use "numpy.fft", SciPy would use "numpy.scipy.fft" and since none are submodules of the other, there's no issue. 😄

This comment has been minimized.

Copy link
@peterbell10

peterbell10 Jul 15, 2019

Author Member

But what about __ua_domain__ = "numpy".

This comment has been minimized.

Copy link
@hameerabbasi

hameerabbasi Jul 15, 2019

Contributor

We should change the documentation for this.

This comment has been minimized.

Copy link
@pv

pv Jul 15, 2019

Member

When you decide what it should be, please add a comment explaining why it is what it is.

This comment has been minimized.

Copy link
@hameerabbasi

hameerabbasi Jul 15, 2019

Contributor

But what about ua_domain = "numpy"

The multimethods are separate objects, so in theory there still shouldn't be an issue.

@hameerabbasi
Copy link
Contributor

left a comment

Thank you so much for all your awesome work, @peterbell10!

@larsoner
Copy link
Member

left a comment

A few PEP8/docstring consistency things, and a couple questions about a note and licensing concerns

Show resolved Hide resolved scipy/fft/_backend.py
Show resolved Hide resolved scipy/fft/_backend.py Outdated
Show resolved Hide resolved scipy/fft/_backend.py Outdated
Show resolved Hide resolved scipy/fft/_backend.py Outdated
Show resolved Hide resolved scipy/fft/_backend.py Outdated
Show resolved Hide resolved scipy/fft/_backend.py Outdated
Show resolved Hide resolved scipy/fft/_backend.py Outdated
Show resolved Hide resolved scipy/fft/_backend.py Outdated
Show resolved Hide resolved scipy/fft/_basic.py
try:
import pyfftw.interfaces.numpy_fft as pyfftw_fft
except ImportError:
pyfftw_fft = {}

This comment has been minimized.

Copy link
@larsoner

larsoner Jul 16, 2019

Member

From a licensing standpoint I'm a bit uneasy about importing anything from pyfftw in the SciPy module itself, even if it's only imported on demand. It might be better to stick this in with the benchmarks where you might actually use it (?).

But maybe I'm being too paranoid. @rgommers do you know the limitations here?

This comment has been minimized.

Copy link
@peterbell10

peterbell10 Jul 16, 2019

Author Member

pyFFTW/pyFFTW#229 may be relevant here.

Since it's not greatly important either way, I've moved it into the benchmarks as suggested.

@peterbell10 peterbell10 force-pushed the peterbell10:uarray-backend branch from f3381f5 to d162732 Jul 16, 2019

@peterbell10 peterbell10 force-pushed the peterbell10:uarray-backend branch from 7b3d270 to 3604f47 Jul 16, 2019

@larsoner

This comment has been minimized.

Copy link
Member

commented Jul 16, 2019

Thanks @peterbell10 let's go ahead and merge once CI's are happy. I might be away from the computer when it turns green, so if someone else notices, feel free to click the button.

@larsoner

This comment has been minimized.

Copy link
Member

commented Jul 17, 2019

@grlee77 I just noticed that you have a "change requested" according to GitHub, just want to make sure you're happy with it at this point.

If so, feel free to set the green button to "Squash and merge" and click it :)

@grlee77 grlee77 self-requested a review Jul 17, 2019

@grlee77 grlee77 merged commit 7802b0e into scipy:master Jul 17, 2019

9 checks passed

ci/circleci: build_docs Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
scipy.scipy Build #20190716.10 succeeded
Details
scipy.scipy (Linux_Python_36_32bit_full) Linux_Python_36_32bit_full succeeded
Details
scipy.scipy (Windows Python35-64bit-full) Windows Python35-64bit-full succeeded
Details
scipy.scipy (Windows Python36-32bit-full) Windows Python36-32bit-full succeeded
Details
scipy.scipy (Windows Python36-64bit-full) Windows Python36-64bit-full succeeded
Details
scipy.scipy (Windows Python37-64bit-full) Windows Python37-64bit-full succeeded
Details

@peterbell10 peterbell10 deleted the peterbell10:uarray-backend branch Jul 17, 2019

@peterbell10 peterbell10 referenced this pull request Jul 17, 2019

Merged

scipy.fft interface #269

@WarrenWeckesser

This comment has been minimized.

Copy link
Member

commented Jul 17, 2019

I'm using an oldish Mac (it won't die, it still works great, the keyboard isn't flaky, it has an escape key!) with its original version of Mac OSX (10.9.5). I haven't upgraded the OS out of paranoia about breaking stuff ("if it ain't broke..." and all that).

I now get the following error when building scipy:

<snip>
building 'scipy._lib._uarray._uarray' extension
compiling C++ sources
C compiler: g++ -Wno-unused-result -Wsign-compare -Wunreachable-code -DNDEBUG -g -fwrapv -O3 -Wall -I/Users/warren/miniconda36/include -arch x86_64 -I/Users/warren/miniconda36/include -arch x86_64

creating build/temp.macosx-10.7-x86_64-3.6/scipy/_lib/_uarray
compile options: '-I/Users/warren/miniconda36/lib/python3.6/site-packages/numpy/core/include -I/Users/warren/miniconda36/include/python3.6m -c'
extra options: '--std=c++11 -fvisibility=hidden -mmacosx-version-min=10.9'
g++: scipy/_lib/_uarray/_uarray_dispatch.cxx
scipy/_lib/_uarray/_uarray_dispatch.cxx:109:1: error: thread-local storage is unsupported for the current target
thread_local std::unordered_map<
^
scipy/_lib/_uarray/_uarray_dispatch.cxx:894:4: warning: ISO C++11 does not allow conversion from string literal to 'char *'
      [-Wc++11-compat-deprecated-writable-strings]
  {"__dict__", PyObject_GenericGetDict, PyObject_GenericSetDict},
   ^
1 warning and 1 error generated.
error: Command "g++ -Wno-unused-result -Wsign-compare -Wunreachable-code -DNDEBUG -g -fwrapv -O3 -Wall -I/Users/warren/miniconda36/include -arch x86_64 -I/Users/warren/miniconda36/include -arch x86_64 -I/Users/warren/miniconda36/lib/python3.6/site-packages/numpy/core/include -I/Users/warren/miniconda36/include/python3.6m -c scipy/_lib/_uarray/_uarray_dispatch.cxx -o build/temp.macosx-10.7-x86_64-3.6/scipy/_lib/_uarray/_uarray_dispatch.o -MMD -MF build/temp.macosx-10.7-x86_64-3.6/scipy/_lib/_uarray/_uarray_dispatch.o.d --std=c++11 -fvisibility=hidden -mmacosx-version-min=10.9" failed with exit status 1

The minimum supported version is supposed to be OSX 10.9. Has anyone successfully built scipy on OSX 10.9? (Do I finally have to bite the bullet and upgrade the OS?)

(Edited: added the remaining part of the error messages.)

@peterbell10

This comment has been minimized.

Copy link
Member Author

commented Jul 17, 2019

An observation:

creating build/temp.macosx-10.7-x86_64-3.6/scipy/_lib/_uarray

Is it possible that a lower minimum version is being set somewhere?

@peterbell10

This comment has been minimized.

Copy link
Member Author

commented Jul 17, 2019

@grlee77 grlee77 referenced this pull request Aug 1, 2019

Open

Add scipy.fft to cupyx #2355

@WarrenWeckesser

This comment has been minimized.

Copy link
Member

commented Aug 11, 2019

I've updated to a newer OSX, and the build error is gone, but I still get the warning

g++: scipy/_lib/_uarray/_uarray_dispatch.cxx
scipy/_lib/_uarray/_uarray_dispatch.cxx:894:4: warning: ISO C++11 does not allow conversion from string literal to 'char *' [-Wwritable-strings]
  {"__dict__", PyObject_GenericGetDict, PyObject_GenericSetDict},
   ^
1 warning generated.

Any chance that can be cleaned up? It would be nice to ensure that new code is free of warnings, while we continue to plug away at cleaning up the warnings in old code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.