change: default model_fn and predict_fn in default handler #51
Conversation
chuyang-deng
changed the title
| with torch.no_grad(): | ||
| output = model(input_data) | ||
| if os.getenv(INFERENCE_ACCELERATOR_PRESENT_ENV) == 'true': | ||
| logger.info( |
There was a problem hiding this comment.
I noticed that in MXNet, there isn't any logged message indicating that EI is used. But, I think it is useful to have a logging statement like this since then the customer can definitively know that they're using EI. Wanted to point out the discrepancy, but I am onboard with this change
https://github.com/aws/sagemaker-mxnet-serving-container/blob/master/src/sagemaker_mxnet_serving_container/default_inference_handler.py#L62
There was a problem hiding this comment.
Logging per-inference is a lot of logging. May be ok for DEBUG level but I'd suggest removing the log.
There was a problem hiding this comment.
I agree that logging with each inference request is unnecessary, so I removed them.
| logger.info("Saving the model to {}.".format(model_dir)) | ||
| path = os.path.join(model_dir, 'model.pth') | ||
| torch.jit.save(model, path) | ||
| # This file is intentionally left blank |
There was a problem hiding this comment.
nit: Maybe clarify that this is to allow the default model_fn and predict_fn to be used
There was a problem hiding this comment.
added more descriptive comments.
src/sagemaker_pytorch_serving_container/default_inference_handler.py
Outdated
Show resolved
Hide resolved
src/sagemaker_pytorch_serving_container/default_inference_handler.py
Outdated
Show resolved
Hide resolved
src/sagemaker_pytorch_serving_container/default_inference_handler.py
Outdated
Show resolved
Hide resolved
src/sagemaker_pytorch_serving_container/default_inference_handler.py
Outdated
Show resolved
Hide resolved
src/sagemaker_pytorch_serving_container/default_inference_handler.py
Outdated
Show resolved
Hide resolved
| with torch.no_grad(): | ||
| output = model(input_data) | ||
| if os.getenv(INFERENCE_ACCELERATOR_PRESENT_ENV) == 'true': | ||
| logger.info( |
There was a problem hiding this comment.
Logging per-inference is a lot of logging. May be ok for DEBUG level but I'd suggest removing the log.
| @@ -31,10 +43,18 @@ def default_model_fn(self, model_dir): | |||
|
|
|||
There was a problem hiding this comment.
Should the docstrings be updated?
There was a problem hiding this comment.
docstring updated
src/sagemaker_pytorch_serving_container/default_inference_handler.py
Outdated
Show resolved
Hide resolved
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Issue #, if available:
Description of changes:
implement default model_fn and predict_fn when the container is an eia container.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.