Remove rcf jar and fix zip fetching for AD and JS #497

amitgalitz · 2022-04-08T21:45:58Z

Description

Removed RCF jars and updated to fetch RCF 3.0-rc2 from maven
Added zip and folder deletion before fetching zips for bwc and integCluster so there isn't multiple zips
added bwc back to CI
changed V1JsonToV2StateConverter usage V1JsonToV3StateConverter
Added testDeserialize based on 1.3 rcf model

Issues Resolved

resolves #433

Check List

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.

…tegcluster Signed-off-by: Amit Galitzky <[email protected]>

Signed-off-by: Amit Galitzky <[email protected]>

codecov-commenter · 2022-04-08T22:27:18Z

Codecov Report

Merging #497 (c0fb117) into main (ee057e2) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##               main     #497      +/-   ##
============================================
+ Coverage     78.14%   78.17%   +0.03%     
- Complexity     4155     4160       +5     
============================================
  Files           296      296              
  Lines         17654    17654              
  Branches       1877     1877              
============================================
+ Hits          13795    13801       +6     
+ Misses         2963     2959       -4     
+ Partials        896      894       -2

Flag	Coverage Δ
plugin	`78.17% <100.00%> (+0.03%)`	⬆️

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

Impacted Files	Coverage Δ
.../main/java/org/opensearch/ad/ml/CheckpointDao.java	`70.19% <ø> (+0.64%)`	⬆️
.../java/org/opensearch/ad/AnomalyDetectorPlugin.java	`96.53% <100.00%> (ø)`
...ansport/handler/AnomalyResultBulkIndexHandler.java	`69.35% <0.00%> (-1.62%)`	⬇️
.../main/java/org/opensearch/ad/cluster/HashRing.java	`81.37% <0.00%> (-0.41%)`	⬇️
...ain/java/org/opensearch/ad/model/ModelProfile.java	`70.90% <0.00%> (ø)`
...rch/ad/transport/AnomalyResultTransportAction.java	`80.82% <0.00%> (+0.68%)`	⬆️
...va/org/opensearch/ad/AnomalyDetectorJobRunner.java	`64.17% <0.00%> (+0.78%)`	⬆️
...ava/org/opensearch/ad/task/ADHCBatchTaskCache.java	`90.12% <0.00%> (+1.23%)`	⬆️

kaituo · 2022-04-08T22:57:02Z

src/test/java/org/opensearch/ad/ml/CheckpointDaoTests.java

+        String json = Files.readString(Paths.get(filePath), Charset.defaultCharset());
+        Map map = gson.fromJson(json, Map.class);
+        String model = (String) ((Map) ((Map) ((ArrayList) ((Map) map.get("hits")).get("hits")).get(0)).get("_source")).get("modelV2");
+        ThresholdedRandomCutForest forest = checkpointDao.toTrcf(model);


Can you add checks on the specific fields of trcf. Please check my other tests.

kaituo · 2022-04-08T23:01:15Z

src/test/java/org/opensearch/ad/ml/CheckpointDaoTests.java

@@ -154,7 +157,7 @@
    private GenericObjectPool<LinkedBuffer> serializeRCFBufferPool;
    private RandomCutForestMapper mapper;
    private ThresholdedRandomCutForestMapper trcfMapper;
-    private V1JsonToV2StateConverter converter;
+    private V1JsonToV3StateConverter converter;


There is also a V2StateToV3ForestConverter. We need to convert from v1 to v3, v2 to v3. If v2 and v3 are incompatible, we need to add a field modelV3 in checkpoint.

kaituo · 2022-04-08T23:02:06Z

src/test/java/org/opensearch/ad/ml/CheckpointDaoTests.java

