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

Jump to bottom

Fix: correct gradle file function name and comment #795

Merged
merged 2 commits into from
Jul 22, 2024
Merged

Fix: correct gradle file function name and comment #795

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
247824e
fix gradle file function name and comment
yuye-aws Jun 17, 2024
1c3010d
update changelog
yuye-aws Jun 17, 2024
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
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 @@ -20,6 +20,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
- Adds dynamic knn query parameters efsearch and nprobes [#814](https://github.com/opensearch-project/neural-search/pull/814/)
- Enable '.' for nested field in text embedding processor ([#811](https://github.com/opensearch-project/neural-search/pull/811))
### Bug Fixes
- Fix function names and comments in the gradle file for BWC tests ([#795](https://github.com/opensearch-project/neural-search/pull/795/files))
Copy link
Member

@zhichao-aws zhichao-aws Jun 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Could you please provide more details about why we're zip it instead of unzip? For my previous understanding, we fetch the zip archive from the CI website and unzip it to build the cluster.

Copy link
Member Author

Choose a reason for hiding this comment

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

These functions are actually doing zipping instead of unzipping. This PR does not change any code logic and just fix comments and function names to avoid confusion.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, my expression maybe not clear enough. My question is, how we're doing the zip? Could you please describe the whole execution process of the zip* or unzip* function?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is used to install plugin with zip, you can check the gradle file in restart upgrade and rolling upgrade.

versions = [ext.neural_search_bwc_version, opensearch_version]
plugin(project.tasks.unZipBwcMlCommonsPlugin.archiveFile)
plugin(project.tasks.unZipBwcKnnPlugin.archiveFile)
plugin(project.tasks.unZipBwcPlugin.archiveFile)

Copy link
Member Author

@yuye-aws yuye-aws Jun 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Here are the steps:

  1. pullOpensearchArtifact downloads the whole opensearch artifact with plugins.
  2. Tasks like pullBwcPlugin copy the plugin directory within the opensearch artifact plugin folder.
  3. Tasks like zipBwcPlugin zips the plugin directory into a zip file.
  4. testClusters in restart upgrade and rolling upgrade installs the plugin from zip file.

Copy link
Member

Choose a reason for hiding this comment

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

makes sense

- Fix for missing HybridQuery results when concurrent segment search is enabled ([#800](https://github.com/opensearch-project/neural-search/pull/800))
### Infrastructure
- Add BWC for batch ingestion ([#769](https://github.com/opensearch-project/neural-search/pull/769))
Expand Down
16 changes: 8 additions & 8 deletions qa/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,8 +159,8 @@ task pullBwcPlugin {
}
}

// Task to unzip ml-commons plugin from archive
task unZipBwcMlCommonsPlugin(type: Zip) {
// Task to zip ml-commons plugin from archive
task zipBwcMlCommonsPlugin(type: Zip) {
dependsOn "pullMlCommonsBwcPlugin"
from(Path.of(tmp_dir.absolutePath, "opensearch-ml"))
destinationDirectory = tmp_dir
Expand All @@ -170,10 +170,10 @@ task unZipBwcMlCommonsPlugin(type: Zip) {
}
}

// Task to unzip knn plugin from archive
task unZipBwcKnnPlugin(type: Zip) {
// Task to zip knn plugin from archive
task zipBwcKnnPlugin(type: Zip) {
dependsOn "pullKnnBwcPlugin"
dependsOn "unZipBwcMlCommonsPlugin"
dependsOn "zipBwcMlCommonsPlugin"
from(Path.of(tmp_dir.absolutePath, "opensearch-knn"))
destinationDirectory = tmp_dir
archiveFileName = "opensearch-knn-${neural_search_bwc_version_no_qualifier}.zip"
Copy link
Member

Choose a reason for hiding this comment

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

Do you know where is this archiveFileName used?

Copy link
Member Author

Choose a reason for hiding this comment

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

Used to define file for plugin installation.

Copy link
Member

Choose a reason for hiding this comment

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

I did some research and find that this parameter is inhereted from the "zip" task, and is used to indicate the final file of "zip" task

Copy link
Member Author

Choose a reason for hiding this comment

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

You can check AbstractArchiveTask for gradle: https://docs.gradle.org/current/dsl/org.gradle.api.tasks.bundling.AbstractArchiveTask.html

Expand All @@ -182,9 +182,9 @@ task unZipBwcKnnPlugin(type: Zip) {
}
}

// Task to unzip neural search plugin from archive
task unZipBwcPlugin(type: Zip) {
dependsOn "unZipBwcKnnPlugin"
// Task to zip neural search plugin from archive
task zipBwcPlugin(type: Zip) {
dependsOn "zipBwcKnnPlugin"
dependsOn "pullBwcPlugin"
from(Path.of(tmp_dir.absolutePath, "opensearch-neural-search"))
destinationDirectory = tmp_dir
Expand Down
8 changes: 4 additions & 4 deletions qa/restart-upgrade/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,9 +45,9 @@ testClusters {
}
}else{
versions = [ext.neural_search_bwc_version, opensearch_version]
plugin(project.tasks.unZipBwcMlCommonsPlugin.archiveFile)
plugin(project.tasks.unZipBwcKnnPlugin.archiveFile)
plugin(project.tasks.unZipBwcPlugin.archiveFile)
plugin(project.tasks.zipBwcMlCommonsPlugin.archiveFile)
plugin(project.tasks.zipBwcKnnPlugin.archiveFile)
plugin(project.tasks.zipBwcPlugin.archiveFile)
}
setting 'path.repo', "${buildDir}/cluster/shared/repo/${baseName}"
setting 'http.content_type.required', 'true'
Expand All @@ -57,7 +57,7 @@ testClusters {
// Task to run BWC tests against the old cluster
task testAgainstOldCluster(type: StandaloneRestIntegTestTask) {
if(!ext.bwcBundleTest){
dependsOn "unZipBwcPlugin"
dependsOn "zipBwcPlugin"
}
useCluster testClusters."${baseName}"
systemProperty 'tests.rest.bwcsuite_cluster', 'old_cluster'
Expand Down
8 changes: 4 additions & 4 deletions qa/rolling-upgrade/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,9 +45,9 @@ testClusters {
}
}else{
versions = [ext.neural_search_bwc_version, opensearch_version]
plugin(project.tasks.unZipBwcMlCommonsPlugin.archiveFile)
plugin(project.tasks.unZipBwcKnnPlugin.archiveFile)
plugin(project.tasks.unZipBwcPlugin.archiveFile)
plugin(project.tasks.zipBwcMlCommonsPlugin.archiveFile)
plugin(project.tasks.zipBwcKnnPlugin.archiveFile)
plugin(project.tasks.zipBwcPlugin.archiveFile)
}
setting 'path.repo', "${buildDir}/cluster/shared/repo/${baseName}"
setting 'http.content_type.required', 'true'
Expand All @@ -58,7 +58,7 @@ testClusters {
// Task to run BWC tests against the old cluster
task testAgainstOldCluster(type: StandaloneRestIntegTestTask) {
if(!ext.bwcBundleTest){
dependsOn "unZipBwcPlugin"
dependsOn "zipBwcPlugin"
}
useCluster testClusters."${baseName}"
systemProperty 'tests.rest.bwcsuite_cluster', 'old_cluster'
Expand Down
Loading