ml-commons icon indicating copy to clipboard operation
ml-commons copied to clipboard

Remove guava dependency

Open sam-herman opened this issue 1 year ago • 3 comments

Description

Removing unnecessary guava dependencies

Issues Resolved

https://github.com/opensearch-project/ml-commons/issues/1854

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.

sam-herman avatar Jan 11 '24 06:01 sam-herman

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.

sam-herman avatar Jan 12 '24 18:01 sam-herman

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

ylwu-amzn avatar Jan 12 '24 18:01 ylwu-amzn

@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.

Zhangxunmt avatar Jun 18 '24 17:06 Zhangxunmt