Skip to content

[Feature] Offer accept argument in all Deserializers #1977

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 29, 2020 · 2 comments
Closed

[Feature] Offer accept argument in all Deserializers #1977

athewsey opened this issue Oct 29, 2020 · 2 comments

Comments

@athewsey
Copy link
Collaborator

Describe the feature you'd like

Ability to customize Accept header for as many Deserializers as possible, but in particular the "identity" deserializers like BytesDeserializer and StreamDeserializer.

Sometimes we need to explicitly specify an Accept, but don't want the SageMaker SDK to do any processing beyond loading the bytestream.

In my particular case, I noticed that SDKv2 is materially harder to use than v1 with the built-in Semantic Segmentation algorithm - where we used to be able to simply:

# SDK v1 code:
ss_predictor.content_type = 'image/jpeg'
ss_predictor.accept = 'image/png'
return_img = ss_predictor.predict(img)

Now I had to do the below to get the code working:

# Eww:
class MyPNGDeserializer(sagemaker.deserializers.BytesDeserializer):
    ACCEPT = ('image/png',)

ss_predictor.serializer = sagemaker.serializers.IdentitySerializer('image/jpeg')
ss_predictor.accept = MyPNGDeserializer()
return_img = ss_predictor.predict(img)

(Need to specify the Accept type for this algorithm, because default appears not to be PNG)

How would this feature be used? Please describe.

Directly re-use existing deserializers while being able to specify particular Accept:

dictor.serializer = sagemaker.serializers.IdentitySerializer('image/jpeg')
ss_predictor.accept = sagemaker.deserializers.BytesDeserializer('image/png')
return_img = ss_predictor.predict(img)

Describe alternatives you've considered

Perhaps this is a deliberate omission, to encourage users to get familiar with implementing proper Deserializers rather than doing it after the predictor.predict() call? I could appreciate the rationale for that, it just doesn't feel very beginner-friendly.

Additional context

N/A

@metrizable
Copy link
Contributor

Hello @athewsey ,

Thank you for using Amazon SageMaker. Great suggestion. When I read the beginning of your issue, I thought the same thing myself re: passing in an accept to the constructor or, say, an associated classmethod.

Your current use of the implementation is correct, although the UX, as you noted, can objectively be improved. We are always re-evaluating our backlog of features based on customer requests, so we appreciate the feedback on this feature.

@ajaykarpur
Copy link
Contributor

Merged in #1993

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