Merge pull request #528 from matthew-brett/conditional-omp-fix

MRG: move conditional compiling to C

.travis.yml

@@ -27,12 +27,13 @@ matrix:
       env:
         - DEPENDS="cython numpy scipy matplotlib h5py nibabel cvxopt scikit_learn"
 before_install:
+    - source tools/travis_tools.sh
     - virtualenv --python=python venv
     - source venv/bin/activate
     - python --version # just to check
-    - pip install nose # always
+    - retry pip install nose # always
     - sudo apt-get install libblas-dev liblapack-dev libatlas3gf-base
-    - pip install --timeout=60 --no-index -f http://travis-wheels.scikit-image.org $DEPENDS
+    - wheelhouse_pip_install $DEPENDS
     - if [ "${COVERAGE}" == "1" ]; then
       pip install coverage;
       pip install coveralls;

cythexts.py

@@ -125,13 +125,15 @@ def cyproc_exts(exts, cython_min_version,
                         cython_min_version, cyversion))
 
 
-def build_stamp(pyxes):
+def build_stamp(pyxes, include_dirs=()):
     """ Cythonize files in `pyxes`, return pyx, C filenames, hashes
 
     Parameters
     ----------
     pyxes : sequence
         sequence of filenames of files on which to run Cython
+    include_dirs : sequence
+        Any extra include directories in which to find Cython files.
 
     Returns
     -------
@@ -141,11 +143,12 @@ def build_stamp(pyxes):
         hash>; "c_filename", <c filemane>; "c_hash", <c file SHA1 hash>.
     """
     pyx_defs = {}
+    includes = sum([['--include-dir', d] for d in include_dirs], [])
     for source in pyxes:
         base, ext = splitext(source)
         pyx_hash = sha1(open(source, 'rt').read()).hexdigest()
         c_filename = base + '.c'
-        check_call('cython ' + source, shell=True)
+        check_call(['cython'] + includes + [source])
         c_hash = sha1(open(c_filename, 'rt').read()).hexdigest()
         pyx_defs[source] = dict(pyx_hash=pyx_hash,
                                 c_filename=c_filename,
@@ -175,22 +178,19 @@ def write_stamps(pyx_defs, stamp_fname='pyx-stamps'):
                                            pyx_info['c_hash']))
 
 
-def find_pyx(root_dir=None):
+def find_pyx(root_dir):
     """ Recursively find files with extension '.pyx' starting at `root_dir`
 
     Parameters
     ----------
-    root_dir : None or str, optional
-        Directory from which to search for pyx files.  If None, use current
-        working directory.
+    root_dir : str
+        Directory from which to search for pyx files.
 
     Returns
     -------
     pyxes : list
         list of filenames relative to `root_dir`
     """
-    if root_dir is None:
-        root_dir = os.getcwd()
     pyxes = []
     for dirpath, dirnames, filenames in os.walk(root_dir):
         for filename in filenames:
@@ -201,7 +201,8 @@ def find_pyx(root_dir=None):
     return pyxes
 
 
-def get_pyx_sdist(sdist_like=sdist, hash_stamps_fname='pyx-stamps'):
+def get_pyx_sdist(sdist_like=sdist, hash_stamps_fname='pyx-stamps',
+                  include_dirs=()):
     """ Add pyx->c conversion, hash recording to sdist command `sdist_like`
 
     Parameters
@@ -212,6 +213,8 @@ def get_pyx_sdist(sdist_like=sdist, hash_stamps_fname='pyx-stamps'):
     hash_stamps_fname : str, optional
         filename to which to write hashes of pyx / py and c files.  Default is
         ``pyx-stamps``
+    include_dirs : sequence
+        Any extra include directories in which to find Cython files.
 
     Returns
     -------
@@ -242,7 +245,7 @@ def make_distribution(self):
                     base, ext = splitext(source)
                     if ext in ('.pyx', '.py'):
                         pyxes.append(source)
-            self.pyx_defs = build_stamp(pyxes)
+            self.pyx_defs = build_stamp(pyxes, include_dirs)
             for pyx_fname, pyx_info in self.pyx_defs.items():
                 self.filelist.append(pyx_info['c_filename'])
             sdist_like.make_distribution(self)
@@ -256,7 +259,8 @@ def make_release_tree(self, base_dir, files):
     return PyxSDist
 
 
-def build_stamp_source(root_dir=None, stamp_fname='pyx-stamps'):
+def build_stamp_source(root_dir=None, stamp_fname='pyx-stamps',
+                       include_dirs=None):
     """ Build cython c files, make stamp file in source tree `root_dir`
 
     Parameters
