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

aws lambda deployment v2 #380

Merged
merged 50 commits into from Nov 26, 2019
Merged

aws lambda deployment v2 #380

merged 50 commits into from Nov 26, 2019

Conversation

@yubozhao
Copy link
Member

yubozhao commented Nov 14, 2019

bentoml --verbose deploy create test-v2 --bento BENTO_SERVICE_BUNDLE_FROM_GET_STARTED \
--platform aws-lambda --region us-west-2 --memory-size 3008 --timeout 6 \
--s3-path s3://bo-testing-lambda-v2
curl -i \
--header "Content-Type: application/json" \
--request POST \
--data '[[5.1, 3.5, 1.4, 0.2]]' \
https://ENDPOINT_URL_FROM_CREATE_RESULT
@pep8speaks

This comment has been minimized.

Copy link

pep8speaks commented Nov 14, 2019

Hello @yubozhao, Thanks for updating this PR.

Line 139:89: E501 line too long (92 > 88 characters)

Line 26:1: E402 module level import not at top of file
Line 27:1: E402 module level import not at top of file

Line 158:5: F841 local variable 'e' is assigned to but never used
Line 160:5: F841 local variable 'e' is assigned to but never used

Comment last updated at 2019-11-26 00:00:46 UTC
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Nov 14, 2019

Codecov Report

Merging #380 into master will decrease coverage by <.01%.
The diff coverage is 53.51%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #380      +/-   ##
==========================================
- Coverage   59.07%   59.07%   -0.01%     
==========================================
  Files          81       84       +3     
  Lines        5405     5749     +344     
==========================================
+ Hits         3193     3396     +203     
- Misses       2212     2353     +141
Impacted Files Coverage Δ
bentoml/proto/deployment_pb2.py 100% <ø> (ø) ⬆️
bentoml/deployment/serverless/gcp_function.py 0% <0%> (ø) ⬆️
bentoml/yatai/python_api.py 53.47% <0%> (-0.38%) ⬇️
bentoml/deployment/sagemaker/__init__.py 63.78% <100%> (-0.62%) ⬇️
bentoml/deployment/serverless/aws_lambda.py 78.4% <100%> (ø) ⬆️
bentoml/deployment/operator.py 45.16% <25%> (-1.27%) ⬇️
bentoml/utils/s3.py 40% <30.61%> (+13.33%) ⬆️
bentoml/utils/validator/__init__.py 77.27% <37.5%> (-22.73%) ⬇️
bentoml/yatai/deployment_utils.py 81.66% <40%> (+9.6%) ⬆️
bentoml/deployment/aws_lambda/utils.py 42.63% <42.63%> (ø)
... and 7 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 698e3f1...becf5a5. Read the comment docs.

@yubozhao yubozhao force-pushed the yubozhao:use-s3-for-serverless branch 3 times, most recently from ad42ea5 to 38f4788 Nov 15, 2019
@yubozhao yubozhao marked this pull request as ready for review Nov 19, 2019
@yubozhao yubozhao requested a review from parano Nov 20, 2019
@yubozhao yubozhao force-pushed the yubozhao:use-s3-for-serverless branch from 9b1bc92 to 2eb2173 Nov 20, 2019
@yubozhao yubozhao requested a review from parano Nov 21, 2019
api_names = (
[aws_config.api_name]
if aws_config.api_name
else [api.name for api in bento_service_metadata.apis]

This comment has been minimized.

Copy link
@parano

parano Nov 21, 2019

Member

If aws_config.api_name is None and len(bento_service_metadata.apis) > 1, let's add a log warning:

logger.warning("When no api_name specified for lambda deployment, BentoML will create one lambda function for every API in BentoService by default. APIs found in target BentoService bundle are {}".format(','.join([api.name for api in bento_service_metadata.apis]))
bentoml/cli/deployment.py Outdated Show resolved Hide resolved
@yubozhao yubozhao force-pushed the yubozhao:use-s3-for-serverless branch from 9a148c5 to becf5a5 Nov 26, 2019
@yubozhao yubozhao merged commit 4910544 into bentoml:master Nov 26, 2019
1 of 3 checks passed
1 of 3 checks passed
codecov/patch 53.51% of diff hit (target 59.07%)
Details
codecov/project 59.07% (-0.01%) compared to 698e3f1
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
logger.debug('Use existing s3 bucket')
except ClientError as error:
if error.response and error.response['Error']['Code'] == 'NoSuchBucket':
logger.debug('Creating s3 bucket: {}'.format(bucket_name))

This comment has been minimized.

Copy link
@parano

parano Nov 26, 2019

Member

I believe linter will warn about this..

This comment has been minimized.

Copy link
@parano

parano Nov 26, 2019

Member

W1202: Use % formatting in logging functions and pass the % parameters as arguments (logging-format-interpolation)

This comment has been minimized.

Copy link
@parano

parano Nov 26, 2019

Member

also would be nice to add the region parameter to the log message

_cleanup_s3_bucket(bucket_name, aws_config.region)

logger.debug('Deleting Lambda function and related resources')
cf_client.delete_stack(StackName=stack_name)

This comment has been minimized.

Copy link
@parano

parano Nov 26, 2019

Member

should we check the result of delete_stack?

This comment has been minimized.

Copy link
@yubozhao

yubozhao Nov 26, 2019

Author Member

delete_stack returns None. If it has error, it will be caught on the outer layer

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.