Remove guava dependency #1855

sam-herman · 2024-01-11T06:55:34Z

Description

Removing unnecessary guava dependencies

Issues Resolved

Check List

[ X] New functionality includes testing.
- [X ] All tests pass
[X ] New functionality has been documented.
- [X ] New functionality has javadoc added
[X ] 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: Samuel Herman <[email protected]>

Hailong-am · 2024-01-11T07:24:01Z

ml-algorithms/src/main/java/org/opensearch/ml/engine/memory/MLMemoryManager.java

-import java.util.LinkedList;
-import java.util.List;
-import java.util.Map;
+import java.util.*;


try not use *

Hailong-am · 2024-01-11T07:25:04Z

memory/src/main/java/org/opensearch/ml/memory/index/InteractionsIndex.java

@@ -275,7 +273,7 @@ public void getInteractions(String conversationId, int from, int maxResults, Act
        conversationMetaIndex.checkAccess(conversationId, accessListener);
    }

-    @VisibleForTesting
+    // VisibleForTesting


we can have add customized annotation VisibleForTesting, so we can keep this and update package name only.

The most significant advantage of @VisibleForTesting is that it can be recognized by static analysis tools. These tools can use the annotation to understand that a particular method or field is more accessible than it should be strictly for testing purposes. This understanding can help in various ways, such as:

Ignoring certain access level warnings or errors that would otherwise be flagged in the code analysis.
Ensuring that the broader visibility is not misused or accidentally extended in non-test code.

Personally I don't prefer to use only // VisibleForTesting. I don't prefer to mark guava as unnecessary and then replace this with VisibleForTesting

Same goes for ImmutableMap over Map. I don't prefer to replace ImmutableMap with Map so that we can mark guava is unnecessary. ImmutableMap has several benefits over vanilla Map.

@dhrubo-os curious what are the advantages you see of ImmutableMap over Map? If it's the immutability you seek than Map.of() also creates an immutable collection.

Immutability, thread-safe, better performance

Hailong-am · 2024-01-11T07:28:14Z

plugin/src/main/java/org/opensearch/ml/action/forward/TransportForwardAction.java

-import java.util.HashMap;
-import java.util.Map;
-import java.util.Set;
+import java.util.*;


same here, avoid using *

plugin/src/main/java/org/opensearch/ml/action/handler/MLSearchHandler.java

Hailong-am · 2024-01-11T07:34:07Z

plugin/src/main/java/org/opensearch/ml/autoredeploy/MLModelAutoReDeployer.java

-import java.util.List;
-import java.util.Optional;
-import java.util.Queue;
+import java.util.*;


same here, don't use *

plugin/src/test/java/org/opensearch/ml/rest/MLModelGroupRestIT.java

Hailong-am · 2024-01-11T07:44:13Z

plugin/src/test/java/org/opensearch/ml/rest/MLModelGroupRestIT.java

 import java.util.Map;

+import org.apache.commons.lang3.exception.ExceptionUtils;


How about use ExceptionsHelper provided by OpenSearch instead of ExceptionUtils?

Hailong-am · 2024-01-11T07:44:50Z

plugin/src/test/java/org/opensearch/ml/rest/SecureMLRestIT.java

 import java.util.Map;

+import org.apache.commons.lang3.exception.ExceptionUtils;


How about use ExceptionsHelper provided by OpenSearch instead of ExceptionUtils

Zhangxunmt

Did you compare the opensearch-ml zip size before and after this removal? We can remove it for now but we probably will add some of them back to use Guava cache for better cache management if the zip size doesn't change much.

Zhangxunmt · 2024-01-11T18:11:49Z

...ithms/src/test/java/org/opensearch/ml/engine/algorithms/remote/AwsConnectorExecutorTest.java

-        Map<String, String> credential = ImmutableMap
+        Map<String, String> credential = Map
            .of(ACCESS_KEY_FIELD, encryptor.encrypt("test_key"), SECRET_KEY_FIELD, encryptor.encrypt("test_secret_key"));
