Adding delete/activate/deactivate support for model deployment and registered models #972
Conversation
kumar-shivam-ranjan
requested review from
VipulMascarenhas,
ahosler,
darenr,
mayoor,
mrDzurb and
qiuosier
as code owners
oracle-contributor-agreement
bot
added
the
OCA Verified
kumar-shivam-ranjan
changed the title
There was a problem hiding this comment.
added a few comments. In addition to those, can you add unit tests?
and run pre-commit hook to fix formatting issues?
If you haven't set it up, use:
pip install pre-commit or brew install pre-commit or conda install pre-commit
run pre-commit install
ruff might not let you commit **args and **kwargs methods issue that we've used generously, you can use --no-verify for that until we add a fix.
| ) | ||
| url_parse = urlparse(self.request.path) | ||
| paths = url_parse.path.strip("/") | ||
| if paths.startswith("aqua/deployments/activate"): |
There was a problem hiding this comment.
can this path be aqua/deployments/<model_deployment_id>/activate? Same comment about deactivate as well.
There was a problem hiding this comment.
Updated
ads/aqua/extension/model_handler.py
Outdated
| raise HTTPError(400, Errors.NO_INPUT_DATA) | ||
|
|
||
| inference_container=input_data.get('inference_container') | ||
| if inference_container is not None and inference_container not in [ |
There was a problem hiding this comment.
this validation is fine now since we have SMCs, but we'll have to remove once we use BYOC (we're working on a POC for embeddings). If we need to keep this, can we not hardcode the list of acceptable containers here, instead read from container_index?
There was a problem hiding this comment.
Let us remove this validation. If we decide to have this validation, let us wait till SMC listing API is available. Adding hardcoded list inside ADS code base can be limiting if we plan to add new SMC on the fly.
There was a problem hiding this comment.
Updated
ads/aqua/model/model.py
Outdated
| @telemetry(entry_point="plugin=model&action=delete", name="aqua") | ||
| def delete_registered_model(self,model_id): | ||
| ds_model=DataScienceModel.from_id(model_id) | ||
| is_registered_model=ds_model.freeform_tags.get(Tags.BASE_MODEL_CUSTOM,None) |
There was a problem hiding this comment.
do we want to extend this to fine-tuned model?
There was a problem hiding this comment.
Updated. This method now supports deletion of both registered and finetuned models
ads/aqua/model/model.py
Outdated
| """ | ||
| ds_model=DataScienceModel.from_id(id) | ||
| if ds_model.freeform_tags.get(Tags.BASE_MODEL_CUSTOM,None): |
There was a problem hiding this comment.
same thing as above: can we allow user to edit deployment container for FT model?
There was a problem hiding this comment.
Editing FT model is not supported as of now. only deletion of FT models as per @mayoor 's comment
| except Exception as ex: | ||
| raise AquaRuntimeError(f"The given model already doesn't support finetuning: {ex}") | ||
|
|
||
| custom_metadata_list.remove("modelDescription") |
There was a problem hiding this comment.
modelDescription is required for model by reference - can you comment here why this metadata is removed?
There was a problem hiding this comment.
modelDescription key had boolean value instead of string in custom metadata. editing model with modelDescription key in custom metadata required us to convert boolean to string which i though is additional not so useful step. Removing this key won't have any affect since as per MBR , this key is immutable and won't actually be removed or deleted.
| The inference container family name | ||
| enable_finetuning: str | ||
| Flag to enable or disable finetuning over the model. Defaults to None | ||
There was a problem hiding this comment.
add task to docstrings parameters
ads/aqua/model/model.py
Outdated
| custom_metadata_list=updated_custom_metadata_list, | ||
| freeform_tags=freeform_tags | ||
| ) | ||
| return self.ds_client.update_model(id,update_model_details).data |
There was a problem hiding this comment.
can we reuse update_model() from app.py?
There was a problem hiding this comment.
Updated
ads/aqua/model/model.py
Outdated
|
|
||
| custom_metadata_list.remove("modelDescription") | ||
| if task: | ||
| freeform_tags.update({"task":task}) |
There was a problem hiding this comment.
can we use Tags.TASK instead?
|
|
||
| @telemetry(entry_point="plugin=deployment&action=delete", name="aqua") | ||
| def delete(self,model_deployment_id:str): | ||
| return self.ds_client.delete_model_deployment(model_deployment_id=model_deployment_id).data |
There was a problem hiding this comment.
asking for my understanding - is this an async process or will this wait till MD is deleted? Same comment for activating and deactivating model.
For evaluation delete/cancel - we had to add additional wrapper to handle the wait time.
There was a problem hiding this comment.
This will be async only just like delete/cancel evals.
ads/aqua/extension/model_handler.py
Outdated
| if not input_data: | ||
| raise HTTPError(400, Errors.NO_INPUT_DATA) | ||
|
|
||
| inference_container=input_data.get('inference_container') |
There was a problem hiding this comment.
Please use pre-commit hook and ruff formatter.
There was a problem hiding this comment.
will update
4 participants
This PR intends to add delete/edit support for registered models along with delete/deactivate/activate support for model deployment
MD APIs
Delete registered model
Edit registered unverified model