adding reflections as a dependency #1559

dhrubo-os · 2023-04-19T02:06:05Z

Description

adding reflections as a dependency.

POST _plugins/_ppl
{
  "query": "source=iris_data | fields petal_length_in_cm,petal_width_in_cm | kmeans centroids=3"
}

This command was breaking and the debug log is:

Caused by: java.lang.NoClassDefFoundError: org/reflections/Reflections
opensearch-node1         | 	at org.opensearch.ml.common.MLCommonsClassLoader.loadMLAlgoParameterClassMapping(MLCommonsClassLoader.java:66) ~[?:?]
opensearch-node1         | 	at org.opensearch.ml.common.MLCommonsClassLoader.loadClassMapping(MLCommonsClassLoader.java:51) ~[?:?]
opensearch-node1         | 	at org.opensearch.ml.common.MLCommonsClassLoader.lambda$static$0(MLCommonsClassLoader.java:39) ~[?:?]
opensearch-node1         | 	at java.security.AccessController.doPrivileged(AccessController.java:569) ~[?:?]
opensearch-node1         | 	at org.opensearch.ml.common.MLCommonsClassLoader.<clinit>(MLCommonsClassLoader.java:38) ~[?:?]
opensearch-node1         | 	at org.opensearch.ml.common.output.MLOutput.fromStream(MLOutput.java:35) ~[?:?]
opensearch-node1         | 	at org.opensearch.ml.common.transport.MLTaskResponse.<init>(MLTaskResponse.java:38) ~[?:?]
opensearch-node1         | 	at org.opensearch.ml.common.transport.MLTaskResponse.fromActionResponse(MLTaskResponse.java:55) ~[?:?]
opensearch-node1         | 	at org.opensearch.ml.client.MachineLearningNodeClient.lambda$getMlPredictionTaskResponseActionListener$8(MachineLearningNodeClient.java:191)

Issues Resolved

[List any issues this PR will resolve]

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.

Signed-off-by: Dhrubo Saha <[email protected]>

Yury-Fridlyand · 2023-04-19T02:13:29Z

Could you please add integration tests?

codecov-commenter · 2023-04-19T02:30:58Z

Codecov Report

Merging #1559 (b6f6788) into main (59ef998) will not change coverage.
The diff coverage is n/a.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff            @@
##               main    #1559   +/-   ##
=========================================
  Coverage     97.18%   97.18%           
  Complexity     4106     4106           
=========================================
  Files           371      371           
  Lines         10462    10462           
  Branches        706      706           
=========================================
  Hits          10168    10168           
  Misses          287      287           
  Partials          7        7

Flag	Coverage Δ
sql-engine	`97.18% <ø> (ø)`

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

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

dhrubo-os · 2023-04-19T02:41:01Z

Could you please add integration tests?

I'm brand new in this plugin. Can you please show me how to write a integration test for this?

vamsi-amazon · 2023-04-19T16:12:59Z

Could you please add integration tests?

I'm brand new in this plugin. Can you please show me how to write a integration test for this?
@dhrubo-os

https://github.com/opensearch-project/sql/blob/main/integ-test/src/test/java/org/opensearch/sql/ppl/WhereCommandIT.java
This is an example Integration test file for where commands.

Similarly you could create a separate file for MLCommands or you can subdivide into more files based on different types of ML commands. Most of the useful functions are already present in parent class.

Sample command to run integTests.
./gradlew :integ-test:integTest --tests "org.opensearch.sql.ppl.WhereCommandIT"

dai-chen · 2023-04-19T16:37:31Z

This is a required runtime dependency for ML client, right? If so, could you explain why this is not configured from ML side?

ylwu-amzn · 2023-04-19T18:16:09Z

This is a required runtime dependency for ML client, right? If so, could you explain why this is not configured from ML side?

hi, @dai-chen The background is @Yury-Fridlyand found we have a fat ml-commons client jar opensearch-project/ml-commons#778, later, we removed the common-utils and refection to avoid generating fat jar opensearch-project/ml-commons#796.

And neural search did code change by adding the runtime dependency https://github.com/opensearch-project/neural-search/blob/main/build.gradle#L151

