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

Fix build CI failure on MacOS #1685

Merged
merged 2 commits into from
May 3, 2024

Conversation

junqiu-lei
Copy link
Member

@junqiu-lei junqiu-lei commented May 2, 2024

Description

Fix build CI failure for MacOS. The github CI runner for macos-latest changed to use ARM platform recently, updating to use macos-12 with x86_64 architecture to pass the CI.

Issues Resolved

#1660

Check List

  • Commits are signed as per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@junqiu-lei junqiu-lei force-pushed the fix-macos-ci branch 7 times, most recently from 9cd8cc7 to fe91a0f Compare May 3, 2024 00:21
@junqiu-lei junqiu-lei changed the title Try to fix build CI failure for MacOS Fix build CI failure on MacOS May 3, 2024
@junqiu-lei junqiu-lei added the Infrastructure Changes to infrastructure, testing, CI/CD, pipelines, etc. label May 3, 2024
@junqiu-lei
Copy link
Member Author

junqiu-lei commented May 3, 2024

Currently k-NN native libraries can completely make build in macos-latest CI runner env, the new error occurred in integ test:

»  java.lang.UnsatisfiedLinkError: no opensearchknn_common in java.library.path: [/Users/runner/work/k-NN/k-NN/jni/release]
»  	at java.base/java.lang.ClassLoader.loadLibrary(ClassLoader.java:2678)
»  	at java.base/java.lang.Runtime.loadLibrary0(Runtime.java:830)
»  	at java.base/java.lang.System.loadLibrary(System.java:1890)
»  	at org.opensearch.knn.jni.JNICommons.lambda$static$0(JNICommons.java:26)
»  	at java.base/java.security.AccessController.doPrivileged(Native Method)
»  	at org.opensearch.knn.jni.JNICommons.<clinit>(JNICommons.java:25)
»  	at org.opensearch.knn.index.codec.util.KNNCodecUtil.getFloats(KNNCodecUtil.java:89)
»  	at 

sed -i -e 's/__aarch64__/__undefine_aarch64__/g' external/faiss/faiss/utils/distances_simd.cpp
sed -i -e 's/pragma message WARN/pragma message /g' external/nmslib/similarity_search/src/distcomp_scalar.cc
sed -i -e 's/-march=native/-mcpu=apple-m1/g' external/nmslib/similarity_search/CMakeLists.txt
sed -i -e 's/-mcpu=apple-a14/-mcpu=apple-m1/g' external/nmslib/python_bindings/setup.py
Copy link
Member

Choose a reason for hiding this comment

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

we dont use python bindings so this should be unnecessary.

.github/workflows/CI.yml Outdated Show resolved Hide resolved
sed -i -e 's/pragma message WARN/pragma message /g' external/nmslib/similarity_search/src/distcomp_scalar.cc
sed -i -e 's/-march=native/-mcpu=apple-m1/g' external/nmslib/similarity_search/CMakeLists.txt
sed -i -e 's/-mcpu=apple-a14/-mcpu=apple-m1/g' external/nmslib/python_bindings/setup.py
if sysctl -n machdep.cpu.features | grep -i AVX2;
Copy link
Member

Choose a reason for hiding this comment

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

We dont need this if runner is arm

Copy link
Member

Choose a reason for hiding this comment

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

same as below

export CC=/opt/homebrew/opt/llvm/bin/clang
export CXX=/opt/homebrew/opt/llvm/bin/clang++
cd jni
sed -i -e 's/\/usr\/local\/opt\/libomp\//\/opt\/homebrew\/opt\/llvm\//g' cmake/init-faiss.cmake
Copy link
Member

Choose a reason for hiding this comment

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

Can we just add this check in cmake?

export CXX=/opt/homebrew/opt/llvm/bin/clang++
cd jni
sed -i -e 's/\/usr\/local\/opt\/libomp\//\/opt\/homebrew\/opt\/llvm\//g' cmake/init-faiss.cmake
sed -i -e 's/__aarch64__/__undefine_aarch64__/g' external/faiss/faiss/utils/distances_simd.cpp
Copy link
Member

Choose a reason for hiding this comment

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

This should be possible to avoid. In faiss, I think they are building it without patches: facebookresearch/faiss#3411.

cd jni
sed -i -e 's/\/usr\/local\/opt\/libomp\//\/opt\/homebrew\/opt\/llvm\//g' cmake/init-faiss.cmake
sed -i -e 's/__aarch64__/__undefine_aarch64__/g' external/faiss/faiss/utils/distances_simd.cpp
sed -i -e 's/pragma message WARN/pragma message /g' external/nmslib/similarity_search/src/distcomp_scalar.cc
Copy link
Member

Choose a reason for hiding this comment

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

why do we need to switch the WARN message?

sed -i -e 's/\/usr\/local\/opt\/libomp\//\/opt\/homebrew\/opt\/llvm\//g' cmake/init-faiss.cmake
sed -i -e 's/__aarch64__/__undefine_aarch64__/g' external/faiss/faiss/utils/distances_simd.cpp
sed -i -e 's/pragma message WARN/pragma message /g' external/nmslib/similarity_search/src/distcomp_scalar.cc
sed -i -e 's/-march=native/-mcpu=apple-m1/g' external/nmslib/similarity_search/CMakeLists.txt
Copy link
Member

Choose a reason for hiding this comment

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

I think that native should be fine. We shouldnt need this

@junqiu-lei
Copy link
Member Author

Thank @jmazanec15 for comments. For the arm setup, I followed steps from k-NN developer guide doc. But currently the it has another new error happened during k-NN run time in integ test when we use the macos-latest.

@junqiu-lei
Copy link
Member Author

junqiu-lei commented May 3, 2024

After we just changed the label to use macos-12(x86_64 architecture) which is used from most recent successful run, the CI on MacOS can be passed. https://github.com/opensearch-project/k-NN/actions/runs/8942266898/job/24564490315?pr=1685

@junqiu-lei junqiu-lei marked this pull request as ready for review May 3, 2024 17:48
brew reinstall gcc
export FC=/usr/local/Cellar/gcc/12.2.0/bin/gfortran
brew install llvm
brew install openblas
Copy link
Member

Choose a reason for hiding this comment

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

I saw this message in logs:

2024-05-03T00:54:26.3237060Z openblas is keg-only, which means it was not symlinked into /opt/homebrew,
2024-05-03T00:54:26.3237780Z because macOS provides BLAS in Accelerate.framework.

Can we skip this?

run: |
git submodule update --init --recursive
brew reinstall gcc
Copy link
Member

Choose a reason for hiding this comment

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

If we're using clang, I dont think we need gcc

@junqiu-lei junqiu-lei merged commit 73d5425 into opensearch-project:main May 3, 2024
58 of 60 checks passed
@junqiu-lei junqiu-lei deleted the fix-macos-ci branch May 3, 2024 22:27
opensearch-trigger-bot bot pushed a commit that referenced this pull request May 3, 2024
Signed-off-by: Junqiu Lei <[email protected]>
(cherry picked from commit 73d5425)
junqiu-lei pushed a commit that referenced this pull request May 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x Infrastructure Changes to infrastructure, testing, CI/CD, pipelines, etc. skip-changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants