bug fix for override params issue and ruff updates

Author: VipulMascarenhas

ads/aqua/common/utils.py

@@ -1,8 +1,8 @@
 #!/usr/bin/env python
-# -*- coding: utf-8 -*-
 # Copyright (c) 2024 Oracle and/or its affiliates.
 # Licensed under the Universal Permissive License v 1.0 as shown at https://oss.oracle.com/licenses/upl/
 """AQUA utils and constants."""
+
 import asyncio
 import base64
 import json
@@ -19,13 +19,28 @@
 import oci
 from oci.data_science.models import JobRun, Model
 
-from ads.aqua.common.enums import RqsAdditionalDetails
+from ads.aqua.common.enums import (
+    InferenceContainerParamType,
+    InferenceContainerType,
+    RqsAdditionalDetails,
+)
 from ads.aqua.common.errors import (
     AquaFileNotFoundError,
     AquaRuntimeError,
     AquaValueError,
 )
-from ads.aqua.constants import *
+from ads.aqua.constants import (
+    AQUA_GA_LIST,
+    COMPARTMENT_MAPPING_KEY,
+    CONSOLE_LINK_RESOURCE_TYPE_MAPPING,
+    CONTAINER_INDEX,
+    MAXIMUM_ALLOWED_DATASET_IN_BYTE,
+    MODEL_BY_REFERENCE_OSS_PATH_KEY,
+    SERVICE_MANAGED_CONTAINER_URI_SCHEME,
+    SUPPORTED_FILE_FORMATS,
+    UNKNOWN,
+    UNKNOWN_JSON_STR,
+)
 from ads.aqua.data import AquaResourceIdentifier
 from ads.common.auth import default_signer
 from ads.common.decorator.threaded import threaded
@@ -74,15 +89,15 @@ def get_status(evaluation_status: str, job_run_status: str = None):
 
         status = LifecycleStatus.UNKNOWN
         if evaluation_status == Model.LIFECYCLE_STATE_ACTIVE:
-            if (
-                job_run_status == JobRun.LIFECYCLE_STATE_IN_PROGRESS
-                or job_run_status == JobRun.LIFECYCLE_STATE_ACCEPTED
-            ):
+            if job_run_status in {
+                JobRun.LIFECYCLE_STATE_IN_PROGRESS,
+                JobRun.LIFECYCLE_STATE_ACCEPTED,
+            }:
                 status = JobRun.LIFECYCLE_STATE_IN_PROGRESS
-            elif (
-                job_run_status == JobRun.LIFECYCLE_STATE_FAILED
-                or job_run_status == JobRun.LIFECYCLE_STATE_NEEDS_ATTENTION
-            ):
+            elif job_run_status in {
+                JobRun.LIFECYCLE_STATE_FAILED,
+                JobRun.LIFECYCLE_STATE_NEEDS_ATTENTION,
+            }:
                 status = JobRun.LIFECYCLE_STATE_FAILED
             else:
                 status = job_run_status
@@ -199,10 +214,7 @@ def read_file(file_path: str, **kwargs) -> str:
 @threaded()
 def load_config(file_path: str, config_file_name: str, **kwargs) -> dict:
     artifact_path = f"{file_path.rstrip('/')}/{config_file_name}"
-    if artifact_path.startswith("oci://"):
-        signer = default_signer()
-    else:
-        signer = {}
+    signer = default_signer() if artifact_path.startswith("oci://") else {}
     config = json.loads(
         read_file(file_path=artifact_path, auth=signer, **kwargs) or UNKNOWN_JSON_STR
     )
