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

revert: implement Serializable for ThresholdedRandomCutForestState #306

Closed
wants to merge 1 commit into from

Conversation

ylwu-amzn
Copy link
Contributor

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

AD is using protostuff to serialize/deserialize RCF model. With the change "implement Serializable for ThresholdedRandomCutForestState", AD can't deserialize old models. So we have to revert this change. At the same time, we will use the same way of AD to serialize/deserialize RCF in ml-commons.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@ylwu-amzn
Copy link
Contributor Author

ylwu-amzn commented Mar 25, 2022

Tested every commit after PR #282 (AD is using this commit in 1.3.0), after PR #284 , RCF code can't deserialize 1.3.0 AD RCF model any more. This is different with yesterday's test result that RCF commit 2fedfea can deserialize AD 1.3.0 model. Will confirm with @amitgalitz to learn why the test results are different.
But we still prefer to revert the PR #282 to not add any overhead about AD BWC. And we plan to use the same way of AD to serialize/deserialize RCF model. That will make our future BWC support easier. But cons is we have 2 serialization ways in ml-commons. Check PR opensearch-project/ml-commons#251

Check more details on this issue #305 (comment)

To test the impact of my PR #298 , I applied it to commit 3bb8fce (PR #283) which used by AD 1.3.0, check my branch https://github.com/ylwu-amzn/random-cut-forest-by-aws/tree/test_ad_protostuff, commit ylwu-amzn@bc7944e. Test locally, it can parse AD 1.3.0 model successfully.

@ylwu-amzn ylwu-amzn marked this pull request as draft March 28, 2022 22:48
@ylwu-amzn
Copy link
Contributor Author

As explained above, PR #298 doesn't impact the deserialization of old AD model. And Sudipto fixed the backward compatibility issue in PR #309. So seems no need to revert the PR #298. Will close this PR for now. If we find PR #298 impacts any part, will revert then.

@ylwu-amzn ylwu-amzn closed this Mar 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants