Repository navigation
OpenMP Support #7971
Description
Activity
- added a commit that references this issue
on Oct 25, 2018 @astrofrog, @cdeil, @eteq FYI
I have no idea about the APE procedure, I was thinking we host the conversation here, let me know if there's a better forum?
APE procedure is documented at https://github.com/astropy/astropy-APEs . FYI.
Reacted by Jamie NossReacted by Jamie Noss- added 2 commits that reference this issue
on Oct 25, 2018 I'll start to put together a skelton proposal next week and then close this issue out when that's opened and I ref it here.
Breadcrumb: astropy/astropy-healpix#97 "Remove use of OpenMP?"
@jamienoss - Thank you for splitting this discussion out from #7293 .
It would be great to have an APE document with the relevant infos, and not just discussion scattered across Github issues and PRs.
Basically I think the APE should have some infos on "cost" and "benefit" of using OpenMP in Astropy.
Benefit is speed, for astropy.convolution and maybe other numerics. It would be good to know if there's a strong reason (e.g. "JWST pipeline needs this") or if it's just a nice to have. Personally I think it was a mistake to put in
astropy.convolutionin the first place, this would have been better developed in a numerics package likescipy. That ship has sailed, what's done is done. But IMO the question whether Astropy needs to develop ultra high-performance numerics methods like convolution via OpenMP is still there. As far as I understand the special interpolating functionality thatastropy.convolutionadds (this is the paper cited in the Astropy paper) can be done as a pre-processing step once during data reduction, and for that it's fast enough, and then during model fitting where speed is really needed, the fast scipy convolution functions can be used (see our performance evaluation for convolution in Gammapy here).It's very likely that this is assessment is highly biased and inaccurate, and some missions or astronomers really need the features and performance increase in
astropy.convolution. But that should be the one starting point in the APE: how big is the need / benefit of adding OpenMP? In #7293 you just start with the assumption that Astropy needs and wants very high performance for astropy.convolution, and using OpenMP in Astropy is OK.Cost is extra effort for Astropy maintainers and to a certain degree packagers to support use of OpenMP in Astropy. That has been discussed already quite a bit in the other PR, but it would be good to have a small summary to have some actual infos, not just opinions. Note that packaging for most distribution channels is actually done by astronomers to help support the community, I don't think the standpoint that this is not a concern of the Astropy project is a good position to take.
For conda, my understanding is that the Anaconda base distribution and conda-forge are separate, but have some efforts to merge or be compatible. I see https://github.com/AnacondaRecipes and https://github.com/conda-forge . For Astropy apparently it's the same already? An example where it's still different in Anaconda and conda-forge is e.g. Tensorflow, that is very hard to build and distribute, Anaconda spends a lot of money to make it work (https://www.anaconda.com/blog/developer-blog/tensorflow-in-anaconda/). I'm just giving Tensorflow as an example where different, probably incompatible conda binaries are shipped by different people. I'm not saying that has anything to do with OpenMP, clearly Tensoflow is even much harder because they use GPU. For the Astropy binary wheels at https://pypi.org/project/astropy/ I don't even know how they are built.
It could also be useful to look at other big packages. From a very quick look, I think e.g. pandas is actually using OpenMP via Cython since many years and it's working well for them? If that is the case, that would be a big plus to allow OpenMP in Astropy, because we could just "do the same" without big effort or risk. healpy is probably an example of a project using C++ and OpenMP and having install issues since years that could be mentioned to caution adoption for Astropy. We could ping and ask for a comment on the APE to summarise their use of OpenMP and experience.
For Astropy apparently it's the same already?
My understanding is that the build environment and ecosystem is still different, and that is a non negligible effect.
- added a commit that references this issue
on Oct 25, 2018 I'll start to put together a skelton proposal next week and then close this issue out when that's opened and I ref it here.
That sounds like a good plan - and just to be clear, the APE should be opened as a PR to the astropy-APEs repo. It can be opened in draft form and updated as time goes on.
I think that regardless of what the final decision ends up being, having an APE is the right format for this discussion. I think it would also be good to make sure there are multiple people involved with the APE - in particular OpenMP skeptics could help fill out the 'Alternatives' part of the APE for example.
Reacted by Jamie Noss- added a commit that references this issue
on Oct 26, 2018 Moving this to astropy/astropy-APEs#46
Reacted by P. L. Lim@cdeil
In relation to the following comment #7971 (comment)As far as I understand the special interpolating functionality that astropy.convolution adds (this is the paper cited in the Astropy paper) can be done as a pre-processing step once during data reduction
It's not really a question of can vs can't but a question of whether it needs to. The reason for this is that the interpolation scheme is itself a convolution. It's convolution with a normalized kernel where the normalization is a function of the presence of NaN values. So ok, it can technically be done pre-convolution, however, it would be equivalent to doing the convolution.
So semantically,
convolve(nan_treatment='interpolate')isn't convolution that deals with NaN values by interpolating them, it is an interpolation scheme for NaN values by convolving with a value dependent normalized kernel. The only caveat to this is that it may not be functionally complete because if a region of NaN values is greater or equal to the kernel size, the centroid won't be interpolated thus persisting those centroid NaN values. So some care in its use is required to make sure the kernel is large enough to cover potential NaN regions.At least that's my take on things.
This topic was discussed at the astropy coordination meeting 2018, and the consensus was that we would prefer to focus on support for tool like dask that leverages the Python ecosystem rather than expending resources trying to make OpenMP work consistently across all platforms.
Reacted by Nicholas Earl, Christoph Deil, Michael Seifert and Jamie NossThis topic was discussed at the astropy coordination meeting 2018, and the consensus was that we would prefer to focus on support for tool like dask that leverages the Python ecosystem rather than expending resources trying to make OpenMP work consistently across all platforms.
For the record, I'm definitely all for this too. If we can abstract these problems away and make them someone else's then that's great! Also, something like DASK is more accessible to end users, as we know, some already use it to parallelize astropy tasks.
My concern was only ever about parallelizing C extensions. I don't think DASK will help there, without re-writing the code back into the Python layer, right? Is it's functionality trivially or at all accessible from the C layer?
I'm not overly familiar with DASK, does it have thread level communication/locking functionality, i.e. mutex implementations? I know it has an MPI equivalent communication, but that's not the same thing. My understanding was that it was only designed (thread wise) for pure functions (those only dependent on their input and having no side-effects). If it has no mutex functionality it won't be able to solve all types of parallel problems. Just something to keep in mind and/or investigate first.
I'm opening this as place-holder for the conversation of using OpenMP within Astropy.
At least in
convolvution.convolve()as a consequence of #7293