Skip to content
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

Add transform_fn #126

Merged
merged 2 commits into from
Jul 10, 2020
Merged

Add transform_fn #126

merged 2 commits into from
Jul 10, 2020

Conversation

edwardjkim
Copy link
Contributor

@edwardjkim edwardjkim commented Jul 7, 2020

Description of changes:

We've had customer feedback that transform_fn (where customers can combine input_fn, predict_fn and output_fn in a single function to handle inference requests) is lacking in xgboost. This PR enables the use of transform_fn in script mode.

The actual code change to enable transform_fn is minimal in _user_module_transformer() of src/sagemaker_xgboost_container/serving.py, so the review can be focused on serving.py. The rest of the changes are for adding container tests for xgboost abalone script. The sample script at test/resources/abalone/abalone_distributed.py is the same script from sagemaker examples for xgboost distributed training.

After this PR is merged, the following text will be added to the Python SDK documentation after the section on input_fn, predict_fn, and output_fn:

If you would rather not structure your code around the three methods described above, you can instead define your own transform_fn to handle inference requests. An error is thrown if a transform_fn is present in conjunction with any input_fn, predict_fn, and/or output_fn. transform_fn has the following signature:

def transform_fn(model, request_body, content_type, accept_type)

where model is the model objecte loaded by model_fn, request_body is the data from the inference request, content_type is the content type of the request, and accept_type is the request content type for the response.

This one function should handle processing the input, performing a prediction, and processing the output. The return object should have the response data and accept_type (the content type of the response data).

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Testing
New container tests in this PR.
Existing integration tests succeeded.

@edwardjkim edwardjkim marked this pull request as ready for review July 7, 2020 22:43
output_fn = getattr(user_module, "output_fn", None)
transform_fn = getattr(user_module, "transform_fn", None)

if transform_fn and (input_fn or predict_fn or output_fn):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Transformer class is already throwing a ValueError for the same condition so curious if this would be redundant.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic in this PR is structured so that the Transformer won't enter that if statement. Also, I thought it should be raised as a customer error on our side rather than a generic ValueError.

@edwardjkim edwardjkim merged commit 5a13522 into aws:master Jul 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants