Feature/aqua ms changes #1019

vikasray0208 · 2024-12-05T06:53:22Z

This Pull Request is for adding/updating API for listing SMM ,Model custom and defined metadata artifact in DataScienceModel in ADS.

This PR will enable following changes

Service Model Listing: Enable the listing of service models without requiring the service compartment ID and cross-tenancy access.
Metadata Storage: Enable additional metadata store , such as license content, README.md, deployment config, manifest.yaml, and fine-tuning config. Also, implement a search functionality for the license and keys from the README via RQS.

API Spec :- https://confluence.oci.oraclecorp.com/pages/viewpage.action?spaceKey=ODSC&title=API+changes+for+Model+store+for+Aqua
Design Doc :- https://confluence.oci.oraclecorp.com/display/ODSC/Model+Store+Enhancements+For+AQUA+-+Design

github-actions · 2024-12-05T08:02:54Z

📌 Cov diff with main:

📌 Overall coverage:

vikasray0208 · 2024-12-05T08:27:03Z

Model API test to List SMM and query parameter category changes

Model Version Set API test to List SMM and query parameter category changes

Custom Metadata API Test

Defined Metadata API Test

github-actions · 2024-12-06T08:42:07Z

📌 Cov diff with main:

📌 Overall coverage:

mrDzurb · 2024-12-12T04:20:56Z

ads/model/model_version_set.py

@@ -454,14 +454,16 @@ def kind(self) -> str:
        return "modelVersionSet"

    @classmethod
-    def list(cls, compartment_id: str = None, **kwargs) -> List["ModelVersionSet"]:
+    def list(cls, compartment_id: str = None, category: str = "USER", **kwargs) -> List["ModelVersionSet"]:


Let's move category to constants.

class ModelCategory (str, metaclass=ExtendedEnumMeta): USER = "USER" SERVICE = "SERVICE"

This variable is used across many modules.

mrDzurb · 2024-12-13T01:22:19Z

ads/model/datascience_model.py

