Repository: bentoml/BentoML
Pull request: [Enhancement]Add remote path support for save_to_dir function
<!--- Thanks for sending a pull request! Please make sure to read the contribution guidelines, then fill out the blanks below before requesting a code review. -->

## Description
<!--- Describe your changes in detail -->
<!--- Attach screenshots here if appropriate. -->
Add S3 and Google Cloud Storage support for `save_to_dir` function

```python
# Save to S3
save_to_dir(my_bento_service, path='s3://bucket_name/path/to/save')

# Save to GCS
save_to_dir(my_bento_service, path='gs://bucket_name/path/to/save')
```

## Motivation and Context
<!--- Why is this change required? What problem does it solve? -->
<!--- If it fixes an open issue, please link to the issue here. -->
<!--- If it is based on a conversation in slack channel, pls quote related messages here -->

## How Has This Been Tested?
<!--- Please describe in detail how you tested your changes. -->
<!--- Include details of your testing environment, and the tests you ran to -->
<!--- see how your change affects other areas of the code, etc. -->

## Types of changes
<!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: -->
- [ ] 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
<!--- [ ] Others: describe the type of change if it does not fit to categories listed -->

## 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:
<!--- Go over all the following points, and put an `x` in all the boxes that apply. -->
<!--- If you're unsure about any of these, don't hesitate to ask. We're here to help! -->
<!--- If you plan to update documentation or tests in follow-up, please note -->
- [ ] My code follows the bentoml code style, both `./dev/format.sh` and
  `./dev/lint.sh` script have passed
  ([instructions](https://github.com/bentoml/BentoML/blob/master/DEVELOPMENT.md#style-check-and-auto-formatting-your-code)).
- [ ] 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


Problem statement:
`bentoml get` fails for one-off "save to s3" bundle
**Describe the bug**

```bash
$ bentoml get BENTO_TAG

BentoService BENTO_TAG not found - INTERNAL:Bento BENTO_TAG not found in target repository
```

**To Reproduce**
Steps to reproduce the behavior:
1. Caa BentoService#save with s3 path
```
# update last line in guide/quick-start/main.py to 
# saved_path = iris_classifier_service.save('s3://my-test-bucket/')
$ python guide/quick-start/main.py
```
2. run `bentoml get IrisClassifier:latest`

**Expected behavior**
Expect CLI to display relevant information for the saved BentoService

**Environment:**
 - OS: [e.g. MacOS 10.15.5]
 - Python/BentoML Version [e.g. Python 3.7.3, BentoML-0.7.7]

Hints from issue discussion:
Problem is CLI will initialize a YataiService with LocalBentoRepo, when getting information for a BentoService using s3 storage. It will use the LocalBentoRepo instead of the S3 one
Note: `.save("s3://..")` is a legacy behavior, when the `path` parameter is provided, bentoml should assume the user does not want to use the model management feature but only the model packaging and API server feature. BentoML should actually avoid saving this BentoService to the DB as well. The YataiService is designed to use with only one type of storage system. It would be nice to give the user a better error message.

We should probably add a warning to `BentoService#save` method, when the `path` is provided there, tell the user:
```python
logger.warning("""
Executing one-off save to path '{path}', the saved bundle can be loaded with `bentoml.load(path)`
and API server can be started with CLI command `bentoml serve {path}`. But the saved bundle
will not appear in the `bentoml list` command output and will not be stored in the YataiService model registry.
""")
```


This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@yubozhao would be great to think about this in the upcoming YataiService push/pull related changes
cc @yubozhao 

1. Recommend `save(yatai_url=...)`, raise exception when using `save(path)`, ask user to use `save_to_dir` instead
2. `save_to_dir(file_path)` 👈 add support for s3 path

Commit message: log additional info

Patch to review:
diff --git a/bentoml/saved_bundle/bundler.py b/bentoml/saved_bundle/bundler.py
index cb24b561b74..4c369a36099 100644
--- a/bentoml/saved_bundle/bundler.py
+++ b/bentoml/saved_bundle/bundler.py
@@ -16,11 +16,13 @@
 import shutil
 import stat
 import logging
-
+import tarfile
+from urllib.parse import urlparse
 
 from bentoml.configuration import _is_pip_installed_bentoml
 
 from bentoml.exceptions import BentoMLException
+from bentoml.saved_bundle.loader import _is_remote_path
 from bentoml.saved_bundle.local_py_modules import copy_local_py_modules
 from bentoml.saved_bundle.templates import (
     BENTO_SERVICE_BUNDLE_SETUP_PY_TEMPLATE,
@@ -28,6 +30,8 @@
     MODEL_SERVER_DOCKERFILE_CPU,
     INIT_PY_TEMPLATE,
 )
+from bentoml.utils import is_s3_url, is_gcs_url
+from bentoml.utils.tempdir import TempDirectory
 from bentoml.utils.usage_stats import track_save
 from bentoml.saved_bundle.config import SavedBundleConfig
 
@@ -49,7 +53,9 @@ def save_to_dir(bento_service, path, version=None, silent=False):
     is not empty, this call may override existing files in the given path.
 
     :param bento_service (bentoml.service.BentoService): a Bento Service instance
-    :param path (str): Destination of where the bento service will be saved
+    :param path (str): Destination of where the bento service will be saved. The
+        destination can be local path or remote path. The remote path supports both
+        AWS S3('s3://bucket/path') and Google Cloud Storage('gs://bucket/path').
     :param version (str): Override the service version with given version string
     :param silent (boolean): whether to hide the log message showing target save path
     """
@@ -61,6 +67,18 @@ def save_to_dir(bento_service, path, version=None, silent=False):
         raise BentoMLException(
             "save_to_dir only works with instances of custom BentoService class"
         )
+    remote_save_path = None
+    if _is_remote_path(path):
+        # If user provided path is an remote location, the bundle will first save to
+        # a temporary directory and then upload to the remote location
+        logger.info(
+            'Saving bento to an remote path. BentoML will first save the bento '
+            'to a local temporary directory and then upload to the remote path.'
+        )
+        remote_save_path = path
+        temp_dir = TempDirectory()
+        temp_dir.create()
+        path = temp_dir.path
 
     if version is not None:
         # If parameter version provided, set bento_service version
@@ -187,12 +205,16 @@ def save_to_dir(bento_service, path, version=None, silent=False):
     bundled_pip_dependencies_path = os.path.join(path, 'bundled_pip_dependencies')
     _bundle_local_bentoml_if_installed_from_source(bundled_pip_dependencies_path)
 
+    # Upload saved bento bundle from temp directory to the remote location
+    if remote_save_path is not None:
+        _upload_bento_to_remote_path(remote_save_path, path, bento_service.name)
+
     if not silent:
         logger.info(
             "BentoService bundle '%s:%s' created at: %s",
             bento_service.name,
             bento_service.version,
-            path,
+            remote_save_path if remote_save_path is not None else path,
         )
 
 
@@ -241,3 +263,38 @@ def _bundle_local_bentoml_if_installed_from_source(target_path):
 
         # clean up sdist build files
         shutil.rmtree(source_dir)
+
+
+def _upload_bento_to_remote_path(remote_path, bento_path, bento_name):
+    with TempDirectory() as temp_dir:
+        file_name = f'{bento_name}.tar'
+        tarfile_path = f'{temp_dir}/{file_name}'
+        with tarfile.open(tarfile_path, mode="w:gz") as tar:
+            tar.add(bento_path, arcname=bento_name)
+
+        parsed_url = urlparse(remote_path)
+        bucket_name = parsed_url.netloc
+        object_prefix_path = parsed_url.path.lstrip('/')
+        object_path = f'{object_prefix_path}/{file_name}'
+        if is_s3_url(remote_path):
+            try:
+                import boto3
+            except ImportError:
+                raise BentoMLException(
+                    '"boto3" package is required for saving bento to AWS S3 bucket'
+                )
+            s3_client = boto3.client('s3')
+            with open(tarfile_path, 'rb') as f:
+                s3_client.upload_fileobj(f, bucket_name, object_path)
+        elif is_gcs_url(remote_path):
+            try:
+                from google.cloud import storage
+            except ImportError:
+                raise BentoMLException(
+                    '"google.cloud" package is required for saving bento to Google '
+                    'Cloud Storage'
+                )
+            gcs_client = storage.Client()
+            bucket = gcs_client.bucket(bucket_name)
+            blob = bucket.blob(object_path)
+            blob.upload_from_filename(tarfile_path)
diff --git a/bentoml/saved_bundle/loader.py b/bentoml/saved_bundle/loader.py
index c4207b3be7f..34262568488 100644
--- a/bentoml/saved_bundle/loader.py
+++ b/bentoml/saved_bundle/loader.py
@@ -40,7 +40,7 @@ def _is_http_url(bundle_path):
 
 
 def _is_remote_path(bundle_path):
-    return (
+    return isinstance(bundle_path, str) and (
         is_s3_url(bundle_path) or is_gcs_url(bundle_path) or _is_http_url(bundle_path)
     )
 
diff --git a/bentoml/yatai/client/bento_repository_api.py b/bentoml/yatai/client/bento_repository_api.py
index 8878165b84c..da1ec013104 100644
--- a/bentoml/yatai/client/bento_repository_api.py
+++ b/bentoml/yatai/client/bento_repository_api.py
@@ -191,7 +191,7 @@ def upload_from_dir(self, saved_bento_path, labels=None):
                 http_response = requests.put(
                     response.uri.s3_presigned_url, data=fileobj
                 )
-            else:
+            elif response.uri.uri == BentoUri.GCS:
                 http_response = requests.put(
                     response.uri.gcs_presigned_url, data=fileobj
                 )


Code review comments:
diff_hunk: @@ -49,7 +53,9 @@ def save_to_dir(bento_service, path, version=None, silent=False):
     is not empty, this call may override existing files in the given path.
 
     :param bento_service (bentoml.service.BentoService): a Bento Service instance
-    :param path (str): Destination of where the bento service will be saved
+    :param path (str): Destination of where the bento service will be saved. The
path: bentoml/saved_bundle/bundler.py
text: @user1:
let's just add another method `save_to_tar`, and leave the upload to the user?

Otherwise you would also add support for s3/gcs in `load_from_dir` right?  It's quite confusing when `save_to_dir` is actually creating a tarball and upload the tarball to an object under the given s3 path, which is inconsistent with the saving to local directory behavior

@author:
We actually already support loading from tarball in s3/gcs with `load_from_dir`. 

The inconsistent between saving to a local path or an remote path is not very desiring

@user1:
Add http location here as well then to make it consistent?

diff_hunk: @@ -61,6 +67,18 @@ def save_to_dir(bento_service, path, version=None, silent=False):
         raise BentoMLException(
             "save_to_dir only works with instances of custom BentoService class"
         )
+    remote_save_path = None
path: bentoml/saved_bundle/bundler.py
text: @user1:
 can you refactor this to make it cleaner?  I think we should be able to avoid this