change: add integ test for tagging #735
Conversation
tests/integ/test_tf_script_mode.py
Outdated
| @@ -122,7 +123,8 @@ def test_mnist_async(sagemaker_session): | |||
| sagemaker_session=sagemaker_session, | |||
| py_version='py3', | |||
| framework_version=TensorFlow.LATEST_VERSION, | |||
| base_job_name='test-tf-sm-mnist') | |||
| base_job_name='test-tf-sm-mnist', | |||
There was a problem hiding this comment.
use the unique name function
tests/integ/test_tf_script_mode.py
Outdated
| _assert_endpoint_tags_match(sagemaker_session.sagemaker_client, predictor.endpoint, TAGS) | ||
| _assert_model_tags_match(sagemaker_session.sagemaker_client, estimator.latest_training_job.name, TAGS) | ||
|
|
||
| _assert_training_job_tags_match(sagemaker_session.sagemaker_client, estimator.latest_training_job.name, TAGS) |
There was a problem hiding this comment.
move this up above with the training part of the test
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
There is only test with that base name in test_server_side_encryption. i guess we can have multiple pr builds at the same time and still have collision. I will fix it. |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
| @@ -49,7 +50,7 @@ def test_mnist(sagemaker_session, instance_type): | |||
| py_version='py3', | |||
| framework_version=TensorFlow.LATEST_VERSION, | |||
| metric_definitions=[{'Name': 'train:global_steps', 'Regex': r'global_step\/sec:\s(.*)'}], | |||
| base_job_name='test-tf-sm-mnist') | |||
| base_job_name=unique_name_from_base('test-tf-sm-mnist')) | |||
There was a problem hiding this comment.
this won't work... need to remove base_job_name, and use unique name as job_name arg to fit function. otherwise the base_job_name may be truncated before standard timestamp is added, and you end up with duplicate names again.
There was a problem hiding this comment.
oops. i missed this comments. I will make another pr to fix this.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
icywang86rui
dismissed
jesterhazy’s stale review
The comments are already addressed
Issue #, if available:
Description of changes:
Merge Checklist
Put an
xin 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.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.