-
Notifications
You must be signed in to change notification settings - Fork 47
ADS changes for MS enhancements #1036
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left some minor comments, overall looks good.
…feature/aqua_ms_changes_2
The merge-base changed after approval.
ads/common/utils.py
Outdated
@@ -82,6 +85,14 @@ | |||
color=["teal", "blueviolet", "forestgreen", "peru", "y", "dodgerblue", "r"] | |||
) | |||
|
|||
|
|||
# Metadata artifact path type can be either local path or OSS path. It can also be the content itself. | |||
class MetadataArtifactPathType(ExtendedEnum): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the class will be used only within ads.models
, then it might make sense to move it to the ads.models.common.utils.py
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated. Thanks!
@@ -212,6 +239,15 @@ def random_valid_ocid(prefix="ocid1.dataflowapplication.oc1.iad"): | |||
return f"{left}.{fake}" | |||
|
|||
|
|||
def read_file(file_path: str, **kwargs) -> str: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you double check, we might already have something similar in common utils
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't create this function , just moved it from ads.aqua.common.utils
to ads.common.utils
due to circular import issue.
Link to the comment: #1036 (comment)
ads/model/model_metadata.py
Outdated
if isinstance(key_value_map["value"], str): | ||
try: | ||
key_value_map["value"] = json.loads(oci_metadata_item.get("value")) | ||
key_value_map["has_artifact"] = bool( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should use some bool parser method for this?
def parse_bool(value: Any) -> bool:
"""
Converts a value to boolean. For strings, it interprets 'true', '1', or 'yes'
(case insensitive) as True; everything else as False.
Parameters
----------
value : Any
The value to convert to boolean.
Returns
-------
bool
The boolean interpretation of the value.
"""
if isinstance(value, bool):
return value
if isinstance(value, str):
return value.strip().lower() in ("true", "1", "yes")
return bool(value)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated. Thanks!
ads/model/datascience_model.py
Outdated
|
||
Returns | ||
------- | ||
Dict |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of the comments in this file are still incorrect, for instance, the return type of this API is ModelMetadataArtifactDetails
, notDict
. Could you update all of them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated. Thanks!
) | ||
|
||
def get_custom_metadata_artifact( | ||
self, metadata_key_name: str, target_dir: str, override: bool = False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this idea, we can do this in both cases, even if the target_dir provided, we can still return the content.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated. Thanks!
@@ -3,7 +3,6 @@ | |||
# Licensed under the Universal Permissive License v 1.0 as shown at https://oss.oracle.com/licenses/upl/ | |||
"""This module defines constants used in ads.aqua module.""" | |||
|
|||
UNKNOWN = "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we remove UNKNOWN
here, it causes some error in feature/multi_model_deploy
branch. I'll add it back to the corresponding pr. @mrDzurb
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was moved to ads.common.utils since this function was moved to ads.common.utils
to avoid circular imports
def read_file(file_path: str, **kwargs) -> str:
try:
with fsspec.open(file_path, "r", **kwargs.get("auth", {})) as f:
return f.read()
except Exception as e:
logger.debug(f"Failed to read file {file_path}. {e}")
return UNKNOWN
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is a generic function , it was adviced to move this to ads.common.utils
Description
This PR is intended to add/update the following changes in ADS in accordance with AQUA MS enhancements
This PR will enable following changes
Example
Unit tests with latest preview oci sdk
Version: 2.142.1+preview.1.1
References
UPDATE:
public oci sdk has been released: https://github.com/oracle/oci-python-sdk/releases/tag/v2.148.0