Add sagemaker integration tests #13
Conversation
andremoeller
left a comment
andremoeller
left a comment
There was a problem hiding this comment.
LGTM, just a few things to check though
| predictor = pytorch.deploy(initial_instance_count=1, instance_type=instance_type) | ||
|
|
||
| batch_size = 100 | ||
| data = np.zeros(shape=(batch_size, 1, 28, 28)) |
There was a problem hiding this comment.
You might need dtype='float32' if you're planning on using NPY (or else these will be deserialized as float64)
There was a problem hiding this comment.
I am currently always creating FloatTensors when deserializing:
https://github.com/aws/sagemaker-pytorch-containers/blob/master/src/pytorch_container/serving.py#L37
|
|
||
|
|
||
| @pytest.fixture(name='ecr_image', scope='session') | ||
| def fixture_ecr_image(docker_registry, docker_base_name, tag): |
There was a problem hiding this comment.
this will run sagemaker integ tests with repo sagemaker-pytorch, right? but i think integ tests push to preprod-pytorch. i'm not sure if this will run against the image pushed after local tests.
There was a problem hiding this comment.
that's a good point!
test/integration/__init__.py
Outdated
| model_gpu_dir = os.path.join(mnist_path, 'model_gpu') | ||
| model_gpu_1d_dir = os.path.join(model_gpu_dir, '1d') | ||
|
|
||
| ENTRYPOINT = ["python", "-m", "pytorch_container.start"] |
There was a problem hiding this comment.
Is this needed? The entrypoint is in the container anyway, right? https://github.com/aws/sagemaker-pytorch-containers/blob/master/docker/0.3.1/final/py3/Dockerfile.gpu#L7
There was a problem hiding this comment.
I guess it is not, I just never really thought about it since we were planning to switch to the proper local mode.
|
|
||
| @pytest.fixture(scope='session') | ||
| def docker_base_name(request): | ||
| @pytest.fixture(scope='session', name='docker_base_name') |
There was a problem hiding this comment.
Just asking: why is this new way preferred?
There was a problem hiding this comment.
To avoid pylint's warning about "Shadows name from outer scope".
http://blog.pytest.org/2016/whats-new-in-pytest-30/ Fixture name parameter:
"This solves the problem where the function argument shadows the argument name, which annoys pylint and might cause bugs if one forgets to pull a fixture into a test function."
pytorch 1.3.1 changes
2 participants
Adding sagemaker integration tests
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.