feature: all de/serializers support content type #1993
Conversation
Add support for content_type constructor arg for all serializers, and accept constructor arg for all deserializers. This will make our de/serializers easier to re-purpose for models with specific header requirements but standard content formats.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
^ Looking at the log, I think that CI job had some bigger problem than my changeset? But happy to look at fixing it if somebody disagrees! |
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 |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
OK the second one (integration) looks like it was my bad as some tests are (incorrectly I might add!) trying to set |
And refactor some others to use SimpleBaseSerializer
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 |
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 |
Extend content_type and accept args to the RecordIO de/serializers hiding over in the sagemaker.amazon package that were missed in the initial commit.
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 |
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 |
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 |
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: #1977
Description of changes:
Add support for
content_typeconstructor argument in all providedSerializers, and foracceptconstructor argument in all providedDeserializers.This change will reduce the frequency with which users need to create custom de/serializers, by increasing the flexibility of the pre-provided ones.
For example: consider the built-in SageMaker Semantic Segmentation algorithm, where
BytesDeserializermay be appropriate for receiving animage/pngresult - but theAcceptheader must explicitly indicate that this result is preferred rather than anapplication/x-recordio-protobuf.I've strived for maximal backward compatibility in the proposed changeset, and believe the change is non-breaking:
SimpleBase*classes are introduced as a compromise between wanting to keep code DRY / implementing the API exactly once, versus not wanting to change the advertisedBase*APIs.ACCEPTandCONTENT_TYPEbacked by non-private-marked instance variablesacceptandcontent_type(rather than e.g._accept, and maybe making these lowercase names the actual read/writable properties) - I kept the pattern for consistency with existing implementations in e.g.NumpyDeserializerandIdentitySerializer.I guess one small doc change introduced is that the
NumpyDeserializerin particular used to carry a warning that multiple accept types may not work with legacy images. That warning is no longer present and it's now possible to assign multiple content types to this Deserializer... Maybe could consider adding some generic wording to everyacceptdocstring?Also added a bit of a note on the
CSVDeserializerdocstring as I only realised while making this change that theNumpyDeserializersupports additional content-types and is a good choice for text/csv->ndarray!Testing done:
Because the change is implemented via optional constructor args, no new code paths are introduced and so no new tests have been added.
I've checked the existing unit tests all pass, and have done some anecdotal experiments with a couple of classes to check they seem to work as expected.
Merge Checklist
Put an
xin the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your pull request.General
I have passed the region in to all S3 and STS clients that I've initialized as part of this change.Tests
I have added tests that prove my fix is effective or that my feature works (if appropriate)I have usedunique_name_from_baseto create resource names in integ tests (if appropriate)By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.