From c77fd6ac0044fe5e0cd56ea4b280967bf16139e5 Mon Sep 17 00:00:00 2001 From: Dan Choi Date: Thu, 1 Nov 2018 17:56:13 -0700 Subject: [PATCH 1/8] add explicit pull for local serve --- src/sagemaker/local/image.py | 10 ++++++++++ tests/unit/test_image.py | 15 ++++++++++++++- 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/src/sagemaker/local/image.py b/src/sagemaker/local/image.py index 67f488fae4..8eea77258a 100644 --- a/src/sagemaker/local/image.py +++ b/src/sagemaker/local/image.py @@ -668,3 +668,13 @@ def _ecr_login_if_needed(boto_session, image): cmd = "docker login -u AWS -p %s %s" % (token, ecr_url) subprocess.check_output(cmd, shell=True) + + return True + + +def _pull_image(image): + pull_image_command = ('docker pull %s' % image).strip() + print('docker command: {}'.format(pull_image_command)) + + subprocess.check_output(pull_image_command, shell=True) + print('image pulled: {}'.format(image)) \ No newline at end of file diff --git a/tests/unit/test_image.py b/tests/unit/test_image.py index ac4cab8ae2..7e70e13a5e 100644 --- a/tests/unit/test_image.py +++ b/tests/unit/test_image.py @@ -513,13 +513,26 @@ def test_ecr_login_needed(check_output): } session_mock.client('ecr').get_authorization_token.return_value = response image = '520713654638.dkr.ecr.us-east-1.amazonaws.com/image-i-need:1.1' - sagemaker.local.image._ecr_login_if_needed(session_mock, image) + result = sagemaker.local.image._ecr_login_if_needed(session_mock, image) expected_command = 'docker login -u AWS -p %s https://520713654638.dkr.ecr.us-east-1.amazonaws.com' % token check_output.assert_called_with(expected_command, shell=True) session_mock.client('ecr').get_authorization_token.assert_called_with(registryIds=['520713654638']) + assert result + + +@patch('subprocess.check_output', return_value=''.encode('utf-8')) +def test_pull_image(check_output): + image = '520713654638.dkr.ecr.us-east-1.amazonaws.com/image-i-need:1.1' + + sagemaker.local.image._pull_image(image) + + expected_command = 'docker pull %s' % image + + check_output.assert_called_once_with(expected_command, shell=True) + def test__aws_credentials_with_long_lived_credentials(): credentials = Credentials(access_key=_random_string(), secret_key=_random_string(), token=None) From 9b18511e8903b4f45fabedcf35260f79a4727776 Mon Sep 17 00:00:00 2001 From: Dan Choi Date: Thu, 1 Nov 2018 17:59:11 -0700 Subject: [PATCH 2/8] update changelog --- CHANGELOG.rst | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index dd29ed26fa..4585220227 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -2,6 +2,11 @@ CHANGELOG ========= +1.13.1dev +====== + +* enhancement: Local Mode: add explicit pull for serving + 1.13.0 ====== From cf652c285ca12fc0cc3c78c6126a1551b249df92 Mon Sep 17 00:00:00 2001 From: Dan Choi Date: Fri, 2 Nov 2018 11:25:28 -0700 Subject: [PATCH 3/8] newline --- src/sagemaker/local/image.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sagemaker/local/image.py b/src/sagemaker/local/image.py index 8eea77258a..ac0a2099d9 100644 --- a/src/sagemaker/local/image.py +++ b/src/sagemaker/local/image.py @@ -677,4 +677,4 @@ def _pull_image(image): print('docker command: {}'.format(pull_image_command)) subprocess.check_output(pull_image_command, shell=True) - print('image pulled: {}'.format(image)) \ No newline at end of file + print('image pulled: {}'.format(image)) From c3d8b90015ad47ce266a45391c514e8e39ae9d4a Mon Sep 17 00:00:00 2001 From: Dan Choi Date: Fri, 2 Nov 2018 14:32:50 -0700 Subject: [PATCH 4/8] actually call it --- src/sagemaker/local/image.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/sagemaker/local/image.py b/src/sagemaker/local/image.py index ac0a2099d9..d1001409f5 100644 --- a/src/sagemaker/local/image.py +++ b/src/sagemaker/local/image.py @@ -115,7 +115,9 @@ def train(self, input_data_config, hyperparameters, job_name): additional_env_vars=training_env_vars) compose_command = self._compose() - _ecr_login_if_needed(self.sagemaker_session.boto_session, self.image) + if _ecr_login_if_needed(self.sagemaker_session.boto_session, self.image): + _pull_image(self.image) + process = subprocess.Popen(compose_command, stdout=subprocess.PIPE, stderr=subprocess.STDOUT) try: @@ -167,7 +169,8 @@ def serve(self, model_dir, environment): if parsed_uri.scheme == 'file': volumes.append(_Volume(parsed_uri.path, '/opt/ml/code')) - _ecr_login_if_needed(self.sagemaker_session.boto_session, self.image) + if _ecr_login_if_needed(self.sagemaker_session.boto_session, self.image): + _pull_image(self.image) self._generate_compose_file('serve', additional_env_vars=environment, From 3c7c45a5ea8ef600e8659fbc601a9688a30294b2 Mon Sep 17 00:00:00 2001 From: Dan Choi Date: Sun, 4 Nov 2018 22:46:10 -0800 Subject: [PATCH 5/8] return False --- src/sagemaker/local/image.py | 4 ++-- tests/unit/test_image.py | 8 +++++--- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/src/sagemaker/local/image.py b/src/sagemaker/local/image.py index d1001409f5..480dcd04ac 100644 --- a/src/sagemaker/local/image.py +++ b/src/sagemaker/local/image.py @@ -651,11 +651,11 @@ def _write_json_file(filename, content): def _ecr_login_if_needed(boto_session, image): # Only ECR images need login if not ('dkr.ecr' in image and 'amazonaws.com' in image): - return + return False # do we have the image? if _check_output('docker images -q %s' % image).strip(): - return + return False if not boto_session: raise RuntimeError('A boto session is required to login to ECR.' diff --git a/tests/unit/test_image.py b/tests/unit/test_image.py index 7e70e13a5e..9aea48aae3 100644 --- a/tests/unit/test_image.py +++ b/tests/unit/test_image.py @@ -473,9 +473,10 @@ def test_prepare_serving_volumes_with_local_model(sagemaker_session): def test_ecr_login_non_ecr(): session_mock = Mock() - sagemaker.local.image._ecr_login_if_needed(session_mock, 'ubuntu') + result = sagemaker.local.image._ecr_login_if_needed(session_mock, 'ubuntu') session_mock.assert_not_called() + assert result is False @patch('sagemaker.local.image._check_output', return_value='123451324') @@ -483,10 +484,11 @@ def test_ecr_login_image_exists(_check_output): session_mock = Mock() image = '520713654638.dkr.ecr.us-east-1.amazonaws.com/image-i-have:1.0' - sagemaker.local.image._ecr_login_if_needed(session_mock, image) + result = sagemaker.local.image._ecr_login_if_needed(session_mock, image) session_mock.assert_not_called() _check_output.assert_called() + assert result is False @patch('subprocess.check_output', return_value=''.encode('utf-8')) @@ -520,7 +522,7 @@ def test_ecr_login_needed(check_output): check_output.assert_called_with(expected_command, shell=True) session_mock.client('ecr').get_authorization_token.assert_called_with(registryIds=['520713654638']) - assert result + assert result is True @patch('subprocess.check_output', return_value=''.encode('utf-8')) From 709c18af5c311c95cb5af2d3244423679ef482bf Mon Sep 17 00:00:00 2001 From: Dan Choi Date: Fri, 16 Nov 2018 16:52:04 -0800 Subject: [PATCH 6/8] change print to log --- src/sagemaker/local/image.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/sagemaker/local/image.py b/src/sagemaker/local/image.py index 46a536d256..03a36c1b87 100644 --- a/src/sagemaker/local/image.py +++ b/src/sagemaker/local/image.py @@ -659,11 +659,11 @@ def _write_json_file(filename, content): def _ecr_login_if_needed(boto_session, image): # Only ECR images need login if not ('dkr.ecr' in image and 'amazonaws.com' in image): - return False + return # do we have the image? if _check_output('docker images -q %s' % image).strip(): - return False + return if not boto_session: raise RuntimeError('A boto session is required to login to ECR.' @@ -685,7 +685,7 @@ def _ecr_login_if_needed(boto_session, image): def _pull_image(image): pull_image_command = ('docker pull %s' % image).strip() - print('docker command: {}'.format(pull_image_command)) + logger.info('docker command: {}'.format(pull_image_command)) subprocess.check_output(pull_image_command, shell=True) - print('image pulled: {}'.format(image)) + logger.info('image pulled: {}'.format(image)) From 849e95a5fe02609b75d60bc36e66ad9ef5899ebe Mon Sep 17 00:00:00 2001 From: Dan Choi Date: Mon, 19 Nov 2018 13:54:57 -0800 Subject: [PATCH 7/8] update unit tests --- tests/unit/test_image.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/tests/unit/test_image.py b/tests/unit/test_image.py index 5a2cbb39fe..f2a7b7fec9 100644 --- a/tests/unit/test_image.py +++ b/tests/unit/test_image.py @@ -537,10 +537,9 @@ def test_prepare_serving_volumes_with_local_model(get_data_source_instance, sage def test_ecr_login_non_ecr(): session_mock = Mock() - result = sagemaker.local.image._ecr_login_if_needed(session_mock, 'ubuntu') + sagemaker.local.image._ecr_login_if_needed(session_mock, 'ubuntu') session_mock.assert_not_called() - assert result is False @patch('sagemaker.local.image._check_output', return_value='123451324') @@ -548,11 +547,10 @@ def test_ecr_login_image_exists(_check_output): session_mock = Mock() image = '520713654638.dkr.ecr.us-east-1.amazonaws.com/image-i-have:1.0' - result = sagemaker.local.image._ecr_login_if_needed(session_mock, image) + sagemaker.local.image._ecr_login_if_needed(session_mock, image) session_mock.assert_not_called() _check_output.assert_called() - assert result is False @patch('subprocess.check_output', return_value=''.encode('utf-8')) From 09270f25de98ee7fce421721bcdc7ab6defdf9f5 Mon Sep 17 00:00:00 2001 From: Dan Choi Date: Mon, 19 Nov 2018 14:40:43 -0800 Subject: [PATCH 8/8] add returns --- src/sagemaker/local/image.py | 4 ++-- tests/unit/test_image.py | 6 ++++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/sagemaker/local/image.py b/src/sagemaker/local/image.py index 03a36c1b87..8416e47048 100644 --- a/src/sagemaker/local/image.py +++ b/src/sagemaker/local/image.py @@ -659,11 +659,11 @@ def _write_json_file(filename, content): def _ecr_login_if_needed(boto_session, image): # Only ECR images need login if not ('dkr.ecr' in image and 'amazonaws.com' in image): - return + return False # do we have the image? if _check_output('docker images -q %s' % image).strip(): - return + return False if not boto_session: raise RuntimeError('A boto session is required to login to ECR.' diff --git a/tests/unit/test_image.py b/tests/unit/test_image.py index f2a7b7fec9..5a2cbb39fe 100644 --- a/tests/unit/test_image.py +++ b/tests/unit/test_image.py @@ -537,9 +537,10 @@ def test_prepare_serving_volumes_with_local_model(get_data_source_instance, sage def test_ecr_login_non_ecr(): session_mock = Mock() - sagemaker.local.image._ecr_login_if_needed(session_mock, 'ubuntu') + result = sagemaker.local.image._ecr_login_if_needed(session_mock, 'ubuntu') session_mock.assert_not_called() + assert result is False @patch('sagemaker.local.image._check_output', return_value='123451324') @@ -547,10 +548,11 @@ def test_ecr_login_image_exists(_check_output): session_mock = Mock() image = '520713654638.dkr.ecr.us-east-1.amazonaws.com/image-i-have:1.0' - sagemaker.local.image._ecr_login_if_needed(session_mock, image) + result = sagemaker.local.image._ecr_login_if_needed(session_mock, image) session_mock.assert_not_called() _check_output.assert_called() + assert result is False @patch('subprocess.check_output', return_value=''.encode('utf-8'))