ODSC 68580- Update Evaluation SDK to Support Multi-Model Deployment #1085
Conversation
elizjo
requested review from
darenr,
mayoor,
mrDzurb,
VipulMascarenhas,
qiuosier and
ahosler
as code owners
oracle-contributor-agreement
bot
added
the
OCA Verified
|
📌 Cov diff with main: 📌 Overall coverage: |
ads/aqua/evaluation/evaluation.py
Outdated
| aqua_model, create_aqua_evaluation_details | ||
| ) | ||
|
|
||
| except (AquaRuntimeError, AquaValueError) as err: |
There was a problem hiding this comment.
I think we don't need to add AquaValueError into except section. Ot will work by itself.
There was a problem hiding this comment.
I see. I will remove except (AquaRuntimeError, AquaValueError) as err: and the try block. The evaluation_handler.py post method (that calls .create() method here) has the @handle_exceptions decorator, which has the except AquaError as error. I believe that the decorator will catch the exception, so this code is redundant here.
ads/aqua/evaluation/evaluation.py
Outdated
| @@ -550,6 +571,43 @@ def create( | |||
| parameters=AquaEvalParams(), | |||
| ) | |||
|
|
|||
| @staticmethod | |||
| def validate_name_multi_model( | |||
There was a problem hiding this comment.
Let name it - validate_model_name? Also please add description for this method.
There was a problem hiding this comment.
fixed the name to validate_model_name
ads/aqua/evaluation/evaluation.py
Outdated
| def validate_name_multi_model( | ||
| evaluation_source: DataScienceModel, | ||
| create_aqua_evaluation_details: CreateAquaEvaluationDetails, | ||
| ): |
ads/aqua/evaluation/evaluation.py
Outdated
| f"User did not input model name for multi model deployment evaluation with evaluation source ID: {create_aqua_evaluation_details.evaluation_source_id}" | ||
| ) | ||
| raise AquaValueError( | ||
| "Provide the model name. For evaluation, a single model needs to be targeted using the name in the multi model deployment." |
There was a problem hiding this comment.
I think we can show the valid names in this message.
Provide the model name. For evaluation, a single model needs to be targeted using the name in the multi model deployment. The valid model names for this Model Deployment are {valid_model_names}.
| @@ -449,7 +451,7 @@ def test_create_evaluation( | |||
| mock_from_id.return_value = foundation_model | |||
|
|
|||
| experiment = MagicMock() | |||
| experiment.id = "test_experiment_id" | |||
| experiment.id = "ocid1.datasciencemodelversionset.oc1.iad.amaaaaaav66vvniakngdzelb5hcgjd6yvfejksu2excidvvi3s5s5whtmdea" | |||
|
If someone specifies a model name (correctly) in a single model deployment case is this valid, or does that require the old/current "odsc_model" (or whatever it is) |
ads/aqua/evaluation/evaluation.py
Outdated
| custom_metadata_list = evaluation_source.custom_metadata_list | ||
| user_model_name = user_model_parameters.get("model") | ||
|
|
||
| model_group_count = int( |
There was a problem hiding this comment.
Wouldn't this fail if ModelCustomMetadataFields.MULTIMODEL_GROUP_COUNT is missing from the custom metadata?
ads/aqua/evaluation/evaluation.py
Outdated
| for idx in range(model_group_count) | ||
| ] | ||
|
|
||
| valid_model_names = ", ".join(map(str, model_names)) |
There was a problem hiding this comment.
Why do we need map() here? For the case if some name returns None?
There was a problem hiding this comment.
Yes- I changed it now to not include None values
|
@darenr we would only allow the user to enter the name if the Model Deployment selected is a multi model deployment (we use a freeform tag to ID these deployments). Otherwise, the user will not be able to enter the name and we would be keeping the odsc_llm (default name for MD). Hope this clarifies your question! |
The "name" parameter in the model parameters had to be updated from using the same default name for all deployments to the model's specific name.
This PR introduces a method to validate the user's input when they specify the model name for targeting a single model for evaluation when the model is within a multi model deployment.
Added method validate_name_multi_model():
Wrote test cases in test_deployment() in the test_validate_multi_model_evaluation() method.