[DRCC] Create and Update Metadata artifact content changes #1129
Conversation
kumar-shivam-ranjan
requested review from
darenr,
mayoor,
mrDzurb,
VipulMascarenhas,
qiuosier and
ahosler
as code owners
oracle-contributor-agreement
bot
added
the
OCA Verified
|
📌 Cov diff with main: 📌 Overall coverage: |
|
📌 Cov diff with main: 📌 Overall coverage: |
|
📌 Cov diff with main: 📌 Overall coverage: |
ads/model/datascience_model.py
Outdated
| @@ -2442,7 +2498,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}") | |||
There was a problem hiding this comment.
Should we say to use the override param to override?
There was a problem hiding this comment.
Yes we can. Will update the error message.
| contents = str( | ||
| read_file(file_path=artifact_path_or_content, auth=default_signer()) | ||
| ) | ||
| contents = read_file( |
There was a problem hiding this comment.
just curious, why do we want to distinguish between local and OSS? Can't we use the same logic for both?
There was a problem hiding this comment.
I didn't want to change the read_file function defintion in ads.common.utils since it is already used in many other places.
read_file returns string content of the file and we need bytes.
There was a problem hiding this comment.
Now , if think we could have used same logic though without changing read_file function definition.
Updated it. Thanks!
@mrDzurb
|
📌 Cov diff with main: 📌 Overall coverage: |
| self, artifact_path_or_content: str, path_type: MetadataArtifactPathType | ||
| self, | ||
| artifact_path_or_content: Union[str, bytes], | ||
| path_type: MetadataArtifactPathType, | ||
| ): |
There was a problem hiding this comment.
Can we add a return statement here to be more clear:
def get_metadata_content(
self,
artifact_path_or_content: Union[str, bytes],
path_type: MetadataArtifactPathType,
) -> Union[str, bytes]:
There was a problem hiding this comment.
get_metadata_content always returns bytes. Updated. Thanks!
|
📌 Cov diff with main: 📌 Overall coverage: |
Description
This PR is intended to improve API for creating and updating metadata artifact which was introduced as part of MS enhancements.
Issue
While uploading metadata artifact , the type of content should always be bytes. Earlier it was both bytes and strings.
Example
When uploading the content via local path
When uploading the content via OSS path
When uploading the content itself
Unit tests