feature: Support multiple accept types #61

bveeramani · 2020-07-28T18:16:23Z

Issue #, if available:

Description of changes:

Add support for Accept headers with multiple MIME types

Testing done:

Added a test to test_utils, and updated a test in test_default_inference_handler

Merge Checklist

Put an x in 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 read the CONTRIBUTING doc
I used the commit message format described in CONTRIBUTING
I have used the regional endpoint when creating S3 and/or STS clients (if appropriate)
I have updated any necessary documentation, including READMEs

Tests

I have added tests that prove my fix is effective or that my feature works (if appropriate)
I have checked that my tests are not configured for a specific region or account (if appropriate)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

src/sagemaker_inference/encoder.py

bveeramani · 2020-07-28T18:18:10Z

src/sagemaker_inference/default_inference_handler.py

+        for content_type in utils.parse_accept(accept):
+            if content_type in encoder.SUPPORTED_CONTENT_TYPES:
+                return encoder.encode(prediction, content_type), content_type
+        return encoder.encode(prediction, content_types.JSON), content_types.JSON


I've implemented this in default_inference_handler, but I could've just as easily put it in transformer.

Also, could use recommendations on how to more robustly test this behavior

I've implemented this in default_inference_handler, but I could've just as easily put it in transformer.

I think it makes more sense here - that way, users can still override it if they want to for some reason

bveeramani · 2020-07-28T18:26:10Z

test/unit/test_default_inference_handler.py

+@pytest.mark.parametrize(
+    "accept, expected_content_type",
+    [
+        ("text/csv", "text/csv"),
+        ("text/csv, application/json", "text/csv"),
+        ("unsupported/type, text/csv", "text/csv"),
+        ("unsupported/type", "application/json"),
+    ],
+)


Not sure if I should split this up into separate functions. Benefit would be that I could use descriptive test names describing the intent of the test, but adding more functions might be unnecessarily verbose.

what split were you considering?

Separate test for each pair of accept and expected_content_type arguments.

From https://docs.microsoft.com/en-us/dotnet/core/testing/unit-testing-best-practices, the scenario under test and the expected behavior should be obvious from the name. But with a parameterized test, we're testing multiple scenarios. Not sure how applicable these practices are for Python, though

I think what you have here is fine

sagemaker-bot · 2020-07-28T18:35:45Z

AWS CodeBuild CI Report

CodeBuild project: sagemaker-inference-toolkit-pr
Commit ID: 310e9cb
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

bveeramani · 2020-07-28T20:32:34Z

src/sagemaker_inference/default_inference_handler.py

+        for content_type in utils.parse_accept(accept):
+            if content_type in encoder.SUPPORTED_CONTENT_TYPES:
+                return encoder.encode(prediction, content_type), content_type
+        return encoder.encode(prediction, content_types.JSON), content_types.JSON


Also, not sure if content_types.JSON should be a named constant like DEFAULT_CONTENT_TYPE. If so, which module would I put it in?

I think it'd make more sense to raise an error with having an unsupported content type - presumably if we're at this point in the code, JSON wasn't one of the supported content types

basically the same logic in encode: https://github.com/aws/sagemaker-inference-toolkit/blob/master/src/sagemaker_inference/encoder.py#L107

laurenyu · 2020-07-29T00:19:37Z

src/sagemaker_inference/utils.py

+        (list): A list containing the MIME types that the client is able to
+            understand.
+    """
+    return accept.split(", ")


is it possible it might be comma-delimited without spaces?

I couldn't find anything about that in the specification https://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html, but in all of the examples they use commas with spaces. Also, we would be encoding the Accept header with the Python SDK for most use cases (I think), in which case we would use spaces.

I went and looked at a bunch of Stack Overflow posts and Mozilla's documentation, and it does seem like most people use a space. However, https://developer.mozilla.org/en-US/docs/Glossary/quality_values (linked from this page) seems to imply that the space isn't always needed 🤷‍♀️

laurenyu · 2020-07-29T00:20:43Z

src/sagemaker_inference/default_inference_handler.py

+        for content_type in utils.parse_accept(accept):
+            if content_type in encoder.SUPPORTED_CONTENT_TYPES:
+                return encoder.encode(prediction, content_type), content_type
+        return encoder.encode(prediction, content_types.JSON), content_types.JSON


I've implemented this in default_inference_handler, but I could've just as easily put it in transformer.

I think it makes more sense here - that way, users can still override it if they want to for some reason

laurenyu · 2020-07-29T00:22:46Z

src/sagemaker_inference/default_inference_handler.py

+        for content_type in utils.parse_accept(accept):
+            if content_type in encoder.SUPPORTED_CONTENT_TYPES:
+                return encoder.encode(prediction, content_type), content_type
+        return encoder.encode(prediction, content_types.JSON), content_types.JSON


I think it'd make more sense to raise an error with having an unsupported content type - presumably if we're at this point in the code, JSON wasn't one of the supported content types

basically the same logic in encode: https://github.com/aws/sagemaker-inference-toolkit/blob/master/src/sagemaker_inference/encoder.py#L107

src/sagemaker_inference/encoder.py

laurenyu · 2020-07-29T00:25:24Z

test/unit/test_default_inference_handler.py

+@pytest.mark.parametrize(
+    "accept, expected_content_type",
+    [
+        ("text/csv", "text/csv"),
+        ("text/csv, application/json", "text/csv"),
+        ("unsupported/type, text/csv", "text/csv"),
+        ("unsupported/type", "application/json"),
+    ],
+)


what split were you considering?

sagemaker-bot · 2020-07-29T17:19:12Z

AWS CodeBuild CI Report

CodeBuild project: sagemaker-inference-toolkit-pr
Commit ID: ad7e583
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

laurenyu · 2020-07-29T17:39:45Z

src/sagemaker_inference/utils.py

+        (list): A list containing the MIME types that the client is able to
+            understand.
+    """
+    return accept.replace(" ", "").split(",")


(optional) I have no opinion on which is more optimal/Pythonic, but figured I'd throw it out there:

[s.strip() for s in accept.split(",")]

Support multiple accept

62d8325

bveeramani commented Jul 28, 2020

View reviewed changes

src/sagemaker_inference/encoder.py Show resolved Hide resolved

bveeramani commented Jul 28, 2020

View reviewed changes

Update test_default_inference_handler.py

310e9cb

bveeramani commented Jul 28, 2020

View reviewed changes

bveeramani changed the title ~~[WIP] feature: Support multiple accept types~~ feature: Support multiple accept types Jul 28, 2020

bveeramani commented Jul 28, 2020

View reviewed changes

laurenyu reviewed Jul 29, 2020

View reviewed changes

Balaji Veeramani added 3 commits July 29, 2020 11:57

Address review comments

22653c8

Update test_default_inference_handler.py

81b80fb

Address review comments

ad7e583

laurenyu approved these changes Jul 29, 2020

View reviewed changes

bveeramani merged commit b205d06 into aws:master Jul 29, 2020

bveeramani deleted the support-multiple-accept branch July 29, 2020 17:55

feature: Support multiple accept types #61

feature: Support multiple accept types #61

Uh oh!

Conversation

bveeramani commented Jul 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge Checklist

General

Tests

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sagemaker-bot commented Jul 28, 2020

AWS CodeBuild CI Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sagemaker-bot commented Jul 29, 2020

AWS CodeBuild CI Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bveeramani commented Jul 28, 2020 •

edited

Loading