Enables vpa demo to pick up custom term and model #1494
Conversation
bhanvimenghani
force-pushed
the
vpa_demo
branch
from
1ab29a0 to
52984e2
Compare
src/main/java/com/autotune/analyzer/autoscaler/vpa/VpaAutoscalerImpl.java
Outdated
Show resolved
Hide resolved
src/main/java/com/autotune/analyzer/experiment/ExperimentValidation.java
Outdated
Show resolved
Hide resolved
src/main/java/com/autotune/analyzer/experiment/ExperimentValidation.java
Outdated
Show resolved
Hide resolved
src/main/java/com/autotune/analyzer/recommendations/engine/RecommendationEngine.java
Outdated
Show resolved
Hide resolved
design/KruizeLocalAPI.md
Outdated
| for a single model and a single term. Model can be set to either cost or performance and | ||
| term can be set to short, medium or long term. | ||
|
|
||
| If mode is set to auto or recreate and the above settings are not mentioned then it will |
There was a problem hiding this comment.
@bhanvimenghani Can you please add - If mode is one of auto / recreate, then there can only be one term and one model chosen.
| // common check for terms and models | ||
| if (expObj.getRecommendation_settings().getTermSettings() != null && | ||
| expObj.getRecommendation_settings().getTermSettings().getTerms() != null ) { | ||
| Set<String> validTerms = Set.of("short", "medium", "long"); |
There was a problem hiding this comment.
Please use constants defined AnalyzerConstants.RecommendationSettings.LONG_TERM
|
|
||
| if (expObj.getRecommendation_settings().getModelSettings() != null && | ||
| expObj.getRecommendation_settings().getModelSettings().getModels() != null) { | ||
| Set<String> validModels = Set.of("cost", "performance"); |
There was a problem hiding this comment.
Similarly, please use constants.
| public static void setDefaultTerms(Map<String, Terms> terms, KruizeObject kruizeObject) { | ||
| // TODO: define term names like daily, weekly, fortnightly etc | ||
| // TODO: add CRD for terms | ||
| // for monitoring use case | ||
| terms.put(KruizeConstants.JSONKeys.SHORT_TERM, new Terms(KruizeConstants.JSONKeys.SHORT_TERM, |
There was a problem hiding this comment.
Instead of setting hard coded values, we need to calculate threshold value according to measurement_duration.
terms.put(KruizeConstants.JSONKeys.SHORT_TERM, new Terms(KruizeConstants.JSONKeys.SHORT_TERM,
KruizeConstants.RecommendationEngineConstants.DurationBasedEngine.DurationAmount.SHORT_TERM_DURATION_DAYS,
getTermThresholdInDays(KruizeConstants.JSONKeys.SHORT_TERM, kruizeObject.getTrial_settings().getMeasurement_durationMinutes_inDouble()),
4, 0.25));For getTermThresholdInDays function details, please check here - https://github.com/kruize/autotune/pull/1466/files#diff-5c812482acaccb6f20150f52c889033372cdd32312bb75c76e32e25a3ac2b3baR366-R367
|
|
||
| for (String userInputTerm : termList) { | ||
| if (AnalyzerConstants.RecommendationSettings.SHORT.equalsIgnoreCase(userInputTerm)) { | ||
| terms.put(KruizeConstants.JSONKeys.SHORT_TERM, new Terms(KruizeConstants.JSONKeys.SHORT_TERM, |
There was a problem hiding this comment.
Similarly, for custom terms also instead of setting hard coded values, we need to calculate threshold value according to measurement_duration.
| // Add new models | ||
| recommendationModels = new ArrayList<>(); | ||
| for (String model : modelName) { | ||
| if ("cost".equalsIgnoreCase(model)) { |
There was a problem hiding this comment.
Please replace these values with constants.
Description
This pr makes changes in the vpa updater to enable it to pick up term and model set by the user, removing the hard coding done earlier for short and performance.
This pr is on top of #1457 pr and needs that to be merged before merging this one
Fixes # (issue)
Type of change
How has this been tested?
Please describe the tests that were run to verify your changes and steps to reproduce. Please specify any test configuration required.
Test Configuration
Checklist 🎯
Additional information
Include any additional information such as links, test results, screenshots here