@@ -266,7 +270,13 @@ def build_stamp_source(root_dir=None, stamp_fname='pyx-stamps'):
         working directory.
     stamp_fname : str, optional
         Filename for stamp file we will write
+    include_dirs : None or sequence
+        Any extra Cython include directories
     """
+    if root_dir is None:
+        root_dir = os.getcwd()
+    if include_dirs is None:
+        include_dirs = [pjoin(root_dir, 'src')]
     pyxes = find_pyx(root_dir)
-    pyx_defs = build_stamp(pyxes)
+    pyx_defs = build_stamp(pyxes, include_dirs=include_dirs)
     write_stamps(pyx_defs, stamp_fname)

dipy/align/bundlemin.pyx

@@ -7,15 +7,8 @@ import numpy as np
 cimport numpy as cnp
 cimport cython
 
-include "../../build/config.pxi"
-
-IF HAVE_OPENMP:
-    cimport openmp
-ELSE:
-    msg = 'OpenMP is not available with your compiler.\n'
-    msg += 'Disabled OpenMP based multithreading.'
-    print(msg)
-
+cimport safe_openmp as openmp
+from safe_openmp cimport have_openmp
 
 from cython.parallel import prange
 from libc.stdlib cimport malloc, free
@@ -171,12 +164,11 @@ def _bundle_minimum_distance(double [:, ::1] stat,
         double dist=0
         double * min_j
         double * min_i
-        IF HAVE_OPENMP:
-            openmp.omp_lock_t lock
+        cdef openmp.omp_lock_t lock
 
     with nogil:
 
-        IF HAVE_OPENMP:
+        if have_openmp:
             openmp.omp_init_lock(&lock)
 
         min_j = <double *> malloc(static_size * sizeof(double))
@@ -195,17 +187,17 @@ def _bundle_minimum_distance(double [:, ::1] stat,
                 tmp = min_direct_flip_dist(&stat[i * rows, 0],
                                        &mov[j * rows, 0], rows)
 
-                IF HAVE_OPENMP:
+                if have_openmp:
                     openmp.omp_set_lock(&lock)
                 if tmp < min_j[i]:
                     min_j[i] = tmp
 
                 if tmp < min_i[j]:
                     min_i[j] = tmp
-                IF HAVE_OPENMP:
+                if have_openmp:
                     openmp.omp_unset_lock(&lock)
 
-        IF HAVE_OPENMP:
+        if have_openmp:
             openmp.omp_destroy_lock(&lock)
 
         for i in range(static_size):
@@ -286,4 +278,4 @@ def distance_matrix_mdf(streamlines_a, streamlines_b):
 
             DM[i, j] = min_direct_flip_dist(t1_ptr, t2_ptr,t_len)
 
-    return DM
+    return DM

setup.py

@@ -152,7 +152,7 @@ def run(self):
     build_ext=extbuilder,
     install=installer,
     install_scripts=install_scripts_bat,
-    sdist=get_pyx_sdist())
+    sdist=get_pyx_sdist(include_dirs=['src']))
 
 
 def main(**extra_args):

setup_helpers.py

@@ -27,9 +27,9 @@
 call %py_exe% %pyscript% %*
 """
 
-# File to which to write Cython conditional DEF vars
-CONFIG_PXI = pjoin('build', 'config.pxi')
-# File name (no directory) to which to write Python conditional vars
+# Path of file to which to write C conditional vars from build-time checks
+CONFIG_H = pjoin('build', 'config.h')
+# File name (no directory) to which to write Python vars from build-time checks
 CONFIG_PY = '__config__.py'
 # Directory to which to write libraries for building
 LIB_DIR_TMP = pjoin('build', 'extra_libs')
@@ -95,10 +95,10 @@ def add_flag_checking(build_ext_class, flag_defines, top_package_dir=''):
         will link.  If both compile and link works, we add ``compile_flags`` to
         ``extra_compile_args`` and ``link_flags`` to ``extra_link_args`` of
         each extension when we build the extensions.  If ``defvar`` is not
-        None, it is the name of Cython variable to be defined in
-        ``build/config.pxi`` with True if the combination of
-        (``compile_flags``, ``link_flags``, ``code``) will compile and link,
-        False otherwise. If None, do not write variable.
+        None, it is the name of C variable to be defined in ``build/config.h``
+        with 1 if the combination of (``compile_flags``, ``link_flags``,
+        ``code``) will compile and link, 0 otherwise. If None, do not write
+        variable.
     top_package_dir : str
         String giving name of top-level package, for writing Python file
         containing configuration variables.  If empty, do not write this file.
@@ -148,30 +148,30 @@ def build_extensions(self):
             def_vars = []
             good_compile_flags = []
             good_link_flags = []
