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

Jump to bottom

Fix build failure due to patch conflict #1851

Merged
merged 1 commit into from
Jul 18, 2024
Merged

Fix build failure due to patch conflict #1851

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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 .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ jni/bin/
jni/lib/
jni/jni_test*
jni/googletest*
jni/cmake/*.cmake-e

benchmarks/perf-tool/okpt/output
benchmarks/perf-tool/okpt/dev
Expand Down
27 changes: 21 additions & 6 deletions build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,15 @@ buildscript {
opensearch_group = "org.opensearch"
isSnapshot = "true" == System.getProperty("build.snapshot", "true")
simd_enabled = System.getProperty("simd.enabled", "true")
// Flag to determine whether cmake build system should apply patches and commit. In automated build environments
// set this to false. In dev environments, set to true
// This flag determines whether the CMake build system should apply a custom patch. It prevents build failures
// when the cmakeJniLib task is run multiple times. If the build.lib.commit_patches is true, the CMake build
// system skips applying the patch if the patches have been applied already. If build.lib.commit_patches is
// false, the patches are always applied. To avoid patch conflicts, disable this flag manually after the first
// run of buildJniLib
apply_lib_patches = System.getProperty("build.lib.apply_patches", "true")
// Flag to determine whether cmake build system should commit the patch or not. In automated build environments
// set this to false. In dev environments, set to true. If false, repetitive execution of cmakeJniLib may fail.
// To prevent this, set build.lib.apply_patches to false after the first cmakeJniLib run.
commit_lib_patches = System.getProperty("build.lib.commit_patches", "true")

version_tokens = opensearch_version.tokenize('-')
Expand Down Expand Up @@ -304,13 +311,21 @@ task windowsPatches(type:Exec) {

task cmakeJniLib(type:Exec) {
workingDir 'jni'
def args = []
args.add("cmake")
args.add(".")
args.add("-DKNN_PLUGIN_VERSION=${opensearch_version}")
args.add("-DSIMD_ENABLED=${simd_enabled}")
args.add("-DCOMMIT_LIB_PATCHES=${commit_lib_patches}")
args.add("-DAPPLY_LIB_PATCHES=${apply_lib_patches}")
if (Os.isFamily(Os.FAMILY_WINDOWS)) {
dependsOn windowsPatches
commandLine 'cmake', '.', "-G", "Unix Makefiles", "-DKNN_PLUGIN_VERSION=${opensearch_version}", "-DBLAS_LIBRARIES=$rootDir\\src\\main\\resources\\windowsDependencies\\libopenblas.dll", "-DLAPACK_LIBRARIES=$rootDir\\src\\main\\resources\\windowsDependencies\\libopenblas.dll", "-DSIMD_ENABLED=${simd_enabled}", "-DCOMMIT_LIB_PATCHES=${commit_lib_patches}"
}
else {
commandLine 'cmake', '.', "-DKNN_PLUGIN_VERSION=${opensearch_version}", "-DSIMD_ENABLED=${simd_enabled}", "-DCOMMIT_LIB_PATCHES=${commit_lib_patches}"
Copy link
Member

@martin-gaievski martin-gaievski Jul 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

did we skip -DKNN_PLUGIN_VERSION=${opensearch_version} intentionally?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See line# 317.

args.add("-G")
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure on this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

of course.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oops. Let me check one more time.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It seems the order of -G option doesn't matter. I put it at the end and it works.

args.add("Unix Makefiles")
args.add("-DBLAS_LIBRARIES=$rootDir\\src\\main\\resources\\windowsDependencies\\libopenblas.dll")
args.add("-DLAPACK_LIBRARIES=$rootDir\\src\\main\\resources\\windowsDependencies\\libopenblas.dll")
}
commandLine args
}

task buildJniLib(type:Exec) {
Expand Down
71 changes: 37 additions & 34 deletions jni/cmake/init-faiss.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,44 +12,47 @@ if (NOT EXISTS ${FAISS_REPO_DIR})
execute_process(COMMAND git submodule update --init -- external/faiss WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR})
endif ()

# Define list of patch files
set(PATCH_FILE_LIST)
list(APPEND PATCH_FILE_LIST "${CMAKE_CURRENT_SOURCE_DIR}/patches/faiss/0001-Custom-patch-to-support-multi-vector.patch")
list(APPEND PATCH_FILE_LIST "${CMAKE_CURRENT_SOURCE_DIR}/patches/faiss/0002-Enable-precomp-table-to-be-shared-ivfpq.patch")
list(APPEND PATCH_FILE_LIST "${CMAKE_CURRENT_SOURCE_DIR}/patches/faiss/0003-Custom-patch-to-support-range-search-params.patch")
list(APPEND PATCH_FILE_LIST "${CMAKE_CURRENT_SOURCE_DIR}/patches/faiss/0004-Custom-patch-to-support-binary-vector.patch")
# Apply patches
if(NOT DEFINED APPLY_LIB_PATCHES OR "${APPLY_LIB_PATCHES}" STREQUAL true)
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 skip everything for patches?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ack

# Define list of patch files
set(PATCH_FILE_LIST)
list(APPEND PATCH_FILE_LIST "${CMAKE_CURRENT_SOURCE_DIR}/patches/faiss/0001-Custom-patch-to-support-multi-vector.patch")
list(APPEND PATCH_FILE_LIST "${CMAKE_CURRENT_SOURCE_DIR}/patches/faiss/0002-Enable-precomp-table-to-be-shared-ivfpq.patch")
list(APPEND PATCH_FILE_LIST "${CMAKE_CURRENT_SOURCE_DIR}/patches/faiss/0003-Custom-patch-to-support-range-search-params.patch")
list(APPEND PATCH_FILE_LIST "${CMAKE_CURRENT_SOURCE_DIR}/patches/faiss/0004-Custom-patch-to-support-binary-vector.patch")

# Get patch id of the last commit
execute_process(COMMAND sh -c "git --no-pager show HEAD | git patch-id --stable" OUTPUT_VARIABLE PATCH_ID_OUTPUT_FROM_COMMIT WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/external/faiss)
string(REPLACE " " ";" PATCH_ID_LIST_FROM_COMMIT ${PATCH_ID_OUTPUT_FROM_COMMIT})
list(GET PATCH_ID_LIST_FROM_COMMIT 0 PATCH_ID_FROM_COMMIT)
# Get patch id of the last commit
execute_process(COMMAND sh -c "git --no-pager show HEAD | git patch-id --stable" OUTPUT_VARIABLE PATCH_ID_OUTPUT_FROM_COMMIT WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/external/faiss)
string(REPLACE " " ";" PATCH_ID_LIST_FROM_COMMIT ${PATCH_ID_OUTPUT_FROM_COMMIT})
list(GET PATCH_ID_LIST_FROM_COMMIT 0 PATCH_ID_FROM_COMMIT)

# Find all patch files need to apply
list(SORT PATCH_FILE_LIST ORDER DESCENDING)
set(PATCH_FILES_TO_APPLY)
foreach(PATCH_FILE IN LISTS PATCH_FILE_LIST)
# Get patch id of a patch file
execute_process(COMMAND sh -c "cat ${PATCH_FILE} | git patch-id --stable" OUTPUT_VARIABLE PATCH_ID_OUTPUT)
string(REPLACE " " ";" PATCH_ID_LIST ${PATCH_ID_OUTPUT})
list(GET PATCH_ID_LIST 0 PATCH_ID)
# Find all patch files need to apply
list(SORT PATCH_FILE_LIST ORDER DESCENDING)
set(PATCH_FILES_TO_APPLY)
foreach(PATCH_FILE IN LISTS PATCH_FILE_LIST)
# Get patch id of a patch file
execute_process(COMMAND sh -c "cat ${PATCH_FILE} | git patch-id --stable" OUTPUT_VARIABLE PATCH_ID_OUTPUT)
string(REPLACE " " ";" PATCH_ID_LIST ${PATCH_ID_OUTPUT})
list(GET PATCH_ID_LIST 0 PATCH_ID)

# Add the file to patch list if patch id does not match
if (${PATCH_ID} STREQUAL ${PATCH_ID_FROM_COMMIT})
break()
else()
list(APPEND PATCH_FILES_TO_APPLY ${PATCH_FILE})
endif()
endforeach()
# Add the file to patch list if patch id does not match
if (${PATCH_ID} STREQUAL ${PATCH_ID_FROM_COMMIT})
break()
else()
list(APPEND PATCH_FILES_TO_APPLY ${PATCH_FILE})
endif()
endforeach()

# Apply patch files
list(SORT PATCH_FILES_TO_APPLY)
foreach(PATCH_FILE IN LISTS PATCH_FILES_TO_APPLY)
message(STATUS "Applying patch of ${PATCH_FILE}")
execute_process(COMMAND git ${GIT_PATCH_COMMAND} --3way --ignore-space-change --ignore-whitespace ${PATCH_FILE} WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/external/faiss ERROR_VARIABLE ERROR_MSG RESULT_VARIABLE RESULT_CODE)
if(RESULT_CODE)
message(FATAL_ERROR "Failed to apply patch:\n${ERROR_MSG}")
endif()
endforeach()
# Apply patch files
list(SORT PATCH_FILES_TO_APPLY)
foreach(PATCH_FILE IN LISTS PATCH_FILES_TO_APPLY)
message(STATUS "Applying patch of ${PATCH_FILE}")
execute_process(COMMAND git ${GIT_PATCH_COMMAND} --3way --ignore-space-change --ignore-whitespace ${PATCH_FILE} WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/external/faiss ERROR_VARIABLE ERROR_MSG RESULT_VARIABLE RESULT_CODE)
if(RESULT_CODE)
message(FATAL_ERROR "Failed to apply patch:\n${ERROR_MSG}")
endif()
endforeach()
endif()

if (${CMAKE_SYSTEM_NAME} STREQUAL Darwin)
if(CMAKE_C_COMPILER_ID MATCHES "Clang\$")
Expand Down
56 changes: 30 additions & 26 deletions jni/cmake/init-nmslib.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,32 +12,36 @@ if (NOT EXISTS ${NMS_REPO_DIR})
execute_process(COMMAND git submodule update --init -- external/nmslib WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR})
endif ()

# Define list of patch files
set(PATCH_FILE_LIST)
list(APPEND PATCH_FILE_LIST "${CMAKE_CURRENT_SOURCE_DIR}/patches/nmslib/0001-Initialize-maxlevel-during-add-from-enterpoint-level.patch")
list(APPEND PATCH_FILE_LIST "${CMAKE_CURRENT_SOURCE_DIR}/patches/nmslib/0002-Adds-ability-to-pass-ef-parameter-in-the-query-for-h.patch")

# Get patch id of the last commit
execute_process(COMMAND sh -c "git --no-pager show HEAD | git patch-id --stable" OUTPUT_VARIABLE PATCH_ID_OUTPUT_FROM_COMMIT WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/external/nmslib)
string(REPLACE " " ";" PATCH_ID_LIST_FROM_COMMIT ${PATCH_ID_OUTPUT_FROM_COMMIT})
list(GET PATCH_ID_LIST_FROM_COMMIT 0 PATCH_ID_FROM_COMMIT)

# Find all patch files need to apply
list(SORT PATCH_FILE_LIST ORDER DESCENDING)
set(PATCH_FILES_TO_APPLY)
foreach(PATCH_FILE IN LISTS PATCH_FILE_LIST)
# Get patch id of a patch file
execute_process(COMMAND sh -c "cat ${PATCH_FILE} | git patch-id --stable" OUTPUT_VARIABLE PATCH_ID_OUTPUT)
string(REPLACE " " ";" PATCH_ID_LIST ${PATCH_ID_OUTPUT})
list(GET PATCH_ID_LIST 0 PATCH_ID)

# Add the file to patch list if patch id does not match
if (${PATCH_ID} STREQUAL ${PATCH_ID_FROM_COMMIT})
break()
else()
list(APPEND PATCH_FILES_TO_APPLY ${PATCH_FILE})
endif()
endforeach()

# Apply patches
if(NOT DEFINED APPLY_LIB_PATCHES OR "${APPLY_LIB_PATCHES}" STREQUAL true)
# Define list of patch files
set(PATCH_FILE_LIST)
list(APPEND PATCH_FILE_LIST "${CMAKE_CURRENT_SOURCE_DIR}/patches/nmslib/0001-Initialize-maxlevel-during-add-from-enterpoint-level.patch")
list(APPEND PATCH_FILE_LIST "${CMAKE_CURRENT_SOURCE_DIR}/patches/nmslib/0002-Adds-ability-to-pass-ef-parameter-in-the-query-for-h.patch")

# Get patch id of the last commit
execute_process(COMMAND sh -c "git --no-pager show HEAD | git patch-id --stable" OUTPUT_VARIABLE PATCH_ID_OUTPUT_FROM_COMMIT WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/external/nmslib)
string(REPLACE " " ";" PATCH_ID_LIST_FROM_COMMIT ${PATCH_ID_OUTPUT_FROM_COMMIT})
list(GET PATCH_ID_LIST_FROM_COMMIT 0 PATCH_ID_FROM_COMMIT)

# Find all patch files need to apply
list(SORT PATCH_FILE_LIST ORDER DESCENDING)
set(PATCH_FILES_TO_APPLY)
foreach(PATCH_FILE IN LISTS PATCH_FILE_LIST)
# Get patch id of a patch file
execute_process(COMMAND sh -c "cat ${PATCH_FILE} | git patch-id --stable" OUTPUT_VARIABLE PATCH_ID_OUTPUT)
string(REPLACE " " ";" PATCH_ID_LIST ${PATCH_ID_OUTPUT})
list(GET PATCH_ID_LIST 0 PATCH_ID)

# Add the file to patch list if patch id does not match
if (${PATCH_ID} STREQUAL ${PATCH_ID_FROM_COMMIT})
break()
else()
list(APPEND PATCH_FILES_TO_APPLY ${PATCH_FILE})
endif()
endforeach()
endif()

# Apply patch files
list(SORT PATCH_FILES_TO_APPLY)
Expand Down
4 changes: 3 additions & 1 deletion scripts/build.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,9 @@ cd $work_dir

if [ "$PLATFORM" != "windows" ] && [ "$ARCHITECTURE" = "x64" ]; then
echo "Building k-NN library after enabling AVX2"
./gradlew :buildJniLib -Dsimd.enabled=true -Dbuild.lib.commit_patches=false
# Skip applying patches as patches were applied already from previous :buildJniLib task
# If we apply patches again, it fails with conflict
./gradlew :buildJniLib -Dsimd.enabled=true -Dbuild.lib.commit_patches=false -Dbuild.lib.apply_patches=false
fi

./gradlew publishPluginZipPublicationToZipStagingRepository -Dopensearch.version=$VERSION -Dbuild.snapshot=$SNAPSHOT -Dbuild.version_qualifier=$QUALIFIER
Expand Down
Loading