[AQUA] Integrate aqua to use model group #1214
Conversation
…erated-data-science into integrate_model_group_aqua
…erated-data-science into integrate_model_group_aqua
oracle-contributor-agreement
bot
added
the
OCA Verified
mrDzurb
changed the title
lu-ohai
requested review from
darenr,
mayoor,
mrDzurb,
VipulMascarenhas,
qiuosier and
ahosler
as code owners
ads/aqua/model/model.py
Outdated
| .with_compartment_id(compartment_id) | ||
| .with_project_id(project_id) | ||
| .with_display_name(model_group_display_name) | ||
| .with_description(model_group_description) | ||
| .with_freeform_tags(**tags) | ||
| .with_defined_tags(**(defined_tags or {})) | ||
| .with_custom_metadata_list(model_custom_metadata) | ||
| .with_member_models( | ||
| [MemberModel(model_id=model.model_id).model_dump() for model in models] |
There was a problem hiding this comment.
where is the MemberModel(inference_key = ) assigned? Is the inference key optional? (would this be the model name?)
There was a problem hiding this comment.
We should default to using the model name if user doesn't provide one explicitly. Even though it will not be used in multi-model case, it's still a good practice to include name.
There was a problem hiding this comment.
The inference_key is supposed to be the model name, but the model group has a length restiction for it, making it impossible to set for all models. Leave it empty here for now and will address it back.
ads/aqua/model/model.py
Outdated
|
|
||
| container_params = container_spec.cli_param if container_spec else UNKNOWN | ||
|
|
||
| from ads.aqua.modeldeployment.model_group_config import ModelGroupConfig |
There was a problem hiding this comment.
This has been refactored.
ads/aqua/model/model.py
Outdated
| .with_compartment_id(compartment_id) | ||
| .with_project_id(project_id) | ||
| .with_display_name(model_group_display_name) | ||
| .with_description(model_group_description) | ||
| .with_freeform_tags(**tags) | ||
| .with_defined_tags(**(defined_tags or {})) | ||
| .with_custom_metadata_list(model_custom_metadata) | ||
| .with_member_models( | ||
| [MemberModel(model_id=model.model_id).model_dump() for model in models] |
There was a problem hiding this comment.
We should default to using the model name if user doesn't provide one explicitly. Even though it will not be used in multi-model case, it's still a good practice to include name.
ads/aqua/model/model.py
Outdated
| @@ -235,20 +237,27 @@ def create( | |||
| def create_multi( | |||
| self, | |||
| models: List[AquaMultiModelRef], | |||
| create_deployment_details, | |||
| **create_model_deployment_details | ||
| ).to_oci_model(CreateModelDeploymentDetails) | ||
| return CreateModelDeploymentDetails( | ||
| **ads_utils.batch_convert_case(create_model_deployment_details, "snake") |
There was a problem hiding this comment.
Is it safe to use the batch converter? Why can't we use the old approach?
return OCIDataScienceModelDeployment(
**create_model_deployment_details
).to_oci_model(CreateModelDeploymentDetails)
There was a problem hiding this comment.
Updated to original approach
…erated-data-science into integrate_model_group_aqua
Integrate aqua to use model group
ModelDeploymentmodule with model group.AquaDeploymentAppandAquaModelAppmodules with model group.-- Changed to create model group instead of model while deploying multi models
-- Moved env var
MULTI_MODEL_CONFIGfrom deployment env to model group custom metadata-- Stored
multi_model_metadataas regular custom metadataUnit test
tests/unitary/with_extras/aqua/test_deployment.pytests/unitary/with_extras/aqua/test_model.pytests/unitary/default_setup/model_deployment/test_model_deployment_v2.pytests/unitary/default_setup/model/test_model_group.pytests/unitary/default_setup/model/test_oci_model_group.pyNotebook