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

Disable OpenMP by default #102

Closed
wants to merge 1 commit into from

Conversation

astrofrog
Copy link
Member

This implements one of my suggestions in #97

Basically this makes OpenMP opt-in. We can still decide to opt in by default for conda packages if the OpenMP environment there is more predictable.

@astrofrog astrofrog mentioned this pull request Jul 2, 2018
@coveralls
Copy link

Coverage Status

Coverage remained the same at 96.214% when pulling b78321a on astrofrog:disable-openmp-default into a0b72e7 on astropy:master.

@cdeil cdeil added this to the 0.3 milestone Jul 2, 2018
@cdeil
Copy link
Member

cdeil commented Jul 2, 2018

I would prefer if we remove OpenMP completely. This does not really resolve the concerns I mentioned in #97.

If we offer the option, we'll have packagers and users using OpenMP and have to support it, and it's a portability concern for Astropy core.

Also we have to test it. Note that this PR leaves the option to use OpenMP, but as far as I can see it's completely untested in CI now.

Also: it hasn't really been shown that this multi-threading gives good speed-ups. There have been some benchmarks posted in the PRs adding OpenMP that show a factor 2 speed-up for large arrays (and slow-down for small arrays), but not that our HEALPix C lib is not efficient yet, it can be improved by a factor of a few (see e.g. #34 (comment) and #34 (comment) ) and once it's optimised it's not clear if using many cores helps a lot.

So @astrofrog - If you want to keep the OpenMP, then I agree this is an improvement and we should merge.

@astrofrog
Copy link
Member Author

Removing it is fine by me as long as you are happy with the performance. I only added it originally because there were concerns that without, things were slower than healpy.

@cdeil
Copy link
Member

cdeil commented Oct 17, 2018

Removing it is fine by me as long as you are happy with the performance. I only added it originally because there were concerns that without, things were slower than healpy.

+10 from me to remove OpenMP.

@lpsinger - Acceptable for you?

@astrofrog - Can you update this PR, or should prepare a new one to do it?

@lpsinger
Copy link
Contributor

@lpsinger - Acceptable for you?

👍

@cdeil
Copy link
Member

cdeil commented Oct 23, 2018

Ok, then I'm closing this PR. I did open #108 which removes OpenMP.

@cdeil cdeil closed this Oct 23, 2018
@jamienoss
Copy link
Contributor

@cdeil Check out astropy/astropy-helpers#410. You might want to use the following pattern in your root setup.py:

...
# The current default is to NOT build with OpenMP support, i.e. ``disable_openmp = True``
disable_openmp = os.environ.get('ASTROPY_HEALPIX_USE_OPENMP', '0') == '0'
generate_openmp_enabled_py(NAME, disable_openmp=disable_openmp)
...

You won't then need the conditional around the add_openmp_flags_if_available(). Additionally, once built & installed you can call package.openmp_enabled.is_openmp_enabled() to query whether the package was built with OpenMP, this can be asserted in feedstock tests etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants