[WIP/Draft] Add upload_artifact_and_register_model method to python client
#761
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Crazyglue
force-pushed
the
feat/python-upload-and-register
branch
2 times, most recently
from
fb011b2 to
a603953
Compare
…eholder functions Signed-off-by: Eric Dobroveanu <edobrove@redhat.com>
Crazyglue
force-pushed
the
feat/python-upload-and-register
branch
from
a603953 to
8090a03
Compare
Signed-off-by: Eric Dobroveanu <edobrove@redhat.com>
Crazyglue
force-pushed
the
feat/python-upload-and-register
branch
from
409964d to
6ebe337
Compare
There was a problem hiding this comment.
thank you for sharing this as a draft @Crazyglue !
I think this will be helpful to ease the work of the MLOps eng, DS persona, who typically need to operate these capabilities otherwise manually.
Any general thought of a plug-in mechanism if people want to override an implementation?
ie we are likely to offer a default with boto3 as an extra for the _upload_to_s3(), what mechanism do we want to suggest to other Devs who may want to implement their own tooling for S3 ? (for future extensibility, doens't have to be in this PR)
| if is_s3_uri(destination_uri): | ||
| self._upload_to_s3(artifact_local_path, destination_uri, upload_client_params['region_name']) | ||
| elif is_oci_uri(destination_uri): | ||
| self._upload_to_oci(artifact_local_path, destination_uri) |
There was a problem hiding this comment.
I concur the flow is decided based on destination; just thinking we could leverage here functions to check the required configuration parameters (ie Env variables) are present
There was a problem hiding this comment.
I was thinking that the individual upload methods would be checking for those, as they would instantiate their respective clients (boto3 vs skopeo).
I haven't done it yet in this PR, but the upload_client_params mapping will actually need to contain either the boto3 params OR the skopeo params, and that mapping is passed to both upload methods. Then those methods can determine whether it has enough information to instantiate a client and connect/perform the upload
To be honest I'm not familiar enough with Python to know the "pythonic" way of doing this, haha. but perhaps we can have a kwarg to provide a boto3 or skopeo client as an optional param to this method. if provided, use it, else instantiate a client. after client creation/resolution we can perform the upload. it might be worth considering another option, lower level to the above, which is to create a separate @tarilabs any thoughts on what the right level of abstraction might be in this case? if the two storage options are just s3 and oci, then im not sure the added complexity is worth it |
|
|
||
| def _upload_to_oci( | ||
| self, | ||
| artifact_local_path: str, |
There was a problem hiding this comment.
One thing to consider here, is how to pass/customize the "base image" of the modelcar being built. Do we want that to be an explicit parameter, or a default from a config which could be overridden if needed ?
considering this is to support an internal implementation which likely will need further iterations/refinement, I wouldn't "prematurely overengineer it" :) |
Description
Adds a convenience method to perform a common operation within the python client, to both upload and register a model that exists locally on the filesystem.
How Has This Been Tested?
Merge criteria:
DCOcheck)If you have UI changes