Skip to content

Fixing container path for source files when using local_session #499

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

Merged
merged 4 commits into from
Dec 14, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions CHANGELOG.rst
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,11 @@
CHANGELOG
=========

1.16.3
======

* bug-fix: Local Mode: No longer requires s3 permissions to run local entry point file

1.16.2
======

Expand Down
17 changes: 16 additions & 1 deletion src/sagemaker/local/image.py
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,8 @@ def train(self, input_data_config, output_data_config, hyperparameters, job_name
data_dir = self._create_tmp_folder()
volumes = self._prepare_training_volumes(data_dir, input_data_config, output_data_config,
hyperparameters)
# If local, source directory needs to be updated to mounted /opt/ml/code path
hyperparameters = self._update_local_src_path(hyperparameters, key=sagemaker.estimator.DIR_PARAM_NAME)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

an integ test is failing because it expects this hyperparameter to be JSON (at least for the SageMaker deep learning framework images)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah good catch! Updated hyperparameters with json conversion

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unable to run integration tests on my machine, so please let me know if there are any more errors on those tests


# Create the configuration files for each container that we will create
# Each container will map the additional local volumes (if any).
Expand Down Expand Up @@ -169,6 +171,9 @@ def serve(self, model_dir, environment):
parsed_uri = urlparse(script_dir)
if parsed_uri.scheme == 'file':
volumes.append(_Volume(parsed_uri.path, '/opt/ml/code'))
# Update path to mount location
environment = environment.copy()
environment[sagemaker.estimator.DIR_PARAM_NAME.upper()] = '/opt/ml/code'

if _ecr_login_if_needed(self.sagemaker_session.boto_session, self.image):
_pull_image(self.image)
Expand Down Expand Up @@ -302,7 +307,7 @@ def _prepare_training_volumes(self, data_dir, input_data_config, output_data_con
volumes.append(_Volume(data_source.get_root_dir(), channel=channel_name))

# If there is a training script directory and it is a local directory,
# mount it to the container.
# mount it to the container.
if sagemaker.estimator.DIR_PARAM_NAME in hyperparameters:
training_dir = json.loads(hyperparameters[sagemaker.estimator.DIR_PARAM_NAME])
parsed_uri = urlparse(training_dir)
Expand All @@ -321,6 +326,16 @@ def _prepare_training_volumes(self, data_dir, input_data_config, output_data_con

return volumes

def _update_local_src_path(self, params, key):
if key in params:
src_dir = json.loads(params[key])
parsed_uri = urlparse(src_dir)
if parsed_uri.scheme == 'file':
new_params = params.copy()
new_params[key] = json.dumps('/opt/ml/code')
return new_params
return params

def _prepare_serving_volumes(self, model_location):
volumes = []
host = self.hosts[0]
Expand Down
19 changes: 13 additions & 6 deletions tests/unit/test_image.py
Original file line number Diff line number Diff line change
Expand Up @@ -388,12 +388,18 @@ def test_train_local_code(tmpdir, sagemaker_session):
with open(docker_compose_file, 'r') as f:
config = yaml.load(f)
assert len(config['services']) == instance_count
for h in sagemaker_container.hosts:
assert config['services'][h]['image'] == image
assert config['services'][h]['command'] == 'train'
volumes = config['services'][h]['volumes']
assert '%s:/opt/ml/code' % '/tmp/code' in volumes
assert '%s:/opt/ml/shared' % shared_folder_path in volumes

for h in sagemaker_container.hosts:
assert config['services'][h]['image'] == image
assert config['services'][h]['command'] == 'train'
volumes = config['services'][h]['volumes']
assert '%s:/opt/ml/code' % '/tmp/code' in volumes
assert '%s:/opt/ml/shared' % shared_folder_path in volumes

config_file_root = os.path.join(sagemaker_container.container_root, h, 'input', 'config')
hyperparameters_file = os.path.join(config_file_root, 'hyperparameters.json')
hyperparameters_data = json.load(open(hyperparameters_file))
assert hyperparameters_data['sagemaker_submit_directory'] == json.dumps('/opt/ml/code')


@patch('sagemaker.local.local_session.LocalSession', Mock())
Expand Down Expand Up @@ -506,6 +512,7 @@ def test_serve_local_code(tmpdir, sagemaker_session):

volumes = config['services'][h]['volumes']
assert '%s:/opt/ml/code' % '/tmp/code' in volumes
assert 'SAGEMAKER_SUBMIT_DIRECTORY=/opt/ml/code' in config['services'][h]['environment']


@patch('sagemaker.local.image._HostingContainer.run', Mock())
Expand Down