Skip to content

CI Make no-OpenMP build fail with unprotected cimport openmp #25391

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

Merged
merged 7 commits into from
Jan 13, 2023

Conversation

lesteve
Copy link
Member

@lesteve lesteve commented Jan 13, 2023

Reference Issues/PRs

Fix #24694

What does this implement/fix? Explain your changes.

This is an attempt so that unprotected cimport openmp make the no-OpenMP build fail.

See #24682 (comment) for more details.

@lesteve lesteve changed the title [azure parallel] More stringent test of no-OpenMP CI Make no-OpenMP fail with unprotected cimport openmp Jan 13, 2023
@lesteve
Copy link
Member Author

lesteve commented Jan 13, 2023

This seems to work as intended, adding a unprotected cimport openmp does make the no-OpenMP build fail. Now the question is "is it worth it"?

From this build log

building 'sklearn._isotonic' extension
clang -Wno-unused-result -Wsign-compare -Wunreachable-code -DNDEBUG -fwrapv -O2 -Wall -fPIC -O2 -isystem /usr/local/miniconda/envs/testvenv/include -arch x86_64 -I/usr/local/miniconda/envs/testvenv/include -fPIC -O2 -isystem /usr/local/miniconda/envs/testvenv/include -arch x86_64 -DNPY_NO_DEPRECATED_API=NPY_1_7_API_VERSION -I/usr/local/miniconda/envs/testvenv/lib/python3.9/site-packages/numpy/core/include -I/usr/local/miniconda/envs/testvenv/include/python3.9 -c sklearn/_isotonic.c -o build/temp.macosx-10.9-x86_64-cpython-39/sklearn/_isotonic.o -g0 -O2
sklearn/_isotonic.c:783:10: fatal error: 'omp.h' file not found
#include <omp.h>
         ^~~~~~~

In the log you can also see the omp.h that are inside the conda env and deleted:

+ find /usr/local/miniconda/envs/testvenv -name omp.h -delete -print
/usr/local/miniconda/envs/testvenv/include/omp.h
/usr/local/miniconda/envs/testvenv/lib/clang/14.0.6/include/omp.h

@glemaitre
Copy link
Member

I merged main to avoid the circle ci failure

@glemaitre
Copy link
Member

Now the question is "is it worth it"?

I assume that this is more the spirit of what we tried to do in this specific CI.
@jeremiedbb WDYT?

@lesteve lesteve force-pushed the ci-no-openmp-build branch from 9281d0b to e9c23cc Compare January 13, 2023 13:04
@lesteve lesteve changed the title CI Make no-OpenMP fail with unprotected cimport openmp CI Make no-OpenMP build fail with unprotected cimport openmp Jan 13, 2023
@jeremiedbb
Copy link
Member

jeremiedbb commented Jan 13, 2023

Let me summarize because it took me a while to understand exactely what was not failing and why we want it to fail.

  • When we have a call to some openmp functions, e.g. omp.omp_get_num_threads, it can fail during compilation if there's no openmp installed or if the compiler is not properly set up to be linked against openmp. Unless such calls are protected as explained in the doc. The no openmp job on macos on the CI makes sure of that. We can see the no openmp warning in log installation logs and it would fail if we had unprotected openmp calls.

  • When we just import openmp, cimport openmp, then it will only fail if there's no openmp installed. This is because it just becomes #include <omp.h> but we're not actually trying to access any openmp symbol. This situation is cuurently not triggered by the no openmp job because conda installs openmp no matter what you do.

So I'm fine with just deleting the openmp header in the CI to make sure we don't have unprotected imports or calls.

Copy link
Member

@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

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

Assuming this still works with the current main, LGTM as well.

@ogrisel ogrisel enabled auto-merge (squash) January 13, 2023 14:34
@ogrisel ogrisel merged commit 6823e20 into scikit-learn:main Jan 13, 2023
@lesteve lesteve deleted the ci-no-openmp-build branch January 14, 2023 07:58
jjerphan pushed a commit to jjerphan/scikit-learn that referenced this pull request Jan 20, 2023
…t-learn#25391)



Co-authored-by: Jérémie du Boisberranger <34657725+jeremiedbb@users.noreply.github.com>
jjerphan pushed a commit to jjerphan/scikit-learn that referenced this pull request Jan 20, 2023
…t-learn#25391)



Co-authored-by: Jérémie du Boisberranger <34657725+jeremiedbb@users.noreply.github.com>
jjerphan pushed a commit to jjerphan/scikit-learn that referenced this pull request Jan 23, 2023
…t-learn#25391)



Co-authored-by: Jérémie du Boisberranger <34657725+jeremiedbb@users.noreply.github.com>
adrinjalali pushed a commit that referenced this pull request Jan 24, 2023

Co-authored-by: Jérémie du Boisberranger <34657725+jeremiedbb@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CI "no OpenMP" build environment actually has OpenMP
4 participants