Prioritize generic model deploy parameters. (#239)

Author: lu-ohai

ads/jobs/builders/infrastructure/dsc_job.py

@@ -266,10 +266,9 @@ def load_properties_from_env(self) -> None:
                 # This will skip loading the default configure.
                 nb_session = None
             if nb_session:
-                nb_config = getattr(
-                    nb_session,
-                    "notebook_session_config_details",
-                    getattr(nb_session, "notebook_session_configuration_details", None),
+                nb_config = (
+                    getattr(nb_session, "notebook_session_config_details", None)
+                    or getattr(nb_session, "notebook_session_configuration_details", None)
                 )
 
                 if nb_config:

ads/model/deployment/model_deployment_infrastructure.py

@@ -225,7 +225,18 @@ def _load_default_properties(self) -> Dict:
         if NB_SESSION_OCID:
             try:
                 nb_session = DSCNotebookSession.from_ocid(NB_SESSION_OCID)
-                nb_config = nb_session.notebook_session_configuration_details
+            except Exception as e:
+                logger.warning(
+                    f"Error fetching details about Notebook "
+                    f"session: {NB_SESSION_OCID}. {e}"
+                )
+                logger.debug(traceback.format_exc())
+
+            nb_config = (
+                getattr(nb_session, "notebook_session_config_details", None)
+                or getattr(nb_session, "notebook_session_configuration_details", None)
+            )
+            if nb_config:
                 defaults[self.CONST_SHAPE_NAME] = nb_config.shape
 
                 if nb_config.notebook_session_shape_config_details:
@@ -236,13 +247,6 @@ def _load_default_properties(self) -> Dict:
                         notebook_shape_config_details
                     )
 
-            except Exception as e:
-                logger.warning(
-                    f"Error fetching details about Notebook "
-                    f"session: {NB_SESSION_OCID}. {e}"
-                )
-                logger.debug(traceback.format_exc())
-
         return defaults
 
     @property

ads/model/generic_model.py

@@ -2101,7 +2101,7 @@ def deploy(
         if not display_name:
             display_name = utils.get_random_name_for_resource()
         # populates properties from args and kwargs. Empty values will be ignored.
-        self.properties.with_dict(_extract_locals(locals()))
+        override_properties = _extract_locals(locals())
         # clears out project_id and compartment_id from kwargs, to prevent passing
         # these params to the deployment via kwargs.
         kwargs.pop("project_id", None)
@@ -2110,19 +2110,55 @@ def deploy(
         max_wait_time = kwargs.pop("max_wait_time", DEFAULT_WAIT_TIME)
         poll_interval = kwargs.pop("poll_interval", DEFAULT_POLL_INTERVAL)
 
-        self.properties.compartment_id = (
-            self.properties.compartment_id or _COMPARTMENT_OCID
-        )
-        self.properties.project_id = self.properties.project_id or PROJECT_OCID
-        self.properties.deployment_instance_shape = (
-            self.properties.deployment_instance_shape or MODEL_DEPLOYMENT_INSTANCE_SHAPE
-        )
-        self.properties.deployment_instance_count = (
-            self.properties.deployment_instance_count or MODEL_DEPLOYMENT_INSTANCE_COUNT
-        )
-        self.properties.deployment_bandwidth_mbps = (
-            self.properties.deployment_bandwidth_mbps or MODEL_DEPLOYMENT_BANDWIDTH_MBPS
-        )
+        # GenericModel itself has a ModelDeployment instance. When calling deploy(),
+        # if there are parameters passed in they will override this ModelDeployment instance,
+        # otherwise the properties of the ModelDeployment instance will be applied for deployment.
+        existing_infrastructure = self.model_deployment.infrastructure
+        existing_runtime = self.model_deployment.runtime
+        property_dict = ModelProperties(
+            compartment_id = existing_infrastructure.compartment_id
+            or self.properties.compartment_id
+            or _COMPARTMENT_OCID,
+            project_id = existing_infrastructure.project_id
+            or self.properties.project_id
+            or PROJECT_OCID,
+            deployment_instance_shape = existing_infrastructure.shape_name
+            or self.properties.deployment_instance_shape
+            or MODEL_DEPLOYMENT_INSTANCE_SHAPE,
+            deployment_instance_count = existing_infrastructure.replica
+            or self.properties.deployment_instance_count
+            or MODEL_DEPLOYMENT_INSTANCE_COUNT,
+            deployment_bandwidth_mbps = existing_infrastructure.bandwidth_mbps
+            or self.properties.deployment_bandwidth_mbps
+            or MODEL_DEPLOYMENT_BANDWIDTH_MBPS,
+            deployment_ocpus = existing_infrastructure.shape_config_details.get(
+                "ocpus", None
+            )
+            or self.properties.deployment_ocpus
+            or MODEL_DEPLOYMENT_INSTANCE_OCPUS,
+            deployment_memory_in_gbs = existing_infrastructure.shape_config_details.get(
+                "memoryInGBs", None
+            )
+            or self.properties.deployment_memory_in_gbs
+            or MODEL_DEPLOYMENT_INSTANCE_MEMORY_IN_GBS,
+            deployment_log_group_id = existing_infrastructure.log_group_id
+            or self.properties.deployment_log_group_id,
+            deployment_access_log_id = existing_infrastructure.access_log.get(
+                "log_id", None
+            )
+            or self.properties.deployment_access_log_id,
+            deployment_predict_log_id = existing_infrastructure.predict_log.get(
+                "log_id", None
+            )
+            or self.properties.deployment_predict_log_id,
+            deployment_image = existing_runtime.image
+            or self.properties.deployment_image,
+            deployment_instance_subnet_id = existing_infrastructure.subnet_id
+            or self.properties.deployment_instance_subnet_id
+        ).to_dict()
+
+        property_dict.update(override_properties)
+        self.properties.with_dict(property_dict)
 
         if not self.model_id:
             raise ValueError(
@@ -2140,104 +2176,58 @@ def deploy(
                 "cannot be used without `deployment_log_group_id`."
             )
 
-        existing_infrastructure = self.model_deployment.infrastructure
-        existing_runtime = self.model_deployment.runtime
-
-        web_concurrency = (
-            kwargs.pop("web_concurrency", None)
-            or existing_infrastructure.web_concurrency
-        )
-        if not (
-            self.properties.compartment_id or existing_infrastructure.compartment_id
-        ):
+        if not self.properties.compartment_id:
             raise ValueError("`compartment_id` has to be provided.")
-        if not (self.properties.project_id or existing_infrastructure.project_id):
+        if not self.properties.project_id:
             raise ValueError("`project_id` has to be provided.")
         infrastructure = (
             ModelDeploymentInfrastructure()
-            .with_compartment_id(
-                self.properties.compartment_id or existing_infrastructure.compartment_id
-            )
-            .with_project_id(
-                self.properties.project_id or existing_infrastructure.project_id
-            )
-            .with_bandwidth_mbps(
-                self.properties.deployment_bandwidth_mbps
-                or existing_infrastructure.bandwidth_mbps
-            )
-            .with_shape_name(
-                self.properties.deployment_instance_shape
-                or existing_infrastructure.shape_name
-            )
-            .with_subnet_id(
-                self.properties.deployment_instance_subnet_id
-                or existing_infrastructure.subnet_id
-            )
-            .with_replica(
-                self.properties.deployment_instance_count
-                or existing_infrastructure.replica
-            )
-            .with_web_concurrency(web_concurrency)
+            .with_compartment_id(self.properties.compartment_id)
+            .with_project_id(self.properties.project_id)
+            .with_bandwidth_mbps(self.properties.deployment_bandwidth_mbps)
+            .with_shape_name(self.properties.deployment_instance_shape)
+            .with_replica(self.properties.deployment_instance_count)
+            .with_subnet_id(self.properties.deployment_instance_subnet_id)
         )
 
-        ocpus = (
-            self.properties.deployment_ocpus
-            or existing_infrastructure.shape_config_details.get("ocpus")
-        )
-        memory_in_gbs = (
-            self.properties.deployment_memory_in_gbs
-            or existing_infrastructure.shape_config_details.get("memory_in_gbs")
+        web_concurrency = (
+            kwargs.pop("web_concurrency", None)
+            or existing_infrastructure.web_concurrency
         )
+        if web_concurrency:
+            infrastructure.with_web_concurrency(web_concurrency)
 
         if infrastructure.shape_name.endswith("Flex"):
             infrastructure.with_shape_config_details(
-                ocpus=ocpus or MODEL_DEPLOYMENT_INSTANCE_OCPUS,
-                memory_in_gbs=memory_in_gbs or MODEL_DEPLOYMENT_INSTANCE_MEMORY_IN_GBS,
+                ocpus=self.properties.deployment_ocpus,
+                memory_in_gbs=self.properties.deployment_memory_in_gbs,
             )
 
-        access_log_id = (
-            self.properties.deployment_access_log_id
-            or existing_infrastructure.access_log.get("log_id")
-        )
-        access_log_group_id = (
-            self.properties.deployment_log_group_id
-            or existing_infrastructure.access_log.get("log_group_id")
-        )
-
         # specifies the access log id
-        if access_log_id:
+        if self.properties.deployment_access_log_id:
             infrastructure.with_access_log(
-                log_group_id=access_log_group_id,
-                log_id=access_log_id,
+                log_group_id=self.properties.deployment_log_group_id,
+                log_id=self.properties.deployment_access_log_id,
             )
 
-        predict_log_id = (
-            self.properties.deployment_predict_log_id
-            or existing_infrastructure.predict_log.get("log_id")
-        )
-        predict_log_group_id = (
-            self.properties.deployment_log_group_id
-            or existing_infrastructure.predict_log.get("log_group_id")
-        )
-
         # specifies the predict log id
-        if predict_log_id:
+        if self.properties.deployment_predict_log_id:
             infrastructure.with_predict_log(
-                log_group_id=predict_log_group_id,
-                log_id=predict_log_id,
+                log_group_id=self.properties.deployment_log_group_id,
+                log_id=self.properties.deployment_predict_log_id,
             )
 
         environment_variables = (
             kwargs.pop("environment_variables", {}) or existing_runtime.env
         )
         deployment_mode = (
-            kwargs.pop("deployment_mode", ModelDeploymentMode.HTTPS)
+            kwargs.pop("deployment_mode", None)
             or existing_runtime.deployment_mode
+            or ModelDeploymentMode.HTTPS
         )
 
         runtime = None
-        image = self.properties.deployment_image or existing_runtime.image
-        if image:
+        if self.properties.deployment_image:
             image_digest = (
                 kwargs.pop("image_digest", None) or existing_runtime.image_digest
             )
@@ -2252,7 +2242,7 @@ def deploy(
             )
             runtime = (
                 ModelDeploymentContainerRuntime()
-                .with_image(image)
+                .with_image(self.properties.deployment_image)
                 .with_image_digest(image_digest)
                 .with_cmd(cmd)
                 .with_entrypoint(entrypoint)

tests/unitary/with_extras/model/test_generic_model.py

@@ -1917,6 +1917,132 @@ def test_update(
         self.generic_model.dsc_model.schema_output == self.mock_dsc_model.output_schema
         self.generic_model.dsc_model.metadata_provenance == self.mock_dsc_model.provenance_metadata
 
+    @patch.object(ModelDeployment, "deploy")
+    def test_deploy_combined_initialization(self, mock_deploy):
+        self.generic_model.properties = ModelProperties(
+            deployment_image="default_test_docker_image",
+            compartment_id="default_test_compartment_id",
+            project_id="default_test_project_id",
+        )
+        test_model_id = "ocid.test_model_id"
+        self.generic_model.dsc_model = MagicMock(id=test_model_id)
+        self.generic_model.ignore_conda_error = True
+        infrastructure = ModelDeploymentInfrastructure(
+            **{
+                "shape_name": "test_deployment_instance_shape",
+                "replica": 10,
+                "bandwidth_mbps": 100,
+                "shape_config_details": {"memory_in_gbs": 10, "ocpus": 1},
+                "access_log": {
+                    "log_group_id": "test_deployment_log_group_id",
+                    "log_id": "test_deployment_access_log_id",
+                },
+                "predict_log": {
+                    "log_group_id": "test_deployment_log_group_id",
+                    "log_id": "test_deployment_predict_log_id",
+                },
+                "project_id": "project_id_passed_using_with",
+                "compartment_id": "compartment_id_passed_using_with",
+            }
+        )
+        runtime = ModelDeploymentContainerRuntime(
+            **{
+                "image": "image_passed_using_with",
+                "image_digest": "test_image_digest",
+                "cmd": ["test_cmd"],
+                "entrypoint": ["test_entrypoint"],
+                "server_port": 8080,
+                "health_check_port": 8080,
+                "env": {"test_key": "test_value"},
+                "deployment_mode": "HTTPS_ONLY",
+            }
+        )
+        mock_deploy.return_value = ModelDeployment(
+            display_name="test_display_name",
+            description="test_description",
+            infrastructure=infrastructure,
+            runtime=runtime,
+            model_deployment_url="test_model_deployment_url",
+            model_deployment_id="test_model_deployment_id",
+        )
+        input_dict = {
+            "wait_for_completion": True,
+            "display_name": "test_display_name",
+            "description": "test_description",
+            "deployment_instance_shape": "test_deployment_instance_shape",
+            "deployment_instance_count": 10,
+            "deployment_bandwidth_mbps": 100,
+            "deployment_memory_in_gbs": 10,
+            "deployment_ocpus": 1,
+            "deployment_log_group_id": "test_deployment_log_group_id",
+            "deployment_access_log_id": "test_deployment_access_log_id",
+            "deployment_predict_log_id": "test_deployment_predict_log_id",
+            "cmd": ["test_cmd"],
+            "entrypoint": ["test_entrypoint"],
+            "server_port": 8080,
+            "health_check_port": 8080,
+            "environment_variables": {"test_key": "test_value"},
+            "max_wait_time": 100,
+            "poll_interval": 200,
+        }
+
+        self.generic_model.model_deployment.infrastructure.with_compartment_id(
+            "compartment_id_passed_using_with"
+        ).with_project_id("project_id_passed_using_with")
+        self.generic_model.model_deployment.runtime.with_image(
+            "image_passed_using_with"
+        )
+
+        result = self.generic_model.deploy(
+            **input_dict,
+        )
+        assert result == mock_deploy.return_value
+        assert result.infrastructure.access_log == {
+            "log_id": input_dict["deployment_access_log_id"],
+            "log_group_id": input_dict["deployment_log_group_id"],
+        }
+        assert result.infrastructure.predict_log == {
+            "log_id": input_dict["deployment_predict_log_id"],
+            "log_group_id": input_dict["deployment_log_group_id"],
+        }
+        assert (
+            result.infrastructure.bandwidth_mbps
+            == input_dict["deployment_bandwidth_mbps"]
+        )
+        assert (
+            result.infrastructure.compartment_id == "compartment_id_passed_using_with"
+        )
+        assert result.infrastructure.project_id == "project_id_passed_using_with"
+        assert (
+            result.infrastructure.shape_name == input_dict["deployment_instance_shape"]
+        )
+        assert result.infrastructure.shape_config_details == {
+            "ocpus": input_dict["deployment_ocpus"],
+            "memory_in_gbs": input_dict["deployment_memory_in_gbs"],
+        }
+        assert result.runtime.image == "image_passed_using_with"
+        assert result.runtime.entrypoint == input_dict["entrypoint"]
+        assert result.runtime.server_port == input_dict["server_port"]
+        assert result.runtime.health_check_port == input_dict["health_check_port"]
+        assert result.runtime.env == {"test_key": "test_value"}
+        assert result.runtime.deployment_mode == "HTTPS_ONLY"
+        mock_deploy.assert_called_with(
+            wait_for_completion=input_dict["wait_for_completion"],
+            max_wait_time=input_dict["max_wait_time"],
+            poll_interval=input_dict["poll_interval"],
+        )
+
+        assert (
+            self.generic_model.properties.compartment_id
+            == "compartment_id_passed_using_with"
+        )
+        assert (
+            self.generic_model.properties.project_id == "project_id_passed_using_with"
+        )
+        assert (
+            self.generic_model.properties.deployment_image == "image_passed_using_with"
+        )
+
 
 class TestCommonMethods:
     """Tests common methods presented in the generic_model module."""