Make add_openmp_flags_if_available() work for clang by jamienoss · Pull Request #382 · astropy/astropy-helpers

jamienoss · 2018-03-13T16:18:01Z

The small openmp C test code, used to determine if openmp works, fails to build with clang due to missing include and library paths for openmp. When run, it then fails due to rpath (runtime path) issues.

This PR adds the necessary include, library, and runtime paths to the build.

Signed-off-by: James Noss [email protected]

astropy-bot · 2018-03-13T16:18:04Z

Hi there @jamienoss 👋 - thanks for the pull request! I'm just a friendly 🤖 that checks for issues related to the changelog and making sure that this pull request is milestoned and labeled correctly. This is mainly intended for the maintainers, so if you are not a maintainer you can ignore this, and a maintainer will let you know if any action is required on your part 😃.

Everything looks good from my point of view! 👍

If there are any issues with this message, please report them here.

jamienoss · 2018-03-13T16:20:10Z

Let me know if there's a better/neater/preferred way of doing any of this.

jamienoss · 2018-03-16T04:45:32Z

This is required for astropy/astropy#7293

astrofrog · 2018-03-16T09:25:46Z

Thanks! I will review this next week.

astrofrog

Thanks for working on this! How portable is the solution related to LDFLAGS and CFLAGS on e.g Windows? Should we check that the platform is Linux/MacOS X and only fetch the extra flags from there?

Please update the test here:

astropy-helpers/astropy_helpers/tests/test_openmp_helpers.py

Line 37 in c90cabb

if IS_TRAVIS_LINUX or (IS_APPVEYOR and not PY3_LT_35):

to also make sure that OpenMP works on Mac.

Finally, please add a test for the generate_openmp_enabled_py functionality.

