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

Clean JNI artifacts with ./gradlew clean #2516

Merged
merged 5 commits into from
Feb 11, 2025
Merged

Clean JNI artifacts with ./gradlew clean #2516

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
89ea729
clean JNI artifacts with ./gradlew clean
sam-herman Feb 10, 2025
45d3448
nest release under build directory
sam-herman Feb 11, 2025
867a276
adjust all references to the old release path
sam-herman Feb 11, 2025
25ae31b
remove irrelevant paths from gitignore, add jni/build
sam-herman Feb 11, 2025
faab073
fix logging on linux
sam-herman Feb 11, 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
17 changes: 1 addition & 16 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,26 +14,11 @@ out/
oss/*
*.iml

jni/CMakeCache.txt
jni/CMakeFiles
jni/Makefile
jni/cmake_install.cmake
jni/release
jni/packages
jni/CTestTestfile.cmake
jni/KNNPlugin_JNI.cbp
jni/Testing/
jni/_deps/
jni/bin/
jni/lib/
jni/jni_test*
jni/googletest*
jni/build/
jni/cmake/*.cmake-e
jni/.cmake
jni/.idea
jni/build.ninja
jni/.ninja_deps
jni/.ninja_log

benchmarks/perf-tool/okpt/output
benchmarks/perf-tool/okpt/dev
Expand Down
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 @@ -58,4 +58,5 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
* Upgrade jsonpath from 2.8.0 to 2.9.0[2325](https://github.com/opensearch-project/k-NN/pull/2325)
* Bump Faiss commit from 1f42e81 to 0cbc2a8 to accelerate hamming distance calculation using _mm512_popcnt_epi64 intrinsic and also add avx512-fp16 instructions to boost performance [#2381](https://github.com/opensearch-project/k-NN/pull/2381)
* Enabled indices.breaker.total.use_real_memory setting via build.gradle for integTest Cluster to catch heap CB in local ITs and github CI actions [#2395](https://github.com/opensearch-project/k-NN/pull/2395/)
* Enabled idempotency of local builds when using `./gradlew clean` and nest `jni/release` directory under `jni/build` for easier cleanup [#2516](https://github.com/opensearch-project/k-NN/pull/2516)
### Refactoring
54 changes: 33 additions & 21 deletions build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -373,8 +373,8 @@ tasks.register('cmakeJniLib', Exec) {

def args = []
args.add(cmakePath)
args.add("-S jni")
args.add("-B jni/build")
args.add("-S jni") // CMakelists.txt directory
args.add("-B jni/build") // Build directory
args.add("-DKNN_PLUGIN_VERSION=${opensearch_version}")
args.add("-DAVX2_ENABLED=${avx2_enabled}")
args.add("-DAVX512_ENABLED=${avx512_enabled}")
Expand Down Expand Up @@ -405,44 +405,56 @@ tasks.register('cmakeJniLib', Exec) {
standardOutput = outputStream
}

// Makes sure that `./gradlew clean` removes all JNI build artifacts
tasks.clean.doFirst {
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we not just remove build folder here, since with your earlier change you moved all these files to the build folder

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think Release is still out there, I can see if I can add it under the build folder

Copy link
Contributor Author

@sam-herman sam-herman Feb 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

But you're right, the rest are not needed other than build and release

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@navneet1v should be good now on the paths, let me know.

// Delete JNI build directory
delete "${projectDir}/jni/build"
}

task buildNmslib(type:Exec) {
dependsOn cmakeJniLib
def makePath = findExecutable("make")
logger.lifecycle("Using make at: ${makePath}")
// Ensure makePath is treated as a String. If parsing to an int is required,
// handle it explicitly, though a path typically should not be parsed as an int.
if (makePath.isEmpty()) {
throw new GradleException("Make not found in PATH. Please install Make.")
def cmakePath = findExecutable("cmake")
logger.lifecycle("Using cmake at: ${cmakePath}")

if (cmakePath.isEmpty()) {
throw new GradleException("CMake not found in PATH. Please install CMake.")
}

def outputStream = new ByteArrayOutputStream()
commandLine makePath, '-Cjni/build', 'opensearchknn_nmslib', '-j', "${nproc_count}"
commandLine cmakePath,
'--build', 'jni/build',
'--target', 'opensearchknn_nmslib',
'--parallel', "${nproc_count}"
standardOutput = outputStream
}

task buildJniLib(type:Exec) {
dependsOn cmakeJniLib
def makePath = findExecutable("make")
logger.lifecycle("Using make at: ${makePath}")
// Ensure makePath is treated as a String. If parsing to an int is required,
// handle it explicitly, though a path typically should not be parsed as an int.
if (makePath.isEmpty()) {
throw new GradleException("Make not found in PATH. Please install Make.")
def cmakePath = findExecutable("cmake")
logger.lifecycle("Using cmake at: ${cmakePath}")

if (cmakePath.isEmpty()) {
throw new GradleException("CMake not found in PATH. Please install CMake.")
}

def outputStream = new ByteArrayOutputStream()
commandLine makePath, '-Cjni/build', 'opensearchknn_faiss', 'opensearchknn_common', '-j', "${nproc_count}"
commandLine cmakePath,
'--build', 'jni/build',
'--target', 'opensearchknn_faiss', 'opensearchknn_common',
'--parallel', "${nproc_count}"
standardOutput = outputStream
}

test {
dependsOn buildNmslib
dependsOn buildJniLib
systemProperty 'tests.security.manager', 'false'
systemProperty "java.library.path", "$rootDir/jni/release"
systemProperty "java.library.path", "$rootDir/jni/build/release"
//this change enables mockito-inline that supports mocking of static classes/calls
systemProperty "jdk.attach.allowAttachSelf", true
if (Os.isFamily(Os.FAMILY_WINDOWS)) {
// Add the paths of built JNI libraries and its dependent libraries to PATH variable in System variables
environment('PATH', System.getenv('PATH') + ";$rootDir/jni/release" + ";$rootDir/src/main/resources/windowsDependencies")
environment('PATH', System.getenv('PATH') + ";$rootDir/jni/build/release" + ";$rootDir/src/main/resources/windowsDependencies")
}
}

Expand All @@ -454,7 +466,7 @@ integTest {
}
systemProperty 'tests.security.manager', 'false'
systemProperty 'java.io.tmpdir', opensearch_tmp_dir.absolutePath
systemProperty "java.library.path", "$rootDir/jni/release"
systemProperty "java.library.path", "$rootDir/jni/build/release"
// allows integration test classes to access test resource from project root path
systemProperty('project.root', project.rootDir.absolutePath)

Expand Down Expand Up @@ -506,7 +518,7 @@ testClusters.integTest {
plugin(project.tasks.bundlePlugin.archiveFile)
if (Os.isFamily(Os.FAMILY_WINDOWS)) {
// Add the paths of built JNI libraries and its dependent libraries to PATH variable in System variables
environment('PATH', System.getenv('PATH') + ";$rootDir/jni/release" + ";$rootDir/src/main/resources/windowsDependencies")
environment('PATH', System.getenv('PATH') + ";$rootDir/jni/build/release" + ";$rootDir/src/main/resources/windowsDependencies")
}

// Cluster shrink exception thrown if we try to set numberOfNodes to 1, so only apply if > 1
Expand All @@ -521,7 +533,7 @@ testClusters.integTest {
debugPort += 1
}
}
systemProperty("java.library.path", "$rootDir/jni/release")
systemProperty("java.library.path", "$rootDir/jni/build/release")
systemProperty propertyKeys.breaker.useRealMemory, getBreakerSetting()
}

Expand Down
4 changes: 2 additions & 2 deletions jni/cmake/macros.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@ macro(opensearch_set_common_properties TARGET)

if (NOT "${WIN32}" STREQUAL "")
# Use RUNTIME_OUTPUT_DIRECTORY, to build the target library in the specified directory at runtime.
set_target_properties(${TARGET} PROPERTIES RUNTIME_OUTPUT_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/release)
set_target_properties(${TARGET} PROPERTIES RUNTIME_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/release)
else()
set_target_properties(${TARGET} PROPERTIES LIBRARY_OUTPUT_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/release)
set_target_properties(${TARGET} PROPERTIES LIBRARY_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/release)
endif()
endmacro()
6 changes: 3 additions & 3 deletions qa/restart-upgrade/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ testClusters {
environment "LD_LIBRARY_PATH", "${buildDir}/testclusters/${baseName}-0/distro/${knn_bwc_version_no_qualifier}-ARCHIVE/plugins/opensearch-knn/knnlib;${buildDir}/testclusters/${baseName}-0/distro/${knn_bwc_version_no_qualifier}-ARCHIVE/plugins/opensearch-knn/lib"
if (Os.isFamily(Os.FAMILY_WINDOWS)) {
// While running on Windows OS, setting the PATH environment variable to include the paths to dlls of JNI libraries and windows dependencies
environment('PATH', System.getenv('PATH') + ";$rootDir/jni/release" + ";$rootDir/src/main/resources/windowsDependencies")
environment('PATH', System.getenv('PATH') + ";$rootDir/jni/build/release" + ";$rootDir/src/main/resources/windowsDependencies")
systemProperty "java.library.path", "${buildDir}/testclusters/${baseName}-0/distro/${knn_bwc_version_no_qualifier}-ARCHIVE/plugins/opensearch-knn/knnlib;${buildDir}/testclusters/${baseName}-0/distro/${knn_bwc_version_no_qualifier}-ARCHIVE/plugins/opensearch-knn/lib"
} else {
systemProperty "java.library.path", "${buildDir}/testclusters/${baseName}-0/distro/${knn_bwc_version_no_qualifier}-ARCHIVE/plugins/opensearch-knn/knnlib:${buildDir}/testclusters/${baseName}-0/distro/${knn_bwc_version_no_qualifier}-ARCHIVE/plugins/opensearch-knn/lib"
Expand Down Expand Up @@ -140,8 +140,8 @@ testClusters {
dependsOn rootProject.tasks.assemble
useCluster testClusters."${baseName}"
doFirst {
testClusters."${baseName}".environment("LD_LIBRARY_PATH", "$rootDir/jni/release")
testClusters."${baseName}".systemProperty("java.library.path", "$rootDir/jni/release")
testClusters."${baseName}".environment("LD_LIBRARY_PATH", "$rootDir/jni/build/release")
testClusters."${baseName}".systemProperty("java.library.path", "$rootDir/jni/build/release")
testClusters."${baseName}".upgradeAllNodesAndPluginsToNextVersion([rootProject.tasks.bundlePlugin.archiveFile])
}
systemProperty 'tests.rest.bwcsuite_cluster', 'upgraded_cluster'
Expand Down
14 changes: 7 additions & 7 deletions qa/rolling-upgrade/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ testClusters {
environment "LD_LIBRARY_PATH", "${buildDir}/testclusters/${baseName}-0/distro/${knn_bwc_version_no_qualifier}-ARCHIVE/plugins/opensearch-knn/knnlib;${buildDir}/testclusters/${baseName}-0/distro/${knn_bwc_version_no_qualifier}-ARCHIVE/plugins/opensearch-knn/lib"
if (Os.isFamily(Os.FAMILY_WINDOWS)) {
// While running on Windows OS, setting the PATH environment variable to include the paths to dlls of JNI libraries and windows dependencies
environment('PATH', System.getenv('PATH') + ";$rootDir/jni/release" + ";$rootDir/src/main/resources/windowsDependencies")
environment('PATH', System.getenv('PATH') + ";$rootDir/jni/build/release" + ";$rootDir/src/main/resources/windowsDependencies")
systemProperty "java.library.path", "${buildDir}/testclusters/${baseName}-0/distro/${knn_bwc_version_no_qualifier}-ARCHIVE/plugins/opensearch-knn/knnlib;${buildDir}/testclusters/${baseName}-0/distro/${knn_bwc_version_no_qualifier}-ARCHIVE/plugins/opensearch-knn/lib"
} else {
systemProperty "java.library.path", "${buildDir}/testclusters/${baseName}-0/distro/${knn_bwc_version_no_qualifier}-ARCHIVE/plugins/opensearch-knn/knnlib:${buildDir}/testclusters/${baseName}-0/distro/${knn_bwc_version_no_qualifier}-ARCHIVE/plugins/opensearch-knn/lib"
Expand Down Expand Up @@ -57,8 +57,8 @@ task testAgainstOneThirdUpgradedCluster(type: StandaloneRestIntegTestTask) {
dependsOn rootProject.tasks.assemble
dependsOn "testAgainstOldCluster"
doFirst {
testClusters."${baseName}".getNodes().getAt("${baseName}" + "-0").environment("LD_LIBRARY_PATH", "$rootDir/jni/release")
testClusters."${baseName}".getNodes().getAt("${baseName}" + "-0").systemProperty("java.library.path", "$rootDir/jni/release")
testClusters."${baseName}".getNodes().getAt("${baseName}" + "-0").environment("LD_LIBRARY_PATH", "$rootDir/jni/build/release")
testClusters."${baseName}".getNodes().getAt("${baseName}" + "-0").systemProperty("java.library.path", "$rootDir/jni/build/release")
testClusters."${baseName}".upgradeNodeAndPluginToNextVersion([rootProject.tasks.bundlePlugin.archiveFile])
}
systemProperty 'tests.rest.bwcsuite_cluster', 'mixed_cluster'
Expand All @@ -76,8 +76,8 @@ task testAgainstTwoThirdsUpgradedCluster(type: StandaloneRestIntegTestTask) {
dependsOn "testAgainstOneThirdUpgradedCluster"
useCluster testClusters."${baseName}"
doFirst {
testClusters."${baseName}".getNodes().getAt("${baseName}" + "-1").environment("LD_LIBRARY_PATH", "$rootDir/jni/release")
testClusters."${baseName}".getNodes().getAt("${baseName}" + "-1").systemProperty("java.library.path", "$rootDir/jni/release")
testClusters."${baseName}".getNodes().getAt("${baseName}" + "-1").environment("LD_LIBRARY_PATH", "$rootDir/jni/build/release")
testClusters."${baseName}".getNodes().getAt("${baseName}" + "-1").systemProperty("java.library.path", "$rootDir/jni/build/release")
testClusters."${baseName}".upgradeNodeAndPluginToNextVersion([rootProject.tasks.bundlePlugin.archiveFile])
}
systemProperty 'tests.rest.bwcsuite_cluster', 'mixed_cluster'
Expand All @@ -95,8 +95,8 @@ task testRollingUpgrade(type: StandaloneRestIntegTestTask) {
dependsOn "testAgainstTwoThirdsUpgradedCluster"
useCluster testClusters."${baseName}"
doFirst {
testClusters."${baseName}".getNodes().getAt("${baseName}" + "-2").environment("LD_LIBRARY_PATH", "$rootDir/jni/release")
testClusters."${baseName}".getNodes().getAt("${baseName}" + "-2").systemProperty("java.library.path", "$rootDir/jni/release")
testClusters."${baseName}".getNodes().getAt("${baseName}" + "-2").environment("LD_LIBRARY_PATH", "$rootDir/jni/build/release")
testClusters."${baseName}".getNodes().getAt("${baseName}" + "-2").systemProperty("java.library.path", "$rootDir/jni/build/release")
testClusters."${baseName}".upgradeNodeAndPluginToNextVersion([rootProject.tasks.bundlePlugin.archiveFile])
}
mustRunAfter "testAgainstOneThirdUpgradedCluster"
Expand Down
Loading