fastai / course-v3

The 3rd edition of course.fast.ai
https://course.fast.ai/
Apache License 2.0
4.91k stars 3.57k forks source link

AWS Sagemaker Deployment Instructions Incomplete #533

Closed ehrene closed 3 years ago

ehrene commented 4 years ago

on the page https://course.fast.ai/deployment_amzn_sagemaker.html# there are missing instructions.

  1. where the PyTorchModel object is created there is not an import statement included and you will get an exception if you do not include:
from sagemaker.pytorch import PyTorchModel 
  1. The class definition for ImagePredictor is not clear and will not build the class as written.
class ImagePredictor(RealTimePredictor):
     def __init__(self, endpoint_name, sagemaker_session):
          super().__init__(endpoint_name, sagemaker_session=sagemaker_session, serializer=None, 
                         deserializer = json_deserializer, content_type='image/jpeg')

An exception is thrown that says NameError: name 'RealTimePredictor' is not defined

I believe the import statement needed here is related to the sagemaker SDK and should be:

from sagemaker.predictor import RealTimePredictor

a comment or two on this would be super helpful for those new to SageMaker's SDK. Many of these classes/methods in the sagemaker SDK are not intuitive and require poking around the docs a lot. here's a link to where the object call is referenced: https://docs.aws.amazon.com/sagemaker/latest/dg/ex1-deploy-model.html

  1. The example function script that provides the four functions for the entry point to deploy a model appears to have some syntactical errors as well. The returned values for the predict_fn are not correctly stated for a dict object to be returned. class is a reserved word in python and therefore cannot be used as stated, but more importantly the formatting for creating dict objects is 'key0': value, 'key1': value, etc. etc.. The below is incorrect and the keys need to be strings and the = needs to be a colon, i believe.
    return dict(class = str(predict_class),
        confidence = predict_values[predict_idx.item()].item())

Corrected may look like

    return dict('class':  str(predict_class),
        'confidence': predict_values[predict_idx.item()].item())
  1. it would be helpful if there was some mention of the framework_version parameter in the PyTorchModel() creation step that at least identifies the latest or references when this was created. Not a big deal, but it's not clear if it is a sagemaker thing or maybe a PyTorch thing (i'm new to PyTorch)
jgreenemi commented 3 years ago

Appears that RealTimePredictor has been renamed to simply Predictor.

Found that dict() syntax as shown in this example is valid, using = rather than : for assigning keys: https://docs.python.org/3/tutorial/datastructures.html#dictionaries Did a quick test on my own and found that both approaches do produce a valid dict.