Skip to content

Predictor constructors should accept de/serializer overrides #1958

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

Closed
athewsey opened this issue Oct 16, 2020 · 1 comment
Closed

Predictor constructors should accept de/serializer overrides #1958

athewsey opened this issue Oct 16, 2020 · 1 comment

Comments

@athewsey
Copy link
Collaborator

Describe the feature you'd like

Today, nearly all framework *Predictor classes (with the exception of TensorFlowPredictor - yay for TF!) forcibly set the serializer and deserializer.

While implementing an appropriate default behaviour for each framework is great, it would be much nicer if classes like PyTorchPredictor implemented the same de/serializer constructor options as the base Predictor - and just defaulted these values when not provided.

How would this feature be used? Please describe.

Instead of the currently required:

predictor = PyTorchPredictor(endpoint_name)
predictor.serializer = sagemaker.serializers.CSVSerializer()
predictor.deserializer = sagemaker.deserializers.CSVDeserializer()

User would be able across all frameworks (like they currently can for TensorFlow) to:

predictor = PyTorchPredictor(
    endpoint_name,
    serializer=sagemaker.serializers.CSVSerializer(),
    deserializer=sagemaker.deserializers.CSVDeserializer(),
)

...which may seem minor, but is important because the main API documentation only covers methods, not properties: So it's not actually obvious to new users that modifying the value of predictor.serializer is supported.

It would also provide an opportunity to explicitly note what the default de/serialization is in each framework's docs - which would bring useful clarity!

Describe alternatives you've considered

N/A

Additional context

Add any other context or screenshots about the feature request here.

@ajaykarpur
Copy link
Contributor

Merged in #1997

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants