From ab37458a0ea1b0d5fe08e61a42b88d33e87c2890 Mon Sep 17 00:00:00 2001 From: John Mazanec Date: Wed, 24 Apr 2024 13:48:19 -0700 Subject: [PATCH] Add flag for whether patches should be committed (#1653) Signed-off-by: John Mazanec (cherry picked from commit b3829c3468296c38474562dff43f6d3f5cf9ca23) --- .github/workflows/dco.yml | 18 ------------------ DEVELOPER_GUIDE.md | 7 +++++++ build.gradle | 7 +++++-- jni/CMakeLists.txt | 12 ++++++++++++ jni/cmake/init-faiss.cmake | 4 ++-- jni/cmake/init-nmslib.cmake | 2 +- scripts/build.sh | 6 +++--- 7 files changed, 30 insertions(+), 26 deletions(-) delete mode 100644 .github/workflows/dco.yml diff --git a/.github/workflows/dco.yml b/.github/workflows/dco.yml deleted file mode 100644 index cf30ea89d..000000000 --- a/.github/workflows/dco.yml +++ /dev/null @@ -1,18 +0,0 @@ -name: Developer Certificate of Origin Check - -on: [pull_request] - -jobs: - check: - runs-on: ubuntu-latest - - steps: - - name: Get PR Commits - id: 'get-pr-commits' - uses: tim-actions/get-pr-commits@v1.1.0 - with: - token: ${{ secrets.GITHUB_TOKEN }} - - name: DCO Check - uses: tim-actions/dco@v1.1.0 - with: - commits: ${{ steps.get-pr-commits.outputs.commits }} diff --git a/DEVELOPER_GUIDE.md b/DEVELOPER_GUIDE.md index ca5e7cc52..9c17f4f68 100644 --- a/DEVELOPER_GUIDE.md +++ b/DEVELOPER_GUIDE.md @@ -237,6 +237,13 @@ If you want to make a custom patch on JNI library 3. Place the patch file under `jni/patches` 4. Make a change in `jni/CmakeLists.txt`, `.github/workflows/CI.yml` to apply the patch during build +By default, in the cmake build system, these patches will be applied and committed to the native libraries. In order to +successfully make the commits the `user.name` and `user.email` git configurations need to be setup. If you cannot set +these in your environment, you can disable committing the changes to the library by passing gradle this flag: +`build.lib.commit_patches=false`. For example, `gradlew build -Dbuild.lib.commit_patches=false`. If the patches are +not committed, then the full library build process will run each time `cmake` is invoked. In a development environment, +it is recommended to setup the user git configuration to avoid this cost. + ### Enable SIMD Optimization SIMD(Single Instruction/Multiple Data) Optimization is enabled by default on Linux and Mac which boosts the performance by enabling `AVX2` on `x86 architecture` and `NEON` on `ARM64 architecture` while building the Faiss library. But to enable SIMD, the underlying processor diff --git a/build.gradle b/build.gradle index b608dbb1a..dd2c162c3 100644 --- a/build.gradle +++ b/build.gradle @@ -18,6 +18,9 @@ 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 + commit_lib_patches = System.getProperty("build.lib.commit_patches", "true") version_tokens = opensearch_version.tokenize('-') opensearch_build = version_tokens[0] + '.0' @@ -303,10 +306,10 @@ task cmakeJniLib(type:Exec) { workingDir 'jni' 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}" + 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}" + commandLine 'cmake', '.', "-DKNN_PLUGIN_VERSION=${opensearch_version}", "-DSIMD_ENABLED=${simd_enabled}", "-DCOMMIT_LIB_PATCHES=${commit_lib_patches}" } } diff --git a/jni/CMakeLists.txt b/jni/CMakeLists.txt index 3429671fe..6f0894607 100644 --- a/jni/CMakeLists.txt +++ b/jni/CMakeLists.txt @@ -31,6 +31,18 @@ else() set(CONFIG_ALL OFF) endif () +# `git am` will create commits from the patches in the native libraries. This is ideal for development envs +# because it prevents full lib rebuild everytime cmake is run. However, for build systems that will run the +# build workflow once, it can cause issues because git commits require that the user and the user's email be set. +# See https://github.com/opensearch-project/k-NN/issues/1651. So, we provide a flag that allows users to select between +# the two +if(NOT DEFINED COMMIT_LIB_PATCHES OR ${COMMIT_LIB_PATCHES} STREQUAL true) + set(GIT_PATCH_COMMAND am) +else() + set(GIT_PATCH_COMMAND apply) +endif() +message(STATUS "Using the following git patch command: \"${GIT_PATCH_COMMAND}\"") + # Set OS specific variables if (${CMAKE_SYSTEM_NAME} STREQUAL Darwin) set(CMAKE_MACOSX_RPATH 1) diff --git a/jni/cmake/init-faiss.cmake b/jni/cmake/init-faiss.cmake index 44dd4442a..fefed61e2 100644 --- a/jni/cmake/init-faiss.cmake +++ b/jni/cmake/init-faiss.cmake @@ -18,8 +18,8 @@ find_path(PATCH_FILE NAMES 0001-Custom-patch-to-support-multi-vector.patch 0002- # If it exists, apply patches if (EXISTS ${PATCH_FILE}) message(STATUS "Applying custom patches.") - execute_process(COMMAND git am --3way ${CMAKE_CURRENT_SOURCE_DIR}/patches/faiss/0001-Custom-patch-to-support-multi-vector.patch WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/external/faiss ERROR_VARIABLE ERROR_MSG RESULT_VARIABLE RESULT_CODE) - execute_process(COMMAND git am --3way ${CMAKE_CURRENT_SOURCE_DIR}/patches/faiss/0002-Enable-precomp-table-to-be-shared-ivfpq.patch WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/external/faiss ERROR_VARIABLE ERROR_MSG RESULT_VARIABLE RESULT_CODE) + execute_process(COMMAND git ${GIT_PATCH_COMMAND} --3way --ignore-space-change --ignore-whitespace ${CMAKE_CURRENT_SOURCE_DIR}/patches/faiss/0001-Custom-patch-to-support-multi-vector.patch WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/external/faiss ERROR_VARIABLE ERROR_MSG RESULT_VARIABLE RESULT_CODE) + execute_process(COMMAND git ${GIT_PATCH_COMMAND} --3way --ignore-space-change --ignore-whitespace ${CMAKE_CURRENT_SOURCE_DIR}/patches/faiss/0002-Enable-precomp-table-to-be-shared-ivfpq.patch 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() diff --git a/jni/cmake/init-nmslib.cmake b/jni/cmake/init-nmslib.cmake index edf2296d9..a735bcbd8 100644 --- a/jni/cmake/init-nmslib.cmake +++ b/jni/cmake/init-nmslib.cmake @@ -18,7 +18,7 @@ find_path(PATCH_FILE NAMES 0001-Initialize-maxlevel-during-add-from-enterpoint-l # If it exists, apply patches if (EXISTS ${PATCH_FILE}) message(STATUS "Applying custom patches.") - execute_process(COMMAND git am --3way ${CMAKE_CURRENT_SOURCE_DIR}/patches/nmslib/0001-Initialize-maxlevel-during-add-from-enterpoint-level.patch WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/external/nmslib ERROR_VARIABLE ERROR_MSG RESULT_VARIABLE RESULT_CODE) + execute_process(COMMAND git ${GIT_PATCH_COMMAND} --3way --ignore-space-change --ignore-whitespace ${CMAKE_CURRENT_SOURCE_DIR}/patches/nmslib/0001-Initialize-maxlevel-during-add-from-enterpoint-level.patch WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/external/nmslib ERROR_VARIABLE ERROR_MSG RESULT_VARIABLE RESULT_CODE) if(RESULT_CODE) message(FATAL_ERROR "Failed to apply patch:\n${ERROR_MSG}") diff --git a/scripts/build.sh b/scripts/build.sh index 43a36443d..d2571efb6 100644 --- a/scripts/build.sh +++ b/scripts/build.sh @@ -118,12 +118,12 @@ fi # Build k-NN lib and plugin through gradle tasks cd $work_dir -./gradlew build --no-daemon --refresh-dependencies -x integTest -x test -Dopensearch.version=$VERSION -Dbuild.snapshot=$SNAPSHOT -Dbuild.version_qualifier=$QUALIFIER -./gradlew :buildJniLib -Dsimd.enabled=false +./gradlew build --no-daemon --refresh-dependencies -x integTest -x test -Dopensearch.version=$VERSION -Dbuild.snapshot=$SNAPSHOT -Dbuild.version_qualifier=$QUALIFIER -Dbuild.lib.commit_patches=false +./gradlew :buildJniLib -Dsimd.enabled=false -Dbuild.lib.commit_patches=false if [ "$PLATFORM" != "windows" ] && [ "$ARCHITECTURE" = "x64" ]; then echo "Building k-NN library after enabling AVX2" - ./gradlew :buildJniLib -Dsimd.enabled=true + ./gradlew :buildJniLib -Dsimd.enabled=true -Dbuild.lib.commit_patches=false fi ./gradlew publishPluginZipPublicationToZipStagingRepository -Dopensearch.version=$VERSION -Dbuild.snapshot=$SNAPSHOT -Dbuild.version_qualifier=$QUALIFIER