search

bentoml / aws-sagemaker-deploy

Fast model deployment on AWS Sagemaker
Apache License 2.0
15 stars 15 forks source link

Fixed bug in parameter argv count check #19

Closed cliu0507 closed 3 years ago

cliu0507 commented 3 years ago

Fix the bug in parameter count check in delete/describle/update/deploy scripts.

Reason: config_json is optional and already has a default "sagemaker_config.json" to use. This change will fix the 'raise Exception errors 'issue if users use above scripts without setting config_json. Below is an example, ideally it should NOT raise a exception error. 

Eligible command:
python delete.py my-first-sagemaker-deployment

Unexpected error message:
raise Exception("Please provide deployment name and API name")
cliu0507 commented 3 years ago

@yubozhao @eob @larme @parano

jjmachan commented 3 years ago

Thanks a lot @cliu0507 for taking the time and putting up this PR but currently we were trying to set up argparse for handling command line arguments (Ref: https://github.com/bentoml/aws-ec2-deploy/blob/f1c03c9c43183b2b3cf5b7cb61a006f609ce578e/deploy.py#L143). This is a better approach than using the command line args directly.

If you are interested you can contribute the fix using argparse and that would really help out the users and we would really appreciate the PR too. What do you say?

cliu0507 commented 3 years ago

@jjmachan Sure. Using argparse is definitely a better approach! I will contribute the fix with argparse. I will close this PR

jjmachan commented 3 years ago

Thanks a lot, highly appreciate this 😄 .