Add TTL to un-deploy model automatically #2365
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,6 +7,8 @@ | |
|
|
||
| import static org.opensearch.ml.settings.MLCommonsSettings.ML_COMMONS_MONITORING_REQUEST_COUNT; | ||
|
|
||
| import java.time.Duration; | ||
| import java.time.Instant; | ||
| import java.util.HashSet; | ||
| import java.util.List; | ||
| import java.util.Map; | ||
|
|
@@ -68,6 +70,7 @@ public synchronized void initModelState( | |
| modelCache.setFunctionName(functionName); | ||
| modelCache.setTargetWorkerNodes(targetWorkerNodes); | ||
| modelCache.setDeployToAllNodes(deployToAllNodes); | ||
| modelCache.setLastAccessTime(Instant.now()); | ||
| modelCaches.put(modelId, modelCache); | ||
| } | ||
|
|
||
|
|
@@ -87,6 +90,7 @@ public synchronized void initModelStateLocal( | |
| modelCache.setFunctionName(functionName); | ||
| modelCache.setTargetWorkerNodes(targetWorkerNodes); | ||
| modelCache.setDeployToAllNodes(false); | ||
| modelCache.setLastAccessTime(Instant.now()); | ||
| modelCaches.put(modelId, modelCache); | ||
| } | ||
|
|
||
|
|
@@ -341,6 +345,29 @@ public String[] getLocalDeployedModels() { | |
| .toArray(new String[0]); | ||
| } | ||
|
|
||
| /** | ||
| * Get expired models on node. | ||
| * | ||
| * @return array of expired model id | ||
| */ | ||
| public String[] getExpiredModels() { | ||
| return modelCaches.entrySet().stream().filter(entry -> { | ||
| MLModel mlModel = entry.getValue().getCachedModelInfo(); | ||
|
There was a problem hiding this comment. Cached model info only set when predict request comes. Check https://github.com/opensearch-project/ml-commons/pull/1472/files For a deployed model, if no predict request comes, the cached model info is null. Then the There was a problem hiding this comment. Add this line |
||
| if (mlModel.getDeploySetting() == null) { | ||
| return false; // no TTL, never expire | ||
| } | ||
| Duration liveDuration = Duration.between(entry.getValue().getLastAccessTime(), Instant.now()); | ||
| Long ttlInMinutes = mlModel.getDeploySetting().getModelTTLInMinutes(); | ||
| if (ttlInMinutes < 0) { | ||
| return false; | ||
| } | ||
| Duration ttl = Duration.ofMinutes(ttlInMinutes); | ||
| boolean isModelExpired = liveDuration.getSeconds() >= ttl.getSeconds(); | ||
| return isModelExpired | ||
| && (mlModel.getModelState() == MLModelState.DEPLOYED || mlModel.getModelState() == MLModelState.PARTIALLY_DEPLOYED); | ||
|
There was a problem hiding this comment. Why do we care the model status for model TTL ? For example, if we have some bug, and model stays on DEPLOY_FAILED, I think TTL could be a good way to remove model to avoid memory leak. There was a problem hiding this comment. The Deploy_Failed may indicate some insights to the cluster or ml service. Auto undeploying the model won't fix the problem but only hide it. We shouldn't use this as a tool to clear the failure status? Otherwise, the model will likely be in Failed status when it's deployed again. Let's be conservative in the first release for this mechanism There was a problem hiding this comment. We have auto deploy, if model deploy stuck there, why shouldn't we use TTL to undeploy model to remove the unnecessary memory usage? There was a problem hiding this comment. That's doable. But is it hiding problems that may bite us more in the future? When the deploy stuck, why shouldn't we look into the reasons first and then un-deploy if it's the real solution? Also the auto deploy is only for remote model which takes little memory. So there might be other different reasons for the deployment failure like the one I showed in the meeting? There was a problem hiding this comment. Make sense, you can add a todo and test more |
||
| }).map(entry -> entry.getKey()).collect(Collectors.toList()).toArray(new String[0]); | ||
| } | ||
|
|
||
| /** | ||
| * Check if model is running on node. | ||
| * | ||
|
|
@@ -403,6 +430,16 @@ public void setTargetWorkerNodes(String modelId, List<String> targetWorkerNodes) | |
| } | ||
| } | ||
|
|
||
| /** | ||
| * Set the last access time to Instant.now() | ||
| * | ||
| * @param modelId model id | ||
| */ | ||
| public void refreshLastAccessTime(String modelId) { | ||
| MLModelCache modelCache = modelCaches.get(modelId); | ||
| modelCache.setLastAccessTime(Instant.now()); | ||
| } | ||
|
|
||
| /** | ||
| * Remove model. | ||
| * | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why separate hours and minutes? Seems use minutes should be enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initially only used Hours. But it may take too much time to test expiration in hours. Having both hours and minutes cover most cases. Most likely only hours will be used, e.g. 24 hours, 72 hours (6 days), etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just use minutes ? 24 hours will be 24 * 60,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be easier to set a longer time in hours? For 1 week of 7 days, it would be 10080 minutes, but this is not an obvious number to understand the time length.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't require both hours and minutes, only 1 of them will create a valid deploySetting. If both of them are null, default values are -1 and this model will never expire.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest just use the minimal time unit for this, that could be easier. If you add hours, why not add days and months?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.