Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Gotchas when installing on MacOS #106

Closed
djhoese opened this issue Jan 14, 2024 · 4 comments · Fixed by #107
Closed

Gotchas when installing on MacOS #106

djhoese opened this issue Jan 14, 2024 · 4 comments · Fixed by #107

Comments

@djhoese
Copy link
Collaborator

djhoese commented Jan 14, 2024

Modern versions of pykdtree are released on PyPI as both binary wheels and a source distribution (sdist). Pykdtree includes C extensions that require compilation if installed from the sdist, but if installed from the binary wheel then no C compiler is required on the user's machine as the shared libraries (ex. .so files on linux) are bundled with the wheel. Pykdtree also optionally depends on OpenMP. If the extensions are built on a system with OpenMP installed then the extensions are linked to and therefore require OpenMP to exist at runtime. This means if pykdtree's wheels on PyPI were built with OpenMP then any user installing the wheels must also have OpenMP installed or they will get an error when importing pykdtree.

This gets complicated in the case of MacOS where the build image used in pykdtree's CI environment includes OpenMP, but it only exists because it is a dependency of one of the other tools/libraries that GitHub Actions includes (via homebrew) in their runner image. Since this isn't a "standard" package when users do pip install pykdtree and get the wheel built with OpenMP, they get an error at import time. See bjlittle/geovista#620 for one of these cases.
The workarounds are to:

  1. Install OpenMP (ex. brew install omp I think is the command - I don't have a mac)
  2. Install from source which requires a compiler (ex. pip install --force-reinstall --no-binary pykdtree pykdtree)

So our (as pykdtree maintainers) options are:

  1. Always require the user to build from source on macos (requires a compiler) by not building a macos wheel.
  2. Provide a no OpenMP version which will install, but not perform as well as it could and users might expect.
  3. Keep the OpenMP-based wheel and have some users run into runtime issues if they don't have OpenMP installed.

We could (and should) of course document all of these gotchas.

@mraspaud
Copy link
Collaborator

Is there a way we could activate openmp support at runtime? so that it's compiled with it, but if not present at runtime it issues a warning and runs without it?
Otherwise I think 3 might be the best, if we then issue an error with a link on how to recompile without openmp when openmp isn't installed.

@user27182
Copy link

In bjlittle/geovista#620 the pykdtree runtime error occurred only when importing pykdtree.kdtree. It does not occur if only pykdtree is imported. Perhaps this means that a simple try..catch block can be used in an __init__.py file somewhere to, at the very least, convert this cryptic and broad error

ImportError: dlopen(/.../venv/lib/python3.12/site-packages/pykdtree/kdtree.cpython-312-darwin.so, 0x0002): symbol not found in flat namespace '___kmpc_for_static_fini'`

into a much more helpful error message which may point the user to a sensible solution? (e.g. msg = "install with brew or install with --no-binary", or msg = "visit pykdree/#106 for details")?

This way, although the runtime error is still present, at least the user can easily try one of the two solutions hinted from the error message, and resolve this issue very quickly and easily. Perhaps this isn't ideal, but at least the user will still be able to take advantage of the performance improvements with OpenMP.

@djhoese
Copy link
Collaborator Author

djhoese commented Jan 16, 2024

I had a similar thought. I was wondering if it would be possible at build time to build both versions of the extensions. We always build the one without OpenMP, but if we detect OpenMP we build that version too and have two possible extensions to import. Then we try/except on the OpenMP extension import and warn and fallback if we have to use the non-OpenMP version. If the OpenMP extension file(s) don't even exist then we know that the build process wasn't even able to find OpenMP so there is no reason to warn in that case...I think.

@djhoese
Copy link
Collaborator Author

djhoese commented Jan 17, 2024

So I played around with my idea mentioned above with having two extensions, one compiled with OpenMP or at least attempted and one explicitly not including OpenMP. It all kind of worked except it seemed that Cython only compiled the cython module for a specific name once so when trying to import the second no-OpenMP version it gets confused by what the module name is.

Subject: [PATCH] Dual extensions with and without openmp
---
Index: setup.py
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/setup.py b/setup.py
--- a/setup.py	(revision 75804c99cc873ccc74b19e47c3a90003fd6fce4b)
+++ b/setup.py	(date 1705517886814)
@@ -20,8 +20,9 @@
 import re
 
 import numpy as np
-from Cython.Build import cythonize
-from setuptools import setup, Extension
+from Cython.Build import build_ext
+from Cython.Distutils import Extension
+from setuptools import setup
 from setuptools.command.build_ext import build_ext
 
 
@@ -56,17 +57,20 @@
         comp = self.compiler.compiler_type
         omp_comp, omp_link = _omp_compile_link_args(comp)
         if comp in ('unix', 'cygwin', 'mingw32'):
-            extra_compile_args = ['-std=c99', '-O3'] + omp_comp
+            extra_compile_args = ['-std=c99', '-O3']
             extra_link_args = omp_link
         elif comp == 'msvc':
-            extra_compile_args = ['/Ox'] + omp_comp
+            extra_compile_args = ['/Ox']
             extra_link_args = omp_link
         else:
             # Add support for more compilers here
             raise ValueError('Compiler flags undefined for %s. Please modify setup.py and add compiler flags'
                              % comp)
-        self.extensions[0].extra_compile_args = extra_compile_args
+        # Only apply OpenMP specific flags to OpenMP
+        self.extensions[0].extra_compile_args = extra_compile_args + omp_comp
         self.extensions[0].extra_link_args = extra_link_args
+        # No OpenMP version of the extension
+        self.extensions[1].extra_compile_args = extra_compile_args
         build_ext.build_extensions(self)
 
 
@@ -186,12 +190,19 @@
     readme = readme_file.read()
 
 extensions = [
-    Extension('pykdtree.kdtree', sources=['pykdtree/kdtree.pyx', 'pykdtree/_kdtree_core.c'],
+    Extension('pykdtree.kdtree_tryomp', sources=['pykdtree/kdtree.pyx', 'pykdtree/_kdtree_core.c'],
+              include_dirs=[np.get_include()],
+              define_macros=[("NPY_NO_DEPRECATED_API", "NPY_1_7_API_VERSION")],
+              cython_directives={"language_level": "3"},
+              ),
+    # NOTE: Compilation flags handled in build_ext_subclass (assumes second extension is noomp)
+    Extension('pykdtree.kdtree_noomp', sources=['pykdtree/kdtree.pyx', 'pykdtree/_kdtree_core.c'],
               include_dirs=[np.get_include()],
               define_macros=[("NPY_NO_DEPRECATED_API", "NPY_1_7_API_VERSION")],
-              compiler_directions={"language_level": "3"},
+              cython_directives={"language_level": "3"},
               ),
 ]
+
 
 setup(
     name='pykdtree',
@@ -206,7 +217,7 @@
     install_requires=['numpy'],
     tests_require=['pytest'],
     zip_safe=False,
-    ext_modules=cythonize(extensions),
+    ext_modules=extensions,
     cmdclass={'build_ext': build_ext_subclass},
     classifiers=[
       'Development Status :: 5 - Production/Stable',
Index: pykdtree/__init__.py
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/pykdtree/__init__.py b/pykdtree/__init__.py
--- a/pykdtree/__init__.py	(revision 75804c99cc873ccc74b19e47c3a90003fd6fce4b)
+++ b/pykdtree/__init__.py	(date 1705518033734)
@@ -1,0 +1,15 @@
+"""Simple wrapper around importing pykdtree for OpenMP use or not."""
+
+try:
+    from . import kdtree_tryomp as kdtree
+    raise ImportError("pykdtree")
+except ImportError:
+    import warnings
+    warnings.warn(
+        """Pykdtree failed to import its C extension. This usually means it
+was built with OpenMP (C-level parallelization library) support but could not
+find it on your system. Pykdtree will use a less performant version of the
+algorithm instead. To enable better performance OpenMP must be installed
+(ex. ``brew install omp`` on Mac with HomeBrew)."""
+    )
+    from . import kdtree_noomp as kdtree
\ No newline at end of file
$ python -c "from pykdtree import kdtree"
/home/davidh/repos/git/pykdtree/pykdtree/__init__.py:8: UserWarning: Pykdtree failed to import it's C extension. This usually means it
was built with OpenMP (C-level parellelization library) support but could not
find it on your system. Pykdtree will use a less performant version of the
algorithm instead. To enable better performance OpenMP must be installed
(ex. ``brew install omp`` on Mac with HomeBrew).
  warnings.warn(
Traceback (most recent call last):
  File "/home/davidh/repos/git/pykdtree/pykdtree/__init__.py", line 5, in <module>
    raise ImportError("pykdtree")
ImportError: pykdtree

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/home/davidh/repos/git/pykdtree/pykdtree/__init__.py", line 15, in <module>
    from . import kdtree_noomp as kdtree
ImportError: dynamic module does not define module export function (PyInit_kdtree_noomp)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants