Skip to content

Adding inference container URI for edit model handler #1030

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.

Sign up for GitHub

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

Merged
merged 12 commits into from
Jan 29, 2025
Merged

Adding inference container URI for edit model handler #1030

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
2295252
Adding inference container uri for edit model
kumar-shivam-ranjan Dec 19, 2024
3aedb48
Formatting
kumar-shivam-ranjan Dec 19, 2024
a95a678
Formatting
kumar-shivam-ranjan Dec 19, 2024
5b3a316
inference container uri validation check
kumar-shivam-ranjan Dec 19, 2024
16b7ace
formatting
kumar-shivam-ranjan Dec 19, 2024
c6f87ec
formatting
kumar-shivam-ranjan Dec 19, 2024
3a9fa5b
Addressing review comments
kumar-shivam-ranjan Jan 20, 2025
f196331
Formatting
kumar-shivam-ranjan Jan 20, 2025
8edce5c
Merge branch 'main' into ODSC-66457/update-edit-model-handler
kumar-shivam-ranjan Jan 20, 2025
e8d366f
Addressing review comments
kumar-shivam-ranjan Jan 20, 2025
557509e
Addressing review comments
kumar-shivam-ranjan Jan 20, 2025
1ba166a
Merge branch 'main' into ODSC-66457/update-edit-model-handler
kumar-shivam-ranjan Jan 29, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion ads/aqua/extension/model_handler.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
from tornado.web import HTTPError

from ads.aqua.common.decorator import handle_exceptions
from ads.aqua.common.enums import InferenceContainerTypeFamily
from ads.aqua.common.errors import AquaRuntimeError, AquaValueError
from ads.aqua.common.utils import (
get_hf_model_info,
Expand Down Expand Up @@ -163,10 +164,13 @@ def put(self, id):
raise HTTPError(400, Errors.NO_INPUT_DATA)

inference_container = input_data.get("inference_container")
inference_container_uri = input_data.get("inference_container_uri")
inference_containers = AquaModelApp.list_valid_inference_containers()
Copy link
Member

@mrDzurb mrDzurb Jan 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will it always return a list?

Copy link
Contributor Author

@kumar-shivam-ranjan kumar-shivam-ranjan Jan 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes , either an empty list or list of valid containers from container_index.json stored in OSS bucket

if (
inference_container is not None
and inference_container not in inference_containers
and inference_container
Copy link
Member

@VipulMascarenhas VipulMascarenhas Dec 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To avoid hardcoding, would it be better to move AQUA_TEI_CONTAINER_FAMILY from InferenceContainerTypeFamily to a new enum CustomInferenceContainerTypeFamily(str, metaclass=ExtendedEnumMeta)? That way the final inference_containers list can be inference_containers.append(CustomInferenceContainerTypeFamily.values()).

!= InferenceContainerTypeFamily.AQUA_TEI_CONTAINER_FAMILY
):
raise HTTPError(
400, Errors.INVALID_VALUE_OF_PARAMETER.format("inference_container")
Expand All @@ -176,7 +180,13 @@ def put(self, id):
task = input_data.get("task")
app = AquaModelApp()
self.finish(
app.edit_registered_model(id, inference_container, enable_finetuning, task)
app.edit_registered_model(
id,
inference_container,
inference_container_uri,
enable_finetuning,
task,
)
)
app.clear_model_details_cache(model_id=id)

Expand Down
47 changes: 39 additions & 8 deletions ads/aqua/model/model.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -377,7 +377,9 @@ def delete_model(self, model_id):
)

@telemetry(entry_point="plugin=model&action=delete", name="aqua")
Copy link
Member

@mrDzurb mrDzurb Jan 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why in telemetry we say delete?

Copy link
Contributor Author

@kumar-shivam-ranjan kumar-shivam-ranjan Jan 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated. Thanks

def edit_registered_model(self, id, inference_container, enable_finetuning, task):
def edit_registered_model(
self, id, inference_container, inference_container_uri, enable_finetuning, task
):
"""Edits the default config of unverified registered model.

Parameters
Expand All @@ -386,6 +388,8 @@ def edit_registered_model(self, id, inference_container, enable_finetuning, task
The model OCID.
inference_container: str.
The inference container family name
inference_container_uri: str
The inference container uri for embedding models
Copy link
Member

@mrDzurb mrDzurb Jan 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s it only for embedding models?

Copy link
Contributor Author

@kumar-shivam-ranjan kumar-shivam-ranjan Jan 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes as per now , we accept inference container URI for embedding models only

enable_finetuning: str
Flag to enable or disable finetuning over the model. Defaults to None
task:
Expand All @@ -407,13 +411,40 @@ def edit_registered_model(self, id, inference_container, enable_finetuning, task
custom_metadata_list = ds_model.custom_metadata_list
freeform_tags = ds_model.freeform_tags
if inference_container:
custom_metadata_list.add(
key=ModelCustomMetadataFields.DEPLOYMENT_CONTAINER,
value=inference_container,
category=MetadataCustomCategory.OTHER,
description="Deployment container mapping for SMC",
replace=True,
)
if (
inference_container
Copy link
Member

@VipulMascarenhas VipulMascarenhas Dec 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this can be replaced with inference_container in CustomInferenceContainerTypeFamily.values()

== InferenceContainerTypeFamily.AQUA_TEI_CONTAINER_FAMILY
and inference_container_uri is None
):
raise AquaRuntimeError(
f"Failed to edit model:{id}. Inference container URI must be provided."
)
else:
custom_metadata_list.add(
Copy link
Member

@mrDzurb mrDzurb Jan 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need to store this info in metadata? Can't it be extracted from MD?

Copy link
Contributor Author

@kumar-shivam-ranjan kumar-shivam-ranjan Jan 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This metadata contains deployment container. Via edit feature , user can edit/change deployment container as well. Hence , adding updated container to metadata.

Copy link
Member

@mrDzurb mrDzurb Jan 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However user can update deployment from console, and in this case the container stored on the model level will not match a real one?

Copy link
Contributor Author

@kumar-shivam-ranjan kumar-shivam-ranjan Jan 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes but source of truth here should be model right , not the model deployment.
User can create multiple model deployments from a single model. What we are trying to do here is changing/updating the model config.
What if user registered a model for a particular deployment container. Now , they due to some reason want to change the deployment container config of the model.

key=ModelCustomMetadataFields.DEPLOYMENT_CONTAINER,
value=inference_container,
category=MetadataCustomCategory.OTHER,
description="Deployment container mapping for SMC",
replace=True,
)
if inference_container_uri:
if (
inference_container
== InferenceContainerTypeFamily.AQUA_TEI_CONTAINER_FAMILY
or inference_container is None
):
custom_metadata_list.add(
key=ModelCustomMetadataFields.DEPLOYMENT_CONTAINER_URI,
value=inference_container_uri,
category=MetadataCustomCategory.OTHER,
description=f"Inference container URI for {ds_model.display_name}",
replace=True,
)
else:
raise AquaRuntimeError(
f"Failed to edit model:{id}. Inference container URI can be edited only with TEI container."
Copy link
Member

@VipulMascarenhas VipulMascarenhas Dec 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

f"Inference container URI can be edited only with container values: {CustomInferenceContainerTypeFamily.values()}".

)

if enable_finetuning is not None:
if enable_finetuning.lower() == "true":
custom_metadata_list.add(
Expand Down
Loading