opensearch-project / k-NN

🆕 Find the k-nearest neighbors (k-NN) for your vector data
https://opensearch.org/docs/latest/search-plugins/knn/index/
Apache License 2.0
155 stars 114 forks source link

Add Windows Support #157

Closed dblock closed 1 year ago

dblock commented 2 years ago

Coming from https://github.com/opensearch-project/opensearch-plugins/issues/95, add Windows support.

bbarani commented 2 years ago

@vamshin We have started the work on adding support for Windows distribution and need your inputs on the timeline to support KNN on Windows.

setiah commented 2 years ago

Hey @vamshin, do we have any updates here?

vamshin commented 2 years ago

Hi @setiah @bbarani , we are prioritizing this task. Will update timeline soon. Thanks

setiah commented 2 years ago

Hi @bbarani @peterzhuamazon, since we are tracking Windows effort with milestones, is there a specific milestone we would like to map the KNN effort with, to ensure we have these artifacts in time for the 2.4 release? That should help @vamshin and team to plan accordingly. cc @elfisher

peterzhuamazon commented 2 years ago

Current issues:

  1. Windows mingw libgfortran-5.dll is version 5 and we are currently renaming it to libgfortran-3.dll to use.
  2. The OpenBlas lib for windows is old on binary https://sourceforge.net/projects/openblas/files/v0.2.9.rc2/ seems like the official releases have not release any win version after 0.2.19. We need to deep dive a bit into seeing how to compile openblas, possibly, on windows to match the version on linux.
  3. Add lib env vars to user specific env var section, or we can set it to global.
peterzhuamazon commented 2 years ago

New issues, the lib files provided by the knn team is invalid:

# Yours
$ file lib*
libopensearchknn_faiss.dll:  data
libopensearchknn_nmslib.dll: data

# Download a random windows valid dll online
$ file A3D.dll
A3D.dll: PE32 executable (DLL) (GUI) Intel 80386, for MS Windows
peterzhuamazon commented 2 years ago

New issues:

How to make sure the plugin/libs folder present in java.library.path:

java.library.path = 
        C:\Windows\Sun\Java\bin
        C:\Windows\system32
        C:\Windows
        C:\Windows\system32
        C:\Windows
......

Probably will try to append the path like linux version through the OS startup batch script.

peterzhuamazon commented 2 years ago

I am able to add the path to java.library.path

On windows as long as you expose the path into Path variable it will expose to the java.library.path

We can add a batch file through cmd commands to dynamically update the Path variable during startup

peterzhuamazon commented 2 years ago

We are able to successfully install knn on 2.3.0 snapshot windows zip and complete the KNN integTest run.

Notes:

  1. Remove the prefix lib in front of the faiss and nmslib .dll file. As the lib prefix is specifically used in linux. In windows, it seems to look for the exact naming conversion you defined in the code: https://github.com/opensearch-project/k-NN/blob/main/src/main/java/org/opensearch/knn/jni/FaissService.java#L34 https://github.com/opensearch-project/k-NN/blob/main/src/main/java/org/opensearch/knn/common/KNNConstants.java#L63-L95

  2. As long as the path is added to Path env vars, it will show up in the java.library.path variable and will be loaded into the process.

  3. Use .bat to run everything and ignore the bash version.

peterzhuamazon commented 2 years ago
$  ./gradlew integTest -Dopensearch.version=2.3.0-SNAPSHOT -Dbuild.snapshot=true -Dtests.rest.cluster="localhost:9200" -Dtests.cluster="localhost:9200" -Dtests.clustername="opensearch-integrationtest" -Dhttps=false -Duser=admin -Dpassword=admin --console=plain
=======================================
OpenSearch Build Hamster says Hello!
  Gradle Version        : 7.5
  OS Info               : Windows Server 2019 10.0 (amd64)
  JDK Version           : 17 (Eclipse Temurin JDK)
  JAVA_HOME             : C:\Users\Administrator\scoop\apps\temurin17-jdk\17.0.4-101
  Random Testing Seed   : 400D094C45C29453
  In FIPS 140 mode      : false