astrofrog · 2018-03-22T11:07:31Z

 }
 """

+def _split_option_from_var(option, var, delim=' '):


This function needs a docstring for clarity (astropy-helpers code has typically been under-documented in the past, causing problems down the road, so we need to make sure all new code is easy to understand/read). Also please include some more comments below in the function to explain a bit more what is going on.

I'm going to slightly refac this and rename it, sorry.

astrofrog · 2018-03-22T11:12:40Z

-        log.info("Compiling Cython extension with OpenMP support")
-        extension.extra_compile_args.append(compile_flag)
-        extension.extra_link_args.append(link_flag)
+        log.info("Compiling Cython/C/C++/ extension with OpenMP support")


remove / after C++

astrofrog · 2018-03-22T11:15:15Z

+def _get_library_path():
+    return _split_option_from_var('-L', 'LDFLAGS')

 def add_openmp_flags_if_available(extension):


Add a note in the docstring that one can pass None to find out if OpenMP compilation would work without doing it for a specific extension.

Will do. Maybe it's also a good idea to make it the default, extension=None, and also add a "wafer thin" wrapper with a more semantically correct name?

Should probably suppress the exception when None also(?)

I don't know if it's super important to have the semantically correct name, you can always just put a comment above the call to the function to explain what it does in that case?

astrofrog · 2018-03-22T11:15:48Z

+    if packagename.lower() == 'astropy':
+        packagetitle = 'Astropy'
+    else:
+        packagetitle = 'Astropy-affiliated package ' + packagename


Not all packages using astropy-helpers are affiliated packages, so I would just set packagetitle = packagename

jamienoss · 2018-03-22T16:07:39Z

@astrofrog

How portable is the solution related to LDFLAGS and CFLAGS on e.g Windows? Should we check that the platform is Linux/MacOS X and only fetch the extra flags from there?

Good call, I'll make this conditional for now and we can address later(?).

astrofrog · 2018-03-22T16:18:19Z

Good call, I'll make this conditional for now and we can address later(?).

Sounds good!

jamienoss · 2018-03-22T16:37:34Z

@astrofrog
Regarding the Travis test, do you want to keep this exploratory as the following comment suggests or explicit? By this I mean, should I make the assertion conditional upon the version of clang/llvm-openmp?

 # Make sure that on Travis (Linux) and AppVeyor OpenMP does get used (for
    # MacOS X usually it will not work but this will depend on the compiler).
    # 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.

jamienoss · 2018-03-22T20:04:11Z

@astrofrog
I made requested changes, though, upon looking at it again, I didn't like the hacky overloaded use of add_openmp_flags_if_available(extension) so I split it up. Sorry, I know this means more for you to review.

Regarding the new test test_generate_openmp_enabled_py(), I was really only taking a guess here,
I have no idea of the test framework when it comes to writing to the filesystem during the test. I took a look at some in test_git_helpers.py for the autogeneration of version.py but it looked like overkill for what I needed, but perhaps this is the part where I have no idea.

jamienoss · 2018-03-22T20:17:06Z

@astrofrog hold.....

jamienoss · 2018-03-22T22:31:08Z

@astrofrog and go...

Sorry, I messed up the testing.

I'm looking into what can be done about the OSX test but you're all good to review otherwise. Cheers

jamienoss · 2018-04-19T18:13:04Z

By the way, just a thought - could one option be to include a test against the new compilers in conda, which I think should support OpenMP?

This was the first thing I tried and it didn't work. I tried it again, just now, as a sanity check but still nothing. I don't know why as this works fine on my machine, it was how I initially installed an OpenMP working clang, which is why I tried it first. Any ideas what might be going on?

Also, there is an unstable failure of the import of the generated openmp_enabled, but only on OSX and only sporadically will it import ok.

E       ImportError: No module named 'openmp_enabled'

Any ideas here? I tried adding in a sleep before the import (rationale=magic) but it did nothing. I wondered if it might be a caching issue but couldn't think how.

I would gratefully appreciate some affirmative direction on this PR as I need to move on and spend my time elsewhere.

jamienoss · 2018-04-19T18:14:09Z

I used - export CC=/anaconda/bin/clang maybe this is not correct for the Travis build?

jamienoss · 2018-04-19T18:20:08Z

I used - export CC=/anaconda/bin/clang maybe this is not correct for the Travis build?

I just noticed that CC=gcc occurs after the above... Trying again and moving the export to before_install.

jamienoss · 2018-04-19T18:55:09Z

Trying /Users/travis/miniconda/envs/test/bin/clang...

jamienoss · 2018-04-20T17:15:30Z

@astrofrog I think this should be all done now.

I got clang working via conda, I did the same for gcc on OSX to drop all of the scripts and reduce the number of OSX tests.

I addressed the import stability issue by appending the current dir, '.', to sys.path, this shouldn't be needed but it seems to work. Here are the last three successful test builds

astrofrog

Great that you got things working with the conda compilers! So just to check, do we still need the added complexity in terms of checking environment variables in the OpenMP helpers? Or do the conda compilers support -openmp properly?

Just to be clear, I'm in favor of the refactoring that makes it possible to know if a package was compiled with OpenMP, but I just want to make sure all the other changes are still needed.

astrofrog · 2018-04-24T15:03:57Z

+
+      # Test OSX without OpenMP support
+      # Since the matrix OSX tests use the OS shipped version of clang, they double up
+      # as exploritory tests for when the shipped version has automatic OpenMP support.


exploritory -> exploratory

astrofrog · 2018-04-24T15:04:57Z

+      # 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


Minor: I'd set TEST_OPENMP=True as a global higher up and just set it to False on MacOS X

It's never good design to split conditionals like this. State logic should be as independent as it can be and contained within a single place. Splitting it out increases complexity and dependency, this is especially true here when considering stage overloading. Basically, it should only be set once and since this can't exist as a global (due to it being conditional on OS), it needs to exist within an actual eval stage.

If before_install is overloaded and TEST_OPENMP not set, the test will raise an exception here alerting the developer that they have forgotten to add this to their overloaded version.

jamienoss · 2018-04-24T17:28:58Z

So just to check, do we still need the added complexity in terms of checking environment variables in the OpenMP helpers? Or do the conda compilers support -openmp properly?

Yes, they are needed to find and link with libomp. -fopenmp just tells the compiler to except the OpenMP pragma/directives and, when used as a wrapper to the linker, to link with libomp. With gcc, the library is packaged and shipped with the core libraries so only -fopenmp is needed, this is generally not true of the others though. It is also possible to use llvm-openmp library (or any other) when compiling with gcc, so making the build more explicit allows for this.

...I just want to make sure all the other changes are still needed.

They are.

jamienoss · 2018-04-25T15:55:47Z

@astrofrog

...I just want to make sure all the other changes are still needed.

They are.

Well... the one thing I am slightly unsure of is how the path flags play out, especially -rpath, when it gets built and released by conda. ~~I don't know how to test this either.~~

I just spoke to @rendinam about this and I can try it out using conda-build and then installing it into a diff conda env. I will do so now.

The small openmp C test code used to determine if openmp works, fails to build with clang due to missing include and library paths for openmp. When run, it then fails due to rpath (runtime path) issues. This PR adds the necessary include, library, and runtime paths to the build. Signed-off-by: James Noss <[email protected]>

jamienoss · 2018-04-26T19:30:02Z

Note: ~~experiencing~~ plagued by astropy/astropy#6424

Opened astropy/astropy#7409

astrofrog · 2018-05-25T17:09:00Z

@jamienoss - just to check, is this ready to merge from your side?

jamienoss · 2018-05-25T17:18:51Z

@astrofrog

just to check, is this ready to merge from your side?

Yes and no, sorry. I finally got the conda build working, using astropy/astropy#7293 as the test case. It built, tests passed but only when using gcc due to conda's LLVM (v4.0.1) bug, which is why I originally went with the manualy installs here.

What I haven't managed to get around to, is taking that build and trying to install it on another machine or when anaconda has a diff path (i.e. not root - which it is on my (build) machine so may just end up working by coincidence). This last test would test any rpath concerns.

Other than this though, it is good by me.

jamienoss · 2018-05-25T18:16:18Z

@astrofrog Just tested it, it works. Only thing is that, again, this is with gcc and not clang and llvm, which this PR is semantically designed to address. The rpaths didn't create an issue. I checked them using otool -L (on lib_convolve*.so) and conda had replaced the path literals. I think this should be enough, without testing on the actual conda build machines/env.

What are your thoughts?

bsipocz · 2018-05-25T19:49:21Z

+      # 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


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

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.

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.

bsipocz · 2018-05-25T19:50:07Z

+      env:
+        - PYTHON_VERSION=3.5
+        - CONDA_DEPENDENCIES="setuptools sphinx cython numpy pytest-cov clang llvm-openmp"
+      before_install:


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

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.

c.f. https://github.com/astropy/astropy-helpers/pull/382/files#r191006134

bsipocz · 2018-05-25T19:51:40Z

  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.


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.

bsipocz · 2018-05-25T19:57:05Z

 import sys
-import glob
 import tempfile
-import subprocess


Please don't shuffle the imports around but just add the new ones, they were sorted by length on purpose.

Woops, sorry, probably an auto-format.

Do you mean import name length? If so, I should insert the two additions accordingly, right, rather than just append them to the list?

bsipocz · 2018-05-25T19:58:21Z


+def _get_flag_value_from_var(flag, var, delim=' '):
+    """
+    Utility to extract `flag` value from `os.environ[`var`]` or, if not present,


