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

Add functions to check if OpenMP is working #1000

Merged
merged 14 commits into from
Dec 9, 2022
Merged

Conversation

hsinfan1996
Copy link
Contributor

I haven't written in C language for a while. This is how I check if OpenMP is working on the compiled pyccl. Checked on macOS 10.15.7 with Apple Clang with OpenMP disabled and homebrew-installed Clang 15, the function is working as expected even if the environment variable "OMP_NUM_THREAD" is set to 0.

@coveralls
Copy link

coveralls commented Oct 11, 2022

Pull Request Test Coverage Report for Build 3645632564

  • 2 of 4 (50.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.04%) to 97.524%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pyccl/pyutils.py 2 4 50.0%
Totals Coverage Status
Change from base Build 3481038780: -0.04%
Covered Lines: 4844
Relevant Lines: 4967

💛 - Coveralls

@hsinfan1996
Copy link
Contributor Author

hsinfan1996 commented Oct 11, 2022

@tilmantroester I wonder if add_link_options("LINKER:-lomp") will prevent you from compiling on your machine, since it looks like the link option is not needed for Apple Clang on macOS 12.

@tilmantroester
Copy link
Contributor

Is the add_link_options("LINKER:-lomp") necessary? This should be automatically added if openmp is found. Can you add file(GENERATE OUTPUT "omp_lib_generator.txt" CONTENT $<$<BOOL:${OpenMP_C_FOUND}>:OpenMP::OpenMP_C>) to the CMakeLists.txt and check if there's any output? Setting set( CMAKE_VERBOSE_MAKEFILE on ) also shows the full build command and should show if the openmp library is added to the linker.

@hsinfan1996
Copy link
Contributor Author

Is the add_link_options("LINKER:-lomp") necessary? This should be automatically added if openmp is found. Can you add file(GENERATE OUTPUT "omp_lib_generator.txt" CONTENT $<$<BOOL:${OpenMP_C_FOUND}>:OpenMP::OpenMP_C>) to the CMakeLists.txt and check if there's any output? Setting set( CMAKE_VERBOSE_MAKEFILE on ) also shows the full build command and should show if the openmp library is added to the linker.

This is what I got in the file

OpenMP::OpenMP_C

@hsinfan1996 hsinfan1996 marked this pull request as draft October 11, 2022 17:25
@nikfilippas
Copy link
Contributor

This is the 1000th tag! 🎉🎉🎉

@hsinfan1996
Copy link
Contributor Author

The c unit tests in ci on ubuntu did not run any tests (or the log did not show it?). Is this behaviour expected?
https://github.com/LSSTDESC/CCL/actions/runs/3237240260/jobs/5304053146#step:10:18

@hsinfan1996
Copy link
Contributor Author

hsinfan1996 commented Oct 13, 2022

Just want to leave a note for Apple Clang users with homebrew-installed libomp on macOS 10.15. If the installation failed or there are errors when python tries to import pyccl, in the shell, execute export LDFLAGS="-L/usr/local/lib -lomp" and export CFLAGS="-I/usr/local/opt/libomp/include" before pip install . (or python setup.py install).

@hsinfan1996 hsinfan1996 marked this pull request as ready for review October 13, 2022 23:18
@damonge
Copy link
Collaborator

damonge commented Nov 16, 2022

@hsinfan1996 @tilmantroester can I check where we stand on this PR?

@hsinfan1996
Copy link
Contributor Author

hsinfan1996 commented Nov 16, 2022

@hsinfan1996 @tilmantroester can I check where we stand on this PR?

The functions checking OpenMP version and threads are done. However, some Apple Clang compilers (e.g. on my machine running MacOS 10.15) cannot link homebrew-installed libomp. The latest commit (d892a30) solves this issue by forcing libomp to link with all compiled library. I will revert the commit if this is not a proper way to solve the problem. I am happy with the changes to CMakeLists.txt that @tilmantroester made. If there is no more to add to CMakeLists.txt, then this PR is ready for review.

@damonge
Copy link
Collaborator

damonge commented Nov 16, 2022

OK, can we run the tests and check they pass?

@damonge
Copy link
Collaborator

damonge commented Nov 16, 2022

@tilmantroester you've paid more attention than me to this. Are you happy to approve?

pyccl/pyutils.py Outdated
if openmp_version > 0:
print(f'OpenMP version: {openmp_version}')
else:
print('OpenMP is not supported')
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be preferable to have functions that return the numerical values with the expected values documented in a doc string, rather than print messages to stdout.
Right now they can't be used in automated scripts, for example.

Copy link
Contributor

@tilmantroester tilmantroester left a comment

Choose a reason for hiding this comment

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

LGTM!

@hsinfan1996 hsinfan1996 changed the title Add function to check if OpenMP is working Add functions to check if OpenMP is working Dec 9, 2022
@hsinfan1996 hsinfan1996 merged commit bc129ec into master Dec 9, 2022
@hsinfan1996 hsinfan1996 deleted the openmp_check branch December 9, 2022 07:26
@hsinfan1996 hsinfan1996 mentioned this pull request Jan 6, 2023
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 this pull request may close these issues.

5 participants