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

Clean JNI artifacts with ./gradlew clean #2516

Conversation

sam-herman
Copy link
Contributor

@sam-herman sam-herman commented Feb 10, 2025

Description

More build tidiness changes:

  1. Clean JNI artifacts with ./gradlew clean to make local builds idempotent and more reliable.
  2. Places release under binary_dir instead of under source_dir this makes all artifacts only show up under single directory without the need to hunt them down in multiple places when cleaning up.
  3. Switch to cmake instead of make when building native JNI for improved readability

Related Issues

Resolves #2515

Check List

  • New functionality includes testing.
  • New functionality has been documented.
  • API changes companion pull request created.
  • Commits are signed per the DCO using --signoff.
  • Public documentation issue/PR created.

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.

@@ -405,6 +405,18 @@ 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

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.

@navneet1v navneet1v added skip-changelog Maintenance Add support for new versions of OpenSearch/Dashboards from upstream labels Feb 10, 2025
@naveentatikonda
Copy link
Member

@sam-herman do you know why we are not able to see the build logs of faiss on Linux after your change while it is logged for other OS?

With these logs how do we know which version of faiss library(avx2, avx512, etc) are we building. Can you atleast list all the libraries that were built at the end of make so that it will be easy to debug if something goes wrong while loading the library during runtime ?

Logs on Linux

> Task :buildJniLib
/__w/k-NN/k-NN/jni/src/org_opensearch_knn_jni_FaissService.cpp: In function 'jlong Java_org_opensearch_knn_jni_FaissService_loadIndex(JNIEnv*, jclass, jstring)':
/__w/k-NN/k-NN/jni/src/org_opensearch_knn_jni_FaissService.cpp:245:10: warning: converting to non-pointer type 'long int' from NULL [-Wconversion-null]
  245 |   return NULL;
      |          ^~~~
/__w/k-NN/k-NN/jni/src/org_opensearch_knn_jni_FaissService.cpp: In function 'jlong Java_org_opensearch_knn_jni_FaissService_loadIndexWithStream(JNIEnv*, jclass, jobject)':
/__w/k-NN/k-NN/jni/src/org_opensearch_knn_jni_FaissService.cpp:267:12: warning: converting to non-pointer type 'long int' from NULL [-Wconversion-null]
  267 |     return NULL;
      |            ^~~~
/__w/k-NN/k-NN/jni/src/org_opensearch_knn_jni_FaissService.cpp: In function 'jlong Java_org_opensearch_knn_jni_FaissService_loadBinaryIndex(JNIEnv*, jclass, jstring)':
/__w/k-NN/k-NN/jni/src/org_opensearch_knn_jni_FaissService.cpp:277:12: warning: converting to non-pointer type 'long int' from NULL [-Wconversion-null]
  277 |     return NULL;
      |            ^~~~
/__w/k-NN/k-NN/jni/src/org_opensearch_knn_jni_FaissService.cpp: In function 'jlong Java_org_opensearch_knn_jni_FaissService_loadBinaryIndexWithStream(JNIEnv*, jclass, jobject)':
/__w/k-NN/k-NN/jni/src/org_opensearch_knn_jni_FaissService.cpp:299:12: warning: converting to non-pointer type 'long int' from NULL [-Wconversion-null]
  299 |     return NULL;
      |            ^~~~
/__w/k-NN/k-NN/jni/src/org_opensearch_knn_jni_FaissService.cpp: In function 'jboolean Java_org_opensearch_knn_jni_FaissService_isSharedIndexStateRequired(JNIEnv*, jclass, jlong)':
/__w/k-NN/k-NN/jni/src/org_opensearch_knn_jni_FaissService.cpp:311:12: warning: converting to non-pointer type 'jboolean' {aka 'unsigned char'} from NULL [-Wconversion-null]
  311 |     return NULL;
      |            ^~~~
/__w/k-NN/k-NN/jni/src/org_opensearch_knn_jni_FaissService.cpp: In function 'jlong Java_org_opensearch_knn_jni_FaissService_initSharedIndexState(JNIEnv*, jclass, jlong)':
/__w/k-NN/k-NN/jni/src/org_opensearch_knn_jni_FaissService.cpp:322:12: warning: converting to non-pointer type 'long int' from NULL [-Wconversion-null]
  322 |     return NULL;
      |            ^~~~

@sam-herman
Copy link
Contributor Author

@sam-herman do you know why we are not able to see the build logs of faiss on Linux after your change while it is logged for other OS?

With these logs how do we know which version of faiss library(avx2, avx512, etc) are we building. Can you atleast list all the libraries that were built at the end of make so that it will be easy to debug if something goes wrong while loading the library during runtime ?

Logs on Linux

