Integrated Google Cloud Storage #1017

Korusuke · 2020-08-24T04:07:38Z

Co-authored-by: korusuke karan.sheth@somaiya.edu

Description

Adds Google Cloud Storage (GCS) support for storing BentoML service, this is an alternative to S3/MiniIO.

This internally changes s3_presigned_url to cloud_presigned_url, this will allow for better readability as more cloud storage can be supported in future. No existing S3 functionality is affected/changed due to this PR.

Motivation and Context

#661

How Has This Been Tested?

Manually tested
End-to-end tests are added, but they were not tested cause of storage issues😅

Types of changes

Breaking change (fix or feature that would cause existing functionality to change)
New feature and improvements (non-breaking change which adds/improves functionality)
Bug fix (non-breaking change which fixes an issue)
Code Refactoring (internal change which is not user facing)
Documentation
Test, CI, or build

Component(s) if applicable

BentoService (service definition, dependency management, API input/output adapters)
Model Artifact (model serialization, multi-framework support)
Model Server (mico-batching, dockerisation, logging, OpenAPI, instruments)
YataiService gRPC server (model registry, cloud deployment automation)
YataiService web server (nodejs HTTP server and web UI)
Internal (BentoML's own configuration, logging, utility, exception handling)
BentoML CLI

Checklist:

My code follows the bentoml code style, both ./dev/format.sh and
./dev/lint.sh script have passed
(instructions).
My change reduces project test coverage and requires unit tests to be added
I have added unit tests covering my code change
My change requires a change to the documentation
I have updated the documentation accordingly

Co-authored-by: korusuke <karan.sheth@somaiya.edu>

codecov · 2020-08-24T04:09:35Z

Codecov Report

Merging #1017 into master will decrease coverage by 0.66%.
The diff coverage is 33.69%.

@@            Coverage Diff             @@
##           master    #1017      +/-   ##
==========================================
- Coverage   63.58%   62.92%   -0.67%     
==========================================
  Files         125      130       +5     
  Lines        8262     8123     -139     
==========================================
- Hits         5253     5111     -142     
- Misses       3009     3012       +3

Impacted Files	Coverage Δ
bentoml/cli/bento_management.py	`27.11% <0.00%> (-3.92%)`	⬇️
bentoml/yatai/client/bento_repository_api.py	`69.56% <0.00%> (-3.17%)`	⬇️
bentoml/yatai/proto/repository_pb2.py	`100.00% <ø> (ø)`
bentoml/saved_bundle/loader.py	`52.67% <20.00%> (-6.21%)`	⬇️
bentoml/yatai/repository/gcs_repository.py	`28.84% <28.84%> (ø)`
bentoml/yatai/yatai_service_impl.py	`44.54% <50.00%> (+0.05%)`	⬆️
bentoml/cli/bento_service.py	`69.81% <75.00%> (-1.62%)`	⬇️
bentoml/utils/gcs.py	`75.00% <75.00%> (ø)`
bentoml/yatai/repository/repository.py	`81.81% <75.00%> (-1.52%)`	⬇️
bentoml/adapters/annotated_image_input.py	`84.90% <0.00%> (-15.10%)`	⬇️
... and 45 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7253155...606f22b. Read the comment docs.

yubozhao · 2020-08-25T21:14:56Z

Hi, @Korusuke I talked with @PrabhanshuAttri Let's add unit tests and e2e test for Google cloud storage. This is a very critical piece of Yatai service and a lot of people are depending on this. Want to be more deliberate with this.

yubozhao · 2020-08-25T21:21:50Z

bentoml/cli/bento_service.py

@@ -120,7 +121,7 @@ def resolve_bundle_path(bento, pip_installed_bundle_path):
        ), "pip installed BentoService commands should not have Bento argument"
        return pip_installed_bundle_path

-    if os.path.isdir(bento) or is_s3_url(bento):
+    if os.path.isdir(bento) or is_s3_url(bento) or is_gcs_url(bento):
        # saved_bundle already support loading local and s3 path


Update the comment to reflect this change

yubozhao · 2020-08-25T21:49:04Z

bentoml/saved_bundle/loader.py

+        if response.status_code != 200:
+            raise BentoMLException(
+                f"Error retrieving BentoService bundle. "
+                f"{response.status_code}: {response.text}"
+            )


yubozhao · 2020-08-25T21:49:51Z

bentoml/utils/gcs.py

+
+def is_gcs_url(url):
+    """
+    Check if url is an gs url


Check if the URL is a GCS URL.

yubozhao · 2020-08-25T22:02:31Z

bentoml/utils/gcs.py

+    Check if url is an gs url
+    """
+    try:
+        return urlparse(url).scheme in ["gs"]


Is gs:// the standard way for Google cloud URI? Could we document this, if you have a reference?

@yubozhao Should we put the documentation link as a comment?

Here is the documentation for the same https://cloud.google.com/storage/docs/gsutil

Ok great. Would you mind include that link in the form of comments for this funciton?

yubozhao · 2020-08-25T22:07:27Z

bentoml/yatai/client/bento_repository_api.py

