Remove use of OpenMP?

[cdeil]

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.healpix is 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?

[mreineck]

How difficult is it to make OpenMP optional? healpy does that, right?
(Unfortunately I'm not at all familiar with setuptools)

[cdeil]

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]

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]

@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-openmp flag when running setup.py (so basically just checking for that flag in sys.argv in setup_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]

Just an idea - I wonder if we can make use of the extras_require functionality to allow something like:

pip install astropy-healpix[openmp]

I can try and investigate at some point.

[astrofrog]

See #102 for an example

[jamienoss]

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]

Let me check the code, perhaps it is the case that the C OpenMP pragmas #pragma omp .. are not pp wrapped by #ifdef _OPENMP as they should be.

[lpsinger]

Just an idea - I wonder if we can make use of the extras_require functionality to allow something like:

pip install astropy-healpix[openmp]
I can try and investigate at some point.

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]

Let me check the code, perhaps it is the case that the C OpenMP pragmas #pragma omp .. are not pp wrapped by #ifdef _OPENMP as they should be.

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]

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.

Exactly. At worst you get a compiler warning, but that can be silenced in a simpler way.

[jamienoss]

@lpsinger & @mreineck you guys are totally right, I was completely jumping the gun here - brain fart over.

[jamienoss]

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?

there are users that don't want the now default behavior 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.

Isn't this only an argument for default behavior and better education, and not complete removal?

[cdeil]

@jamienoss - I tried to give my view in the original description above, and I still feel that way.

The first point was

simple install for astropy.healpix is extremely important, as far as I know OpenMP is not used in Astropy core yet (true?)

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]

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]

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]

Healpy is complicated to build because:

1. It uses C++.
2. It uses bleeding-edge C++ features.
3. It enable lots of compiler optimization flags.
4. It makes an effort to be able to compile and link against the standard healpix-cxx library, which may or may not have been built with OpenMP enabled, and may or may not have been built with a C++ toolchain with incompatible ABI or name mangling.

I'm at fault for a lot of this. Sorry! 🤷‍♂️

[mreineck]

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]

I even suspect that many of the operations could be more efficiently coded in pure numpy, rather than wrapping C. But that would be work!

…

On Wed, Aug 29, 2018 at 4:29 PM mreineck ***@***.***> wrote: 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. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#97 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABBD_Zm6hC7OjHwbCY1GgQGP5C6O3oy1ks5uVvmegaJpZM4U67ir> .

[mreineck]

I even suspect that many of the operations could be more efficiently coded
in pure numpy, rather than wrapping C. But that would be work!

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]

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.take and np.put operations 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]

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.