Skip to content
This repository was archived by the owner on Nov 11, 2024. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,24 @@ matrix:
env: PYTHON_VERSION=3.6 PIP_DEPENDENCIES='git+https://github.com/sphinx-doc/sphinx.git#egg=sphinx coveralls'
CONDA_DEPENDENCIES="setuptools cython numpy pytest-cov"

# Test conda's clang
- os: osx
env:
- PYTHON_VERSION=3.5
- CONDA_DEPENDENCIES="setuptools sphinx cython numpy pytest-cov clang llvm-openmp"
before_install:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure there is no need for these before_install sections either, put those variable into the env section.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, I added these here following the Travis documentation. Also, because of the generic before_install, which then has to be overloaded per explicit job.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

- export TEST_OPENMP="True"
- export CC=clang

# Test gcc on OSX
- os: osx
env:
- PYTHON_VERSION=3.5
- CONDA_DEPENDENCIES="setuptools sphinx cython numpy pytest-cov gcc"
before_install:
- export TEST_OPENMP="True"
- export CC=gcc

# Uncomment the following if there are issues in setuptools that we
# can't work around quickly - otherwise leave uncommented so that
# we notice when things go wrong.
Expand All @@ -45,6 +63,16 @@ matrix:
# CONDA_DEPENDENCIES='sphinx cython numpy pytest-cov'
# EVENT_TYPE='push pull_request cron'

before_install:

# Test OSX without OpenMP support
# Since the matrix OSX tests use the OS shipped version of clang, they double up
# as exploratory tests for when the shipped version has automatic OpenMP support.
# These tests will then fail and at such a time a new one should be added
# to explicitly remove OpenMP support.
- if [[ "$TRAVIS_OS_NAME" == "osx" ]]; then export TEST_OPENMP="False";
else export TEST_OPENMP="True"; fi

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the before_install section and define the env variables in the jobs above.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll double check this, but if I remember correctly, it needs to be here so as to also work with the matrix permutations. There is no way to add this to them as they will be evaluated not at the correct stage, i.e. TRAVIS_OS_NAME is undefined. For the explicit tests it is, which is why I do so there also, but it isn't for the env:matrix: sec.

@jamienoss jamienoss Jun 4, 2018 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regardless of OS, TEST_OPENMP has to be set to something. If set as a global env and to true it will prevent the OSX tests from passing. If set to false it will prevent the linux tests from passing (unless we explicitly turn off the openmp support - which is altogether silly). One alternative is to make it such that all global matrixed OS tests pass with either state, which probably means getting OpenMP to work for all the OSX tests. The other alternative is to make TEST_OPENMP conditional and this therefore can't happen in the global env stage by definition. It has to go in the install or better still (due to overloading and its current use, not to mention semantically) in the before_install stage.
Let me know if there is an alternative, perhaps the entire idea needs to be re-thought, however, I honestly don't see the cause to not use the ``before_install_ stage.


install:

