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

Added SageMaker batch transform support. #317

Merged
merged 7 commits into from
Sep 26, 2019

Conversation

jaheba
Copy link
Contributor

@jaheba jaheba commented Sep 16, 2019

Issue #, if available:

Description of changes:

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

TODO:

  • add tests
  • add checks
    • ensure that the right parameters are set? (is this needed)
  • add graceful error handling

To use batch inference, one has to set the environment variable: INFERENCE_CONFIG to the right json string.

@jaheba jaheba added this to the v0.4 milestone Sep 16, 2019
@szha
Copy link
Member

szha commented Sep 16, 2019

Job PR-317/1 is complete.
Docs are uploaded to http://gluon-ts-staging.s3-accelerate.dualstack.amazonaws.com/PR-317/1/index.html

@szha
Copy link
Member

szha commented Sep 16, 2019

Job PR-317/2 is complete.
Docs are uploaded to http://gluon-ts-staging.s3-accelerate.dualstack.amazonaws.com/PR-317/2/index.html

@szha
Copy link
Member

szha commented Sep 16, 2019

Job PR-317/3 is complete.
Docs are uploaded to http://gluon-ts-staging.s3-accelerate.dualstack.amazonaws.com/PR-317/3/index.html

@codecov-io
Copy link

codecov-io commented Sep 16, 2019

Codecov Report

Merging #317 into master will increase coverage by 39.89%.
The diff coverage is 72.35%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #317       +/-   ##
===========================================
+ Coverage   40.32%   80.22%   +39.89%     
===========================================
  Files         142      144        +2     
  Lines        8024     8183      +159     
===========================================
+ Hits         3236     6565     +3329     
+ Misses       4788     1618     -3170
Impacted Files Coverage Δ
src/gluonts/shell/__main__.py 41.93% <0%> (+41.93%) ⬆️
src/gluonts/model/forecast.py 72.42% <100%> (+21.16%) ⬆️
src/gluonts/shell/serve/app.py 49.35% <49.35%> (ø)
src/gluonts/shell/testutil.py 85.88% <81.25%> (+49.51%) ⬆️
src/gluonts/shell/serve/util.py 86.66% <86.66%> (ø)
src/gluonts/shell/sagemaker/__init__.py 86.66% <87.5%> (+48.2%) ⬆️
src/gluonts/shell/serve/__init__.py 88.75% <88.75%> (ø)
src/gluonts/time_feature/__init__.py 85.71% <0%> (ø) ⬆️
... and 103 more

output_types: Set[OutputType]
quantiles: List[str] # FIXME: validate list elements
num_eval_samples: int = pydantic.Schema(100, alias="num_samples")
output_types: Set[OutputType] = {"qunatiles", "mean"}
Copy link
Contributor

Choose a reason for hiding this comment

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

Fix spelling of qunatiles to quantiles.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, thanks!

@szha
Copy link
Member

szha commented Sep 16, 2019

Job PR-317/4 is complete.
Docs are uploaded to http://gluon-ts-staging.s3-accelerate.dualstack.amazonaws.com/PR-317/4/index.html

dcmaddix
dcmaddix previously approved these changes Sep 16, 2019
vafl
vafl previously approved these changes Sep 16, 2019
Copy link
Contributor

@vafl vafl left a comment

Choose a reason for hiding this comment

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

Looks good overall.

@@ -0,0 +1,169 @@
# Copyright 2018 Amazon.com, Inc. or its affiliates. All Rights Reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we maybe put this in another file and just import the stuff we want to expose here?
This has been our convention so far regarding __init__.py.

import numpy as np


def jsonify_floats(json_object):
Copy link
Contributor

Choose a reason for hiding this comment

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

We could subclass json.JSONEncoder -- but this is fine too.

forecaster_type: Optional[Type[Union[Estimator, Predictor]]],
settings: Settings,
) -> Application:
check_gpu_support()
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess for supporting gpu's properly, we need a bit more logic for handling the number of proceses.

Also, the server should probably be started with the following environment variables to ensure that each server has one cpu and mxnet does not try to parallelize a single request.

OMP_NUM_THREADS=1
MXNET_ENGINE_TYPE=NaiveEngine

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. These changes were just moved around, but we should still do what you’ve suggested.

@jaheba jaheba dismissed stale reviews from vafl and dcmaddix via 151fadb September 19, 2019 16:45
@szha
Copy link
Member

szha commented Sep 19, 2019

Job PR-317/5 is complete.
Docs are uploaded to http://gluon-ts-staging.s3-accelerate.dualstack.amazonaws.com/PR-317/5/index.html

dcmaddix
dcmaddix previously approved these changes Sep 25, 2019
@jaheba jaheba merged commit e5ff6c5 into awslabs:master Sep 26, 2019
@jaheba jaheba deleted the batch-transform branch September 26, 2019 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants