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.

Sign up for GitHub

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 broken build flag, move build to one directory #2442

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

fix broken build flag, move build to one directory #2442

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c7c3025
move build to one directory, fix broken flag
sam-herman Jan 25, 2025
232f840
fix make path
sam-herman Jan 25, 2025
53943ec
changelog update
sam-herman Jan 25, 2025
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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
* Fixing it to retrieve space_type from index setting when both method and top level don't have the value. [#2374](https://github.com/opensearch-project/k-NN/pull/2374)
* Fixing the bug where setting rescore as false for on_disk knn_vector query is a no-op (#2399)[https://github.com/opensearch-project/k-NN/pull/2399]
* Fixing bug where mapping accepts both dimension and model-id (#2410)[https://github.com/opensearch-project/k-NN/pull/2410]
* Fixing bug where cmake condition to detect flag was broken and fix build path for JNI (#2442)[https://github.com/opensearch-project/k-NN/pull/2442]
### Infrastructure
* Updated C++ version in JNI from c++11 to c++17 [#2259](https://github.com/opensearch-project/k-NN/pull/2259)
* Upgrade bytebuddy and objenesis version to match OpenSearch core and, update github ci runner for macos [#2279](https://github.com/opensearch-project/k-NN/pull/2279)
Expand Down
4 changes: 2 additions & 2 deletions build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -333,7 +333,7 @@ task cmakeJniLib(type:Exec) {
workingDir 'jni'
def args = []
args.add("cmake")
args.add(".")
Copy link
Member

Choose a reason for hiding this comment

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

Good update. This makes cleaning some much easier. That being said, in the buildJniLib, instead of changing workingDir, can we just do:

make -Cbuild

I worry if we add "/" we might end up having issues with path resolution on different platforms (unless gradle handles this for us)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The windows build seems to work fine https://github.com/opensearch-project/k-NN/actions/runs/13020810027/job/36325044749?pr=2442

args.add("-B build")
args.add("-DKNN_PLUGIN_VERSION=${opensearch_version}")
args.add("-DAVX2_ENABLED=${avx2_enabled}")
args.add("-DAVX512_ENABLED=${avx512_enabled}")
Expand All @@ -358,7 +358,7 @@ task cmakeJniLib(type:Exec) {

task buildJniLib(type:Exec) {
dependsOn cmakeJniLib
workingDir 'jni'
workingDir 'jni/build'
commandLine 'make', 'opensearchknn_nmslib', 'opensearchknn_faiss', 'opensearchknn_common', '-j', "${nproc_count}"
}

Expand Down
6 changes: 3 additions & 3 deletions jni/cmake/init-faiss.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,10 +111,10 @@ endif()
if(NOT DEFINED AVX512_SPR_ENABLED)
# Check if the system is Intel(R) Sapphire Rapids or a newer-generation processor
execute_process(COMMAND bash -c "lscpu | grep -q 'GenuineIntel' && lscpu | grep -i 'avx512_fp16' | grep -i 'avx512_bf16' | grep -i 'avx512_vpopcntdq'" OUTPUT_VARIABLE SPR_FLAGS OUTPUT_STRIP_TRAILING_WHITESPACE)
if (AND NOT "${SPR_FLAGS}" STREQUAL "")
Copy link
Member

Choose a reason for hiding this comment

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

Good catch @sam-herman . @mulugetam Can you check this and make sure this change makes sense?

set(AVX512_SPR_ENABLED true)
if (NOT "${SPR_FLAGS}" STREQUAL "")
set(AVX512_SPR_ENABLED true)
else()
set(AVX512_SPR_ENABLED false)
set(AVX512_SPR_ENABLED false)
endif()
endif()

Expand Down
Loading