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

Remove checked-in ml-commons dependency #529

Merged
merged 1 commit into from
Apr 5, 2022
Merged

Remove checked-in ml-commons dependency #529

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
13 changes: 12 additions & 1 deletion doctest/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,9 @@ doctest.finalizedBy stopOpenSearch
build.dependsOn doctest
clean.dependsOn(cleanBootstrap)

String mlCommonsRemoteFile = 'https://ci.opensearch.org/ci/dbc/distribution-build-opensearch/1.3.1/latest/linux/x64/builds/opensearch/plugins/opensearch-ml-1.3.1.0.zip'
Copy link
Member

Choose a reason for hiding this comment

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

Does this version need to match the version in the project? Externalize the variable and substitute 1.3.1 or we'll always forget to update it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it needs to match the version in the project because we are installing ml-commons plugin here.

I prefer not to externalize the variable here because it could potentially block version bumping on main line.
Taking the current situation as an example, main line has been bumped to 2.0, however, the 2.0 ml-commons remote file doesn't exist yet. So if we use a global/external version variable here, the main line version bump build would fail.

I do agree we need to use global version variable whenever we can so that we don't forget to update it during version bumping though.

Copy link
Contributor Author

@jackiehanyang jackiehanyang Apr 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry, I was confused by all the version numbers and branches... I was wrong about some statements I mentioned above, here's the correction👇

Correction: Using an external version variable won't block version bumping on main line (it will just block the build of this PR). Because even if we don't use a global version variable here, main line version bump build will still fail because ml-commons.1.3.0.zip cannot be installed with 2.0.0, which is the version in the project now.

In order to make version bump procedure more easier and more smooth, we need to ensure the remote zip files are always published before we start making version bump PRs. If that cannot be guaranteed, then we can check if the remote file with the latest version exist first, if not, then still use a checked-in dependency until the remote file is published to avoid build failure.

String mlCommonsPlugin = "ml-commons"

testClusters {
docTestCluster {
plugin(provider(new Callable<RegularFile>(){
Expand All @@ -56,7 +59,15 @@ testClusters {
return new RegularFile() {
@Override
File getAsFile() {
return fileTree("resources/ml-commons").getSingleFile()
File dir = new File('./doctest/' + mlCommonsPlugin)
if (!dir.exists()) {
dir.mkdirs()
}
File f = new File(mlCommonsPlugin, dir)
if (!f.exists()) {
new URL(mlCommonsRemoteFile).withInputStream{ ins -> f.withOutputStream{ it << ins }}
}
return fileTree(mlCommonsPlugin).getSingleFile()
}
}
}
Expand Down