ODSC-64654 register model artifact reference #1008
Conversation
tkg2261
requested review from
darenr,
mayoor,
mrDzurb,
VipulMascarenhas,
qiuosier and
ahosler
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
tkg2261
changed the title
oracle-contributor-agreement
bot
added
OCA Verified
| 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.
What would be the full user experience? Could you provide end-to-end example?
There was a problem hiding this comment.
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. |
tkg2261
force-pushed
the
ODSC-64654-register-model-artifact-reference
branch
3 times, most recently
from
dd3208d to
298ccc6
Compare
ODSC-64654-register-model-artifact-reference
tkg2261
force-pushed
the
ODSC-64654-register-model-artifact-reference
branch
from
298ccc6 to
7042af5
Compare
tkg2261
changed the title
| @@ -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.
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 will download_artifact() method still work?
There was a problem hiding this comment.
@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 with export_model_artifact method, which in a way does similar job.
Yes, the download_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? |
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.