Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enhance serverless deployment #321

Merged
merged 12 commits into from Oct 2, 2019

Conversation

@yubozhao
Copy link
Member

commented Oct 1, 2019

(Thanks for sending a pull request! Please make sure to read the contribution guidelines, then fill out the blanks below.)

What changes were proposed in this pull request?

Does this close any currently open issues?

How was this patch tested?

@codecov-io

This comment has been minimized.

Copy link

commented Oct 1, 2019

Codecov Report

Merging #321 into master will decrease coverage by 0.21%.
The diff coverage is 11.11%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #321      +/-   ##
==========================================
- Coverage   54.94%   54.72%   -0.22%     
==========================================
  Files          75       75              
  Lines        5018     5047      +29     
==========================================
+ Hits         2757     2762       +5     
- Misses       2261     2285      +24
Impacted Files Coverage Δ
bentoml/deployment/serverless/gcp_function.py 0% <0%> (ø) ⬆️
bentoml/deployment/serverless/aws_lambda.py 0% <0%> (ø) ⬆️
bentoml/deployment/serverless/serverless_utils.py 0% <0%> (ø) ⬆️
bentoml/exceptions.py 100% <100%> (ø) ⬆️
bentoml/deployment/utils.py 66.66% <100%> (+2.38%) ⬆️

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 5a7d2a4...627b709. Read the comment docs.

@@ -131,6 +132,11 @@ def generate_handler_py(bento_name, apis, output_path):

class AwsLambdaDeploymentOperator(DeploymentOperatorBase):
def apply(self, deployment_pb, repo, prev_deployment=None):
if which('docker') is None:
raise BentoMLMissingDepdencyException(

This comment has been minimized.

Copy link
@parano

parano Oct 1, 2019

Member

this only test if docker client is installed, it may fail to connect to docker daemon, or docker daemon has not been started yet, let's add a common method for checking if docker is available?

This comment has been minimized.

Copy link
@yubozhao

yubozhao Oct 1, 2019

Author Member

yeah, lets do that

This comment has been minimized.

Copy link
@parano

parano Oct 1, 2019

Member

one potential way to do that is run 'docker info' and parse the output

This comment has been minimized.

Copy link
@yubozhao

yubozhao Oct 1, 2019

Author Member

yeah, I was reading their documentation, that's the recommend way to test. Also, I tested locally. Using subprocess.check_output with docker info if Docker is not running, it will throw exception

@yubozhao yubozhao force-pushed the yubozhao:enhance-serverless-deployment branch from 38bc53b to e09ed91 Oct 1, 2019
@pep8speaks

This comment has been minimized.

Copy link

commented Oct 1, 2019

Hello @yubozhao, Thanks for updating this PR.

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

Comment last updated at 2019-10-02 00:42:17 UTC
yubozhao added 4 commits Oct 1, 2019
@parano parano added the LGTM label Oct 2, 2019
@yubozhao yubozhao merged commit 626d835 into bentoml:master Oct 2, 2019
1 of 3 checks passed
1 of 3 checks passed
codecov/patch 11.11% of diff hit (target 54.94%)
Details
codecov/project 54.72% (-0.22%) compared to 5a7d2a4
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.