-
Notifications
You must be signed in to change notification settings - Fork 50
[AQUA] Added support for deploy stack model. #1223
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: feature/model_group
Are you sure you want to change the base?
Conversation
""" | ||
fine_tune_weights = ( |
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.
looks like there are several place where we do same validation. Wouldn't it be better to have something like:
if isinstance(model_id, AquaMultiModelRef):
....
else:
....
f"Aqua Model {custom_model.id} created with the service model {model_id}." | ||
) | ||
custom_model = None | ||
if fine_tune_weights: |
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.
Wouldn’t it be better, instead of handling everything in one place, to split the implementation into two separate methods, one for creating the MC record and another for creating the group?
if create_deployment_details.model_id: | ||
if ( | ||
create_deployment_details.model_id | ||
or create_deployment_details.deployment_type == DEFAULT_DEPLOYMENT_TYPE |
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 think this is a bit dangerous, what if someone change the default deployment type?
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 can add a validation for it and only MODEL_STACK
is supported at this moment.
@@ -206,13 +214,28 @@ def create( | |||
|
|||
# Create an AquaModelApp instance once to perform the deployment creation. | |||
model_app = AquaModelApp() | |||
if create_deployment_details.model_id: | |||
if ( | |||
create_deployment_details.model_id |
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 think this assumption is a bit wrong, no?
create_deployment_details.model_id
can be provided only for a single model deployment. In case of MMD or Stacked deployment we should rely on create_deployment_details.models
, no?
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 think we talked about this before, that is for single dployment, we adopt model_id
and for stacked deployment, we adopt models
with deployment_type
. This assumption will filter out both single and stack deployment cases.
) | ||
|
||
build_model_group_details = copy.deepcopy(self._spec) | ||
build_model_group_details.pop(self.CONST_CUSTOM_METADATA_LIST) | ||
build_model_group_details.pop(self.CONST_MEMBER_MODELS) | ||
build_model_group_details.pop(self.CONST_CUSTOM_METADATA_LIST, None) |
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.
Add a comment here, why we are doing this?
Added support for deploy stack model.
MODEL_STACK
as single model deployment using model groupmodels
anddeployment_type=MODEL_STACK
tocreate
at the same time will trigger this type of deploymentMODEL
will be added and the--served-model-name
inPARAMS
will be updated to the model id.Integration tests
Unit tests