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.

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

use protostuff to serialize/deserialize RCF model #251

Merged
merged 1 commit into from
Mar 25, 2022

Conversation

ylwu-amzn
Copy link
Collaborator

@ylwu-amzn ylwu-amzn commented Mar 25, 2022

Signed-off-by: Yaliang Wu [email protected]

Description

We have to use java object stream to serialize/deserialize tribuo models as protostuff will throw exceptions and when we release 1.3, protostuff has reflection warning issue. So decided to use same way to serialize/deserialize RCF model. And to do that, we have to change RCF state class to implement Serializable interface, check aws/random-cut-forest-by-aws#298.

But with this change, AD can't deserialize old RCF models. So we revert last change in this PR aws/random-cut-forest-by-aws#306. And now protostuff has released 1.8 which fixed the reflection issue. This PR add a new RCFModelSerDeSer to serialize/deserialize RCF model, the same way of AD to use protostuff. For other tribuo models like Kmeans and LinearRegression, we still use existing object stream to serialize/deserialize.

This PR also updated the local RCF jar which built from my branch commit ylwu-amzn/random-cut-forest-by-aws@ad37748, Will check if we have enough time to upload latest RCF jar to maven. If no, will build latest RCF jar from main branch and upload. Create a Github issue to track #250.

Issues Resolved

Close aws/random-cut-forest-by-aws#305

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc 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.

@ylwu-amzn ylwu-amzn requested review from a team, kaituo and Zhangxunmt March 25, 2022 05:32
Zhangxunmt
Zhangxunmt previously approved these changes Mar 25, 2022
Copy link
Collaborator

@Zhangxunmt Zhangxunmt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on my understanding, the protostuff problems are fixed in the new version so we should use it to deserialize/serialize all the tribuo models like linear regression, etc? Also, what is the problem for keep using java object stream in serialize/deserialize?

@ylwu-amzn
Copy link
Collaborator Author

Based on my understanding, the protostuff problems are fixed in the new version so we should use it to deserialize/serialize all the tribuo models like linear regression, etc? Also, what is the problem for keep using java object stream in serialize/deserialize?

protostuff can't serialize tribuo models, will throw similar errors like this for org.tribuo.clustering.kmeans.KMeansModel

