feat(py): Add support for storing models in S3 - [DRAFT] #765
Conversation
Signed-off-by: syntaxsdev <[email protected]>
|
[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 |
google-oss-prow
bot
requested review from
andreyvelich,
tarilabs and
zijianjoy
There was a problem hiding this comment.
thank you @syntaxsdev for this!
some initial comments below.
which type of test can we consider to make sure the functionality is covered?
I'm thinking we could have some dedicated e2e test by extending the current opt-in pytest mechanism and deploy minio in that "scenario" of e2e testing. Do you have some additional ideas?
| secret_access_key=secret_access_key, | ||
| ) | ||
| try: | ||
| s3.upload_file(file, bucket_name, name) |
There was a problem hiding this comment.
see above comment amount "file" vs "path" (don't recall if there is any native boto3 api to do for the folder)
There was a problem hiding this comment.
not understanding what you meant by this, but I have renamed the parameter to path.
If you give it a relative location it will resolve the file
There was a problem hiding this comment.
it was in relation if it uploads recursively or need an explicit orchestration
There was a problem hiding this comment.
It does not. See here: #765 (comment)
|
|
||
| def save_to_s3( | ||
| self, | ||
| file: str, |
There was a problem hiding this comment.
I presume if this is a path, it uploads recursively the path contents.
Can we confirm this, and describe it also in the pydoc?
There was a problem hiding this comment.
No, it only uploads a singular file due to how upload_file works. Are you suggesting writing a wrapper to achieve recursive path uploads?
There was a problem hiding this comment.
see in some of the tutorials how we show usage of S3 for multiple files in a bucket, wdyt?
There was a problem hiding this comment.
what exactly are you referring to?
if you are referring to this then that's not exactly what I was talking about.
Afaik, boto3 S3 does not have a multiple upload definition or allow recursive. uploads, we'd have to build that.
that's not a problem - the issue is do we want to add that built into this method and if so, see #765 (comment)
There was a problem hiding this comment.
what exactly are you referring to?
tutorials (of ODH, but also other projects) which show how to persist multiple files in the identified bucket; sorry if I was not clear
…sting with minio locally Signed-off-by: syntaxsdev <[email protected]>
syntaxsdev
force-pushed
the
feat/store-on-s3-py
branch
from
176b6a8 to
afdc8cf
Compare
| os.remove(model_file.name) | ||
|
|
||
|
|
||
| @pytest.fixture |
There was a problem hiding this comment.
no scope added here because MonkeyPatch needs to use a function scope and default is function so its omitted
| @@ -623,3 +629,44 @@ def test_hf_import_default_env(client: ModelRegistry): | |||
|
|
|||
| for k in env_values: | |||
| os.environ.pop(k) | |||
|
|
|||
|
|
|||
| @pytest.mark.dd | |||
Within the Python client, users will be able to directly store models to an S3 compatible object storage.
[DRAFT]
Description
The bulk of the changes were done in
clients/python/src/_client.pyHow Has This Been Tested?
Merge criteria:
DCOcheck)If you have UI changes