From cf22cb293218b6532b83606a860bc93803bcd466 Mon Sep 17 00:00:00 2001 From: Kumar Ranjan Date: Fri, 21 Mar 2025 17:14:31 +0530 Subject: [PATCH 1/5] Fixing bug related to upload metadata artifact changes --- ads/common/utils.py | 18 +----- ads/model/datascience_model.py | 75 +++++++++++++++++----- ads/model/service/oci_datascience_model.py | 69 ++++++++++++-------- 3 files changed, 100 insertions(+), 62 deletions(-) diff --git a/ads/common/utils.py b/ads/common/utils.py index f2fd992b3..09a11f401 100644 --- a/ads/common/utils.py +++ b/ads/common/utils.py @@ -1,6 +1,6 @@ #!/usr/bin/env python -# Copyright (c) 2020, 2024 Oracle and/or its affiliates. +# Copyright (c) 2020, 2025 Oracle and/or its affiliates. # Licensed under the Universal Permissive License v 1.0 as shown at https://oss.oracle.com/licenses/upl/ @@ -152,22 +152,6 @@ def oci_key_location(): ) -def text_sanitizer(content): - if isinstance(content, str): - return ( - content.replace("“", '"') - .replace("”", '"') - .replace("’", "'") - .replace("‘", "'") - .replace("—", "-") - .encode("utf-8", "ignore") - .decode("utf-8", "ignore") - ) - if isinstance(content, dict): - return json.dumps(content) - return str(content) - - @deprecated( "2.5.10", details="Deprecated, use: from ads.common.auth import AuthState; AuthState().oci_config_path", diff --git a/ads/model/datascience_model.py b/ads/model/datascience_model.py index 2d682fb9b..6258c0c03 100644 --- a/ads/model/datascience_model.py +++ b/ads/model/datascience_model.py @@ -89,6 +89,11 @@ class InvalidArtifactType(Exception): # pragma: no cover pass +class InvalidArtifactPathTypeOrContentError(Exception): # pragma: no cover + def __init__(self, msg="Invalid type of Metdata artifact content"): + super().__init__(msg) + + class CustomerNotificationType(ExtendedEnum): NONE = "NONE" ALL = "ALL" @@ -2238,7 +2243,7 @@ def find_model_idx(): def create_custom_metadata_artifact( self, metadata_key_name: str, - artifact_path_or_content: str, + artifact_path_or_content: Union[str, bytes], path_type: MetadataArtifactPathType = MetadataArtifactPathType.LOCAL, ) -> ModelMetadataArtifactDetails: """Creates model custom metadata artifact for specified model. @@ -2248,8 +2253,10 @@ def create_custom_metadata_artifact( metadata_key_name: str The name of the model custom metadata key - artifact_path_or_content: str - The model custom metadata artifact path to be upload. It can also be the actual content of the custom metadata + artifact_path_or_content: Union[str,bytes] + The model custom metadata artifact path to be upload. It can also be the actual content of the defined metadata + The type is string when it represents local path or oss path. + The type is bytes when it represents content itself path_type: MetadataArtifactPathType Can be either of MetadataArtifactPathType.LOCAL , MetadataArtifactPathType.OSS , MetadataArtifactPathType.CONTENT @@ -2273,16 +2280,23 @@ def create_custom_metadata_artifact( } """ + if path_type == MetadataArtifactPathType.CONTENT and not isinstance( + artifact_path_or_content, bytes + ): + raise InvalidArtifactPathTypeOrContentError( + f"Invalid type of artifact content: {type(artifact_path_or_content)}. It should be bytes." + ) + return self.dsc_model.create_custom_metadata_artifact( metadata_key_name=metadata_key_name, - artifact_path=artifact_path_or_content, + artifact_path_or_content=artifact_path_or_content, path_type=path_type, ) def create_defined_metadata_artifact( self, metadata_key_name: str, - artifact_path_or_content: str, + artifact_path_or_content: Union[str, bytes], path_type: MetadataArtifactPathType = MetadataArtifactPathType.LOCAL, ) -> ModelMetadataArtifactDetails: """Creates model defined metadata artifact for specified model. @@ -2292,8 +2306,10 @@ def create_defined_metadata_artifact( metadata_key_name: str The name of the model defined metadata key - artifact_path_or_content: str + artifact_path_or_content: Union[str,bytes] The model defined metadata artifact path to be upload. It can also be the actual content of the defined metadata + The type is string when it represents local path or oss path. + The type is bytes when it represents content itself path_type: MetadataArtifactPathType Can be either of MetadataArtifactPathType.LOCAL , MetadataArtifactPathType.OSS , MetadataArtifactPathType.CONTENT @@ -2317,16 +2333,23 @@ def create_defined_metadata_artifact( } """ + if path_type == MetadataArtifactPathType.CONTENT and not isinstance( + artifact_path_or_content, bytes + ): + raise InvalidArtifactPathTypeOrContentError( + f"Invalid type of artifact content: {type(artifact_path_or_content)}. It should be bytes." + ) + return self.dsc_model.create_defined_metadata_artifact( metadata_key_name=metadata_key_name, - artifact_path=artifact_path_or_content, + artifact_path_or_content=artifact_path_or_content, path_type=path_type, ) def update_custom_metadata_artifact( self, metadata_key_name: str, - artifact_path_or_content: str, + artifact_path_or_content: Union[str, bytes], path_type: MetadataArtifactPathType = MetadataArtifactPathType.LOCAL, ) -> ModelMetadataArtifactDetails: """Update model custom metadata artifact for specified model. @@ -2336,8 +2359,10 @@ def update_custom_metadata_artifact( metadata_key_name: str The name of the model custom metadata key - artifact_path_or_content: str - The model custom metadata artifact path. It can also be the actual content of the custom metadata + artifact_path_or_content: Union[str,bytes] + The model custom metadata artifact path to be upload. It can also be the actual content of the defined metadata + The type is string when it represents local path or oss path. + The type is bytes when it represents content itself path_type: MetadataArtifactPathType Can be either of MetadataArtifactPathType.LOCAL , MetadataArtifactPathType.OSS , MetadataArtifactPathType.CONTENT @@ -2361,16 +2386,23 @@ def update_custom_metadata_artifact( } """ + if path_type == MetadataArtifactPathType.CONTENT and not isinstance( + artifact_path_or_content, bytes + ): + raise InvalidArtifactPathTypeOrContentError( + f"Invalid type of artifact content: {type(artifact_path_or_content)}. It should be bytes." + ) + return self.dsc_model.update_custom_metadata_artifact( metadata_key_name=metadata_key_name, - artifact_path=artifact_path_or_content, + artifact_path_or_content=artifact_path_or_content, path_type=path_type, ) def update_defined_metadata_artifact( self, metadata_key_name: str, - artifact_path_or_content: str, + artifact_path_or_content: Union[str, bytes], path_type: MetadataArtifactPathType = MetadataArtifactPathType.LOCAL, ) -> ModelMetadataArtifactDetails: """Update model defined metadata artifact for specified model. @@ -2380,8 +2412,10 @@ def update_defined_metadata_artifact( metadata_key_name: str The name of the model defined metadata key - artifact_path_or_content: str - The model defined metadata artifact path. It can also be the actual content of the defined metadata + artifact_path_or_content: Union[str,bytes] + The model defined metadata artifact path to be upload. It can also be the actual content of the defined metadata + The type is string when it represents local path or oss path. + The type is bytes when it represents content itself path_type: MetadataArtifactPathType Can be either of MetadataArtifactPathType.LOCAL , MetadataArtifactPathType.OSS , MetadataArtifactPathType.CONTENT @@ -2405,9 +2439,16 @@ def update_defined_metadata_artifact( } """ + if path_type == MetadataArtifactPathType.CONTENT and not isinstance( + artifact_path_or_content, bytes + ): + raise InvalidArtifactPathTypeOrContentError( + f"Invalid type of artifact content: {type(artifact_path_or_content)}. It should be bytes." + ) + return self.dsc_model.update_defined_metadata_artifact( metadata_key_name=metadata_key_name, - artifact_path=artifact_path_or_content, + artifact_path_or_content=artifact_path_or_content, path_type=path_type, ) @@ -2442,7 +2483,7 @@ def get_custom_metadata_artifact( ) artifact_file_path = os.path.join(target_dir, f"{metadata_key_name}") - if not override and os.path.exists(artifact_file_path): + if not override and is_path_exists(artifact_file_path): raise FileExistsError(f"File already exists: {artifact_file_path}") with open(artifact_file_path, "wb") as _file: @@ -2481,7 +2522,7 @@ def get_defined_metadata_artifact( ) artifact_file_path = os.path.join(target_dir, f"{metadata_key_name}") - if not override and os.path.exists(artifact_file_path): + if not override and is_path_exists(artifact_file_path): raise FileExistsError(f"File already exists: {artifact_file_path}") with open(artifact_file_path, "wb") as _file: diff --git a/ads/model/service/oci_datascience_model.py b/ads/model/service/oci_datascience_model.py index 5522522bd..ba6dbac94 100644 --- a/ads/model/service/oci_datascience_model.py +++ b/ads/model/service/oci_datascience_model.py @@ -1,6 +1,6 @@ #!/usr/bin/env python -# Copyright (c) 2022, 2024 Oracle and/or its affiliates. +# Copyright (c) 2022, 2025 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 @@ -28,7 +28,7 @@ from ads.common.oci_mixin import OCIWorkRequestMixin from ads.common.oci_resource import SEARCH_TYPE, OCIResource from ads.common.serializer import DataClassSerializable -from ads.common.utils import extract_region, read_file, text_sanitizer +from ads.common.utils import extract_region, read_file from ads.common.work_request import DataScienceWorkRequest from ads.model.common.utils import MetadataArtifactPathType from ads.model.deployment import ModelDeployment @@ -621,15 +621,19 @@ def is_model_created_by_reference(self): return False def get_metadata_content( - self, artifact_path_or_content: str, path_type: MetadataArtifactPathType + self, + artifact_path_or_content: Union[str, bytes], + path_type: MetadataArtifactPathType, ): """ returns the content of the metadata artifact Parameters ---------- - artifact_path_or_content: str + artifact_path_or_content: Union[str,bytes] The path of the file (local or oss) containing metadata artifact or content. + The type is string when it represents local path or oss path. + The type is bytes when it represents content itself path_type: str can be one of local , oss or actual content itself @@ -656,10 +660,9 @@ def get_metadata_content( elif path_type == MetadataArtifactPathType.OSS: if not utils.is_path_exists(artifact_path_or_content): raise FileNotFoundError(f"File not found: {artifact_path_or_content}") - - contents = str( - read_file(file_path=artifact_path_or_content, auth=default_signer()) - ) + contents = read_file( + file_path=artifact_path_or_content, auth=default_signer() + ).encode() logger.debug(f"The metadata artifact content - {contents}") return contents @@ -670,7 +673,7 @@ def get_metadata_content( def create_custom_metadata_artifact( self, metadata_key_name: str, - artifact_path: str, + artifact_path_or_content: Union[str, bytes], path_type: MetadataArtifactPathType, ) -> ModelMetadataArtifactDetails: """Creates model custom metadata artifact for specified model. @@ -680,8 +683,10 @@ def create_custom_metadata_artifact( metadata_key_name: str The name of the model metadatum in the metadata. - artifact_path: str - The model custom metadata artifact path to be upload. + artifact_path_or_content: Union[str,bytes] + The path of the file (local or oss) containing metadata artifact or content. + The type is string when it represents local path or oss path. + The type is bytes when it represents content itself path_type: MetadataArtifactPathType can be one of local , oss or actual content itself @@ -704,12 +709,13 @@ def create_custom_metadata_artifact( """ contents = self.get_metadata_content( - artifact_path_or_content=artifact_path, path_type=path_type + artifact_path_or_content=artifact_path_or_content, path_type=path_type ) + response = self.client.create_model_custom_metadatum_artifact( self.id, metadata_key_name, - text_sanitizer(contents), + contents, content_disposition="form" '-data; name="file"; filename="readme.*"', ) response_data = convert_model_metadata_response( @@ -723,7 +729,7 @@ def create_custom_metadata_artifact( def create_defined_metadata_artifact( self, metadata_key_name: str, - artifact_path: str, + artifact_path_or_content: Union[str, bytes], path_type: MetadataArtifactPathType, ) -> ModelMetadataArtifactDetails: """Creates model defined metadata artifact for specified model. @@ -733,8 +739,10 @@ def create_defined_metadata_artifact( metadata_key_name: str The name of the model metadatum in the metadata. - artifact_path: str - The model custom metadata artifact path to be upload. + artifact_path_or_content: Union[str,bytes] + The path of the file (local or oss) containing metadata artifact or content. + The type is string when it represents local path or oss path. + The type is bytes when it represents content itself path_type: MetadataArtifactPathType can be one of local , oss or actual content itself. @@ -757,12 +765,13 @@ def create_defined_metadata_artifact( """ contents = self.get_metadata_content( - artifact_path_or_content=artifact_path, path_type=path_type + artifact_path_or_content=artifact_path_or_content, path_type=path_type ) + response = self.client.create_model_defined_metadatum_artifact( self.id, metadata_key_name, - text_sanitizer(contents), + contents, content_disposition='form-data; name="file"; filename="readme.*"', ) response_data = convert_model_metadata_response( @@ -776,7 +785,7 @@ def create_defined_metadata_artifact( def update_defined_metadata_artifact( self, metadata_key_name: str, - artifact_path: str, + artifact_path_or_content: Union[str, bytes], path_type: MetadataArtifactPathType, ) -> ModelMetadataArtifactDetails: """Update model defined metadata artifact for specified model. @@ -786,8 +795,10 @@ def update_defined_metadata_artifact( metadata_key_name: str The name of the model metadatum in the metadata. - artifact_path: str - The model defined metadata artifact path to be upload. + artifact_path_or_content: Union[str,bytes] + The path of the file (local or oss) containing metadata artifact or content. + The type is string when it represents local path or oss path. + The type is bytes when it represents content itself path_type:MetadataArtifactPathType can be one of local , oss or actual content itself. @@ -809,12 +820,12 @@ def update_defined_metadata_artifact( """ contents = self.get_metadata_content( - artifact_path_or_content=artifact_path, path_type=path_type + artifact_path_or_content=artifact_path_or_content, path_type=path_type ) response = self.client.update_model_defined_metadatum_artifact( self.id, metadata_key_name, - text_sanitizer(contents), + contents, content_disposition='form-data; name="file"; filename="readme.*"', ) response_data = convert_model_metadata_response( @@ -828,7 +839,7 @@ def update_defined_metadata_artifact( def update_custom_metadata_artifact( self, metadata_key_name: str, - artifact_path: str, + artifact_path_or_content: Union[str, bytes], path_type: MetadataArtifactPathType, ) -> ModelMetadataArtifactDetails: """Update model custom metadata artifact for specified model. @@ -838,8 +849,10 @@ def update_custom_metadata_artifact( metadata_key_name: str The name of the model metadatum in the metadata. - artifact_path: str - The model custom metadata artifact path to be upload. + artifact_path_or_content: Union[str,bytes] + The path of the file (local or oss) containing metadata artifact or content. + The type is string when it represents local path or oss path. + The type is bytes when it represents content itself path_type: MetadataArtifactPathType can be one of local , oss or actual content itself. @@ -862,12 +875,12 @@ def update_custom_metadata_artifact( """ contents = self.get_metadata_content( - artifact_path_or_content=artifact_path, path_type=path_type + artifact_path_or_content=artifact_path_or_content, path_type=path_type ) response = self.client.update_model_custom_metadatum_artifact( self.id, metadata_key_name, - text_sanitizer(contents), + contents, content_disposition="form" '-data; name="file"; filename="readme.*"', ) response_data = convert_model_metadata_response( From 05381443b508789faab9089100eec00469f031c7 Mon Sep 17 00:00:00 2001 From: Kumar Ranjan Date: Mon, 24 Mar 2025 12:44:25 +0530 Subject: [PATCH 2/5] Updating docstring --- ads/model/datascience_model.py | 25 ++++++++++++++++++++----- 1 file changed, 20 insertions(+), 5 deletions(-) diff --git a/ads/model/datascience_model.py b/ads/model/datascience_model.py index 6258c0c03..7ed77194d 100644 --- a/ads/model/datascience_model.py +++ b/ads/model/datascience_model.py @@ -2254,14 +2254,21 @@ def create_custom_metadata_artifact( The name of the model custom metadata key artifact_path_or_content: Union[str,bytes] - The model custom metadata artifact path to be upload. It can also be the actual content of the defined metadata + The model custom metadata artifact path to be uploaded. It can also be the actual content of the custom metadata artifact The type is string when it represents local path or oss path. The type is bytes when it represents content itself path_type: MetadataArtifactPathType Can be either of MetadataArtifactPathType.LOCAL , MetadataArtifactPathType.OSS , MetadataArtifactPathType.CONTENT Specifies what type of path is to be provided for metadata artifact. - Can be either local , oss or the actual content itself + + Example: + >>> ds_model=DataScienceModel.from_id("ocid1.datasciencemodel.iad.xxyxz...") + >>> ds_model.create_custom_metadata_artifact( + ... "README", + ... artifact_path_or_content="/Users//Downloads/README.md", + ... path_type=MetadataArtifactPathType.LOCAL + ... ) Returns ------- @@ -2307,7 +2314,7 @@ def create_defined_metadata_artifact( The name of the model defined metadata key artifact_path_or_content: Union[str,bytes] - The model defined metadata artifact path to be upload. It can also be the actual content of the defined metadata + The model defined metadata artifact path to be uploaded. It can also be the actual content of the defined metadata The type is string when it represents local path or oss path. The type is bytes when it represents content itself @@ -2316,6 +2323,14 @@ def create_defined_metadata_artifact( Specifies what type of path is to be provided for metadata artifact. Can be either local , oss or the actual content itself + Example: + >>> ds_model=DataScienceModel.from_id("ocid1.datasciencemodel.iad.xxyxz...") + >>> ds_model.create_defined_metadata_artifact( + ... "README", + ... artifact_path_or_content="oci://path/to/bucket/README.md", + ... path_type=MetadataArtifactPathType.OSS + ... ) + Returns ------- ModelMetadataArtifactDetails @@ -2360,7 +2375,7 @@ def update_custom_metadata_artifact( The name of the model custom metadata key artifact_path_or_content: Union[str,bytes] - The model custom metadata artifact path to be upload. It can also be the actual content of the defined metadata + The model custom metadata artifact path to be uploaded. It can also be the actual content of the custom metadata The type is string when it represents local path or oss path. The type is bytes when it represents content itself @@ -2413,7 +2428,7 @@ def update_defined_metadata_artifact( The name of the model defined metadata key artifact_path_or_content: Union[str,bytes] - The model defined metadata artifact path to be upload. It can also be the actual content of the defined metadata + The model defined metadata artifact path to be uploaded. It can also be the actual content of the defined metadata The type is string when it represents local path or oss path. The type is bytes when it represents content itself From d6b56c06ff689e7c59434d2de662714d3e927909 Mon Sep 17 00:00:00 2001 From: Kumar Ranjan Date: Tue, 25 Mar 2025 14:24:36 +0530 Subject: [PATCH 3/5] Addressing review comments --- ads/model/datascience_model.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/ads/model/datascience_model.py b/ads/model/datascience_model.py index 7ed77194d..44d93d687 100644 --- a/ads/model/datascience_model.py +++ b/ads/model/datascience_model.py @@ -2499,7 +2499,9 @@ def get_custom_metadata_artifact( artifact_file_path = os.path.join(target_dir, f"{metadata_key_name}") if not override and is_path_exists(artifact_file_path): - raise FileExistsError(f"File already exists: {artifact_file_path}") + raise FileExistsError( + f"File already exists: {artifact_file_path}. Please use boolean override parameter to override the file content." + ) with open(artifact_file_path, "wb") as _file: _file.write(file_content) @@ -2538,7 +2540,9 @@ def get_defined_metadata_artifact( artifact_file_path = os.path.join(target_dir, f"{metadata_key_name}") if not override and is_path_exists(artifact_file_path): - raise FileExistsError(f"File already exists: {artifact_file_path}") + raise FileExistsError( + f"File already exists: {artifact_file_path}. Please use boolean override parameter to override the file content." + ) with open(artifact_file_path, "wb") as _file: _file.write(file_content) From 9b1728497a42bffbcd3dd9f3fc1fc7fc81db109e Mon Sep 17 00:00:00 2001 From: Kumar Ranjan Date: Tue, 25 Mar 2025 14:49:32 +0530 Subject: [PATCH 4/5] Updating get_metadata_content to have same local/oss path logic --- ads/model/service/oci_datascience_model.py | 22 ++++++++-------------- 1 file changed, 8 insertions(+), 14 deletions(-) diff --git a/ads/model/service/oci_datascience_model.py b/ads/model/service/oci_datascience_model.py index ba6dbac94..cdcd600f0 100644 --- a/ads/model/service/oci_datascience_model.py +++ b/ads/model/service/oci_datascience_model.py @@ -645,23 +645,17 @@ def get_metadata_content( if path_type == MetadataArtifactPathType.CONTENT: return artifact_path_or_content - elif path_type == MetadataArtifactPathType.LOCAL: - if not utils.is_path_exists(artifact_path_or_content): - raise FileNotFoundError( - f"File not found: {artifact_path_or_content} . " - ) - - with open(artifact_path_or_content, "rb") as f: - contents = f.read() - logger.info(f"The metadata artifact content - {contents}") - - return contents - - elif path_type == MetadataArtifactPathType.OSS: + elif ( + path_type == MetadataArtifactPathType.LOCAL + or path_type == MetadataArtifactPathType.OSS + ): if not utils.is_path_exists(artifact_path_or_content): raise FileNotFoundError(f"File not found: {artifact_path_or_content}") + signer = ( + default_signer() if path_type == MetadataArtifactPathType.OSS else {} + ) contents = read_file( - file_path=artifact_path_or_content, auth=default_signer() + file_path=artifact_path_or_content, auth=signer ).encode() logger.debug(f"The metadata artifact content - {contents}") From cec856b1eecbf6261e4b33f1f62be9a8725b5bb3 Mon Sep 17 00:00:00 2001 From: Kumar Ranjan Date: Wed, 26 Mar 2025 01:03:50 +0530 Subject: [PATCH 5/5] Updating return type for get_metadata_content --- ads/model/service/oci_datascience_model.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/ads/model/service/oci_datascience_model.py b/ads/model/service/oci_datascience_model.py index cdcd600f0..66c62daba 100644 --- a/ads/model/service/oci_datascience_model.py +++ b/ads/model/service/oci_datascience_model.py @@ -624,7 +624,7 @@ def get_metadata_content( self, artifact_path_or_content: Union[str, bytes], path_type: MetadataArtifactPathType, - ): + ) -> bytes: """ returns the content of the metadata artifact @@ -639,7 +639,8 @@ def get_metadata_content( Returns ------- - metadata artifact content + bytes + metadata artifact content in bytes """ if path_type == MetadataArtifactPathType.CONTENT: