run auto deploy remote model in partially deployed status #3423
Conversation
Signed-off-by: Xun Zhang <[email protected]>
Zhangxunmt
requested review from
b4sjoo,
dhrubo-os,
mingshl,
jngz-es,
model-collapse,
rbhavna,
ylwu-amzn,
zane-neo,
austintlee,
HenryL27 and
xinyual
as code owners
Zhangxunmt
had a problem deploying
to
ml-commons-cicd-env
GitHub Actions
Failure
Zhangxunmt
had a problem deploying
to
ml-commons-cicd-env
GitHub Actions
Failure
| @@ -2460,6 +2460,10 @@ public int getWorkerNodesSize(String modelId, FunctionName functionName) { | |||
| return getWorkerNodes(modelId, functionName, false).length; | |||
| } | |||
|
|
|||
| public String[] getTargetWorkerNodes(String modelId) { | |||
| return modelCacheHelper.getTargetWorkerNodes(modelId); | |||
There was a problem hiding this comment.
what do you think returning empty string array here instead of null? since null may cause NPE
| if (modelCache == null) { | ||
| return null; | ||
| } | ||
| return modelCache.getTargetWorkerNodes(); |
There was a problem hiding this comment.
We should consider deploy to all nodes case.
If deploy to all nodes is try and target work nodes may be [node1, node2] on day1.
Then day2, user add one more nodes to cluster, now the target work nodes should be [node1, node2, node3], so we can deploy to all nodes.
I think the work node from cache will still be [node1, node2], right ? Can't remember the details, maybe it returns null or empty for deploy to all node case ? Can you confirm ?
There was a problem hiding this comment.
From what I see the synup job only syncs up the worker nodes, but not the target worker nodes. This is another enhancement needed. Both the target worker nodes and running worker nodes needs to be up-to-date in the memory so it can cover all kinds of cases.
| @@ -2460,6 +2460,10 @@ public int getWorkerNodesSize(String modelId, FunctionName functionName) { | |||
| return getWorkerNodes(modelId, functionName, false).length; | |||
| } | |||
|
|
|||
| public String[] getTargetWorkerNodes(String modelId) { | |||
There was a problem hiding this comment.
Can we add java doc to the public method?
|
Can we add tests? |
Description
Currently the remote model auto-deploy only happens when the model is not deployed at all, by checking the running worker nodes == 0. But in some edge cases, we'd like to auto-deploy the model even it's in PARTIALLY_DEPLOYED status.
Related Issues
Resolves #[Issue number to be closed when this PR is merged]
Check List
--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.