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

Remove checked-in ml-commons dependency #529

Merged
merged 1 commit into from
Apr 5, 2022
Merged

Remove checked-in ml-commons dependency #529

merged 1 commit into from
Apr 5, 2022

Conversation

jackiehanyang
Copy link
Contributor

@jackiehanyang jackiehanyang commented Apr 4, 2022 •
edited
Loading

Signed-off-by: jackieyanghan [email protected]

Description

Remove checked-in ml-commons dependency used for doctest

Issues Resolved

#495
opensearch-project/opensearch-build#716 (comment)

Check List

  • New functionality includes testing.
    • All tests pass, including unit test, integration test and doctest
  • New functionality has been documented.
    • New functionality has javadoc added
    • New functionality has user manual doc added
  • Commits are signed per the DCO using --signoff

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.

@jackiehanyang jackiehanyang requested a review from a team as a code owner April 4, 2022 17:25
@codecov-commenter
Copy link

codecov-commenter commented Apr 4, 2022 •
edited
Loading

Codecov Report

Merging #529 (5e09c48) into 1.3 (20bff4c) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##                1.3     #529   +/-   ##
=========================================
  Coverage     95.25%   95.25%           
  Complexity     2746     2746           
=========================================
  Files           276      276           
  Lines          7416     7416           
  Branches        545      546    +1     
=========================================
  Hits           7064     7064           
  Misses          298      298           
  Partials         54       54
Flag Coverage Δ
query-workbench 62.91% <ø> (ø)
sql-engine 98.40% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 20bff4c...5e09c48. Read the comment docs.

@ylwu-amzn
Copy link
Contributor

ylwu-amzn commented Apr 4, 2022 •
edited
Loading

I see this PR is going to merge to 1.3 branch, why not merge to main branch ? I think we can keep 1.3 as is, no need to backport to 1.3 @penghuo any suggestion?

@jackiehanyang
Copy link
Contributor Author

I see this PR is going to merge to 1.3 branch, why not merge to main branch ? I think we can keep 1.3 as is, no need to backport to 1.3 @penghuo any suggestion?

@ylwu-amzn Main branch is building with 2.0. Once we have ml-commons 2.0 zip file published. We can make this change on main line.

@ylwu-amzn
Copy link
Contributor

Got it, is it possible to merge into 1.x first? Then we can backport to 1.3 if needed?

dblock
dblock reviewed Apr 4, 2022
View reviewed changes
doctest/build.gradle
@@ -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.

@jackiehanyang
Copy link
Contributor Author

Got it, is it possible to merge into 1.x first? Then we can backport to 1.3 if needed?

@ylwu-amzn 1.x is using version 1.3.0 at the moment, not 1.3.1.
So I think we can merge it into 1.3 for now, then backport to 1.x when it's ready

@penghuo penghuo merged commit d85f861 into opensearch-project:1.3 Apr 5, 2022
ps48 added a commit that referenced this pull request Jul 28, 2022
@ps48 @penghuo
@jackiehanyang @Yury-Fridlyand @opensearch-trigger-bot @davidcui1225 @eugenesk24 @gaiksaya @joshuali925 @zelinh @prudhvigodithi @anirudha
Updating 1.x branch to latest 1.3 (#714)
fdb17f6 
* Disable flaky test in JdbcTestIT. (#475)

Signed-off-by: penghuo <[email protected]>

* Update dependency opensearch-ml-client group name (#477)

Signed-off-by: jackieyanghan <[email protected]>

* Collected fixes related to bugs #280, #302, #783, #696 and 317 all together. (#479)

Signed-off-by: Yury Fridlyand <[email protected]>

* Add JDK 8 to CI Matrix  (#483) (#484)

* fixed references in java 8 and added to ci matrix
* filter test that fails due to jdk 8 bug
* use different degree to radian conversion for difference in jdk
* skip date time format tests that cause error due to jdk bug
* only disable DateTime unit test on Java 8

Bypassing failed DCO check

Signed-off-by: David Cui <[email protected]>
(cherry picked from commit b6fccbd)

Co-authored-by: David Cui <[email protected]>

* Add parse docs to PPL commands index (#486) (#487)

Signed-off-by: Joshua Li <[email protected]>

* Add 1.3.0 release notes (#492)

Signed-off-by: Eugene Lee <[email protected]>

* Add Kmeans and AD command documentation (#493) (#497)

Signed-off-by: jackieyanghan <[email protected]>
(cherry picked from commit ee4bce0)

* Bump version to 1.3.1

Signed-off-by: Sayali Gaikawad <[email protected]>

* Bump ml

Signed-off-by: Sayali Gaikawad <[email protected]>

* Add new zip

Signed-off-by: Sayali Gaikawad <[email protected]>

* Updated release drafter

Signed-off-by: Sayali Gaikawad <[email protected]>

* Remove checked-in ml-commons dependency (#529)

Signed-off-by: jackieyanghan <[email protected]>

* Bump workbench to 1.3.1 (#540)

Signed-off-by: Joshua Li <[email protected]>

* Bug Fix, return default ID when log4j ThreadContext is empty (#538) (#539) (#543)

Signed-off-by: penghuo <[email protected]>
(cherry picked from commit 54b7257)

* Incremented version to 1.3.2. (#593)

Signed-off-by: Zelin Hao <[email protected]>
Signed-off-by: Joshua Li <[email protected]>

Co-authored-by: Joshua Li <[email protected]>

* Version bump 1.3.3

Signed-off-by: pgodithi <[email protected]>

* [1.3] Bump spring-beans to 5.2.20 (#632)

Signed-off-by: Joshua Li <[email protected]>

* Version bump 1.3.3

Signed-off-by: pgodithi <[email protected]>

* Incremented version to 1.3.4. (#681)

* Incremented version to 1.3.4.

Signed-off-by: Zelin Hao <[email protected]>

* bump jackson to 2.13.2

Signed-off-by: Joshua Li <[email protected]>

Co-authored-by: Joshua Li <[email protected]>

Co-authored-by: Peng Huo <[email protected]>
Co-authored-by: Jackie Han <[email protected]>
Co-authored-by: Yury-Fridlyand <[email protected]>
Co-authored-by: opensearch-trigger-bot[bot] <98922864+opensearch-trigger-bot[bot]@users.noreply.github.com>
Co-authored-by: David Cui <[email protected]>
Co-authored-by: Eugene Lee <[email protected]>
Co-authored-by: Sayali Gaikawad <[email protected]>
Co-authored-by: Sayali Gaikawad <[email protected]>
Co-authored-by: Joshua Li <[email protected]>
Co-authored-by: Zelin Hao <[email protected]>
Co-authored-by: pgodithi <[email protected]>
Co-authored-by: Anirudha (Ani) Jadhav <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dblock dblock dblock left review comments

@penghuo penghuo penghuo approved these changes

@joshuali925 joshuali925 joshuali925 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jackiehanyang @codecov-commenter @ylwu-amzn @dblock @penghuo @joshuali925