- Notifications
You must be signed in to change notification settings - Fork50
ODSC-64654 register model artifact reference#1008
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Thank you for your pull request and welcome to our community! To contribute, please sign the Oracle Contributor Agreement (OCA).
To sign the OCA, pleasecreate an Oracle account and sign the OCA inOracle'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 mainOracle GitHub organization, and yourmembership in this organization is public. |
| The ``.restore_model()`` method of Model catalog restores the model for a specified number of hours. Restored models can be downloaded for 1-240 hours, defaulting to 24 hours. |
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.
What would be the full user experience? Could you provide end-to-end example?
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.
Hi@mrDzurb this is an existing piece of code, my code starts below this, don't know why the diff is showing this as changed.
@tkg2261 please add PR description. |
dd3208d to298ccc6CompareODSC-64654-register-model-artifact-reference
298ccc6 to7042af5Compare| @@ -1405,6 +1405,26 @@ def restore_model( | |||
| restore_model_for_hours_specified=restore_model_for_hours_specified, | |||
| ) | |||
| def register_model_artifact_reference(self,bucket_uri_list: List[str]) -> None: | |||
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.
Just curious, since we already have the.with_artifact() logic, why introduce a new method? Wouldn't it be better to enhance the existing one? It's a well-known method, and users are already familiar with it. Also willdownload_artifact() method still work?
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.
@mrDzurb.with_artifact() method already does model-by-ref but using uploadArtifact api. We want to keep that flow too for now, till we completely enable and transition to registerArtifact API. I didn't want to introduce any breaking change, that's why have NOT modified existing method. Moreover,register_model_artifact_reference as a separate method is also consistent withexport_model_artifact method, which in a way does similar job.
Yes, thedownload_artifact method will still work, it will download the json configuration file uploaded by the registerArtifact api.
@tkg2261 changes look fine but unit tests are failing. Could you please fix them so that we can approve and merge? |
Thank you for your pull request and welcome to our community! To contribute, please sign the Oracle Contributor Agreement (OCA).
To sign the OCA, pleasecreate an Oracle account and sign the OCA inOracle'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 mainOracle GitHub organization, and yourmembership in this organization is public. |
| DataScienceWorkRequest(work_request_id).wait_work_request( | ||
| progress_bar_description="Registering model artifact references." | ||
| ) | ||
| logger.info("Artifact references registered successfully.") |
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.
change this to debug
Uh oh!
There was an error while loading.Please reload this page.
This Pull Request is for adding new API for Model by Reference, i.e. register_model_artifact_reference in DataScienceModel in ADS.
The new API will enable users to simply give us the object storage locations of their existing artifacts as reference for using against a newly created DataScienceModel, instead of having to upload or export artifacts which is a costly operation.
API Spec :-https://confluence.oci.oraclecorp.com/pages/viewpage.action?spaceKey=ODSC&title=API+changes+for+Model+store+for+Aqua
It has been tested in my local environment - Attached Screenshots. One screenshot is for successful scenario, and another is for one of the failure scenarios to demonstrate that in case of failure, we are fetching the work request logs and showing the same in ADS logs.
One screenshot below these is how the new changes in model_catalog.rst will look like.