-
Notifications
You must be signed in to change notification settings - Fork 1.2k
add explicit pull for local serve #455
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
Changes from 6 commits
c77fd6a
9b18511
cf652c2
c3d8b90
3c7c45a
06cc963
2de24fb
5af37dc
709c18a
ad331a2
849e95a
09270f2
a2226b9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -118,7 +118,9 @@ def train(self, input_data_config, output_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: | ||||
|
@@ -164,7 +166,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, | ||||
|
@@ -657,11 +660,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 | ||||
jesterhazy marked this conversation as resolved.
Show resolved
Hide resolved
|
||||
|
||||
if not boto_session: | ||||
raise RuntimeError('A boto session is required to login to ECR.' | ||||
|
@@ -677,3 +680,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 | ||||
jesterhazy marked this conversation as resolved.
Show resolved
Hide resolved
|
||||
|
||||
|
||||
def _pull_image(image): | ||||
pull_image_command = ('docker pull %s' % image).strip() | ||||
print('docker command: {}'.format(pull_image_command)) | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't this be done through the logger? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We set the log level to warning, so it ends up not showing up in jupyter :(.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. that's what log level are for. why force user to see this info-level message if they don't want to? |
||||
|
||||
subprocess.check_output(pull_image_command, shell=True) | ||||
print('image pulled: {}'.format(image)) |
Uh oh!
There was an error while loading. Please reload this page.