-        Map<String, String> parameters = ImmutableMap.of(REGION_FIELD, "us-west-2", SERVICE_NAME_FIELD, "sagemaker");
+        Map<String, String> parameters = Map.of(REGION_FIELD, "us-west-2", SERVICE_NAME_FIELD, "sagemaker");


Can we keep this ImmutableMap at least for these ones? It doesn't look like we want to update them in the code and also we need to handle null values? We can use the equivalent in aws sdk through "import software.amazon.awssdk.utils.ImmutableMap"

Hmm.. the Map.of() is already immutable. Any reason we are interested to keep ImmutableMap ?

Good question. The Map.of() only gives you the very basic immutableMap which is not convenient to handle Null value, factory pattern, builder pattern etc. I think you will find more advantages by searching the difference online. because AWS already provided the same ImmutableMap, why not use it without adding any new dependencies?

I think it's better to use core language unless there is a clear reason to add new external dependency with a clear use case to show why those are needed. So far I haven't seen one and honestly the shaded repackage thing breaks my IDE (besides mess of exclusions in the gradle.build), which is kinda annoying as well.

Specifically regarding the AWS SDK vs Guava, I would rather keep Guava rather than rely on AWS SDK implementation.

With that being said, if the community doesn't want to change this dependency I'm fine abandoning this as well.

shaded repackage thing breaks my IDE

try to using latest IntelliJ IDEA version of 2023.3, the shadow jar was fix in this version. reference here https://youtrack.jetbrains.com/issue/IDEA-321592

Zhangxunmt · 2024-01-11T18:12:12Z

plugin/src/main/java/org/opensearch/ml/action/forward/TransportForwardAction.java

-import java.util.HashMap;
-import java.util.Map;
-import java.util.Set;
+import java.util.*;


…Handler.java Co-authored-by: Hailong Cui <[email protected]> Signed-off-by: samuel-oci <[email protected]>

….java Co-authored-by: Hailong Cui <[email protected]> Signed-off-by: samuel-oci <[email protected]>

sam-herman · 2024-01-12T18:48:09Z

Did you compare the opensearch-ml zip size before and after this removal? We can remove it for now but we probably will add some of them back to use Guava cache for better cache management if the zip size doesn't change much.

Hi @Zhangxunmt this PR is not really have the zip size optimization as it's main focus, but rather it was more meant about making the build file clean from exclusion and shading acrobatics and avoiding dependencies that can collide with opensearch runtime.

Now if we intend on keeping Guava going forward for various other reasons (e.g. Cache) then should we use runtime dependency instead of compile time?
You can pass the guava to testImplementation instead and avoid the exclusions and shading?

PS: If that approach would work better then I'm open to iterate on it to move it there.

ylwu-amzn · 2024-01-12T18:50:03Z

Some new PR merged to main branch. Rebase to make the CI pass?

Zhangxunmt · 2024-06-18T17:49:31Z

@samuel-oci Can you rebase this PR and merge? It's been a long time. Not sure if it has new conflicts with recent commits. Currently still no plan to use guava for cache.

dhrubo-os · 2025-01-10T00:11:16Z

@sam-herman are you still working on this issue? We are planning to close this PR if there's no update/progress in this month of January. Thanks.

sam-herman added 2 commits January 10, 2024 22:29

removing guava dependencies

00c22c5

Signed-off-by: Samuel Herman <[email protected]>

fix test

8db913e

Signed-off-by: Samuel Herman <[email protected]>

sam-herman requested review from b4sjoo, dhrubo-os, jngz-es, model-collapse, rbhavna, ylwu-amzn, zane-neo, Zhangxunmt, austintlee and HenryL27 as code owners January 11, 2024 06:55

sam-herman had a problem deploying to ml-commons-cicd-env January 11, 2024 06:55 — with GitHub Actions Failure

sam-herman had a problem deploying to ml-commons-cicd-env January 11, 2024 06:55 — with GitHub Actions Error

