Upgrade plugin to 2.4 and refactor zip dependencies #25
Conversation
|
Seems that CI is checking out wrong commit: "HEAD is now at 272d803 Integrate model inference to build query (#20)" from https://github.com/opensearch-project/neural-search/actions/runs/3284862131/jobs/5411313863 |
jmazanec15
force-pushed
the
upgrade-2.4
branch
from
6932cf8
to
5e4173a
Compare
Upgrades plugin to 2.4. Fixes a couple compile time dependencies with ml-commons. Refactors dependencies on zips for ml-commons and k-NN. Signed-off-by: John Mazanec <jmazane@amazon.com>
Signed-off-by: John Mazanec <jmazane@amazon.com>
jmazanec15
force-pushed
the
upgrade-2.4
branch
from
5e4173a
to
6993ca4
Compare
| zipArchive group: 'org.opensearch.plugin', name:'opensearch-knn', version: "${opensearch_build}" | ||
| zipArchive group: 'org.opensearch', name:'opensearch-ml-plugin', version: "${opensearch_build}" |
There was a problem hiding this comment.
Why we need ML-Plugin as dependency? Seems like we just need this for integ tests? If yes what was issue with older behavior?
There was a problem hiding this comment.
They dependency is for integ tests. We specify a custom dependency configuration "zipArchive" to define the scope of the dependency. Declaring a dependency on it this way, we can avoid hardcoding urls and manually downloading from urls.
There was a problem hiding this comment.
The problem with this approach is opensearch-ml-plugin will be downloaded in those cases where we don't need to run the integration tests. But with the URLs approach we make sure that we download this only when it is required not in every case. I can see this as a downside of this approach. Also, how zip archive is making sure that we are downloading the right platform version of the dependency?
There was a problem hiding this comment.
The problem with this approach is opensearch-ml-plugin will be downloaded in those cases where we don't need to run the integration tests.
From here I saw this:
"At runtime, Gradle will locate the declared dependencies if needed for operating a specific task. The dependencies might need to be downloaded from a remote repository, retrieved from a local directory or requires another project to be built in a multi-project setting. This process is called dependency resolution." So, gradle should be able to identify when a dependency is needed and identify that only the k-NN zip is required for compileJava and only download the ml-commons zip for integ test cluster. However, if this isnt the case, we could create a new configuration that extends zipArchive so that only the k-NN zip is downloaded for compileJava. Ill leave this FAR.
Also, how zip archive is making sure that we are downloading the right platform version of the dependency?
Unsure at the moment how this is done, but will investigate. OpenSearch core dependency is platform specific so Im guessing a similar mechanism is followed.
This is a minimal model used for integration tests. Ideally, it would be smaller and/or located somewhere we could download it from, but for now we need to check it in. |
jmazanec15
added
the
Maintenance
Description
Upgrades plugin to 2.4. Fixes a couple compile time dependencies with ml-commons.
Refactors dependencies on zips for ml-commons and k-NN. Currently, both ml-commons and k-NN post zips to maven. We already depend on k-NN for the jar. We can refactor dependencies to rely only on these artifacts and not have to download separately.
Check List
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.