Added the python model download methods for S3 and GCS. #116

Tomcli · 2019-05-24T00:13:25Z

What this PR does / why we need it:
This PR implements the python model download methods for S3 and GCS. Since KFServing can provide S3 and GCS credentials as environment variables, we will be using those environment variables for user authentication.

I also added the gsutil to the docker images since we need it for accessing public GCS data artifacts.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #87

Special notes for your reviewer:

It would be nice if we can upload the current sklearn and xgboost sample models to the gs://kfserving-samples bucket, so we can update the example to use gcs and don't have to include the sample models in the docker image.

Release note:

This change is

k8s-ci-robot · 2019-05-24T00:13:40Z

Hi @Tomcli. Thanks for your PR.

I'm waiting for a kubeflow member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Tomcli · 2019-05-24T00:14:30Z

/assign @ellis-bigelow
/assign @animeshsingh

animeshsingh · 2019-05-24T00:26:42Z

python/sklearn.Dockerfile

+RUN apt-get update && apt-get install -y curl lsb-release gnupg
+RUN export CLOUD_SDK_REPO="cloud-sdk-$(lsb_release -c -s)" && echo "deb http://packages.cloud.google.com/apt $CLOUD_SDK_REPO main" | tee -a /etc/apt/sources.list.d/google-cloud-sdk.list
+RUN curl https://packages.cloud.google.com/apt/doc/apt-key.gpg | apt-key add -
+RUN apt-get update && apt-get install -y google-cloud-sdk


There is no python version for google cloud sdk - available with pip install?

There is, I used it when the user provide the GOOGLE_APPLICATION_CREDENTIALS for their own bucket. However, for public accessible data, I only can pull it using either gsutil or rest API. You can visit the details at https://cloud.google.com/storage/docs/access-public-data.

If anyone found a solution to access gcs public data using the python SDK, I would happy to implement that.

Also, the pip install version of gsutil only works on python 2.7 and our image is using python 3.7

https://pypi.org/project/google-cloud-storage/

Just to be clear over here, the google cloud python client is available on python 3.7. However, we need the gsutil command line client to access public data, and the pip install for gsutil is python 2.7 only https://cloud.google.com/storage/docs/gsutil_install#alt-install.

Therefore, I went with installing the whole google cloud command line SDK which won't depends on the local machine python version.

@ellis-bigelow is that the normal user experience on GCS? Two different SDKs based on scenarios?

I think we might be able to use https://googleapis.github.io/google-cloud-python/latest/storage/client.html#google.cloud.storage.client.Client.create_anonymous_client