use double backticks for parameters as a good practice, in public functions it would trip sphinx over otherwise (single backticks are for intersphinx links)

Ok, cool, thanks for the intel, will do.

bsipocz · 2018-05-25T19:59:13Z

+    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".


for which OS? Please keep comments it OS agnostic

This is why I added the "E.g." and "might".

bsipocz · 2018-05-25T20:04:59Z

-    try:
+    return {'compiler_flags':compile_flags, 'linker_flags':link_flags}
+
+def test_openmp_support(openmp_flags=None, silent=False):


rename this function to check_openmp_support or something, it's very confusing to see a test function in the main codebase (and I'm not 100% certain that pytest likes it either).

Good point, I hadn't thought of the pytest 'test_' namespace clash, cheers.

bsipocz · 2018-05-25T20:05:44Z

+        Expecting `{'compiler_flags':<flags>, 'linker_flags':<flags>}`.
+        These are passed as `extra_postargs` to `compile()` and
+        `link_executable()` respectively.
+    silent : bool, optional


Why is this parameter needed? Surely the verbosity can already been tuned via the logger

How? I couldn't find anything other than setting log.set_threshold(log.FATAL), which I am unsure of its semantic correctness.

bsipocz

There are still a few places that could benefit some cleanup.

bsipocz · 2018-05-25T20:09:33Z

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


