Fix Recursion Issue in OCIDatascienceModel Due to is_model_by_reference Conflict in New OCI SDK (#1073)

Author: mrDzurb

ads/model/artifact_downloader.py

@@ -1,5 +1,4 @@
 #!/usr/bin/env python
-# -*- coding: utf-8; -*-
 
 # Copyright (c) 2022, 2024 Oracle and/or its affiliates.
 # Licensed under the Universal Permissive License v 1.0 as shown at https://oss.oracle.com/licenses/upl/
@@ -12,9 +11,9 @@
 from zipfile import ZipFile
 
 from ads.common import utils
+from ads.common.object_storage_details import ObjectStorageDetails
 from ads.common.utils import extract_region
 from ads.model.service.oci_datascience_model import OCIDataScienceModel
-from ads.common.object_storage_details import ObjectStorageDetails
 
 
 class ArtifactDownloader(ABC):
@@ -169,9 +168,9 @@ def __init__(
 
     def _download(self):
         """Downloads model artifacts."""
-        self.progress.update(f"Importing model artifacts from catalog")
+        self.progress.update("Importing model artifacts from catalog")
 
-        if self.dsc_model.is_model_by_reference() and self.model_file_description:
+        if self.dsc_model._is_model_by_reference() and self.model_file_description:
             self.download_from_model_file_description()
             self.progress.update()
             return

ads/model/datascience_model.py

@@ -1778,7 +1778,7 @@ def _update_from_oci_dsc_model(
             artifact_info = self.dsc_model.get_artifact_info()
             _, file_name_info = cgi.parse_header(artifact_info["Content-Disposition"])
 
-            if self.dsc_model.is_model_by_reference():
+            if self.dsc_model._is_model_by_reference():
                 _, file_extension = os.path.splitext(file_name_info["filename"])
                 if file_extension.lower() == ".json":
                     bucket_uri, _ = self._download_file_description_artifact()

ads/model/service/oci_datascience_model.py

@@ -1,35 +1,32 @@
 #!/usr/bin/env python
-# -*- coding: utf-8; -*-
 
 # Copyright (c) 2022, 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 logging
-import time
 from functools import wraps
 from io import BytesIO
 from typing import Callable, Dict, List, Optional
 
 import oci.data_science
-from ads.common import utils
-from ads.common.object_storage_details import ObjectStorageDetails
-from ads.common.oci_datascience import OCIDataScienceMixin
-from ads.common.oci_mixin import OCIWorkRequestMixin
-from ads.common.oci_resource import SEARCH_TYPE, OCIResource
-from ads.common.utils import extract_region
-from ads.common.work_request import DataScienceWorkRequest
-from ads.model.deployment import ModelDeployment
 from oci.data_science.models import (
     ArtifactExportDetailsObjectStorage,
     ArtifactImportDetailsObjectStorage,
     CreateModelDetails,
     ExportModelArtifactDetails,
     ImportModelArtifactDetails,
     UpdateModelDetails,
-    WorkRequest,
 )
 from oci.exceptions import ServiceError
 
+from ads.common.object_storage_details import ObjectStorageDetails
+from ads.common.oci_datascience import OCIDataScienceMixin
+from ads.common.oci_mixin import OCIWorkRequestMixin
+from ads.common.oci_resource import SEARCH_TYPE, OCIResource
+from ads.common.utils import extract_region
+from ads.common.work_request import DataScienceWorkRequest
+from ads.model.deployment import ModelDeployment
+
 logger = logging.getLogger(__name__)
 
 _REQUEST_INTERVAL_IN_SEC = 3
@@ -282,7 +279,7 @@ def get_artifact_info(self) -> Dict:
         msg="Model needs to be restored before the archived artifact content can be accessed."
     )
     def restore_archived_model_artifact(
-            self, restore_model_for_hours_specified: Optional[int] = None
+        self, restore_model_for_hours_specified: Optional[int] = None
     ) -> None:
         """Restores the archived model artifact.
 
@@ -304,7 +301,8 @@ def restore_archived_model_artifact(
         """
         return self.client.restore_archived_model_artifact(
             model_id=self.id,
-            restore_model_for_hours_specified=restore_model_for_hours_specified).headers["opc-work-request-id"]
+            restore_model_for_hours_specified=restore_model_for_hours_specified,
+        ).headers["opc-work-request-id"]
 
     @check_for_model_id(
         msg="Model needs to be saved to the Model Catalog before the artifact content can be read."
@@ -581,7 +579,7 @@ def from_id(cls, ocid: str) -> "OCIDataScienceModel":
             raise ValueError("Model OCID not provided.")
         return super().from_ocid(ocid)
 
-    def is_model_by_reference(self):
+    def _is_model_by_reference(self):
         """Checks if model is created by reference
         Returns
         -------

tests/unitary/default_setup/model/test_artifact_downloader.py

@@ -189,7 +189,7 @@ def test_download_large_artifact_from_model_file_description(self, mock_download
         """Tests whether model_file_description is loaded within downloader and is parsed, and also if
         #  download_from_model_file_description is appropriately called."""
 
-        self.mock_dsc_model.is_model_by_reference.return_value = True
+        self.mock_dsc_model._is_model_by_reference.return_value = True
         self.mock_artifact_file_path = os.path.join(
             self.curr_dir, "test_files/model_description.json"
         )

tests/unitary/default_setup/model/test_datascience_model.py

@@ -613,7 +613,7 @@ def test__to_oci_dsc_model(self):
             True,
         ],
     )
-    @patch.object(OCIDataScienceModel, "is_model_by_reference")
+    @patch.object(OCIDataScienceModel, "_is_model_by_reference")
     @patch.object(OCIDataScienceModel, "get_artifact_info")
     @patch.object(OCIDataScienceModel, "get_model_provenance")
     @patch.object(DataScienceModel, "_download_file_description_artifact")

tests/unitary/default_setup/model/test_oci_datascience_model.py

@@ -473,7 +473,7 @@ def test_is_model_by_reference(self):
             category="Other",
         )
         self.mock_model.custom_metadata_list = [metadata_item]
-        assert not self.mock_model.is_model_by_reference()
+        assert not self.mock_model._is_model_by_reference()
 
         metadata_item = ModelCustomMetadataItem(
             key="modelDescription",
@@ -483,4 +483,4 @@ def test_is_model_by_reference(self):
         )
         self.mock_model.custom_metadata_list = [metadata_item]
 
-        assert self.mock_model.is_model_by_reference()
+        assert self.mock_model._is_model_by_reference()