From 0c89d5694ed32cad827a4d23b57ec37c3d1852aa Mon Sep 17 00:00:00 2001 From: Jonathan Esterhazy Date: Wed, 21 Nov 2018 16:32:35 -0800 Subject: [PATCH 1/4] fix FileNotFoundError when user provides an entry_point but no source_dir --- src/sagemaker/fw_utils.py | 28 ++++++++++++++------------- tests/unit/test_fw_utils.py | 38 ++++++++++++++++++++++++++++++++++++- 2 files changed, 52 insertions(+), 14 deletions(-) diff --git a/src/sagemaker/fw_utils.py b/src/sagemaker/fw_utils.py index dde632c477..76bbf6ab35 100644 --- a/src/sagemaker/fw_utils.py +++ b/src/sagemaker/fw_utils.py @@ -131,27 +131,29 @@ def tar_and_upload_dir(session, bucket, s3_key_prefix, script, directory, depend Returns: sagemaker.fw_utils.UserCode: An object with the S3 bucket and key (S3 prefix) and script name. """ - dependencies = dependencies or [] - key = '%s/sourcedir.tar.gz' % s3_key_prefix - if directory and directory.lower().startswith('s3://'): return UploadedCode(s3_prefix=directory, script_name=os.path.basename(script)) - else: - tmp = tempfile.mkdtemp() - try: - source_files = _list_files_to_compress(script, directory) + dependencies - tar_file = sagemaker.utils.create_tar_file(source_files, os.path.join(tmp, _TAR_SOURCE_FILENAME)) + script_name = script if directory else os.path.basename(script) + dependencies = dependencies or [] + key = '%s/sourcedir.tar.gz' % s3_key_prefix + tmp = tempfile.mkdtemp() - session.resource('s3').Object(bucket, key).upload_file(tar_file) - finally: - shutil.rmtree(tmp) + try: + source_files = _list_files_to_compress(script, directory) + dependencies + tar_file = sagemaker.utils.create_tar_file(source_files, os.path.join(tmp, _TAR_SOURCE_FILENAME)) - script_name = script if directory else os.path.basename(script) - return UploadedCode(s3_prefix='s3://%s/%s' % (bucket, key), script_name=script_name) + session.resource('s3').Object(bucket, key).upload_file(tar_file) + finally: + shutil.rmtree(tmp) + + return UploadedCode(s3_prefix='s3://%s/%s' % (bucket, key), script_name=script_name) def _list_files_to_compress(script, directory): + if directory is None: + return [script] + basedir = directory if directory else os.path.dirname(script) return [os.path.join(basedir, name) for name in os.listdir(basedir)] diff --git a/tests/unit/test_fw_utils.py b/tests/unit/test_fw_utils.py index 0cd328e462..1c482a5e57 100644 --- a/tests/unit/test_fw_utils.py +++ b/tests/unit/test_fw_utils.py @@ -19,6 +19,7 @@ import pytest from mock import Mock, patch +from contextlib import contextmanager from sagemaker import fw_utils from sagemaker.utils import name_from_image @@ -30,6 +31,14 @@ TIMESTAMP = '2017-10-10-14-14-15' +@contextmanager +def cd(path): + old_dir = os.getcwd() + os.chdir(path) + yield + os.chdir(old_dir) + + @pytest.fixture() def sagemaker_session(): boto_mock = Mock(name='boto_session', region_name=REGION) @@ -132,7 +141,7 @@ def test_validate_source_dir_file_not_in_dir(): def test_tar_and_upload_dir_not_s3(sagemaker_session): - bucket = 'mybucker' + bucket = 'mybucket' s3_key_prefix = 'something/source' script = os.path.basename(__file__) directory = os.path.dirname(os.path.abspath(inspect.getfile(inspect.currentframe()))) @@ -166,6 +175,33 @@ def test_tar_and_upload_dir_no_directory(sagemaker_session, tmpdir): assert {'/train.py'} == list_source_dir_files(sagemaker_session, tmpdir) +def test_tar_and_upload_dir_no_directory_only_entrypoint(sagemaker_session, tmpdir): + source_dir = file_tree(tmpdir, ['train.py', 'not_me.py']) + entrypoint = os.path.join(source_dir, 'train.py') + + with patch('shutil.rmtree'): + result = fw_utils.tar_and_upload_dir(sagemaker_session, 'bucket', 'prefix', entrypoint, None) + + assert result == fw_utils.UploadedCode(s3_prefix='s3://bucket/prefix/sourcedir.tar.gz', + script_name='train.py') + + assert {'/train.py'} == list_source_dir_files(sagemaker_session, tmpdir) + + +def test_tar_and_upload_dir_no_directory_bare_filename(sagemaker_session, tmpdir): + source_dir = file_tree(tmpdir, ['train.py']) + entrypoint = 'train.py' + + with patch('shutil.rmtree'): + with cd(source_dir): + result = fw_utils.tar_and_upload_dir(sagemaker_session, 'bucket', 'prefix', entrypoint, None) + + assert result == fw_utils.UploadedCode(s3_prefix='s3://bucket/prefix/sourcedir.tar.gz', + script_name='train.py') + + assert {'/train.py'} == list_source_dir_files(sagemaker_session, tmpdir) + + def test_tar_and_upload_dir_with_directory(sagemaker_session, tmpdir): file_tree(tmpdir, ['src-dir/train.py']) source_dir = os.path.join(str(tmpdir), 'src-dir') From af35bbec927cb29aa3aac9bb5ca06e719c4aec5f Mon Sep 17 00:00:00 2001 From: Jonathan Esterhazy Date: Wed, 21 Nov 2018 17:28:30 -0800 Subject: [PATCH 2/4] update docstring --- src/sagemaker/fw_utils.py | 33 +++++++++++++++++++++------------ 1 file changed, 21 insertions(+), 12 deletions(-) diff --git a/src/sagemaker/fw_utils.py b/src/sagemaker/fw_utils.py index 76bbf6ab35..2a65379bd5 100644 --- a/src/sagemaker/fw_utils.py +++ b/src/sagemaker/fw_utils.py @@ -12,15 +12,15 @@ # language governing permissions and limitations under the License. from __future__ import absolute_import +from collections import namedtuple + import os import re +import sagemaker.utils import shutil import tempfile -from collections import namedtuple from six.moves.urllib.parse import urlparse -import sagemaker.utils - _TAR_SOURCE_FILENAME = 'source.tar.gz' UploadedCode = namedtuple('UserCode', ['s3_prefix', 'script_name']) @@ -112,24 +112,32 @@ def validate_source_dir(script, directory): def tar_and_upload_dir(session, bucket, s3_key_prefix, script, directory, dependencies=None): - """Pack and upload source files to S3 only if directory is empty or local. + """Package source files and upload a compress tar file to S3. The S3 location will be + ``s3:///s3_key_prefix/sourcedir.tar.gz``. + + If directory is an S3 URI, an UploadedCode object will be returned, but nothing will be + uploaded to S3 (this allow reuse of code already in S3). + + If directory is None, the script will be added to the archive at ``./``. - Note: - If the directory points to S3 no action is taken. + If directory is not None, the (recursive) contents of the directory will be added to + the archive. directory is treated as the base path of the archive, and the script name is + assumed to be a filename or relative path inside the directory. Args: session (boto3.Session): Boto session used to access S3. bucket (str): S3 bucket to which the compressed file is uploaded. s3_key_prefix (str): Prefix for the S3 key. - script (str): Script filename. - directory (str or None): Directory containing the source file. If it starts with - "s3://", no action is taken. - dependencies (List[str]): A list of paths to directories (absolute or relative) + script (str): Script filename or path. + directory (str): Optional. Directory containing the source file. If it starts with "s3://", + no action is taken. + dependencies (List[str]): Optional. A list of paths to directories (absolute or relative) containing additional libraries that will be copied into /opt/ml/lib Returns: - sagemaker.fw_utils.UserCode: An object with the S3 bucket and key (S3 prefix) and script name. + sagemaker.fw_utils.UserCode: An object with the S3 bucket and key (S3 prefix) and + script name. """ if directory and directory.lower().startswith('s3://'): return UploadedCode(s3_prefix=directory, script_name=os.path.basename(script)) @@ -141,7 +149,8 @@ def tar_and_upload_dir(session, bucket, s3_key_prefix, script, directory, depend try: source_files = _list_files_to_compress(script, directory) + dependencies - tar_file = sagemaker.utils.create_tar_file(source_files, os.path.join(tmp, _TAR_SOURCE_FILENAME)) + tar_file = sagemaker.utils.create_tar_file(source_files, + os.path.join(tmp, _TAR_SOURCE_FILENAME)) session.resource('s3').Object(bucket, key).upload_file(tar_file) finally: From 24dbe6c9d7a6aa4833073a6b1df70c775c16b884 Mon Sep 17 00:00:00 2001 From: Jonathan Esterhazy Date: Wed, 21 Nov 2018 17:42:40 -0800 Subject: [PATCH 3/4] bump version to 1.15.2 --- CHANGELOG.rst | 7 +++++++ doc/conf.py | 2 +- src/sagemaker/__init__.py | 2 +- 3 files changed, 9 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index df44402565..de123b9d17 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -2,6 +2,13 @@ CHANGELOG ========= +1.15.2 +====== + +* bug-fix: Fix FileNotFoundError for entry_point without source_dir +* doc-fix: Add missing feature 1.5.0 in change log +* doc-fix: Add README for airflow + 1.15.1 ====== diff --git a/doc/conf.py b/doc/conf.py index 150ca9d28c..63147cf24b 100644 --- a/doc/conf.py +++ b/doc/conf.py @@ -32,7 +32,7 @@ def __getattr__(cls, name): 'numpy', 'scipy', 'scipy.sparse'] sys.modules.update((mod_name, Mock()) for mod_name in MOCK_MODULES) -version = '1.15.1' +version = '1.15.2' project = u'sagemaker' # Add any Sphinx extension module names here, as strings. They can be extensions diff --git a/src/sagemaker/__init__.py b/src/sagemaker/__init__.py index 30d5cdb3ec..9c046f8ce9 100644 --- a/src/sagemaker/__init__.py +++ b/src/sagemaker/__init__.py @@ -37,4 +37,4 @@ from sagemaker.session import s3_input # noqa: F401 from sagemaker.session import get_execution_role # noqa: F401 -__version__ = '1.15.1' +__version__ = '1.15.2' From a83343f29670b79507691521ef4e3ac055f368df Mon Sep 17 00:00:00 2001 From: Jonathan Esterhazy Date: Wed, 21 Nov 2018 19:29:49 -0800 Subject: [PATCH 4/4] fix integ tests failing due to breaking ExecuteUserScriptError change --- tests/integ/test_chainer_train.py | 2 +- tests/integ/test_mxnet_train.py | 2 +- tests/integ/test_pytorch_train.py | 2 +- tests/integ/test_tf.py | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/integ/test_chainer_train.py b/tests/integ/test_chainer_train.py index ed82946334..993cfeb12b 100644 --- a/tests/integ/test_chainer_train.py +++ b/tests/integ/test_chainer_train.py @@ -115,7 +115,7 @@ def test_failed_training_job(sagemaker_session, chainer_full_version): with pytest.raises(ValueError) as e: chainer.fit() - assert 'This failure is expected' in str(e.value) + assert 'ExecuteUserScriptError' in str(e.value) def _run_mnist_training_job(sagemaker_session, instance_type, instance_count, diff --git a/tests/integ/test_mxnet_train.py b/tests/integ/test_mxnet_train.py index 83a2f55ba9..8ab86ed4b7 100644 --- a/tests/integ/test_mxnet_train.py +++ b/tests/integ/test_mxnet_train.py @@ -112,4 +112,4 @@ def test_failed_training_job(sagemaker_session, mxnet_full_version): with pytest.raises(ValueError) as e: mx.fit() - assert 'This failure is expected' in str(e.value) + assert 'ExecuteUserScriptError' in str(e.value) diff --git a/tests/integ/test_pytorch_train.py b/tests/integ/test_pytorch_train.py index fb627baf53..2f78d4ffc2 100644 --- a/tests/integ/test_pytorch_train.py +++ b/tests/integ/test_pytorch_train.py @@ -109,7 +109,7 @@ def test_failed_training_job(sagemaker_session, pytorch_full_version): with pytest.raises(ValueError) as e: pytorch.fit() - assert 'This failure is expected' in str(e.value) + assert 'ExecuteUserScriptError' in str(e.value) def _upload_training_data(pytorch): diff --git a/tests/integ/test_tf.py b/tests/integ/test_tf.py index 75db642faa..52e5b19e65 100644 --- a/tests/integ/test_tf.py +++ b/tests/integ/test_tf.py @@ -162,4 +162,4 @@ def test_failed_tf_training(sagemaker_session, tf_full_version): with pytest.raises(ValueError) as e: estimator.fit() - assert 'This failure is expected' in str(e.value) + assert 'ExecuteUserScriptError' in str(e.value)