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

Add dependency on k-NN plugin #10

Merged
merged 1 commit into from
Oct 6, 2022
Merged

Add dependency on k-NN plugin #10

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
18 changes: 18 additions & 0 deletions build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,12 +124,30 @@ repositories {
maven { url "https://plugins.gradle.org/m2/" }
}

configurations {
zipArchive
}

def knnJarDirectory = "$buildDir/dependencies/opensearch-knn"

dependencies {
api "org.opensearch:opensearch:${opensearch_version}"
zipArchive group: 'org.opensearch.plugin', name:'opensearch-knn', version: "${opensearch_build}"
api fileTree(dir: knnJarDirectory, include: '*.jar')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why we are taking it as a proper dependency?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also why can't we just pull from maven central?

Copy link
Member Author

Choose a reason for hiding this comment

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

We can get the zip from maven, but we need to add the jar to the class path. To do this, we must unzip the zip file.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Did we try using api thing. It will add in the class path.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes I tried. api wont unzip the zipfile. We have to do it on our own.

Copy link
Collaborator

Choose a reason for hiding this comment

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

How this is different from getting the dependency of ML commons or lets say any other dependency like apache commons in the project? They all get downloaded from maven.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maven does not necessarily have to be jars. Following opensearch-project/opensearch-build#1916, k-NN published the zip using the custom pluginzip function (see k-NN's build.gradle).

ml-commons a different method to just publish the jar to maven. k-NN could also do this, but does not right now. Hence, we have to rely on the zip.

Copy link

@ylwu-amzn ylwu-amzn Oct 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@jmazanec15 , seems it's reasonable to publish just shared part as jar to maven like ml-commons, so client plugin like neural search doesn't need to add the whole zip as dependency. But that takes time. I think it's fine for current phase to rely on zip. I don't see other risks, but we'd better confirm there is no big concern from others like asking infra team?

Copy link
Member Author

Choose a reason for hiding this comment

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

@ylwu-amzn I think this is good for now. I dont see any risks. Integ tests install ml-commons, k-NN and neural-search in same cluster and there are not any issues, so I think it will be okay. What are the other risks involved?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Lets go ahead and create a issue on k-NN for this. Approving the PR meanwhile.

Comment on lines +135 to +136
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
zipArchive group: 'org.opensearch.plugin', name:'opensearch-knn', version: "${opensearch_build}"
api fileTree(dir: knnJarDirectory, include: '*.jar')
api group: 'org.opensearch.plugin', name:'opensearch-knn', version: "${opensearch_build}"

Copy link
Member Author

Choose a reason for hiding this comment

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

Refer to above comment

api group: 'org.opensearch', name:'opensearch-ml-client', version: "${opensearch_build}"
}

// From maven, we can get the k-NN plugin as a zip. In order to add the jar to the classpath, we need to unzip the
// k-NN zip and then copy it into a directory that we specify as a dependency.
task unzip(type: Copy) {
configurations.zipArchive.asFileTree.each {
from(zipTree(it))
}
into knnJarDirectory
}

compileJava {
dependsOn unzip
options.compilerArgs.addAll(["-processor", 'lombok.launch.AnnotationProcessorHider$AnnotationProcessor'])
}
compileTestJava {
Expand Down
10 changes: 4 additions & 6 deletions src/test/java/org/opensearch/neuralsearch/plugin/NeuralSearchTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,13 @@

package org.opensearch.neuralsearch.plugin;

import org.junit.Assert;
import org.opensearch.knn.common.KNNConstants;
import org.opensearch.knn.index.util.KNNEngine;
import org.opensearch.test.OpenSearchTestCase;

public class NeuralSearchTests extends OpenSearchTestCase {

/**
* Dummy test case for passing the build.
*/
public void testDemo() {
Assert.assertTrue(true);
public void testValidateKNNDependency() {
assertEquals(KNNConstants.LUCENE_NAME, KNNEngine.LUCENE.getName());
}
}