opensearch-trigger-bot bot added the documentation Improvements or additions to documentation label Jan 11, 2024

sam-herman had a problem deploying to ml-commons-cicd-env January 11, 2024 06:55 — with GitHub Actions Failure

sam-herman had a problem deploying to ml-commons-cicd-env January 11, 2024 06:55 — with GitHub Actions Error

Hailong-am reviewed Jan 11, 2024

View reviewed changes

Zhangxunmt reviewed Jan 11, 2024

View reviewed changes

Update plugin/src/main/java/org/opensearch/ml/action/handler/MLSearch…

091c47c

…Handler.java Co-authored-by: Hailong Cui <[email protected]> Signed-off-by: samuel-oci <[email protected]>

sam-herman had a problem deploying to ml-commons-cicd-env January 12, 2024 04:58 — with GitHub Actions Error

sam-herman had a problem deploying to ml-commons-cicd-env January 12, 2024 04:58 — with GitHub Actions Failure

sam-herman had a problem deploying to ml-commons-cicd-env January 12, 2024 04:58 — with GitHub Actions Error

Update plugin/src/test/java/org/opensearch/ml/rest/MLModelGroupRestIT…

ca39e61

….java Co-authored-by: Hailong Cui <[email protected]> Signed-off-by: samuel-oci <[email protected]>

sam-herman had a problem deploying to ml-commons-cicd-env January 12, 2024 18:42 — with GitHub Actions Error

sam-herman had a problem deploying to ml-commons-cicd-env January 12, 2024 18:43 — with GitHub Actions Failure

sam-herman had a problem deploying to ml-commons-cicd-env January 12, 2024 18:43 — with GitHub Actions Error

sam-herman had a problem deploying to ml-commons-cicd-env January 12, 2024 18:43 — with GitHub Actions Failure

sam-herman mentioned this pull request Jan 19, 2024

Issue 1878/1879/1880 fixing index not found for model group/model/tasks #1889

Merged

1 task

mingshl closed this Jan 10, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove guava dependency #1855

Remove guava dependency #1855

sam-herman commented Jan 11, 2024

Hailong-am Jan 11, 2024

Hailong-am Jan 11, 2024

dhrubo-os Jan 12, 2024

sam-herman Jan 12, 2024

dhrubo-os Jan 10, 2025

Hailong-am Jan 11, 2024

Zhangxunmt Jan 11, 2024

Hailong-am Jan 11, 2024

Hailong-am Jan 11, 2024

Hailong-am Jan 11, 2024

Zhangxunmt left a comment

Zhangxunmt Jan 11, 2024 •

edited

Loading

sam-herman Jan 12, 2024

Zhangxunmt Jan 12, 2024

sam-herman Jan 12, 2024

Hailong-am Jan 12, 2024 •

edited

Loading

Zhangxunmt Jan 11, 2024

sam-herman commented Jan 12, 2024 •

edited

Loading

ylwu-amzn commented Jan 12, 2024 •

edited

Loading

Zhangxunmt commented Jun 18, 2024

dhrubo-os commented Jan 10, 2025

		import java.util.Map;

		import org.apache.commons.lang3.exception.ExceptionUtils;

Remove guava dependency #1855

Remove guava dependency #1855

Conversation

sam-herman commented Jan 11, 2024

Description

Issues Resolved

Check List

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zhangxunmt left a comment

Choose a reason for hiding this comment

Zhangxunmt Jan 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hailong-am Jan 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sam-herman commented Jan 12, 2024 • edited Loading

ylwu-amzn commented Jan 12, 2024 • edited Loading

Zhangxunmt commented Jun 18, 2024

dhrubo-os commented Jan 10, 2025

Zhangxunmt Jan 11, 2024 •

edited

Loading

Hailong-am Jan 12, 2024 •

edited

Loading

sam-herman commented Jan 12, 2024 •

edited

Loading

ylwu-amzn commented Jan 12, 2024 •

edited

Loading