> Task :buildJniLib
/__w/k-NN/k-NN/jni/src/org_opensearch_knn_jni_FaissService.cpp: In function 'jlong Java_org_opensearch_knn_jni_FaissService_loadIndex(JNIEnv*, jclass, jstring)':
/__w/k-NN/k-NN/jni/src/org_opensearch_knn_jni_FaissService.cpp:245:10: warning: converting to non-pointer type 'long int' from NULL [-Wconversion-null]
  245 |   return NULL;
      |          ^~~~
/__w/k-NN/k-NN/jni/src/org_opensearch_knn_jni_FaissService.cpp: In function 'jlong Java_org_opensearch_knn_jni_FaissService_loadIndexWithStream(JNIEnv*, jclass, jobject)':
/__w/k-NN/k-NN/jni/src/org_opensearch_knn_jni_FaissService.cpp:267:12: warning: converting to non-pointer type 'long int' from NULL [-Wconversion-null]
  267 |     return NULL;
      |            ^~~~
/__w/k-NN/k-NN/jni/src/org_opensearch_knn_jni_FaissService.cpp: In function 'jlong Java_org_opensearch_knn_jni_FaissService_loadBinaryIndex(JNIEnv*, jclass, jstring)':
/__w/k-NN/k-NN/jni/src/org_opensearch_knn_jni_FaissService.cpp:277:12: warning: converting to non-pointer type 'long int' from NULL [-Wconversion-null]
  277 |     return NULL;
      |            ^~~~
/__w/k-NN/k-NN/jni/src/org_opensearch_knn_jni_FaissService.cpp: In function 'jlong Java_org_opensearch_knn_jni_FaissService_loadBinaryIndexWithStream(JNIEnv*, jclass, jobject)':
/__w/k-NN/k-NN/jni/src/org_opensearch_knn_jni_FaissService.cpp:299:12: warning: converting to non-pointer type 'long int' from NULL [-Wconversion-null]
  299 |     return NULL;
      |            ^~~~
/__w/k-NN/k-NN/jni/src/org_opensearch_knn_jni_FaissService.cpp: In function 'jboolean Java_org_opensearch_knn_jni_FaissService_isSharedIndexStateRequired(JNIEnv*, jclass, jlong)':
/__w/k-NN/k-NN/jni/src/org_opensearch_knn_jni_FaissService.cpp:311:12: warning: converting to non-pointer type 'jboolean' {aka 'unsigned char'} from NULL [-Wconversion-null]
  311 |     return NULL;
      |            ^~~~
/__w/k-NN/k-NN/jni/src/org_opensearch_knn_jni_FaissService.cpp: In function 'jlong Java_org_opensearch_knn_jni_FaissService_initSharedIndexState(JNIEnv*, jclass, jlong)':
/__w/k-NN/k-NN/jni/src/org_opensearch_knn_jni_FaissService.cpp:322:12: warning: converting to non-pointer type 'long int' from NULL [-Wconversion-null]
  322 |     return NULL;
      |            ^~~~

It should show, I'll try to build on a linux machine to debug this one. Will make updates

@navneet1v navneet1v changed the title clean JNI artifacts with ./gradlew clean Clean JNI artifacts with ./gradlew clean Feb 11, 2025
navneet1v
navneet1v previously approved these changes Feb 11, 2025
Signed-off-by: Samuel Herman <[email protected]>
@sam-herman
Copy link
Contributor Author

@naveentatikonda I fixed the logging issue on linux and tested, see the last commit on this change.