- git clone git://github.com/astropy/ci-helpers.git
Expand Down
7 changes: 7 additions & 0 deletions CHANGES.rst
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,13 @@ astropy-helpers Changelog
by the ``sphinx-astropy`` package in conjunction with the ``astropy-theme-sphinx``,
``sphinx-automodapi``, and ``numpydoc`` packages. [#368]

- openmp_helpers.py: Make add_openmp_flags_if_available() work for clang.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changelog is way too verbose, any chance to cut it back a bit? E.g. no need for filenames, but can use double backticks to highlight the name of the new functions.

The necessary include, library, and runtime paths now get added to the C test code
used to determine if openmp works.
Autogenerator utility added ``openmp_enabled.is_openmp_enabled()``
which can be called post build to determine state of OpenMP support.
[#382]


3.0.2 (unreleased)
------------------
Expand Down
208 changes: 182 additions & 26 deletions astropy_helpers/openmp_helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,14 @@
import os
import sys
import glob
import time
import datetime
import tempfile
import subprocess

from distutils import log
from distutils.ccompiler import new_compiler
from distutils.sysconfig import customize_compiler
from distutils.sysconfig import customize_compiler, get_config_var
from distutils.errors import CompileError, LinkError

from .setup_helpers import get_compiler_option
Expand All @@ -39,69 +41,223 @@
}
"""

def _get_flag_value_from_var(flag, var, delim=' '):
"""
Utility to extract ``flag`` value from `os.environ[``var``]` or, if not present,
from `distutils.sysconfig.get_config_var(``var``)`.
E.g. to get include path: _get_flag_value_from_var('-I', 'CFLAGS')
might return "/usr/local/include".

def add_openmp_flags_if_available(extension):
Notes
-----
Not yet tested and therefore not yet supported on Windows
"""
Add OpenMP compilation flags, if available (if not a warning will be
printed to the console and no flags will be added)

Returns `True` if the flags were added, `False` otherwise.
if sys.platform.startswith('win'):
return None

# Simple input validation
if not var or not flag:
return None
flag_length = len(flag)
if not flag_length:
return None

# Look for var in os.eviron
try:
flags = os.environ[var]
except KeyError:
flags = None

# If os.environ was unsuccesful try in sysconfig
if not flags:
try:
flags = get_config_var(var)
except KeyError:
return None

# Extract flag from {var:value}
if flags:
for item in flags.split(delim):
if flag in item[:flag_length]:
return item[flag_length:]

return None

def _get_include_path():
return _get_flag_value_from_var('-I', 'CFLAGS')

def _get_library_path():
return _get_flag_value_from_var('-L', 'LDFLAGS')

def get_openmp_flags():
"""
Utility for returning compiler and linker flags possibly needed for
OpenMP support.

ccompiler = new_compiler()
customize_compiler(ccompiler)
Returns
-------
result : `{'compiler_flags':<flags>, 'linker_flags':<flags>}`

tmp_dir = tempfile.mkdtemp()
Notes
-----
The flags returned are not tested for validity, use
`check_openmp_support(openmp_flags=get_openmp_flags())` to do so.
"""

start_dir = os.path.abspath('.')
compile_flags = []
include_path = _get_include_path()
if include_path:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a comment explaining why you need to add library and include paths - e.g. does Python ignore environment variables by default? Sorry for being picky with comments, but we've been burned in the past with undocumented workarounds for issues in astropy-helpers, so I'm just making sure this is understandable by other developers :)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think commenting in get_openmp_flags is correct, semantically the use for why you want these flags belongs to the caller as they don't actually have to be passed to a compiler at this stage, they may just be checked or printed etc. _get_flag_value_from_var already docs extraction method, i.e. from os.environ[var] or, if not present, from distutils.sysconfig.get_config_var(var).

I believe the level of doc you want already exists here, where they are actually being used as flags passed to the compiler.

        # customize_compiler() extracts info from os.environ. If certain keys
        # exist it uses these plus those from sysconfig.get_config_vars().
        # If the key is missing in os.environ it is not extracted from 
        # sysconfig.get_config_var(). E.g. 'LDFLAGS' get left out, preventing
        # clang from finding libomp.dylib because -L<path> is not passed to linker.
        # Call get_openmp_flags() to get flags missed by customize_compiler().

Is this cool as is?

compile_flags.append('-I' + include_path)

link_flags = []
lib_path = _get_library_path()
if lib_path:
link_flags.append('-L' + lib_path)

if get_compiler_option() == 'msvc':
compile_flag = '-openmp'
link_flag = ''
compile_flags.append('-openmp')
else:
compile_flag = '-fopenmp'
link_flag = '-fopenmp'
compile_flags.append('-fopenmp')
link_flags.append('-fopenmp')
if lib_path:
link_flags.append('-Wl,-rpath,' + lib_path)

try:
return {'compiler_flags':compile_flags, 'linker_flags':link_flags}

def check_openmp_support(openmp_flags=None):
"""
Compile and run OpenMP test code to determine viable support.

Parameters
----------
openmp_flags : dictionary, optional
Expecting `{'compiler_flags':<flags>, 'linker_flags':<flags>}`.
These are passed as `extra_postargs` to `compile()` and
`link_executable()` respectively.

Returns
-------
result : bool
`True` if the test passed, `False` otherwise.
"""

ccompiler = new_compiler()
customize_compiler(ccompiler)

if not openmp_flags:
# customize_compiler() extracts info from os.environ. If certain keys
# exist it uses these plus those from sysconfig.get_config_vars().
# If the key is missing in os.environ it is not extracted from
# sysconfig.get_config_var(). E.g. 'LDFLAGS' get left out, preventing
# clang from finding libomp.dylib because -L<path> is not passed to linker.
# Call get_openmp_flags() to get flags missed by customize_compiler().
openmp_flags = get_openmp_flags()
compile_flags = openmp_flags['compiler_flags'] if 'compiler_flags' in openmp_flags else None
link_flags = openmp_flags['linker_flags'] if 'linker_flags' in openmp_flags else None

tmp_dir = tempfile.mkdtemp()
start_dir = os.path.abspath('.')

try:
os.chdir(tmp_dir)

# Write test program
with open('test_openmp.c', 'w') as f:
f.write(CCODE)

os.mkdir('objects')

# Compile, link, and run test program
ccompiler.compile(['test_openmp.c'], output_dir='objects', extra_postargs=[compile_flag])
ccompiler.link_executable(glob.glob(os.path.join('objects', '*' + ccompiler.obj_extension)), 'test_openmp', extra_postargs=[link_flag])
ccompiler.compile(['test_openmp.c'], output_dir='objects', extra_postargs=compile_flags)
ccompiler.link_executable(glob.glob(os.path.join('objects', '*' + ccompiler.obj_extension)), 'test_openmp', extra_postargs=link_flags)
output = subprocess.check_output('./test_openmp').decode(sys.stdout.encoding or 'utf-8').splitlines()

if 'nthreads=' in output[0]:
nthreads = int(output[0].strip().split('=')[1])
if len(output) == nthreads:
using_openmp = True
is_openmp_supported = True
else:
log.warn("Unexpected number of lines from output of test OpenMP "
"program (output was {0})".format(output))
using_openmp = False
is_openmp_supported = False
else:
log.warn("Unexpected output from test OpenMP "
"program (output was {0})".format(output))
using_openmp = False

is_openmp_supported = False
except (CompileError, LinkError):

using_openmp = False
is_openmp_supported = False

finally:

os.chdir(start_dir)

return is_openmp_supported

def is_openmp_supported():
"""
Utility to determine whether the build compiler
has OpenMP support.
"""

# don't log
log_threshold = log.set_threshold(log.FATAL)
ret = check_openmp_support()
log.set_threshold(log_threshold)
return ret

def add_openmp_flags_if_available(extension):
"""
Add OpenMP compilation flags, if supported (if not a warning will be
printed to the console and no flags will be added.)

Returns `True` if the flags were added, `False` otherwise.
"""

openmp_flags = get_openmp_flags()
using_openmp = check_openmp_support(openmp_flags=openmp_flags)

if using_openmp:
log.info("Compiling Cython extension with OpenMP support")
extension.extra_compile_args.append(compile_flag)
extension.extra_link_args.append(link_flag)
compile_flags = openmp_flags['compiler_flags'] if 'compiler_flags' in openmp_flags else None
link_flags = openmp_flags['linker_flags'] if 'linker_flags' in openmp_flags else None
log.info("Compiling Cython/C/C++ extension with OpenMP support")
extension.extra_compile_args.extend(compile_flags)
extension.extra_link_args.extend(link_flags)
else:
log.warn("Cannot compile Cython extension with OpenMP, reverting to non-parallel code")

return using_openmp

_IS_OPENMP_ENABLED_SRC = """
# Autogenerated by {packagetitle}'s setup.py on {timestamp!s}

def is_openmp_enabled():
\'\'\'
Autogenerated utility to determine, post build, whether the package
was built with or without OpenMP support.
\'\'\'
return {return_bool}
"""[1:]

def generate_openmp_enabled_py(packagename, srcdir='.'):
"""
Utility for creating `openmp_enabled.is_openmp_enabled()`
used to determine, post build, whether the package was built
with or without OpenMP support.
"""

if packagename.lower() == 'astropy':
packagetitle = 'Astropy'
else:
packagetitle = packagename

epoch = int(os.environ.get('SOURCE_DATE_EPOCH', time.time()))
timestamp = datetime.datetime.utcfromtimestamp(epoch)

src = _IS_OPENMP_ENABLED_SRC.format(packagetitle=packagetitle,
timestamp=timestamp,
return_bool=is_openmp_supported())

package_srcdir = os.path.join(srcdir, *packagename.split('.'))
is_openmp_enabled_py = os.path.join(package_srcdir, 'openmp_enabled.py')
with open(is_openmp_enabled_py, 'w') as f:
f.write(src)
46 changes: 43 additions & 3 deletions astropy_helpers/tests/test_openmp_helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,23 @@
from copy import deepcopy
from distutils.core import Extension

from ..openmp_helpers import add_openmp_flags_if_available
from ..openmp_helpers import add_openmp_flags_if_available, generate_openmp_enabled_py
from ..setup_helpers import _module_state, register_commands

IS_TRAVIS_LINUX = os.environ.get('TRAVIS_OS_NAME', None) == 'linux'
IS_TRAVIS_OSX = os.environ.get('TRAVIS_OS_NAME', None) == 'osx'
IS_APPVEYOR = os.environ.get('APPVEYOR', None) == 'True'
PY3_LT_35 = sys.version_info[0] == 3 and sys.version_info[1] < 5

_state = None

try:
TEST_OPENMP = 'True' == os.environ['TEST_OPENMP']
except KeyError:
if IS_APPVEYOR:
TEST_OPENMP = True
else:
raise

def setup_function(function):
global state
Expand All @@ -34,5 +42,37 @@ def test_add_openmp_flags_if_available():
# Having this is useful because we'll find out if OpenMP no longer works
# for any reason on platforms on which it does work at the time of writing.
# OpenMP doesn't work on Python 3.x where x<5 on AppVeyor though.
if IS_TRAVIS_LINUX or (IS_APPVEYOR and not PY3_LT_35):
assert using_openmp
if IS_TRAVIS_LINUX or IS_TRAVIS_OSX or (IS_APPVEYOR and not PY3_LT_35):
if TEST_OPENMP:
assert using_openmp
else:
assert not using_openmp

def test_generate_openmp_enabled_py():

register_commands('openmp_autogeneration_testing', '0.0', False)

# Test file generation
generate_openmp_enabled_py('')
assert os.path.isfile('openmp_enabled.py')

with open('openmp_enabled.py', 'r') as fid:
contents = fid.read()
print(contents)

# Travis OSX tests experience an unstable module import error.
# Sometimes finding and importing the module, `openmp_enabled`,
# more often not. Adding '.' to the path seems to stabilize the
# issue, even though it should not be needed.
sys.path.append('.')
from openmp_enabled import is_openmp_enabled

is_openmp_enabled = is_openmp_enabled()

# Test is_openmp_enabled()
assert isinstance(is_openmp_enabled, bool)

if TEST_OPENMP:
assert is_openmp_enabled
else:
assert not is_openmp_enabled