dai-chen · 2023-04-19T19:18:04Z

@ylwu-amzn Thanks for the context! I thought this can be a runtime dependency in ml-commons and SQL or other plugin introduces the dependency transitively. Will take a look at the link you shared to confirm.

ylwu-amzn · 2023-04-19T19:29:32Z

@ylwu-amzn Thanks for the context! I thought this can be a runtime dependency in ml-commons and SQL or other plugin introduces the dependency transitively. Will take a look at the link you shared to confirm.

Yes, actually team is working on that part now. We have not found a good way to add this to ml-commons (opensearch-project/ml-commons#808, we tried this in PR opensearch-project/ml-commons#811, not working actually), then other plugins like SQL can download the transitive dependencies automatically. How about let's add this runtime dependency for now to unblock 2.7 release. We can enhance this part later

dai-chen · 2023-04-19T19:34:33Z

@ylwu-amzn I see. I thought ml-commons is already in Maven repo with its own dependencies defined. We can improve this later in case there're more dependencies required. Thanks!

Btw, could you or @dhrubo-os think about how to add integration test in SQL or elsewhere? I think it's missing in SQL. Not sure if you have it from your side.

dai-chen · 2023-04-19T19:37:18Z

@dhrubo-os @ylwu-amzn Should I backport this to 2.7 branch? Is code freeze now?

dai-chen · 2023-04-19T19:40:16Z

Just realized BWC failed. I reran it. Please have a look if it fails still.

dhrubo-os · 2023-04-19T20:17:53Z

@dai-chen

for build (ubuntu-latest, 17)

  2> REPRODUCE WITH: ./gradlew ':integ-test:integTest' --tests "org.opensearch.sql.ppl.StandaloneIT.testSourceFieldQuery" -Dtests.seed=E70277A01E44958E -Dtests.security.manager=false -Dtests.locale=tr-TR -Dtests.timezone=Africa/Lusaka -Druntime.java=17
  2> org.junit.ComparisonFailure: expected:<...,
          "type": "str[i]ng"
        }
      ],
      "da...> but was:<...,
          "type": "str[ı]ng"
        }
      ],
      "da...>
        at __randomizedtesting.SeedInfo.seed([E70277A01E44958E:7D80FF271D471980]:0)
        at org.junit.Assert.assertEquals(Assert.java:117)
  1> [2023-04-19T21:54:17,800][INFO ][o.o.s.p.StandaloneIT     ] [testSourceFieldQuery] initializing REST clients against [http://[::1]:33605, http://127.0.0.1:45711]
  1> [2023-04-19T21:54:18,501][INFO ][o.o.s.p.PPLService       ] [testSourceFieldQuery{request_id=66b4593b-34c7-46c7-bf2c-b60c8bceb62c}] [66b4593b-34c7-46c7-bf2c-

and for build (ubuntu-latest, 11)

Task :integ-test:sqlBwcCluster#mixedClusterTask FAILED

Should we restart the test again? Is there anything I can do?

I talked with @zelinh , we need to do that soon, we are getting close to generating the RC.

dai-chen · 2023-04-19T20:32:25Z

Rerunning the failed workflow

penghuo

Please add the issue to follow up after 2.7 release.

dhrubo-os · 2023-04-19T21:36:39Z

Please add the issue to follow up after 2.7 release.

@penghuo can you please approve this PR: #1561

Yury-Fridlyand · 2023-04-20T19:33:13Z

BWC still fails.

Signed-off-by: Dhrubo Saha <[email protected]> (cherry picked from commit 4bbd12a)

Signed-off-by: Dhrubo Saha <[email protected]> (cherry picked from commit 4bbd12a) Co-authored-by: Dhrubo Saha <[email protected]>

Signed-off-by: Dhrubo Saha <[email protected]> Signed-off-by: Mitchell Gale <[email protected]>

adding reflections as a dependency

b6f6788

Signed-off-by: Dhrubo Saha <[email protected]>

dhrubo-os requested review from anirudha, penghuo, ps48, kavithacm, derek-ho, joshuali925, dai-chen, YANG-DB, rupal-bq, mengweieric, vamsi-amazon, Yury-Fridlyand and MaxKsyunz as code owners April 19, 2023 02:06

dhrubo-os mentioned this pull request Apr 19, 2023

adding reflections as a dependency #1558

Closed

6 tasks

vamsi-amazon added bug Something isn't working backport 2.x labels Apr 19, 2023

dai-chen approved these changes Apr 19, 2023

View reviewed changes

penghuo approved these changes Apr 19, 2023

View reviewed changes

dai-chen mentioned this pull request Apr 19, 2023

adding reflections as a dependency #1561

Merged

6 tasks

This was referenced Apr 19, 2023

[FEATURE] Add missing Integration test for PPL integration with MLCommons #1563

Open

[FEATURE] Remove reflections from dependency when transitive dependency of MLCommons works. #1564

Open

Yury-Fridlyand approved these changes May 9, 2023

View reviewed changes

Yury-Fridlyand merged commit 4bbd12a into opensearch-project:main May 9, 2023

opensearch-trigger-bot bot mentioned this pull request May 9, 2023

[Backport 2.x] adding reflections as a dependency #1618

Merged

opensearch-trigger-bot bot pushed a commit that referenced this pull request May 9, 2023

adding reflections as a dependency (#1559)

252fc62

Signed-off-by: Dhrubo Saha <[email protected]> (cherry picked from commit 4bbd12a)

Yury-Fridlyand pushed a commit that referenced this pull request May 11, 2023

adding reflections as a dependency (#1559) (#1618)

2bb7a65

Signed-off-by: Dhrubo Saha <[email protected]> (cherry picked from commit 4bbd12a) Co-authored-by: Dhrubo Saha <[email protected]>

MitchellGale pushed a commit to Bit-Quill/opensearch-project-sql that referenced this pull request Jun 12, 2023

adding reflections as a dependency (opensearch-project#1559)

42ee0e1

Signed-off-by: Dhrubo Saha <[email protected]> Signed-off-by: Mitchell Gale <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adding reflections as a dependency #1559

adding reflections as a dependency #1559

dhrubo-os commented Apr 19, 2023

Yury-Fridlyand commented Apr 19, 2023

codecov-commenter commented Apr 19, 2023 •

edited

Loading

dhrubo-os commented Apr 19, 2023

vamsi-amazon commented Apr 19, 2023

dai-chen commented Apr 19, 2023

ylwu-amzn commented Apr 19, 2023

dai-chen commented Apr 19, 2023

ylwu-amzn commented Apr 19, 2023 •

edited

Loading

dai-chen commented Apr 19, 2023 •

edited

Loading

dai-chen commented Apr 19, 2023

dai-chen commented Apr 19, 2023

dhrubo-os commented Apr 19, 2023

dai-chen commented Apr 19, 2023

penghuo left a comment

dhrubo-os commented Apr 19, 2023

Yury-Fridlyand commented Apr 20, 2023

adding reflections as a dependency #1559

adding reflections as a dependency #1559

Conversation

dhrubo-os commented Apr 19, 2023

Description

Issues Resolved

Check List

Yury-Fridlyand commented Apr 19, 2023

codecov-commenter commented Apr 19, 2023 • edited Loading

Codecov Report

dhrubo-os commented Apr 19, 2023

vamsi-amazon commented Apr 19, 2023

dai-chen commented Apr 19, 2023

ylwu-amzn commented Apr 19, 2023

dai-chen commented Apr 19, 2023

ylwu-amzn commented Apr 19, 2023 • edited Loading

dai-chen commented Apr 19, 2023 • edited Loading

dai-chen commented Apr 19, 2023

dai-chen commented Apr 19, 2023

dhrubo-os commented Apr 19, 2023

dai-chen commented Apr 19, 2023

penghuo left a comment

Choose a reason for hiding this comment

dhrubo-os commented Apr 19, 2023

Yury-Fridlyand commented Apr 20, 2023

codecov-commenter commented Apr 19, 2023 •

edited

Loading

ylwu-amzn commented Apr 19, 2023 •

edited

Loading

dai-chen commented Apr 19, 2023 •

edited

Loading