Let`s jus stick to gsutil then. Less error prone

I take a look at the google.cloud.storage source code, looks like with anonymous client and .bucket() will able to get the object without credentials. I think the google cloud docs is somewhat misleading the users to use .get_bucket() which required access credentials.
https://cloud.google.com/storage/docs/downloading-objects#storage-download-object-code_sample
Also the access-public-data docs doesn't show any client SDK example except gsutil.

Let me investigate a bit more and see can I do it in pure python SDK. If not, then we can stick back with gsutil.

@ellis-bigelow So I did find a way to access public data with the Python client using the anonymous client. I reimplemented the gcs function with pure gcs python SDK now.

animeshsingh · 2019-05-24T00:27:36Z

python/kfserving/kfserving/test_storage.py

+S3_PATH = 's3://bucket/path'
+S3_ENDPOINT = ''
+AWS_ACCESS_KEY_ID = ''
+AWS_SECRET_ACCESS_KEY = ''



Is prepending with 'AWS_' required by Minio, even if its not AWS backend?

These environment variable name are based on PR #94. If we want a different environment variable name, then we have to change the go controller as well.

I think AWS_ envs work for other S3 compatible storages too.

yuzisun · 2019-05-24T02:10:48Z

python/kfserving/kfserving/storage.py

+                object_file_name = obj.object_name.replace(bucket_path, "", 1).strip("/")
+                client.fget_object(bucket_name, obj.object_name, temp_dir + "/" + object_file_name)
+        except Exception as e:
+            raise Exception(e)


what's the point of catching generic exception and rethrow the same again ?

Thanks for the review, I updated the catching with a specific exception now.

yuzisun · 2019-05-24T02:23:00Z

python/kfserving/kfserving/test_storage.py

+S3_PATH = 's3://bucket/path'
+S3_ENDPOINT = ''
+AWS_ACCESS_KEY_ID = ''
+AWS_SECRET_ACCESS_KEY = ''



I think AWS_ envs work for other S3 compatible storages too.

yuzisun · 2019-05-24T02:29:21Z

python/kfserving/kfserving/storage.py

+            url = re.compile(r"https?://")
+            minioClient = Minio(url.sub("", os.getenv("S3_ENDPOINT", "")),
+                                access_key=os.getenv("AWS_ACCESS_KEY_ID", ""),
+                                secret_key=os.getenv("AWS_SECRET_ACCESS_KEY", ""),


hmm I thought once envs are set you no longer need to pass these to minio client.. This is definitely not needed for boto but I am not sure about minio client.

I think for minio you will need to pass it as parameters. Else it will just create an anonymous client according to the docs. https://docs.min.io/docs/python-client-api-reference

yuzisun · 2019-05-24T02:30:12Z

python/kfserving/kfserving/storage.py

+        except IndexError as err:
+            raise Exception('Error: Incorrect syntax with the S3 endpoint or credentials. \n' + err)
+        except Exception as e:
+            raise Exception(e)


same question as above

I updated the catching with a specific exception now.

yuzisun · 2019-05-24T02:37:40Z

python/kfserving/kfserving/test_storage.py

+
+
+def test_private_gcs():
+    if GOOGLE_APPLICATION_CREDENTIALS:


maybe i am missing something, but looks GOOGLE_APPLICATION_CREDENTIALS is set to empty string how this branch get tested ?

Since I don't have access to kfserving's testing infrastructure, this test won't run unless someone set the global value GOOGLE_APPLICATION_CREDENTIALS.

Do the kfserving CI/CD expose GOOGLE_APPLICATION_CREDENTIALS and S3 credentials in the testing environment? If so, I can update these tests accordingly.

here we are doing unit tests not integration tests, so you can set the env in the tests.

I updated the unit test to run it when the environment variables are available.

yuzisun · 2019-05-24T02:38:07Z

python/kfserving/kfserving/test_storage.py

+
+
+def test_private_s3():
+    if S3_ENDPOINT and AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY:


same question as above

same as above

yuzisun · 2019-05-24T03:05:56Z

python/kfserving/kfserving/test_storage.py

+def test_no_permission_buckets():
+    bad_s3_path = "s3://random/path"
+    bad_gcs_path = "gs://random/path"
+    with pytest.raises(Exception):


Is there a specific exception for no permission error?

I updated the exceptions, and now they should be catching errors from minio and google.cloud.

ellistarn · 2019-05-24T16:47:52Z

python/kfserving/kfserving/storage.py

+                object_file_name = obj.object_name.replace(bucket_path, "", 1).strip("/")
+                client.fget_object(bucket_name, obj.object_name, temp_dir + "/" + object_file_name)
+        except error.NoSuchBucket as e:
+            raise error.NoSuchBucket("Bucket is not found, please double check the bucket name.")


Can we just pass the error directly?

I updated the PR now to pass errors unless it's necessary to catch it.

animeshsingh · 2019-05-24T17:31:33Z

/ok-to-test

…ead of exception raise

ellistarn · 2019-05-28T04:05:15Z

python/kfserving/kfserving/storage.py

+        bucket_path = bucket_args[1] if len(bucket_args) > 1 else ""
+        objects = client.list_objects(bucket_name, prefix=bucket_path, recursive=True)
+        for obj in objects:
+            object_file_name = obj.object_name.replace(bucket_path, "", 1).strip("/")


How about basename? https://docs.python.org/2/library/os.path.html

Maybe the name here is confusing, I renamed the variable as subdir_object_key. What we want to do over here is to replace the prefix from s3 or gcs with the temp_dir path on the local file system. I didn't use basename since we want to keep the object key with all the subdirectories after the prefix and append it to temp_dir.

Makes sense.

ellistarn · 2019-05-28T04:06:03Z

python/kfserving/kfserving/storage.py

+        objects = client.list_objects(bucket_name, prefix=bucket_path, recursive=True)
+        for obj in objects:
+            object_file_name = obj.object_name.replace(bucket_path, "", 1).strip("/")
+            client.fget_object(bucket_name, obj.object_name, temp_dir + "/" + object_file_name)


How about join? https://docs.python.org/2/library/os.path.html

python/kfserving/kfserving/storage.py

ellistarn

Thanks for the hard work Tommy! I think it's almost there.

A couple things I missed before:

Our tests shouldn't interact with the outside world.
We should use python file utilities in some of these cases. I think that might squash a few lurking bugs.

ellistarn · 2019-05-28T04:11:37Z

python/kfserving/kfserving/storage.py

+        blobs = bucket.list_blobs(prefix=bucket_path)
+        for blob in blobs:
+            object_file_name = blob.name.replace(bucket_path, "", 1).strip("/")
+            # Create necessary subdirectory


Can we use exist_ok=True
https://docs.python.org/3/library/os.html#os.makedirs

ellistarn · 2019-05-28T04:17:04Z

python/kfserving/kfserving/test_storage.py

+
+def test_public_gcs():
+    gcs_path = 'gs://kfserving-samples/models/tensorflow/flowers'
+    assert kfserving.Storage.download(gcs_path)


It's typically bad practice for unit tests to reach out into the real world like this. Can we mock the minio and gcs client instead? Otherwise we're implementing non-hermetic which can be extremely brittle to changes -- like if I delete this file.

I think we can use python MagicMock to mock the response and test different exceptions.
https://docs.python.org/3/library/unittest.mock.html

In my recent commits, I used magicmock and mock.patch to simulate some of the outputs for unit tests.

ellistarn · 2019-05-28T04:17:24Z

python/kfserving/kfserving/test_storage.py

+def test_private_gcs():
+    if os.getenv("GOOGLE_APPLICATION_CREDENTIALS", "") and os.getenv("GCS_PRIVATE_PATH", ""):
+        assert kfserving.Storage.download(os.getenv("GCS_PRIVATE_PATH", ""))
+    else:


Same comment via mocking, our tests should not turn off if environment variables aren't set.

ellistarn · 2019-05-28T23:02:36Z

Nice work!
/lgtm
Given the scope, I'll wait for another approver to give you a thumbs up as well.

animeshsingh · 2019-05-28T23:52:17Z

/lgtm

animeshsingh · 2019-05-28T23:53:16Z

@ellis-bigelow can we move the sklearn and xgboost models to default gcs account you are using for TF flower sample?

animeshsingh · 2019-05-29T00:51:02Z

they are here @ellis-bigelow
https://github.com/kubeflow/kfserving/blob/master/python/xgbserver/model.bst
https://github.com/kubeflow/kfserving/blob/master/python/sklearnserver/model.joblib

yuzisun · 2019-05-29T03:22:31Z

python/kfserving/kfserving/test_storage.py

+    bad_s3_path = "s3://random/path"
+    bad_gcs_path = "gs://random/path"
+    # Access private buckets without credentials
+    mock_minio.return_value = Minio("s3.us.cloud-object-storage.appdomain.cloud", secure=True)


I think here we need to patch urllib3.PoolManager as well to mock the exception response
mock_connection.side_effect = error.AccessDenied(None)

done, thanks for the advice.

yuzisun · 2019-05-29T03:22:34Z

python/kfserving/kfserving/test_storage.py

+    mock_minio.return_value = Minio("s3.us.cloud-object-storage.appdomain.cloud", secure=True)
+    with pytest.raises(error.AccessDenied):
+        kfserving.Storage.download(bad_s3_path)
+    with pytest.raises(exceptions.Forbidden):


yuzisun · 2019-05-29T18:30:31Z

Looks good now, thanks!

/lgtm
/approve

k8s-ci-robot · 2019-05-29T18:30:42Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: yuzisun

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [yuzisun]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

* update kfserving storage to handle s3 and gcs * update dockerfile * update exception catches * updated gcs with anonymous client trick, passing original errors instead of exception raise * update readme related to the gcs changes * update unit tests to use mock client * add mock sideeffect for exception tests

Signed-off-by: Laurent Grangeau <laurent.grangeau@gmail.com>

…erve-28 Red Hat Konflux update kserve-28

Tomcli added 2 commits May 23, 2019 17:00

update kfserving storage to handle s3 and gcs

06ac036

update dockerfile

31014db

k8s-ci-robot added the needs-ok-to-test label May 24, 2019

k8s-ci-robot added the size/L label May 24, 2019

k8s-ci-robot requested review from gaocegege and rakelkar May 24, 2019 00:14

k8s-ci-robot assigned animeshsingh and ellistarn May 24, 2019

animeshsingh reviewed May 24, 2019

View reviewed changes

yuzisun reviewed May 24, 2019

View reviewed changes

update exception catches

0ce377f

ellistarn reviewed May 24, 2019

View reviewed changes

k8s-ci-robot added ok-to-test and removed needs-ok-to-test labels May 24, 2019

updated gcs with anonymous client trick, passing original errors inst…

136c0aa

…ead of exception raise

k8s-ci-robot added size/M and removed size/L labels May 24, 2019

update readme related to the gcs changes

2befe8a

ellistarn reviewed May 28, 2019

View reviewed changes

python/kfserving/kfserving/storage.py Show resolved Hide resolved

ellistarn reviewed May 28, 2019

View reviewed changes

update unit tests to use mock client

4d8d812

k8s-ci-robot added the lgtm label May 28, 2019

yuzisun reviewed May 29, 2019

View reviewed changes

add mock sideeffect for exception tests

0462a56

k8s-ci-robot removed the lgtm label May 29, 2019

k8s-ci-robot assigned yuzisun May 29, 2019

k8s-ci-robot added the lgtm label May 29, 2019

k8s-ci-robot added the approved label May 29, 2019

k8s-ci-robot merged commit 69cc3b4 into kserve:master May 29, 2019

Tomcli mentioned this pull request Jun 5, 2019

Add Tomcli to the org member list kubeflow/internal-acls#112

Merged

Surya-smart619 pushed a commit to Surya-smart619/kserve that referenced this pull request May 11, 2022

fix: Typo in first_isvc doc (kserve#116)

600e9bc

Signed-off-by: Laurent Grangeau <laurent.grangeau@gmail.com>

terrytangyuan pushed a commit to terrytangyuan/kserve that referenced this pull request May 13, 2024

Merge pull request kserve#116 from red-hat-data-services/appstudio-ks…

1903c9e

…erve-28 Red Hat Konflux update kserve-28



		def test_private_s3():
		if S3_ENDPOINT and AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY:

Added the python model download methods for S3 and GCS. #116

Added the python model download methods for S3 and GCS. #116

Conversation

Tomcli commented May 24, 2019 • edited by jlewi Loading

k8s-ci-robot commented May 24, 2019

Tomcli commented May 24, 2019

Choose a reason for hiding this comment

Tomcli May 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tomcli May 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

animeshsingh commented May 24, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellistarn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellistarn commented May 28, 2019

animeshsingh commented May 28, 2019

animeshsingh commented May 28, 2019

animeshsingh commented May 29, 2019

yuzisun May 29, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuzisun commented May 29, 2019

k8s-ci-robot commented May 29, 2019

Tomcli commented May 24, 2019 •

edited by jlewi

Loading

Tomcli May 24, 2019 •

edited

Loading

Tomcli May 24, 2019 •

edited

Loading

yuzisun May 29, 2019 •

edited

Loading