-
Notifications
You must be signed in to change notification settings - Fork 47
[AQUA] Edit single model deployment #1162
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
base: main
Are you sure you want to change the base?
Conversation
…ence into ODSC-68322/edit_single_model_deployment
ads/aqua/modeldeployment/entities.py
Outdated
@@ -391,15 +406,9 @@ class Config: | |||
extra = "allow" | |||
|
|||
|
|||
class CreateModelDeploymentDetails(BaseModel): | |||
"""Class for creating Aqua model deployments.""" | |||
class ModelDeploymentBasicDetails(BaseModel): |
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.
NIT: maybe ModelDeploymentSpec
?
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.
Updated
ads/aqua/modeldeployment/entities.py
Outdated
@@ -470,6 +479,20 @@ class CreateModelDeploymentDetails(BaseModel): | |||
None, description="Defined tags for model deployment." | |||
) | |||
|
|||
class Config: | |||
extra = "ignore" |
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.
let's allow the extra fields, just in case
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.
Updated
ads/aqua/modeldeployment/entities.py
Outdated
extra = "ignore" | ||
|
||
|
||
class CreateModelDeploymentDetails(ModelDeploymentBasicDetails): |
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 also rename this to ModelDeploymentCreateSpec
, however this might bring many changes.
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.
Updated
cmd_var=cmd_var, | ||
) | ||
|
||
def _get_container_details( |
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.
Maybe we can think about more suitable name for this method?
_extract_model_deployment_runtime_config
_get_runtime_config_from_model_deployment
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.
Updated
@@ -329,6 +336,58 @@ def _create( | |||
AquaDeployment | |||
An Aqua deployment instance. | |||
""" | |||
( |
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'm not a big fan of returning the result in this format. I would suggest using a Pydantic class to represent the model deployment runtime configuration. Eventually, we might have two classes: one for single model deployments and another for multi-model deployments.
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 can modify this together with updating multi model deployment so that I can have a better picture of this implementation.
update_deployment_details.deployment_id | ||
) | ||
|
||
if aqua_deployment.lifecycle_state != State.ACTIVE.name: |
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.
We should verify whether a Model Deployment can be updated while in a failed state. For instance, if a user provides an incorrect configuration, resulting in a failed deployment, can they subsequently correct the configuration and update the MD? From a service perspective, is this scenario supported? We should investigate this use case to ensure proper handling.
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.
The service end takes care of this, that is, if a model deployment fails to be updated due to incorrect configurations passed in, the model deployment will remain the same as ACTIVE
. We can keep track of this failure from the workrequest, but I'm not quite sure if we should block the process while waiting for the update workrequest to be done.
Edit single model deployment
update
inAquaDeploymentApp
to support editing model deployment.AquaDeploymentDetail
for getting model deploymentUnit test
Notebook