@naveentatikonda naveentatikonda merged commit 359a37b into opensearch-project:main Feb 11, 2025
35 checks passed
@sam-herman sam-herman deleted the 2515/gradle-clean-jni-artifacts branch February 11, 2025 16:01
navneet1v pushed a commit that referenced this pull request Feb 17, 2025
…vice (#2532)

* fix broken build flag, move build to one directory (#2442)

* move build to one directory, fix broken flag

Signed-off-by: Samuel Herman <[email protected]>

* fix make path

Signed-off-by: Samuel Herman <[email protected]>

* changelog update

Signed-off-by: Samuel Herman <[email protected]>

* add fix for classpath change and for cmake discovery on macos

Signed-off-by: Samuel Herman <[email protected]>

* fix make discovery for gradle

Signed-off-by: Samuel Herman <[email protected]>

* fix cmake path for macOS

Signed-off-by: Samuel Herman <[email protected]>

---------

Signed-off-by: Samuel Herman <[email protected]>

* Update package name to fix compilation issue (#2513)

* Update package name to fix compilation issue

Core renamed this package in opensearch-project/OpenSearch#17272
This commit renames package accordingly

Signed-off-by: Balasubramanian <[email protected]>

* Update build.gradle and build.sh to separate x64 linux nmslib build with different gcc versions (#2506) (#2508)

* Update build gradle to separate nmslib / faiss generation

Signed-off-by: Peter Zhu <[email protected]>

* Update scripts/build.sh to separate enable gcc switch

Signed-off-by: Peter Zhu <[email protected]>

* Remove test comments

Signed-off-by: Peter Zhu <[email protected]>

* Remove test comments

Signed-off-by: Peter Zhu <[email protected]>

* Remove test comments

Signed-off-by: Peter Zhu <[email protected]>

* Updating restart and rolling upgrade bwc test bundle.gradle

Signed-off-by: Peter Zhu <[email protected]>

* Enforce gcc10 for nmslib to compile and avx512_spr have no-op

Signed-off-by: Peter Zhu <[email protected]>

---------

Signed-off-by: Peter Zhu <[email protected]>
(cherry picked from commit 107c4f1)

Co-authored-by: Peter Zhu <[email protected]>

---------

Signed-off-by: Balasubramanian <[email protected]>
Co-authored-by: opensearch-trigger-bot[bot] <98922864+opensearch-trigger-bot[bot]@users.noreply.github.com>
Co-authored-by: Peter Zhu <[email protected]>

* Adds debug logs for KNNQuery and KNNWeight (#2466) (#2470)

* Adds debug logs for KNNQuery and KNNWeight

Signed-off-by: Tejas Shah <[email protected]>

* Adds check to see if log is enabled to start and stop StopWatch

Signed-off-by: Tejas Shah <[email protected]>

* Addressing comments on the PR

Signed-off-by: Tejas Shah <[email protected]>

* Adds shard and segment info in the logs

Signed-off-by: Tejas Shah <[email protected]>

* Removes unnecessary segment name param from exact search

Signed-off-by: Tejas Shah <[email protected]>

* Fixes the build

Signed-off-by: Tejas Shah <[email protected]>

---------

Signed-off-by: Tejas Shah <[email protected]>
(cherry picked from commit f322e27)

Co-authored-by: Tejas Shah <[email protected]>

* Clean JNI artifacts with ./gradlew clean (#2516)

* clean JNI artifacts with ./gradlew clean

Signed-off-by: Samuel Herman <[email protected]>

* nest release under build directory

Signed-off-by: Samuel Herman <[email protected]>

* adjust all references to the old release path

Signed-off-by: Samuel Herman <[email protected]>

* remove irrelevant paths from gitignore, add jni/build

Signed-off-by: Samuel Herman <[email protected]>

* fix logging on linux

Signed-off-by: Samuel Herman <[email protected]>

---------

Signed-off-by: Samuel Herman <[email protected]>

* Backport to main (#2520)

* Remove skip building graph check for quantization use case (#2430)

For quantization indices, we don't have to apply building graph check
since it is already faster, this is now only applied for fp32/16 indices
and where threshold is configured.

Signed-off-by: Vijayan Balasubramanian <[email protected]>

* Update default to 0 to always build graph as default behavior (#2452)

Signed-off-by: Balasubramanian <[email protected]>

* Update changelog

Signed-off-by: Balasubramanian <[email protected]>

---------

Signed-off-by: Vijayan Balasubramanian <[email protected]>
Signed-off-by: Balasubramanian <[email protected]>

* [Backport main] Add release notes for 2.19.0 (#2503)

* Add release notes for 2.19.0

Signed-off-by: Kunal Kotwani <[email protected]>

* Fix links for release notes

Co-authored-by: John Mazanec <[email protected]>
Signed-off-by: Kunal Kotwani <[email protected]>

---------

Signed-off-by: Kunal Kotwani <[email protected]>
Co-authored-by: John Mazanec <[email protected]>

* Fix main knnlib dir in build script based on #2442 (#2526)

Signed-off-by: Peter Zhu <[email protected]>

* Minor performance improvments in KNNQueryBuilder (#2528)

Signed-off-by: Tejas Shah <[email protected]>

* Initial implementation of control flow

Signed-off-by: Rohan Chitale <[email protected]>

---------

Signed-off-by: Samuel Herman <[email protected]>
Signed-off-by: Balasubramanian <[email protected]>
Signed-off-by: Vijayan Balasubramanian <[email protected]>
Signed-off-by: Kunal Kotwani <[email protected]>
Signed-off-by: Peter Zhu <[email protected]>
Signed-off-by: Tejas Shah <[email protected]>
Signed-off-by: Rohan Chitale <[email protected]>
Co-authored-by: sam-herman <[email protected]>
Co-authored-by: Vijayan Balasubramanian <[email protected]>
Co-authored-by: opensearch-trigger-bot[bot] <98922864+opensearch-trigger-bot[bot]@users.noreply.github.com>
Co-authored-by: Peter Zhu <[email protected]>
Co-authored-by: Tejas Shah <[email protected]>
Co-authored-by: Kunal Kotwani <[email protected]>
Co-authored-by: John Mazanec <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Maintenance Add support for new versions of OpenSearch/Dashboards from upstream skip-changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] gradlew clean doesn't remove JNI artifacts
3 participants