david-cortes / contextualbandits

Python implementations of contextual bandits algorithms
http://contextual-bandits.readthedocs.io
BSD 2-Clause "Simplified" License
751 stars 148 forks source link

Build errors on Mac OS X #23

Closed sumpfork closed 3 years ago

sumpfork commented 4 years ago

I had to change a couple of things to get this compiling on Mac OS X:

Something like:

diff --git a/setup.py b/setup.py
index ea0ff35..8b7e15b 100644
--- a/setup.py
+++ b/setup.py
@@ -9,16 +9,17 @@ class build_ext_subclass( build_ext ):
         if compiler == 'msvc': # visual studio
             for e in self.extensions:
                 e.extra_compile_args += ['/O2', '/openmp']
+        elif platform.startswith('darwin'):
+            for e in self.extensions:
+                e.extra_link_args += ["-lomp"]
+                e.extra_compile_args = ["-Xpreprocessor", "-fopenmp", "-O3", "-march=native"]
+                if e.language == "c++":
+                    e.extra_compile_args += ["-std=c++17"]
         else:
             for e in self.extensions:
                 e.extra_compile_args += ['-O3', '-march=native', '-fopenmp']
                 e.extra_link_args += ['-fopenmp']
-            
-            ### Remove this code if you have a mac with gcc or clang + openmp
-            if platform[:3] == "dar":
-                for e in self.extensions:
-                    e.extra_compile_args = [arg for arg in extra_compile_args if arg != '-fopenmp']
-                    e.extra_link_args    = [arg for arg in extra_link_args    if arg != '-fopenmp']
+
         build_ext.build_extensions(self)

 setup(
david-cortes commented 4 years ago

Thanks for the bug report. I've made some small changes to the setup.py file now. Could you clone/download the repository and check if it works for you? (python setup.py build_ext --inplace --force)

sumpfork commented 4 years ago

Works, though doesn't compile against openmp even if available, making the comment misleading: removing that code won't make the code compile against openmp on a mac.

david-cortes commented 4 years ago

Ok, good to know that it’s working.

I realize it doesn’t end up linking to OpenMP in macOS this way, but that’s not a big deal here, because the only functions which use OpenMP parallelization are topN (at the end, in the stage in which it argsorts each row result) and softmax predict (at the end, in the stage in which it selects according to probabilities), which for typical use-cases will save at most a few miliseconds by adding OpenMP.

Now, I’d like to have it link to OpenMP in every setup that supports it, but the issue is that it’s not possible – or at least I don’t how – to do it in a way that would work with all compiler configurations. For example, if you use gcc or icc on mac, the option -lomp will not do the trick, and I think if you install clang and its modules by compiling them directly it wouldn’t work either. Also -lomp I think sets the linkage to libomp, which not every setup will have, whereas -fopenmp will select the default one (typically libgomp or libiomp).

I’m open to better alternatives if you know of any, but I myself haven’t seen any other Python package with a setup.py script that would work for all setups on macOS.

david-cortes commented 3 years ago

Solved as now it will run a test compile with these arguments and add openmp linkage in mac if it's installed.