=======================================
> Task :processResources UP-TO-DATE
> Task :processTestFixturesResources NO-SOURCE
> Task :generateNotice UP-TO-DATE
> Task :copyPluginPropertiesTemplate UP-TO-DATE
> Task :pluginProperties UP-TO-DATE
> Task :processTestResources UP-TO-DATE
> Task :qa:compileJava NO-SOURCE
> Task :qa:processResources NO-SOURCE
> Task :qa:classes UP-TO-DATE
> Task :qa:processTestResources NO-SOURCE
> Task :qa:restart-upgrade:compileJava NO-SOURCE
> Task :qa:restart-upgrade:processResources NO-SOURCE
> Task :qa:restart-upgrade:classes UP-TO-DATE
> Task :qa:restart-upgrade:processTestResources NO-SOURCE
> Task :qa:rolling-upgrade:compileJava NO-SOURCE
> Task :qa:rolling-upgrade:processResources NO-SOURCE
> Task :qa:rolling-upgrade:classes UP-TO-DATE
> Task :qa:rolling-upgrade:processTestResources NO-SOURCE
> Task :generateEffectiveLombokConfig
> Task :compileJava UP-TO-DATE
> Task :classes UP-TO-DATE
> Task :jar UP-TO-DATE
> Task :bundlePlugin UP-TO-DATE
> Task :generateTestFixturesEffectiveLombokConfig
> Task :compileTestFixturesJava UP-TO-DATE
> Task :generateTestEffectiveLombokConfig
> Task :testFixturesClasses UP-TO-DATE
> Task :testFixturesJar UP-TO-DATE
> Task :compileTestJava UP-TO-DATE
> Task :testClasses UP-TO-DATE

> Task :integTest
OpenJDK 64-Bit Server VM warning: Ignoring option --illegal-access=warn; support was removed in 17.0

> Task :qa:compileTestJava NO-SOURCE
> Task :qa:testClasses UP-TO-DATE
> Task :qa:integTest SKIPPED
> Task :qa:restart-upgrade:compileTestJava UP-TO-DATE
> Task :qa:restart-upgrade:testClasses UP-TO-DATE
> Task :qa:restart-upgrade:integTest SKIPPED
> Task :qa:rolling-upgrade:compileTestJava UP-TO-DATE
> Task :qa:rolling-upgrade:testClasses UP-TO-DATE
> Task :qa:rolling-upgrade:integTest SKIPPED

BUILD SUCCESSFUL in 11m 57s
17 actionable tasks: 4 executed, 13 up-to-date
peterzhuamazon commented 2 years ago

@naveentatikonda Could you help comment on what is the issue with BWC test challenges on KNN for windows support?

cc: @setiah

naveentatikonda commented 2 years ago

@naveentatikonda Could you help comment on what is the issue with BWC test challenges on KNN for windows support?

cc: @setiah

@peterzhuamazon We might need to make few changes to the BWC framework to make it compatible with windows. For example url of the tar file. To test this and make those changes we need an artifact of any BWC version with k-NN plugin bundled in that artifact.

Also, as per our previous conversation I know we need to push our windows changes to 2.x branch for you to include k-NN plugin into that artifact. But, for now will you be able to pick those changes from any feature branch as we are still trying to make our script more organic without any work arounds?

peterzhuamazon commented 2 years ago

Have some discussion with @naveentatikonda and he would provide two branches for 2.3.0 and 2.2.1 respectively. I will use those branches to build bundles on my fork.

Then he can try to implement the bwc test.

Thanks.

naveentatikonda commented 2 years ago

@peterzhuamazon windows_support_v4 this feature branch on my fork points to OS version 2.3.0 and ws_os_version_2.2.1 points to OS version 2.2.1. Please let me know if you need anything else, Thanks!

naveentatikonda commented 2 years ago

@peterzhuamazon windows_support_v4 this feature branch on my fork points to OS version 2.3.0 and ws_os_version_2.2.1 points to OS version 2.2.1. Please let me know if you need anything else, Thanks!

@peterzhuamazon do we have any update on this ? Also, just want to remind you about the changes that you want to make for the jenkins build script

peterzhuamazon commented 2 years ago

@naveentatikonda sorry for the late reply I would have some time tomorrow to get a bundle.

Thanks.

peterzhuamazon commented 2 years ago

Working on building 2.3.0 with windows_support_v4 branch now and also editing the build script.