+        elif response.uri.type == BentoUri.GCS:
+            self._update_bento_upload_progress(
+                bento_service_metadata, UploadStatus.UPLOADING, 0
+            )
+
+            fileobj = io.BytesIO()
+            with tarfile.open(mode="w:gz", fileobj=fileobj) as tar:
+                tar.add(saved_bento_path, arcname=bento_service_metadata.name)
+            fileobj.seek(0, 0)
+
+            http_response = requests.put(response.uri.cloud_presigned_url, data=fileobj)
+
+            if http_response.status_code != 200:
+                self._update_bento_upload_progress(
+                    bento_service_metadata, UploadStatus.ERROR
+                )
+                raise BentoMLException(
+                    f"Error saving BentoService bundle to GCS. "
+                    f"{http_response.status_code}: {http_response.text}"
+                )
+
+            self._update_bento_upload_progress(bento_service_metadata)
+
+            logger.info(
+                "Successfully saved BentoService bundle '%s:%s' to GCS: %s",
+                bento_service_metadata.name,
+                bento_service_metadata.version,
+                response.uri.uri,
+            )
+
+            return response.uri.uri
+


This seems like using the same code path as the S3 one. Let's refactor this.

@yubozhao Would you like us to merge this if condition with S3 if condition?

Yes. The only difference between those two statements is raising exceptions. We should be refactoring them

Makes sense. I have merged them, will push the changes in a while.

yubozhao · 2020-08-25T22:31:42Z

bentoml/yatai/yatai_service_impl.py

                if bento_pb.uri.type == BentoUri.S3:
-                    bento_pb.uri.s3_presigned_url = self.repo.get(
+                    bento_pb.uri.cloud_presigned_url = self.repo.get(
+                        bento_pb.name, bento_pb.version
+                    )
+                if bento_pb.uri.type == BentoUri.GCS:
+                    bento_pb.uri.cloud_presigned_url = self.repo.get(
                        bento_pb.name, bento_pb.version
                    )


Let's refactor these two.

@yubozhao As far as I can understand, we should merge these 2 if conditions. Please confirm.

Yes we should refactor these two if statement.

pep8speaks · 2020-08-28T07:13:22Z

Hello @Korusuke, Thanks for updating this PR.

There are currently no PEP 8 issues detected in this PR. Cheers! 🍻

Comment last updated at 2020-09-06 19:19:53 UTC

bentoml/saved_bundle/loader.py

bentoml/yatai/repository/gcs_repository.py

protos/repository.proto

setup.py

* Integrated Google Cloud Storage Co-authored-by: korusuke <karan.sheth@somaiya.edu> * e2e tests * Addressed PR review comments * formatting * update setup file * remove aws-sam-cli from test requirements * restore s3_prsigned_url and add gcs_presigned_url Co-authored-by: PrabhanshuAttri <contact@prabhanshu.com> Co-authored-by: yubozhao <yubz86@gmail.com>

Integrated Google Cloud Storage

cc5ff91

Co-authored-by: korusuke <karan.sheth@somaiya.edu>

Korusuke marked this pull request as ready for review August 24, 2020 04:07

parano requested a review from yubozhao August 25, 2020 02:09

yubozhao reviewed Aug 25, 2020

View reviewed changes

Korusuke and others added 2 commits August 26, 2020 21:24

e2e tests

af04a83

Addressed PR review comments

6b1a7d9

formatting

111182b

Korusuke requested a review from yubozhao August 28, 2020 08:44

update setup file

ecd114a

yubozhao previously approved these changes Sep 5, 2020

View reviewed changes

parano reviewed Sep 5, 2020

View reviewed changes

bentoml/saved_bundle/loader.py Outdated Show resolved Hide resolved

parano reviewed Sep 5, 2020

View reviewed changes

bentoml/yatai/repository/gcs_repository.py Outdated Show resolved Hide resolved

parano reviewed Sep 5, 2020

View reviewed changes

protos/repository.proto Show resolved Hide resolved

parano reviewed Sep 5, 2020

View reviewed changes

setup.py Outdated Show resolved Hide resolved

remove aws-sam-cli from test requirements

ec42c5e

yubozhao dismissed their stale review via ec42c5e September 6, 2020 06:29

restore s3_prsigned_url and add gcs_presigned_url

606f22b

parano merged commit e064ce1 into bentoml:master Sep 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrated Google Cloud Storage #1017

Integrated Google Cloud Storage #1017

Korusuke commented Aug 24, 2020

codecov bot commented Aug 24, 2020 •

edited

Loading

yubozhao commented Aug 25, 2020

yubozhao Aug 25, 2020

yubozhao Aug 25, 2020

yubozhao Aug 25, 2020

yubozhao Aug 25, 2020

PrabhanshuAttri Aug 26, 2020

yubozhao Aug 26, 2020

yubozhao Aug 25, 2020

PrabhanshuAttri Aug 26, 2020

yubozhao Aug 26, 2020

PrabhanshuAttri Aug 26, 2020

yubozhao Aug 25, 2020

PrabhanshuAttri Aug 26, 2020

yubozhao Aug 26, 2020

PrabhanshuAttri Aug 26, 2020

pep8speaks commented Aug 28, 2020 •

edited

Loading

Integrated Google Cloud Storage #1017

Integrated Google Cloud Storage #1017

Conversation

Korusuke commented Aug 24, 2020

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Component(s) if applicable

Checklist:

codecov bot commented Aug 24, 2020 • edited Loading

Codecov Report

yubozhao commented Aug 25, 2020

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

pep8speaks commented Aug 28, 2020 • edited Loading

Comment last updated at 2020-09-06 19:19:53 UTC

codecov bot commented Aug 24, 2020 •

edited

Loading

pep8speaks commented Aug 28, 2020 •

edited

Loading