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

Add tests for sagemaker and lambda deployments #357

Merged
merged 32 commits into from Nov 8, 2019

Conversation

@yubozhao
Copy link
Member

yubozhao commented Oct 21, 2019

No description provided.

@pep8speaks

This comment has been minimized.

Copy link

pep8speaks commented Oct 21, 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-11-08 01:38:39 UTC
tests/deployment/test_utils.py Outdated Show resolved Hide resolved
"API_NAME": api_name,
"BENTO_SERVER_TIMEOUT": config().get('apiserver', 'default_timeout'),
"BENTO_SERVER_WORKERS": config().get(
'apiserver', 'default_gunicorn_workers_count'

This comment has been minimized.

Copy link
@parano

parano Oct 22, 2019

Member

this is configured on Yatai server but it's not where the workload is running, let's revisit this later, I believe this should be a user-provided parameter when creating sagemaker deployment, and if it's not there we simply don't set this env var, I believe in that case, we will use the bentoml.server.utils.get_bento_recommend_gunicorn_worker_count to get this value

@yubozhao yubozhao force-pushed the yubozhao:deployment-tests branch from 8499706 to 0c741e9 Oct 22, 2019
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Oct 22, 2019

Codecov Report

Merging #357 into master will increase coverage by 5.23%.
The diff coverage is 91.13%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #357      +/-   ##
==========================================
+ Coverage   53.74%   58.97%   +5.23%     
==========================================
  Files          79       81       +2     
  Lines        5303     5380      +77     
==========================================
+ Hits         2850     3173     +323     
+ Misses       2453     2207     -246
Impacted Files Coverage Δ
bentoml/deployment/serverless/aws_lambda.py 78.99% <100%> (+78.99%) ⬆️
bentoml/deployment/serverless/serverless_utils.py 60.46% <100%> (+60.46%) ⬆️
bentoml/archive/archiver.py 96% <100%> (ø) ⬆️
bentoml/deployment/utils.py 59.52% <100%> (+3.07%) ⬆️
bentoml/deployment/sagemaker/__init__.py 65.41% <85%> (+45.07%) ⬆️
bentoml/archive/utils.py 96.29% <96.29%> (ø)
bentoml/handlers/base_handlers.py 72.5% <0%> (-3.18%) ⬇️
bentoml/artifact/keras_model_artifact.py 22.4% <0%> (ø) ⬆️
bentoml/artifact/xgboost_model_artifact.py 40% <0%> (ø) ⬆️
bentoml/utils/__init__.py 89.28% <0%> (ø) ⬆️
... and 14 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 5930657...d8d6ea0. Read the comment docs.

@yubozhao yubozhao marked this pull request as ready for review Oct 23, 2019
client = boto3.client('sagemaker', 'us-west-2')
stubber = Stubber(client)

def test_sagemaker_handle_client_errors(sagemaker_client):

This comment has been minimized.

Copy link
@parano

parano Nov 7, 2019

Member

this is enssentially a unit test for function _parse_aws_client_exception_or_raise, make it simpler without the mock staff:

err = ClientError(
           error_response={
               'Error': {'Code': 'MyCode', 'Message': 'MyMessage'}},
           operation_name='myoperation'
         )
result = _parse_aws_client_exception_or_raise(err)
assert result....

else:
logger.error(error_log_message)
raise e

This comment has been minimized.

Copy link
@parano

parano Nov 7, 2019

Member

Realized an issue when reading through the tests, here it should raise an Exception that is inherited from BentoMLException, only that way the RPC server can return a response with INTERNAL ERROR code I believe

raise BentoMLException(
            'Error interacting with AWS service: {}'.format(str(e))
        )
@@ -40,7 +39,9 @@
SERVERLESS_BIN_COMMAND = '{}/node_modules/.bin/serverless'.format(BENTOML_HOME)


def check_nodejs_comptaible_version():
def check_nodejs_compatible_version():
from bentoml.utils.whichcraft import which

This comment has been minimized.

Copy link
@parano

parano Nov 7, 2019

Member

could you test if the tests pass now without the change? would love to understand why this change was made

yubozhao added 3 commits Nov 7, 2019
@yubozhao yubozhao requested a review from parano Nov 7, 2019
fix
yubozhao added 2 commits Nov 8, 2019
@parano parano merged commit 851bac4 into bentoml:master Nov 8, 2019
3 checks passed
3 checks passed
codecov/patch 91.13% of diff hit (target 53.74%)
Details
codecov/project 58.97% (+5.23%) compared to 5930657
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.