if possible avoid single letter variables

astrofrog · 2018-06-05T11:38:14Z


    openmp_flags = get_openmp_flags()
-    using_openmp = test_openmp_support(openmp_flags=openmp_flags, silent=False)
+    using_openmp = check_openmp_support(openmp_flags=openmp_flags, silent=False)


I think silent=False needs to be removed to avoid the current test failures

Signed-off-by: James Noss <[email protected]>

astrofrog · 2018-06-18T15:06:03Z

I tested this out locally for astropy-healpix and it does indeed work nicely. It also looks like @bsipocz's comments have been addressed so I'll go ahead and merge this. Thanks @jamienoss!

jamienoss force-pushed the openmp.pr branch 7 times, most recently from 3d4b3be to 0eaacc9 Compare March 15, 2018 20:06

jamienoss mentioned this pull request Mar 16, 2018

Performance enhancements for convolution.convolve astropy/astropy#7293

Merged

astrofrog self-requested a review March 16, 2018 09:25

astrofrog suggested changes Mar 22, 2018

View reviewed changes

astrofrog modified the milestone: v3.1.0 Mar 22, 2018

jamienoss force-pushed the openmp.pr branch from 13d4572 to 8c0dd7e Compare March 22, 2018 20:13

jamienoss force-pushed the openmp.pr branch 3 times, most recently from c9fe414 to 8df39c8 Compare March 22, 2018 22:25

jamienoss force-pushed the openmp.pr branch 4 times, most recently from 729c7af to 56dd7b7 Compare March 23, 2018 15:07

astrofrog reviewed Apr 24, 2018

View reviewed changes

jamienoss mentioned this pull request Apr 26, 2018

Move clone of ci-helpers to script #389

Closed

bsipocz reviewed May 25, 2018

View reviewed changes

bsipocz suggested changes May 25, 2018

View reviewed changes

bsipocz reviewed May 25, 2018

View reviewed changes

astrofrog reviewed Jun 5, 2018

View reviewed changes

Post bsipocz review changes

1a635b9

Signed-off-by: James Noss <[email protected]>

astrofrog approved these changes Jun 18, 2018

View reviewed changes

This was referenced Aug 29, 2018

Issue with astropy-healpix when installing using pip astropy/astropy-healpix#78

Closed

Remove use of OpenMP? astropy/astropy-healpix#97

Closed

Conversation

jamienoss commented Mar 13, 2018

Uh oh!

astropy-bot Bot commented Mar 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jamienoss commented Mar 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jamienoss commented Mar 16, 2018

Uh oh!

astrofrog commented Mar 16, 2018

Uh oh!

astrofrog left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamienoss commented Mar 22, 2018

Uh oh!

astrofrog commented Mar 22, 2018

Uh oh!

jamienoss commented Mar 22, 2018

Uh oh!

jamienoss commented Mar 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jamienoss commented Mar 22, 2018

Uh oh!

jamienoss commented Mar 22, 2018

Uh oh!

jamienoss commented Apr 19, 2018

Uh oh!

jamienoss commented Apr 19, 2018

Uh oh!

jamienoss commented Apr 19, 2018

Uh oh!

jamienoss commented Apr 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jamienoss commented Apr 20, 2018

Uh oh!

astrofrog left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamienoss Apr 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamienoss commented Apr 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jamienoss commented Apr 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jamienoss commented Apr 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

astrofrog commented May 25, 2018

Uh oh!

jamienoss commented May 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

astropy-bot Bot commented Mar 13, 2018 •

edited

Loading

jamienoss commented Mar 13, 2018 •

edited

Loading

astrofrog left a comment •

edited

Loading

jamienoss commented Mar 22, 2018 •

edited

Loading

jamienoss commented Apr 19, 2018 •

edited

Loading

jamienoss Apr 24, 2018 •

edited

Loading

jamienoss commented Apr 24, 2018 •

edited

Loading

jamienoss commented Apr 25, 2018 •

edited

Loading

jamienoss commented Apr 26, 2018 •

edited

Loading

jamienoss commented May 25, 2018 •

edited

Loading

jamienoss Jun 4, 2018 •

edited

Loading