+    // .parseReader(new FileReader(new File(getClass().getResource(labalFileName).toURI()), Charset.defaultCharset()))
+
+    public void testDeserializeRCFModel() throws Exception {
+        // Model in file 1_3_0_rcf_model.json not passed initialization yet


So it means the model is empty? If yes, have you tested model fully initialized?

kaituo · 2022-04-08T23:11:23Z

build.gradle

@@ -590,6 +607,9 @@ dependencies {
    compileOnly "org.opensearch:opensearch-job-scheduler-spi:${job_scheduler_version}"
    implementation "org.opensearch:common-utils:${common_utils_version}"
    implementation "org.opensearch.client:opensearch-rest-client:${opensearch_version}"
+    implementation 'software.amazon.randomcutforest:randomcutforest-parkservices:3.0-rc2'


Could you list the tests you have done to upgrade to rc2? What I can think of:

real time single stream and HCAD (including create/start detector, see detector emit results; then stop cluster and restart it to see if results continue to show)

historical single stream and HCAD

backward compatible tests: v1 to v3 model, v2 to v3 model: v1 model in checkpoint should not be overridden, and we add v2 and v3 model in checkpoint if necessary. You can start with a v1 cluster, start real time single stream and hcad detector, let them run and produce results; then upgrade some nodes to v2 cluster, see if anything happens when both v1 and v2 node co-exists in the cluster (this is to simulate our blue/green deployment in service), then remove v1 nodes and let v2 run a while, then upgrade some nodes to v3, and see if they can co-exist, then remove v2 and let v3 nodes run.

Does memory size formula still hold (Check MemoryTracker)? No need to be exactly the same. Want to verify if the ballpark number is similar.

V1 refers to the checkpoint version we have used until #149. v2 is what we have until your PR. v3 is what your PR tries to bring.

I tested single stream real time and historical, I'll do the same for HCAD right now. BWC test were the test added based on Yaliang's code to check deserialize model from 1.3. And converting test to v1 to v3, I didn't add v2 to v3.

The unit tests for bwc is not enough. It just checks the conversion has no exception. We will need to do some e2e testing.

I manually tested that after starting both single and HCAD (both real time and historical) with current RCF and getting results, I can switch to RCF 3.0-rc2 and there is no issues, detectors continue to run the same

After letting the detector run for longer I do get one issue, Failed to generate checkpoint, java.lang.IllegalStateException: There is discepancy in indices

kaituo · 2022-04-08T23:11:55Z

src/test/java/org/opensearch/ad/ml/CheckpointDaoTests.java

@@ -1000,4 +1003,14 @@ public void testFromEntityModelCheckpointWithEntity() throws Exception {
        }
        return point;
    }
+    // .parseReader(new FileReader(new File(getClass().getResource(labalFileName).toURI()), Charset.defaultCharset()))


unused code

will remove

amitgalitz · 2022-04-13T18:13:44Z

Closing in favor of new PR that fetches RCF 3.0-rc1 instead of 3.0-rc2. There are some compatibility issue that need further investigation for AD transition to rc2.

amitgalitz added 5 commits April 8, 2022 20:17

removed rcf jar, added bwc test, fixed maven zip fetch for bwc and in…

75c37bd

…tegcluster Signed-off-by: Amit Galitzky <[email protected]>

fixed to correct directory path for ad bwc zip

cde2f39

Signed-off-by: Amit Galitzky <[email protected]>

spotlessApply formating

90f99b7

Signed-off-by: Amit Galitzky <[email protected]>

add back bwc testing to CI

f216a98

Signed-off-by: Amit Galitzky <[email protected]>

fix CI styling

c0fb117

Signed-off-by: Amit Galitzky <[email protected]>

amitgalitz requested a review from a team April 8, 2022 21:45

opensearch-trigger-bot bot added backport 1.x infra Changes to infrastructure, testing, CI/CD, pipelines, etc. labels Apr 8, 2022

amitgalitz requested review from kaituo and ylwu-amzn April 8, 2022 22:36

amitgalitz removed the backport 1.x label Apr 8, 2022

kaituo reviewed Apr 8, 2022

View reviewed changes

amitgalitz closed this Apr 13, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove rcf jar and fix zip fetching for AD and JS #497

Remove rcf jar and fix zip fetching for AD and JS #497

amitgalitz commented Apr 8, 2022 •

edited

Loading

codecov-commenter commented Apr 8, 2022

kaituo Apr 8, 2022

kaituo Apr 8, 2022

kaituo Apr 8, 2022

kaituo Apr 8, 2022 •

edited

Loading

amitgalitz Apr 8, 2022 •

edited

Loading

kaituo Apr 9, 2022 •

edited

Loading

amitgalitz Apr 12, 2022

amitgalitz Apr 12, 2022

kaituo Apr 8, 2022

amitgalitz Apr 8, 2022

amitgalitz commented Apr 13, 2022

Remove rcf jar and fix zip fetching for AD and JS #497

Remove rcf jar and fix zip fetching for AD and JS #497

Conversation

amitgalitz commented Apr 8, 2022 • edited Loading

Description

Issues Resolved

Check List

codecov-commenter commented Apr 8, 2022

Codecov Report

kaituo Apr 8, 2022

Choose a reason for hiding this comment

kaituo Apr 8, 2022

Choose a reason for hiding this comment

kaituo Apr 8, 2022

Choose a reason for hiding this comment

kaituo Apr 8, 2022 • edited Loading

Choose a reason for hiding this comment

amitgalitz Apr 8, 2022 • edited Loading

Choose a reason for hiding this comment

kaituo Apr 9, 2022 • edited Loading

Choose a reason for hiding this comment

amitgalitz Apr 12, 2022

Choose a reason for hiding this comment

amitgalitz Apr 12, 2022

Choose a reason for hiding this comment

kaituo Apr 8, 2022

Choose a reason for hiding this comment

amitgalitz Apr 8, 2022

Choose a reason for hiding this comment

amitgalitz commented Apr 13, 2022

amitgalitz commented Apr 8, 2022 •

edited

Loading

kaituo Apr 8, 2022 •

edited

Loading

amitgalitz Apr 8, 2022 •

edited

Loading

kaituo Apr 9, 2022 •

edited

Loading