pykdtree
pykdtree copied to clipboard
storpipfugl
Fix compilation with support for OpenMP on OS X
For some reason, OpenMP support is not available for OS X while compiling with Clang.
These changes fixed the compilation errors on OS X (tested on High Sierra, Mojave, and Catalina).
Could you provide some description of what these changes do? What versions of OSX is this available on? What does -Xpreprocessor do?
@djhoese I forgot to write the comment, my bad. This post explains the changes I made, please take a look:
https://iscinumpy.gitlab.io/post/omp-on-high-sierra/ https://stackoverflow.com/a/60564952
Thanks. I think the changes I made a while ago in #37 and #43 haven't been released yet (I don't have permissions on this repository). This should make it so it at least doesn't have errors when it compiles on OSX.
This PR however changes the defaults for OSX by always trying to build with OpenMP which is still not available on all versions of OSX. Additionally the flags you are using are only valid for Clang, right? What if someone used gcc to build on OSX? This also requires that OpenMP be installed based on what I understood from that post you linked to so that means people without it installed will still run in to compilation errors and be unable to install pykdtree from source.
My vote would be to revert the try/except block I had so OpenMP is not built by default on OSX unless it is a conda environment. I think their should also be a check to only include the flags you have if the compiler is clang-based (assuming they are clang-only flags).
You're right, it forces you to install the package with Clang. Although, as far as I can tell clang is the de-facto compiler for OSX. As you said, we can revert to the try-catch and make OpenMP only available for condo environments, or we can have both. If the user is in a conda environment we're done, but if there is no conda environment enabled we can rely on clang and libomp to let the them use the package with OpenMP support.
Maybe, a better idea would be to disable OpenMP by default and create a tag to control whether or not the package should be compiled with support for it.
For example:
pip install pykdtree[omp]
I had thought about turning off OpenMP by default when I originally "fixed" this for OSX, but that will inevitably lead to people suggesting that pykdtree is not as fast as it suggests or that it has become slower in newer versions even though the only difference is how it is compiled. I'm not a maintainer of this project, so I can't make the final call.
I'm wondering if there is a good way to say "are we using clang". Best I found was https://stackoverflow.com/a/37762853/433202. If we can set the flags based on the compiler then this becomes a lot more flexible and the project will get less "I can't install pykdtree on X platform" issues.
I like the [omp] extra, but can that be checked by an extension? As far as I know that is only handled internally by distutils/setuptools to determine the dependencies. It'd be nice if users could control this though a requirements.txt (or similar) dependency list.
I'll have to dig into the python C extensions system, I'm not sure if we can rely on an extension to do that, but if that works it'd be great. Another way I'm thinking of is forcing distutils to use clang when running outside a conda environment on OSX
https://stackoverflow.com/a/16737674
I'm not sure if we can rely on an extension to do that, but if that works it'd be great
What I meant is can our pykdtree Extension (the python object in setup.py wrapping the cython code being compiled) know which compiler is being used. Like could we subclass Extension in setup.py so that it could pick which flags to use or something.
Another way I'm thinking of is forcing distutils to use clang when running outside a conda environment on OSX
This is already true in OSX. If it isn't using clang already then the user made it that way on purpose (ex. export CC=gcc) in which case we shouldn't be modifying CC if it is already set. Setting CC is exactly how I've tested different compilers on pykdtree on OSX in the past.
@mraspaud From what I see the main goal of this PR is to get OpenMP builds working on more OSX systems. The problem is, I fear, that by doing this we'll make it work for some users and break for other users. It comes down to clang needing some flags to compile with OpenMP support and gcc needing others. I no longer have a macbook so I can't easily test this.
I've commented inline with at least one concern I have about the if statement logic. Basically if there was an easy way to say "we are going to use clang to compile" then we could include that it in our if statements in setup.py...but I don't know an easy way to check that.
@antoniomdk I think this is exactly what I was hoping for:
https://stackoverflow.com/a/5192738/433202