@@ -1571,7 +1573,7 @@ def delete(

    @classmethod
    def list(
-        cls, compartment_id: str = None, project_id: str = None, **kwargs
+            cls, compartment_id: str = None, project_id: str = None, category: str = "USER", **kwargs


Let's move category to constants.

class ModelCategory (str, metaclass=ExtendedEnumMeta): USER = "USER" SERVICE = "SERVICE"

This variable is used across many modules.

mrDzurb · 2024-12-13T01:23:31Z

ads/model/datascience_model.py

            )
        ]

    @classmethod
    def list_df(
-        cls, compartment_id: str = None, project_id: str = None, **kwargs
+            cls, compartment_id: str = None, project_id: str = None, category: str = "USER", **kwargs


Could you install a pre-commit hook and validate the changed files with it. Internally we use ruff formatter.

mrDzurb · 2024-12-13T01:34:33Z

ads/model/datascience_model.py

@@ -2193,3 +2199,249 @@ def find_model_idx():
        else:
            # model found case
            self.model_file_description["models"].pop(modelSearchIdx)
+
+    def create_custom_metadata_artifact(self, model_ocid: str, metadata_key_name: str, artifact_path: str) -> dict:
+        """Creates model custom metadata artifact for specified model.


Please add more description for this model. For example when this method needs to be used. Also check the parameters section, looks like there is some formatting issue.
Also it would be better if instead of dict we return some dataclass or pydentic class.

from ads.common.serializer import DataClassSerializable @dataclass class MyClass(): attr1: Optional[str] = None attr2: Optional[str] = None

OR

from pydantic import BaseModel class MyClass(BaseModel): attr1: Optional[str] = None attr2: Optional[str] = None

mrDzurb · 2024-12-13T01:36:48Z

ads/model/datascience_model.py

+                                                       artifact_path=artifact_path)
+
+
+    def create_defined_metadata_artifact(self, model_ocid: str, metadata_key_name: str, artifact_path: str) -> dict:


Why can't we have a one method and pass artifact type as a param?

def create_defined_metadata_artifact(..., type: str): return self.dsc_model.create_defined_metadata_artifact(...)

mrDzurb · 2024-12-13T01:38:15Z

ads/model/datascience_model.py

+        metadata_key_name: str
+            The name of the model metadatum in the metadata.
+
+        artifact_path: str


I suppose we will support only local paths, not OS bucket paths? If so, that needs to be mentioned in the pydocs.

mrDzurb · 2024-12-13T01:49:08Z

ads/model/datascience_model.py

+        artifact_file_path = os.path.join(
+            target_dir, f"{metadata_key_name}"
+        )
+        with open(artifact_file_path, "wb") as _file:


What if the file with the given name already exists? We should allow user to override existing files and fail if they didn't set override param to True, by default it should be false.

mrDzurb · 2024-12-13T01:51:42Z

ads/model/model_version_set.py

@@ -454,14 +454,16 @@ def kind(self) -> str:
        return "modelVersionSet"

    @classmethod
-    def list(cls, compartment_id: str = None, **kwargs) -> List["ModelVersionSet"]:
+    def list(cls, compartment_id: str = None, category: str = "USER", **kwargs) -> List["ModelVersionSet"]:


Create will continue working without changes, right?

mrDzurb · 2024-12-13T01:52:48Z

ads/model/service/oci_datascience_model.py

@@ -89,6 +92,12 @@ def wrapper(self, *args, **kwargs):
    return decorator


+def convert_response_to_dict(headers:dict,status:int):


See one of the comments above, it would be better to use dataclass or pydentic class for this.

mrDzurb · 2024-12-13T01:53:54Z

ads/model/service/oci_datascience_model.py

@@ -596,3 +605,331 @@ def is_model_by_reference(self):
                ):
                    return True
        return False
+
+    def create_custom_metadata_artifact(self, model_ocid: str, metadata_key_name: str, artifact_path: str) -> dict:


See one of the comments above. Why do we need two methods for creating artifacts? Custom and Defined can be combined?

mrDzurb · 2024-12-13T01:55:38Z

Please use pre-commit hook to format changed files. Also we use ruff plugin to autoformat and validate code.

…ature/aqua_ms_changes

github-actions · 2024-12-13T07:50:57Z

📌 Cov diff with main:

📌 Overall coverage:

VipulMascarenhas · 2024-12-17T21:03:13Z

ads/model/datascience_model.py

+        )
+        with open(artifact_file_path, "wb") as _file:
+            _file.write(file_content)
+            print(f"Artifact downloaded to location - {artifact_file_path}")


use logger instead of print throughout the PR

VipulMascarenhas · 2024-12-17T22:44:05Z

ads/model/datascience_model.py

+
+        Parameters
+        ----------
+        model_ocid: str


we don't need to accept model_ocid as input for these functions, can we use the self.id property instead?

VipulMascarenhas · 2024-12-17T22:51:29Z

ads/model/service/oci_datascience_model.py

+            contents = f.read()
+            print(contents)
+
+        response = self.client.create_model_custom_metadatum_artifact(model_ocid, metadata_key_name, contents,


can we do some validation of contents here to avoid incorrect values/data resulting in service errors?

VipulMascarenhas · 2024-12-17T22:53:18Z

ads/model/service/oci_datascience_model.py

+        try:
+            return self.client.get_model_custom_metadatum_artifact_content(model_ocid, metadata_key_name).data.content
+        except ServiceError as ex:
+            if ex.status == 404:


can we log error here and return the message as well with ModelMetadataArtifactNotFoundError?

VipulMascarenhas · 2024-12-17T22:58:05Z

@vikasray0208 can you update the screenshots and hide/obfuscate the ocids and namespaces?

vikasray0208 added 2 commits December 2, 2024 19:37

Added methods for aqua-model store apis change

da78f8c

update modela and mvs list api

e74334d

vikasray0208 requested review from darenr, mayoor, mrDzurb, VipulMascarenhas, qiuosier and ahosler as code owners December 5, 2024 06:53

oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Dec 5, 2024

vikasray0208 requested review from tkg2261, iamishaan and shail2612 December 5, 2024 08:39

vikasray0208 added 2 commits December 11, 2024 14:28

Added methods for aqua-model store apis change

d25c442

update modela and mvs list api

b1a208d

mrDzurb reviewed Dec 13, 2024

View reviewed changes

vikasray0208 added 3 commits December 13, 2024 12:04

Merge remote-tracking branch 'origin/main' into feature/aqua_ms_changes

81db37c

Merge remote-tracking branch 'origin/feature/aqua_ms_changes' into fe…

01bb578

…ature/aqua_ms_changes

sync with feature branch

1e98882

VipulMascarenhas reviewed Dec 17, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/aqua ms changes #1019

Feature/aqua ms changes #1019

vikasray0208 commented Dec 5, 2024

github-actions bot commented Dec 5, 2024

vikasray0208 commented Dec 5, 2024

github-actions bot commented Dec 6, 2024

mrDzurb Dec 12, 2024

mrDzurb Dec 13, 2024

mrDzurb Dec 13, 2024

mrDzurb Dec 13, 2024

mrDzurb Dec 13, 2024

mrDzurb Dec 13, 2024

mrDzurb Dec 13, 2024

mrDzurb Dec 13, 2024

mrDzurb Dec 13, 2024

mrDzurb Dec 13, 2024

mrDzurb commented Dec 13, 2024

github-actions bot commented Dec 13, 2024

VipulMascarenhas Dec 17, 2024

VipulMascarenhas Dec 17, 2024

VipulMascarenhas Dec 17, 2024

VipulMascarenhas Dec 17, 2024

VipulMascarenhas commented Dec 17, 2024

		artifact_path=artifact_path)


		def create_defined_metadata_artifact(self, model_ocid: str, metadata_key_name: str, artifact_path: str) -> dict:

		@@ -89,6 +92,12 @@ def wrapper(self, args, *kwargs):
		return decorator


		def convert_response_to_dict(headers:dict,status:int):

Feature/aqua ms changes #1019

Are you sure you want to change the base?

Feature/aqua ms changes #1019

Conversation

vikasray0208 commented Dec 5, 2024

github-actions bot commented Dec 5, 2024

vikasray0208 commented Dec 5, 2024

github-actions bot commented Dec 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrDzurb commented Dec 13, 2024

github-actions bot commented Dec 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VipulMascarenhas commented Dec 17, 2024