Add integration tests to run training jobs with sagemaker #81
Conversation
test/conftest.py
Outdated
| @@ -36,6 +36,8 @@ def pytest_addoption(parser): | |||
| parser.addoption('--framework-version', default='1.10.0') | |||
| parser.addoption('--processor', default='cpu', choices=['gpu', 'cpu']) | |||
| parser.addoption('--py-version', default='3', choices=['2', '3']) | |||
| parser.addoption('--ecr-image', default=None) | |||
There was a problem hiding this comment.
seems like this redundant with --docker-base-name and --tag - do we really need all of these?
There was a problem hiding this comment.
I think I can use tag for the functional tests and add --account-id instead of --ecr-image. That seems better.
|
|
||
| @pytest.mark.skip_gpu | ||
| def test_mnist_cpu(sagemaker_session, ecr_image, instance_type): | ||
| instance_type = instance_type or 'ml.c4.xlarge' |
There was a problem hiding this comment.
is it possible to have this defaulting behavior in the fixture definition?
There was a problem hiding this comment.
yeah. I can move it there.
|
|
||
| @pytest.mark.skip_cpu | ||
| def test_mnist_gpu(sagemaker_session, ecr_image, instance_type): | ||
| _run_minist_training(sagemaker_session, ecr_image, instance_type) |
There was a problem hiding this comment.
is there still a need to have these separate? won't a test run just use the appropriate instance type?
There was a problem hiding this comment.
yeah, you are right. I will merge it.
There was a problem hiding this comment.
are you planning more tests that will use this _run_mnist_training function?
|
|
||
| @pytest.mark.skip_cpu | ||
| def test_mnist_gpu(sagemaker_session, ecr_image, instance_type): | ||
| _run_minist_training(sagemaker_session, ecr_image, instance_type) |
There was a problem hiding this comment.
are you planning more tests that will use this _run_mnist_training function?
| _run_minist_training(sagemaker_session, ecr_image, instance_type) | ||
|
|
||
|
|
||
| def _run_minist_training(sagemaker_session, ecr_image, instance_type): |
There was a problem hiding this comment.
I thought about just move this to the test but kept it because I plan to add distributed training tests later. And tests for creating endpoint and inference will be added too.
There was a problem hiding this comment.
Actually this function probably will need refactoring anyway. i will merge this in the test for now.
* Add mnist sagemaker tests * Use account-id instead of ecr-image * Merge gpu and cpu sagemaker tests * remove _run_mnist_training
* Add mnist sagemaker tests * Use account-id instead of ecr-image * Merge gpu and cpu sagemaker tests * remove _run_mnist_training
* Add mnist sagemaker tests * Use account-id instead of ecr-image * Merge gpu and cpu sagemaker tests * remove _run_mnist_training
Issue #, if available:
Description of changes:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.