getkeops / keops

KErnel OPerationS, on CPUs and GPUs, with autodiff and without memory overflows
https://www.kernel-operations.io
MIT License
1.02k stars 65 forks source link

Openmp import from conda #332

Closed DavidLapous closed 5 months ago

DavidLapous commented 9 months ago

For some reasons pykeops doesn't find omp.h in a conda environment (from the llvm-openmp conda-forge's package).

https://github.com/getkeops/keops/blob/52ed22a7fbbcf4bd02dbdf5dc2b00bf79cceddf5/keopscore/keopscore/config/config.py#L128

A possible fix is to add the $CONDA_PREFIX/include folder to the include folders list (when $CONDA_PREFIX is not empty), and the following compiles

echo "#include <omp.h>" | g++ -I$CONDA_PREFIX/include -E - -o /dev/null

But I'm not sure how to propagate this change in this config file.

Specs :

DavidLapous commented 9 months ago

It also seems that the compilers package also fills the CXX and CXXFLAGS environment variable by the c++ compiler and its flags respectively; so we can also use these instead of the $CONDA_PREFIX if these variables exist. More specifically, this compiles on my laptop

echo "#include <omp.h>" | $CXX ${=CXXFLAGS} -E - -o /dev/null
joanglaunes commented 9 months ago

Hello @DavidLapous , I just did a commit to ask the user to define a OMP_PATH variable, when omp.h is not found. We do the same when cuda headers are not found, and it is a simple workaround because the user can just add a command it the python code using os.environ. The problem with other variables like CONDA_PREFIX, CXX or CXXFLAGS is that they depend on the configuration. For example on my MacBook I do not have any CXX or CXXFLAGS defined, and I have a CONDA_PREFIX but conda is not supposed to be required to use our library. Let me know if you think there is better way to proceed.

DavidLapous commented 9 months ago

Hi @joanglaunes, Thanks for this workaround 🙂 I like this patch, but it's not perfect imo. For instance in my setup, this current g++ is the systems g++, i.e.

which g++
/usr/bin/g++

which one may want to avoid (because one may not be able to install new system libraries libraries as root to properly link with this gcc, or has an outdated gcc, or ...), by e.g., installing a compiler from conda. As an user using a virtual environment, I would also expect the compiler, and its libraries to be the one in this environment.

Fortunately the CXX and CXXFLAGS are not related to conda, for instance when installing a python package via a setup file

CXX=`/path/to/a/g++` python setup.py build

i noticed that the compiler used is the one given by CXX.

As the cxx-package is standard in conda and does fills environments variable properly, a possibility would be to

This would also allow users with cxx-compiler and llvm-openmp packages installed not having to deal with this issue.

joanglaunes commented 9 months ago

Ok, thanks for your suggestion. Actually we already do this when checking for the compiler, as seen here. If CXX is defined we use the compiler from CXX, otherwise we try just g++, otherwise we ask the user to set CXX. This defines a cxx_compiler variable. But in the part below where we checked for omp.h, we just forgot about this and hard coded g++ in the compile command... So ok I think we can just proceed as you suggest. I will just use cxx_compiler instead of g++ and add a check on CXXFLAGS to be included in the command.

DavidLapous commented 9 months ago

I tried your patch, unfortunately importing numpy is not enough to load libomp (at least, not with the latest version). An option is importing sklearn, in addition to numpy around here https://github.com/getkeops/keops/blob/03295bea05d32e8545b21b7b79d87cbb585c5c62/keopscore/keopscore/config/config.py#L152C29-L152C29 which works.

Another option is to follow sklearns import flag of openmp: https://github.com/scikit-learn/scikit-learn/blob/457b02c61a2f3cd353d2997929b67a3ef890bf60/sklearn/_build_utils/openmp_helpers.py

joanglaunes commented 9 months ago

Hello @DavidLapous , Thank you again for your suggestions. I have added a line to try to import sklearn ; and then other lines to try to directly load the openmp libraries in case all these imports were not enough. With all this it should be ok hopefully with many configurations! Let me know if this solves the issue in your case...

DavidLapous commented 9 months ago

Thanks for this patch 😉, I just had to add a line to a setup.py to import pykeops, with PR #338 and it works, thanks !

joanglaunes commented 9 months ago

Ok thanks, I have merged your PR now.

Gattocrucco commented 8 months ago

I'm having the same issue on a M1 Mac, ~and I can't upgrade to the latest version of keops in full because of gpytorch #2363~. For reference I write here the quickest workaround I found:

Edit: nevermind, it fails with g++: error: unrecognized command-line option '-Xclang'

Edit 2: found a fully working solution: