Feature/ms backup retention #962
Conversation
iamishaan
requested review from
VipulMascarenhas,
ahosler,
darenr,
mayoor,
mrDzurb and
qiuosier
as code owners
|
Thank you for your pull request and welcome to our community! To contribute, please sign the Oracle Contributor Agreement (OCA).
To sign the OCA, please create an Oracle account and sign the OCA in Oracle's Contributor Agreement Application. When signing the OCA, please provide your GitHub username. After signing the OCA and getting an OCA approval from Oracle, this PR will be automatically updated. If you are an Oracle employee, please make sure that you are a member of the main Oracle GitHub organization, and your membership in this organization is public. |
oracle-contributor-agreement
bot
added
the
OCA Required
|
@iamishaan Could you add some user experience example here and also update the user guide? DataScience Model related user guide should be placed under here. |
…erated-data-science into feature/ms_backup_retention
oracle-contributor-agreement
bot
added
OCA Verified
|
📌 Cov diff with main: 📌 Overall coverage: |
|
📌 Cov diff with main: 📌 Overall coverage: |
|
@iamishaan could you add more details in the PR description? |
ads/model/datascience_model.py
Outdated
|
|
||
| def restore_model( | ||
| self, | ||
| model_id: str, |
There was a problem hiding this comment.
This either should be a class method or model_id parameter is not required. See the update nethod.
There was a problem hiding this comment.
With this update the case shared below will not work. I would rather prefer to remove the model_id attribute from the function.
This case will not work:
DataScienceModel.from_id("OCID"). restore_model()
It will expect from e to provide OCID one more time,
like this
DataScienceModel.from_id("OCID"). restore_model("OCID")
But why should i do this, if I already provided the OCID before?
The better solution would be
def restore_model(
cls,
restore_model_for_hours_specified: Optional[int] = HOURS_24,
):
if not self.id:
raise ValueError("Model needs to be saved to the model catalog before it can be updated.")
Or if we really want to support both cases below
DataScienceModel.from_id("OCID"). restore_model()
DataScienceModel.restore_model("OCID")
Then the solution could be
@class_or_instance_method
def restore_model(
cls,
model_id: Optional[str] = None,
restore_model_for_hours_specified: Optional[int] = HOURS_24,
):
if not inspect.isclass(cls): # if the instance used
pass
# otherwise the class is used
# see GenericModel.update_deployment as example.
There was a problem hiding this comment.
Have updated the method to use self and model_id isn't a required parameter anymore now.
just passing it to restore_archived_model_artifact method here
self.dsc_model.restore_archived_model_artifact(
model_id=self.id,
restore_model_for_hours_specified=restore_model_for_hours_specified,
)
|
📌 Cov diff with main: 📌 Overall coverage: |
| msg="Model needs to be restored before the archived artifact content can be accessed." | ||
| ) | ||
| def restore_archived_model_artifact( | ||
| self, model_id: str, restore_model_for_hours_specified: Optional[int] = None |
There was a problem hiding this comment.
The model_id attribute is not required here, the self.id should be used instead.
I think it would be useful to have a default value for the restore_model_for_hours_specified, for example 24 hours.
DITO for the other methods.
There was a problem hiding this comment.
have refactored the method
ads/model/model_metadata.py
Outdated
| self, | ||
| file_path: str, | ||
| storage_options: dict = None, | ||
| self, |
| return state in (cls.PENDING, cls.SUCCEEDED, cls.FAILED) | ||
|
|
||
|
|
||
| class ModelBackupSetting: |
There was a problem hiding this comment.
The class ModelBackupSetting: can be replaced with
@dataclass(repr=False)
class ModelBackupSetting(DataClassSerializable):
is_backup_enabled: Optional[bool] = None,
backup_region: Optional[str] = None,
customer_notification_type: Optional[str] = None
def validate(self) -> bool:
pass
The DataClassSerializable is already implements bunch of the helper functions like form_dict, to_tict, from_json, e.t.c.
DITO for the other classes.
|
📌 Cov diff with main: 📌 Overall coverage: |
| "restore_model_for_hours_specified must be a positive integer." | ||
| ) | ||
|
|
||
| self.dsc_model.restore_archived_model_artifact( |
There was a problem hiding this comment.
The return self.sync() would get the updated model, in this case user would have the object containing the fresh status and details. Otherwise, after the calling this method user will have an old object.
ads/model/datascience_model.py
Outdated
| overwrite_existing_artifact: Optional[bool] = True, | ||
| remove_existing_artifact: Optional[bool] = True, | ||
| timeout: Optional[int] = None, | ||
| self, |
|
📌 Cov diff with main: 📌 Overall coverage: |
|
📌 Cov diff with main: 📌 Overall coverage: |
|
📌 Cov diff with main: 📌 Overall coverage: |
…o feature/ms_backup_retention
|
📌 Cov diff with main: 📌 Overall coverage: |
…o feature/ms_backup_retention
|
📌 Cov diff with main: 📌 Overall coverage: |
3 participants
This PR aims to update the data science model class to introduce new feature of Model Backups and retention.
Also to provide a new method/api call to restore an archived model artifact.
User can enable model backup or retention for their model while creation or while updating a model.
Both of them are Optional fields.
attaching screenshots for model creation with the 2 new fields