-            config_dir = dirname(CONFIG_PXI)
+            config_dir = dirname(CONFIG_H)
             for compile_flags, link_flags, code, def_var in self.flag_defs:
                 compile_flags = list(compile_flags)
                 link_flags = list(link_flags)
                 flags_good = self.can_compile_link(compile_flags,
                                                    link_flags,
                                                    code)
                 if def_var:
-                    def_vars.append('{0} = {1}'.format(
-                        def_var, flags_good))
+                    def_vars.append((def_var, flags_good))
                 if flags_good:
                     good_compile_flags += compile_flags
                     good_link_flags += link_flags
                 else:
                     log.warn("Flags {0} omitted because of compile or link "
                              "error".format(compile_flags + link_flags))
-            if def_vars:  # write config.pxi file
+            if def_vars:  # write config.h file
                 if not exists(config_dir):
                     self.mkpath(config_dir)
-                with open(CONFIG_PXI, 'wt') as fobj:
-                    fobj.write('# Automatically generated; do not edit\n')
-                    fobj.write('# Cython defines from compile checks\n')
-                    for vdef in def_vars:
-                        fobj.write('DEF {0}\n'.format(vdef))
+                with open(CONFIG_H, 'wt') as fobj:
+                    fobj.write('/* Automatically generated; do not edit\n')
+                    fobj.write('   C defines from build-time checks */\n')
+                    for v_name, v_value in def_vars:
+                        fobj.write('int {0} = {1};\n'.format(
+                            v_name, 1 if v_value else 0))
             if def_vars and top_package_dir:  # write __config__.py file
                 config_py_dir = (top_package_dir if self.inplace else
                                  pjoin(self.build_lib, top_package_dir))
@@ -181,7 +181,8 @@ def build_extensions(self):
                 with open(config_py, 'wt') as fobj:
                     fobj.write('# Automatically generated; do not edit\n')
                     fobj.write('# Variables from compile checks\n')
-                    fobj.write('\n'.join(def_vars) + '\n')
+                    for v_name, v_value in def_vars:
+                        fobj.write('{0} = {1}\n'.format(v_name, v_value))
             if def_vars or good_compile_flags or good_link_flags:
                 for ext in self.extensions:
                     ext.extra_compile_args += good_compile_flags

src/conditional_omp.h

@@ -0,0 +1,26 @@
+/* Header file to conditionally wrap omp.h defines
+ *
+ * _OPENMP should be defined if omp.h is safe to include
+ */
+#if defined(_OPENMP)
+#include <omp.h>
+int have_openmp = 1;
+#else
+/* These are fake defines to make these symbols valid in the c / pyx file
+ *
+ * All uses of these symbols should to be prefaced with ``if have_openmp``, as
+ * in:
+ *
+ *     cdef omp_lock_t lock
+ *     if have_openmp:
+ *         openmp.omp_init_lock(&lock)
+ *
+ * */
+typedef int omp_lock_t;
+void omp_init_lock(omp_lock_t *lock) {};
+void omp_destroy_lock(omp_lock_t *lock) {};
+void omp_set_lock(omp_lock_t *lock) {};
+void omp_unset_lock(omp_lock_t *lock) {};
+int omp_test_lock(omp_lock_t *lock) {};
+int have_openmp = 0;
+#endif

src/safe_openmp.pxd

@@ -0,0 +1,9 @@
+cdef extern from "conditional_omp.h":
+    ctypedef struct omp_lock_t:
+        pass
+    extern void omp_init_lock(omp_lock_t *) nogil
+    extern void omp_destroy_lock(omp_lock_t *) nogil
+    extern void omp_set_lock(omp_lock_t *) nogil
+    extern void omp_unset_lock(omp_lock_t *) nogil
+    extern int omp_test_lock(omp_lock_t *) nogil
+    cdef int have_openmp

tools/travis_tools.sh

@@ -0,0 +1,25 @@
+# Tools for working with travis-ci
+export WHEELHOUSE="http://travis-wheels.scikit-image.org/"
+
+retry () {
+    # https://gist.github.com/fungusakafungus/1026804
+    local retry_max=5
+    local count=$retry_max
+    while [ $count -gt 0 ]; do
+        "$@" && break
+        count=$(($count - 1))
+        sleep 1
+    done
+
+    [ $count -eq 0 ] && {
+        echo "Retry failed [$retry_max]: $@" >&2
+        return 1
+    }
+    return 0
+}
+
+
+wheelhouse_pip_install() {
+    # Install pip requirements via travis wheelhouse
+    retry pip install --no-index --find-links $WHEELHOUSE $@
+}