add async trainModel #81
Conversation
| } | ||
| } | ||
|
|
||
| private void trainModelForIteration( |
There was a problem hiding this comment.
iteration means some repeated steps. Suggest to rename to step.
| Entry<Integer, Integer> partitionResults = getPartitionedForestSizes( | ||
| RandomCutForest | ||
| .builder() | ||
| .dimensions(rcfNumFeatures) | ||
| .sampleSize(rcfNumSamplesInTree) | ||
| .numberOfTrees(rcfNumTrees) | ||
| .outputAfter(rcfNumSamplesInTree) | ||
| .parallelExecutionEnabled(false) | ||
| .build(), | ||
| anomalyDetector.getDetectorId() | ||
| ); |
There was a problem hiding this comment.
I changed this in another PR: https://github.com/opendistro-for-elasticsearch/anomaly-detection/pull/83/files#diff-0ba3da6c04a6db2df8146de98b12d850
This is to have a single place to get the number of partitioned forests. Previously, we have redundant code in both ModelManager and ADStateManager.
If you agree, please use the changed getPartitionedForestSizes.
There was a problem hiding this comment.
the changes in that pr are currently unavailable in dev branch. if that is checked in first, this pr can be updated based on that. Or if this pr is checked in first, the refactoring can be done in a separate pr.
| * onFailure is called IllegalArgumentException when training data is invalid | ||
| * onFailure is called LimitExceededException when a limit for training is exceeded | ||
| */ | ||
| public void trainModel(AnomalyDetector anomalyDetector, double[][] dataPoints, ActionListener<Void> listener) { |
There was a problem hiding this comment.
Just transform the sync method to callback, not change any logic, right?
There was a problem hiding this comment.
yes, changed to async inside out
This change adds a new async trainModel implementation with the same business logic to replace the current synchronous implementation.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.