@@ -448,7 +460,7 @@ def _build_resource_identifier(
 
 
 def _get_experiment_info(
-    model: Union[oci.resource_search.models.ResourceSummary, DataScienceModel]
+    model: Union[oci.resource_search.models.ResourceSummary, DataScienceModel],
 ) -> tuple:
     """Returns ocid and name of the experiment."""
     return (
@@ -609,7 +621,7 @@ def extract_id_and_name_from_tag(tag: str):
     base_model_name = UNKNOWN
     try:
         base_model_ocid, base_model_name = tag.split("#")
-    except:
+    except Exception:
         pass
 
     if not (is_valid_ocid(base_model_ocid) and base_model_name):
@@ -646,7 +658,7 @@ def get_resource_name(ocid: str) -> str:
     try:
         resource = query_resource(ocid, return_all=False)
         name = resource.display_name if resource else UNKNOWN
-    except:
+    except Exception:
         name = UNKNOWN
     return name
 
@@ -670,8 +682,8 @@ def get_model_by_reference_paths(model_file_description: dict):
 
     if not models:
         raise AquaValueError(
-            f"Model path is not available in the model json artifact. "
-            f"Please check if the model created by reference has the correct artifact."
+            "Model path is not available in the model json artifact. "
+            "Please check if the model created by reference has the correct artifact."
         )
 
     if len(models) > 0:
@@ -848,3 +860,24 @@ def copy_model_config(artifact_path: str, os_path: str, auth: dict = None):
     except Exception as ex:
         logger.debug(ex)
         logger.debug(f"Failed to copy config folder from {artifact_path} to {os_path}.")
+
+
+def get_container_params_type(container_type_name: str):
+    """The utility function accepts the deployment container type name and returns the corresponding params name.
+    Parameters
+    ----------
+    container_type_name: str
+        type of deployment container, like odsc-vllm-serving or odsc-tgi-serving.
+
+    Returns
+    -------
+        InferenceContainerParamType value
+
+    """
+    # check substring instead of direct match in case container_type_name changes in the future
+    if InferenceContainerType.CONTAINER_TYPE_VLLM in container_type_name:
+        return InferenceContainerParamType.PARAM_TYPE_VLLM
+    elif InferenceContainerType.CONTAINER_TYPE_TGI in container_type_name:
+        return InferenceContainerParamType.PARAM_TYPE_TGI
+    else:
+        return UNKNOWN

ads/aqua/modeldeployment/deployment.py

@@ -1,28 +1,24 @@
 #!/usr/bin/env python
-# -*- coding: utf-8 -*-
 # Copyright (c) 2024 Oracle and/or its affiliates.
 # Licensed under the Universal Permissive License v 1.0 as shown at https://oss.oracle.com/licenses/upl/
 
-import json
 import logging
 from typing import Dict, List, Union
 
-from oci.data_science.models import ModelDeployment
-
 from ads.aqua.app import AquaApp, logger
 from ads.aqua.common.enums import (
-    Tags,
-    InferenceContainerParamType,
     InferenceContainerType,
     InferenceContainerTypeFamily,
+    Tags,
 )
 from ads.aqua.common.errors import AquaRuntimeError, AquaValueError
 from ads.aqua.common.utils import (
+    get_combined_params,
     get_container_config,
     get_container_image,
+    get_container_params_type,
     get_model_by_reference_paths,
     get_ocid_substring,
-    get_combined_params,
     get_params_dict,
     get_params_list,
     get_resource_name,
@@ -38,15 +34,15 @@
 from ads.aqua.data import AquaResourceIdentifier
 from ads.aqua.finetuning.finetuning import FineTuneCustomMetadata
 from ads.aqua.model import AquaModelApp
+from ads.aqua.modeldeployment.constants import (
+    TGIInferenceRestrictedParams,
+    VLLMInferenceRestrictedParams,
+)
 from ads.aqua.modeldeployment.entities import (
     AquaDeployment,
     AquaDeploymentDetail,
     ContainerSpec,
 )
-from ads.aqua.modeldeployment.constants import (
-    VLLMInferenceRestrictedParams,
-    TGIInferenceRestrictedParams,
-)
 from ads.common.object_storage_details import ObjectStorageDetails
 from ads.common.utils import get_log_links
 from ads.config import (
@@ -187,24 +183,24 @@ def create(
                 model_name = aqua_model.custom_metadata_list.get(
                     FineTuneCustomMetadata.FINE_TUNE_SOURCE_NAME
                 ).value
-            except:
+            except ValueError as err:
                 raise AquaValueError(
                     f"Either {FineTuneCustomMetadata.FINE_TUNE_SOURCE} or {FineTuneCustomMetadata.FINE_TUNE_SOURCE_NAME} is missing "
                     f"from custom metadata for the model {config_source_id}"
-                )
+                ) from err
 
         # set up env vars
         if not env_var:
-            env_var = dict()
+            env_var = {}
 
         try:
             model_path_prefix = aqua_model.custom_metadata_list.get(
                 MODEL_BY_REFERENCE_OSS_PATH_KEY
             ).value.rstrip("/")
-        except ValueError:
+        except ValueError as err:
             raise AquaValueError(
                 f"{MODEL_BY_REFERENCE_OSS_PATH_KEY} key is not available in the custom metadata field."
-            )
+            ) from err
 
         if ObjectStorageDetails.is_oci_path(model_path_prefix):
             os_path = ObjectStorageDetails.from_path(model_path_prefix)
@@ -219,7 +215,7 @@ def create(
 
             if not fine_tune_output_path:
                 raise AquaValueError(
-                    f"Fine tuned output path is not available in the model artifact."
+                    "Fine tuned output path is not available in the model artifact."
                 )
 
             os_path = ObjectStorageDetails.from_path(fine_tune_output_path)
@@ -232,7 +228,7 @@ def create(
             container_type_key = aqua_model.custom_metadata_list.get(
                 AQUA_DEPLOYMENT_CONTAINER_METADATA_NAME
             ).value
-        except ValueError:
+        except ValueError as err:
             message = (
                 f"{AQUA_DEPLOYMENT_CONTAINER_METADATA_NAME} key is not available in the custom metadata field "
                 f"for model {aqua_model.id}."
@@ -242,7 +238,7 @@ def create(
                 raise AquaValueError(
                     f"{message}. For unverified Aqua models, container_family parameter should be "
                     f"set and value can be one of {', '.join(InferenceContainerTypeFamily.values())}."
-                )
+                ) from err
             container_type_key = container_family
         try:
             # Check if the container override flag is set. If set, then the user has chosen custom image
@@ -282,11 +278,12 @@ def create(
         )  # Give precendece to the input parameter
 
         deployment_config = self.get_deployment_config(config_source_id)
-        vllm_params = (
+
+        config_params = (
             deployment_config.get("configuration", UNKNOWN_DICT)
             .get(instance_shape, UNKNOWN_DICT)
             .get("parameters", UNKNOWN_DICT)
-            .get(InferenceContainerParamType.PARAM_TYPE_VLLM, UNKNOWN)
+            .get(get_container_params_type(container_type_key), UNKNOWN)
         )
 
         # validate user provided params
@@ -301,7 +298,7 @@ def create(
                     f"and cannot be overridden or are invalid."
                 )
 
-        deployment_params = get_combined_params(vllm_params, user_params)
+        deployment_params = get_combined_params(config_params, user_params)
 
         if deployment_params:
             params = f"{params} {deployment_params}"
@@ -429,7 +426,7 @@ def list(self, **kwargs) -> List["AquaDeployment"]:
                     # tracks unique deployments that were listed in the user compartment
                     # we arbitrarily choose last 8 characters of OCID to identify MD in telemetry
                     self.telemetry.record_event_async(
-                        category=f"aqua/deployment",
+                        category="aqua/deployment",
                         action="list",
                         detail=get_ocid_substring(deployment_id, key_len=8),
                         value=state,
@@ -574,25 +571,12 @@ def get_deployment_default_params(
             container_type_key = container_type_key.lower()
             if container_type_key in InferenceContainerTypeFamily.values():
                 deployment_config = self.get_deployment_config(model_id)
-                config_parameters = (
+                params = (
                     deployment_config.get("configuration", UNKNOWN_DICT)
                     .get(instance_shape, UNKNOWN_DICT)
                     .get("parameters", UNKNOWN_DICT)
+                    .get(get_container_params_type(container_type_key))
                 )
-                if InferenceContainerType.CONTAINER_TYPE_VLLM in container_type_key:
-                    params = config_parameters.get(
-                        InferenceContainerParamType.PARAM_TYPE_VLLM, UNKNOWN
-                    )
-                elif InferenceContainerType.CONTAINER_TYPE_TGI in container_type_key:
-                    params = config_parameters.get(
-                        InferenceContainerParamType.PARAM_TYPE_TGI, UNKNOWN
-                    )
-                else:
-                    params = UNKNOWN
-                    logger.debug(
-                        f"Default inference parameters are not available for the model {model_id} and "
-                        f"instance {instance_shape}."
-                    )
                 if params:
                     # account for param that can have --arg but no values, e.g. --trust-remote-code
                     default_params.extend(get_params_list(params))
@@ -629,7 +613,7 @@ def validate_deployment_params(
                 container_type_key = model.custom_metadata_list.get(
                     AQUA_DEPLOYMENT_CONTAINER_METADATA_NAME
                 ).value
-            except ValueError:
+            except ValueError as err:
                 message = (
                     f"{AQUA_DEPLOYMENT_CONTAINER_METADATA_NAME} key is not available in the custom metadata field "
                     f"for model {model_id}."
@@ -640,7 +624,7 @@ def validate_deployment_params(
                     raise AquaValueError(
                         f"{message}. For unverified Aqua models, container_family parameter should be "
                         f"set and value can be one of {', '.join(InferenceContainerTypeFamily.values())}."
-                    )
+                    ) from err
                 container_type_key = container_family
 
             container_config = get_container_config()
@@ -658,7 +642,7 @@ def validate_deployment_params(
                 f"Parameters {restricted_params} are set by Aqua "
                 f"and cannot be overridden or are invalid."
             )
-        return dict(valid=True)
+        return {"valid": True}
 
     @staticmethod
     def _find_restricted_params(
@@ -689,7 +673,7 @@ def _find_restricted_params(
             default_params_dict = get_params_dict(default_params)
             user_params_dict = get_params_dict(user_params)
 
-            for key, items in user_params_dict.items():
+            for key, _items in user_params_dict.items():
                 if (
                     key in default_params_dict
                     or (
@@ -701,6 +685,6 @@ def _find_restricted_params(
                         and key in TGIInferenceRestrictedParams
                     )
                 ):
-                    restricted_params.append(key.lstrip("--"))
+                    restricted_params.append(key.lstrip("-"))
 
         return restricted_params

tests/unitary/with_extras/aqua/test_data/deployment/deployment_config.json

@@ -2,6 +2,7 @@
   "configuration": {
     "VM.GPU.A10.1": {
       "parameters": {
+        "TGI_PARAMS": "--max-stop-sequences 6",
         "VLLM_PARAMS": "--max-model-len 4096"
       }
     }

tests/unitary/with_extras/aqua/test_deployment.py

@@ -352,22 +352,10 @@ def test_create_deployment_for_fine_tuned_model(
     ):
         """Test to create a deployment for fine-tuned model"""
 
-        # todo: DataScienceModel.from_yaml should update model_file_description attribute, current workaround is to
-        #   load using with_model_file_description property.
-        def yaml_to_json(input_file):
-            with open(input_file, "r") as f:
-                return yaml.safe_load(f)
-
         aqua_model = os.path.join(
             self.curr_dir, "test_data/deployment/aqua_finetuned_model.yaml"
         )
-        model_description_json = json.dumps(
-            yaml_to_json(aqua_model)["spec"]["modelDescription"]
-        )
         datascience_model = DataScienceModel.from_yaml(uri=aqua_model)
-        datascience_model.with_model_file_description(
-            json_string=model_description_json
-        )
         mock_create.return_value = datascience_model
 
         config_json = os.path.join(