Repository navigation
Remove use of OpenMP? #97
Description
We introduced the use of OpenMP in astropy-healpix, to take advantage of multiple cores (see here). This is done using the Cython parallel and prange feature in astropy_healpix/core_cython.pyx. Indeed, for users that care about HEALPix speed and where it works out of the box and they want to use multi-core for that process, getting the extra speed by default is great.
However, using OpenMP has a cost in terms of installation issues. We already have a user where pip install fails due to OpenMP issues and we weren't able to (or had time to) help him: #78
OpenMP is also listed as the first install issue here:
https://github.com/healpy/healpy/blob/master/README.rst#known-issues
I think we should remove the OpenMP usage. My arguments:
- simple install for
astropy.healpixis extremely important, as far as I know OpenMP is not used in Astropy core yet (true?) - there are users that don't want the now default behaviour of using all cores. E.g. many astronomers run their Python script on institute servers with 10 or 100 users, and using all cores by accident causes problems for others, or for them if their sysadmin comes to their office.
- the few users that really need the extra speed might use joblib or dask or chunk their Numpy arrays manually, i.e. use common techniques that are used e.g. also by scikit-image or scikit-learn.
- if several users complain about speed problems with
astropy.healpix, we can always add OpenMP or some other solution later; but I don't think there are many people who need this overall.
@astrofrog @lpsinger and also @mreineck - Thoughts?
Activity
mreineck commentedon Jun 28, 2018
How difficult is it to make OpenMP optional? healpy does that, right?
(Unfortunately I'm not at all familiar with
setuptools)cdeil commentedon Jun 28, 2018
I guess if OpenMP is optional and off by default when someome just does
pip install astropy, then the concern isn't as strong. But if there optionally is OpenMP support, and some people start to use it or packagers to use it when making binaries, then I think there's still a good chance that we'll get issue reports and have to spend time to support that OpenMP case.@astrofrog - Has this discussion or even a decision concerning OpenMP already taken place for the Astropy core package, and I'm just not aware?
lpsinger commentedon Jun 28, 2018
I agree. I think that Healpy does not use OpenMP for ufunc loops; it only uses it for the spherical harmonic transforms. @zonca could confirm.
astrofrog commentedon Jun 28, 2018
@cdeil - we are currently trying to make the OpenMP support in astropy-helpers more robust. No decision has been made yet as to whether to use OpenMP in the core package, but I could see that happening if we can make it robust.
What I would suggest for now is to simply is to not yet remove all the code that adds support for this but rather to simply comment out this one line:
https://github.com/astropy/astropy-healpix/blob/master/astropy_healpix/setup_package.py#L40
This will make the Cython extensions be compiled without OpenMP. A slightly better option would be to make that line contingent on a
--use-openmpflag when runningsetup.py(so basically just checking for that flag insys.argvinsetup_package.py. This will then make it possible for me and others to investigate how to turn it on in a more robust way without impacting users.astrofrog commentedon Jun 29, 2018
Just an idea - I wonder if we can make use of the
extras_requirefunctionality to allow something like:I can try and investigate at some point.
astrofrog commentedon Jul 2, 2018
See #102 for an example
jamienoss commentedon Aug 29, 2018
Chiming in here... I strongly feel that this is completely the wrong direction to go in. We should be writing more threaded code, not removing it.
jamienoss commentedon Aug 29, 2018
Let me check the code, perhaps it is the case that the C OpenMP pragmas
#pragma omp ..are not pp wrapped by#ifdef _OPENMPas they should be.lpsinger commentedon Aug 29, 2018
Extras can only affect package dependencies, not compiler flags. So this could work if you separated the C bindings into dual packages with and without OpenMP, and the
[openmp]extra determined which package you depended on.lpsinger commentedon Aug 29, 2018
Why would that help? If OpenMP has not been enabled by the compiler flags, then the OpenMP directives will be ignored like any other unrecognized pragma.
mreineck commentedon Aug 29, 2018
Exactly. At worst you get a compiler warning, but that can be silenced in a simpler way.
jamienoss commentedon Aug 29, 2018
@lpsinger & @mreineck you guys are totally right, I was completely jumping the gun here - brain fart over.
jamienoss commentedon Aug 29, 2018
In relation to #78 how do we go about investigating whether astropy/astropy-helpers#382 may have resolved this issue? I'm pretty much completely unfamiliar with anything pip.
Is #78 the only known issue?
Isn't this only an argument for default behavior and better education, and not complete removal?
cdeil commentedon Aug 29, 2018
@jamienoss - I tried to give my view in the original description above, and I still feel that way.
The first point was
and I think it's valid: introducing OpenMP in Astropy will cause real issues for users and extra work for maintainers / packagers, it's a feature that comes at a cost that has to be weighed against the benefits / needs for the feature.
For me personally it's not even a close call here, I believe OpenMP isn't needed for 99% of Astropy users (most even won't use astropy.healpix), we shouldn't spend our time discussing OpenMP.
I know others feel differently, that's just one opinion.
lpsinger commentedon Aug 29, 2018
I am a very heavy user of OpenMP, HEALPix, and Astropy, all at once.
But in the very limited context of OpenMP acceleration of basic HEALPix pixel transformations, I concur that it is not necessary. In Healpy, the OpenMP acceleration is a powerful feature in the limited application of the spherical harmonic transformations. For the operations provided by this library, I think it's overkill.
cdeil commentedon Aug 29, 2018
And @lpsinger - you're probably one of the most expert / heavy users of HEALPix from Python.
We in Gammapy are also using it heavily. Performance of healpy or astropy-healpix is not an issue. But I've spent a lot of time myself, and also supporting users with healpy installation issues. For healpy the problem is just that they ship a C++ library with the Python package, not OpenMP. I just mention it as an example of the cost I think we would also get by adding OpenMP to Astropy core and thus shipping a more complex product to many different platforms via many different packaging ways.
lpsinger commentedon Aug 29, 2018
Healpy is complicated to build because:
I'm at fault for a lot of this. Sorry! 🤷♂️
mreineck commentedon Aug 29, 2018
I agree. For everything except spherical harmonic transforms you don't really need OpenMP. In the current state, improving the scalar performance of the basic operations will be much more rewarding.
dstndstn commentedon Aug 29, 2018
mreineck commentedon Aug 29, 2018
I'm not sure. If your arrays are small, the numpy overhead will dominate. If they are large, the array expressions will cause multiple reading/writing of large memory blocks, thrashing the cache.
The native performance of, say, ang2pix, is above 20 million calls per second on a typical laptop CPU. I doubt that numpy will be able to get anywhere close.
cdeil commentedon Aug 29, 2018
Earlier this year I was also wondering if a Numpy implementation could be useful (mentioned in #90). I think it would be a factor of a few slower than C or C++ because of temp array copies, and because to implement the if / else you have to do
np.takeandnp.putoperations to work on subset of pixels in the north polar or central band or south polar zone. And I think some things like especially the query methods are not possible as Numpy expressions, so some C / Cython would always remain.But of course, any concrete proposal to change the implementation to e.g. have some code in Python/Numpy instead of C is welcome; we don't aim to have a C library here, the functionality just needs to be there from Python / Numpy.
cdeil commentedon Oct 23, 2018
There was a lot of discussion here, and some more in #102 .
OpenMP was removed from astropy-healpix in #108 , which will ship via the upcoming v0.3 shortly.
I'm closing this issue and also #78.