=== Standard error of node `node{:opensearch-ml-plugin:integTest-0}` ===
»   ↓ last 40 non error or warning messages from /local/home/ylwu/code/os/ml-commons/plugin/build/testclusters/integTest-0/logs/opensearch.stderr.log ↓
» WARNING: An illegal reflective access operation has occurred
»  WARNING: Illegal reflective access by io.protostuff.runtime.EnumIO (file:/local/home/ylwu/code/os/ml-commons/plugin/build/testclusters/integTest-0/distro/2.0.0-ARCHIVE/plugins/opensearch-ml/protostuff-runtime-1.8.0.jar) to field java.util.EnumMap.keyType
»  WARNING: Please consider reporting this to the maintainers of io.protostuff.runtime.EnumIO
»  WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations
»  WARNING: All illegal access operations will be denied in a future release
»  fatal error in thread [opensearch[integTest-0][OPENSEARCH_ML_TASK_THREAD_POOL][T#1]], exiting
»  java.lang.ExceptionInInitializerError
»       at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
»       at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
»       at java.base/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
»       at java.base/java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:500)
»       at java.base/java.lang.reflect.Constructor.newInstance(Constructor.java:481)
»       at org.opensearch.ml.engine.MLEngineClassLoader.initInstance(MLEngineClassLoader.java:116)
»       at org.opensearch.ml.engine.MLEngineClassLoader.initInstance(MLEngineClassLoader.java:86)
»       at org.opensearch.ml.engine.MLEngine.train(MLEngine.java:24)
»       at org.opensearch.ml.task.MLTrainingTaskRunner.train(MLTrainingTaskRunner.java:196)
»       at org.opensearch.ml.task.MLTrainingTaskRunner.lambda$startTrainingTask$6(MLTrainingTaskRunner.java:166)
»       at org.opensearch.action.ActionListener$1.onResponse(ActionListener.java:78)
»       at org.opensearch.action.support.ThreadedActionListener$1.doRun(ThreadedActionListener.java:76)
»       at org.opensearch.common.util.concurrent.ThreadContext$ContextPreservingAbstractRunnable.doRun(ThreadContext.java:792)
»       at org.opensearch.common.util.concurrent.AbstractRunnable.run(AbstractRunnable.java:50)
»       at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1130)
»       at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:630)
»       at java.base/java.lang.Thread.run(Thread.java:832)
»  Caused by: java.lang.RuntimeException: Could not resolve constructor for class org.tribuo.clustering.kmeans.KMeansModel
»       at io.protostuff.runtime.RuntimeEnv.newInstantiator(RuntimeEnv.java:295)
»       at io.protostuff.runtime.RuntimeSchema.createFrom(RuntimeSchema.java:282)
»       at io.protostuff.runtime.RuntimeSchema.createFrom(RuntimeSchema.java:187)
»       at io.protostuff.runtime.IdStrategy.newSchema(IdStrategy.java:1625)
»       at io.protostuff.runtime.DefaultIdStrategy$Lazy.getSchema(DefaultIdStrategy.java:723)
»       at io.protostuff.runtime.RuntimeSchema.getSchema(RuntimeSchema.java:149)
»       at io.protostuff.runtime.RuntimeSchema.getSchema(RuntimeSchema.java:140)
»       at org.opensearch.ml.engine.algorithms.clustering.KMeans.lambda$static$0(KMeans.java:60)
»       at java.base/java.security.AccessController.doPrivileged(AccessController.java:312)
»       at org.opensearch.ml.engine.algorithms.clustering.KMeans.<clinit>(KMeans.java:59)
»       ... 17 more

@Craigacp
Copy link

Craigacp commented Mar 25, 2022

BTW we (the Tribuo developers) plan to add protobuf serialization support for all Tribuo types which currently implement java.io.Serializable in the 4.3 release (initial draft PR for the first chunk is here - oracle/tribuo#226). Would that improve compatibility across your serialization formats?

@ylwu-amzn
Copy link
Collaborator Author

ylwu-amzn commented Mar 25, 2022

BTW we (the Tribuo developers) plan to add protobuf serialization support for all Tribuo types which currently implement java.io.Serializable in the 4.3 release (initial draft PR for the first chunk is here - oracle/tribuo#226). Would that improve compatibility across your serialization formats?

Thansk @Craigacp , considering backward compatibility, can you add some test to make sure the model serialized by current object stream can be deserialized by protobuf in 4.3? Otherwise, we have to keep using current object stream to deserialize.

@Craigacp
Copy link

It won't be an automatic change. For all models in 4.3 there will be two serialization options, java.io.Serializable and protobuf, and they will use different methods to serialize & deserialize with different on disk formats. You will be able to freely convert between them by loading them into Tribuo and saving in the other format.

I guess we could look at adding a Model.load static method which consumes an InputStream and tries to figure out if it contains a java.io.Serializable object or a protobuf, but I don't know enough about the binary format of protobuf to know if it can emit 0xaced which is the Java object stream header. If it can then there's no way to reliably tell the difference without trying to parse it as both, and that sounds like a bad idea.

@ylwu-amzn
Copy link
Collaborator Author

Keep two serialization options sounds good. It will be good if tribuo can add Model.load(inputstream) / Model.load(bytes[]) methods and also Model.toBytes() / Model.writeTo(outputstream), that can save some effort for the client side.
Have no much experience on protobuf. If it's hard to tell from the binary format, I think it's also ok let client side tell whether it's Serializable object or a protobuf.

kaituo
kaituo previously approved these changes Mar 25, 2022
@ylwu-amzn ylwu-amzn dismissed stale reviews from kaituo and Zhangxunmt via e4e4e9e March 25, 2022 22:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AD&ml-commons fail to deserialize 1.3 model in latest RCF code
5 participants