From 3effb8fa7483e0d5cabbe8c1409d25ecd0751d56 Mon Sep 17 00:00:00 2001 From: Pedro Larroy Date: Fri, 13 Mar 2020 13:49:37 -0700 Subject: [PATCH 01/11] Check that session is a LocalSession when using local mode, if the user passes sagemaker.session.Session the error message is not clear. --- src/sagemaker/estimator.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/sagemaker/estimator.py b/src/sagemaker/estimator.py index 1931725397..af9b62c5c5 100644 --- a/src/sagemaker/estimator.py +++ b/src/sagemaker/estimator.py @@ -218,6 +218,8 @@ def __init__( if self.train_instance_type == "local_gpu" and self.train_instance_count > 1: raise RuntimeError("Distributed Training in Local GPU is not supported") self.sagemaker_session = sagemaker_session or LocalSession() + if not isinstance(self.sagemaker_session, LocalSession): + raise RuntimeError("instance_type local or local_gpu is only supported with an instance of LocalSession") else: self.sagemaker_session = sagemaker_session or Session() From 5a46d3f57c92dd327963ad18c9e6dc4548e7a911 Mon Sep 17 00:00:00 2001 From: Pedro Larroy Date: Thu, 19 Mar 2020 12:24:11 -0700 Subject: [PATCH 02/11] run linter --- src/sagemaker/estimator.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/sagemaker/estimator.py b/src/sagemaker/estimator.py index af9b62c5c5..227f8591d1 100644 --- a/src/sagemaker/estimator.py +++ b/src/sagemaker/estimator.py @@ -219,7 +219,9 @@ def __init__( raise RuntimeError("Distributed Training in Local GPU is not supported") self.sagemaker_session = sagemaker_session or LocalSession() if not isinstance(self.sagemaker_session, LocalSession): - raise RuntimeError("instance_type local or local_gpu is only supported with an instance of LocalSession") + raise RuntimeError( + "instance_type local or local_gpu is only supported with an instance of LocalSession" + ) else: self.sagemaker_session = sagemaker_session or Session() From add08d96c58e6cac8ba613e643260fd96d822ada Mon Sep 17 00:00:00 2001 From: Pedro Larroy Date: Thu, 19 Mar 2020 13:09:23 -0700 Subject: [PATCH 03/11] fix lint, add tests --- src/sagemaker/estimator.py | 3 ++- tests/unit/test_estimator.py | 27 +++++++++++++++++++++++++++ 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/src/sagemaker/estimator.py b/src/sagemaker/estimator.py index 227f8591d1..92b8b03d97 100644 --- a/src/sagemaker/estimator.py +++ b/src/sagemaker/estimator.py @@ -220,7 +220,8 @@ def __init__( self.sagemaker_session = sagemaker_session or LocalSession() if not isinstance(self.sagemaker_session, LocalSession): raise RuntimeError( - "instance_type local or local_gpu is only supported with an instance of LocalSession" + "instance_type local or local_gpu is only supported with an" + "instance of LocalSession" ) else: self.sagemaker_session = sagemaker_session or Session() diff --git a/tests/unit/test_estimator.py b/tests/unit/test_estimator.py index 62245d7500..549499736a 100644 --- a/tests/unit/test_estimator.py +++ b/tests/unit/test_estimator.py @@ -2392,3 +2392,30 @@ def test_encryption_flag_in_non_vpc_mode_invalid(sagemaker_session): '"EnableInterContainerTrafficEncryption" and "VpcConfig" must be provided together' in str(error) ) + + +def test_estimator_local_mode(sagemaker_session): + # When using instance local with a session which is not LocalSession we should error out + with pytest.raises(RuntimeError) as error: + estimator = Estimator( + image_name="some-image", + role="some_image", + train_instance_count=1, + train_instance_type="local", + sagemaker_session=sagemaker_session, + base_job_name="base_job_name", + ) + +def test_estimator_local_mode(sagemaker_local_session): + # When using instance local with a session which is not LocalSession we should error out + estimator = Estimator( + image_name="some-image", + role="some_image", + train_instance_count=1, + train_instance_type="local", + sagemaker_session=sagemaker_local_session, + base_job_name="base_job_name", + ) + + + From 7ad6e8df263fb13f3f453fca150f49c80abe89c8 Mon Sep 17 00:00:00 2001 From: Pedro Larroy Date: Thu, 19 Mar 2020 13:51:14 -0700 Subject: [PATCH 04/11] lint --- tests/unit/test_estimator.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/tests/unit/test_estimator.py b/tests/unit/test_estimator.py index 549499736a..17ab9ab2ca 100644 --- a/tests/unit/test_estimator.py +++ b/tests/unit/test_estimator.py @@ -2406,6 +2406,7 @@ def test_estimator_local_mode(sagemaker_session): base_job_name="base_job_name", ) + def test_estimator_local_mode(sagemaker_local_session): # When using instance local with a session which is not LocalSession we should error out estimator = Estimator( @@ -2416,6 +2417,3 @@ def test_estimator_local_mode(sagemaker_local_session): sagemaker_session=sagemaker_local_session, base_job_name="base_job_name", ) - - - From 4564bb2cf242b4b07c375e9c043bc822c79b2d45 Mon Sep 17 00:00:00 2001 From: Pedro Larroy Date: Thu, 19 Mar 2020 14:01:16 -0700 Subject: [PATCH 05/11] fix ci --- tests/unit/test_estimator.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/unit/test_estimator.py b/tests/unit/test_estimator.py index 17ab9ab2ca..bd67b16ded 100644 --- a/tests/unit/test_estimator.py +++ b/tests/unit/test_estimator.py @@ -2394,10 +2394,10 @@ def test_encryption_flag_in_non_vpc_mode_invalid(sagemaker_session): ) -def test_estimator_local_mode(sagemaker_session): +def test_estimator_local_mode_error(sagemaker_session): # When using instance local with a session which is not LocalSession we should error out - with pytest.raises(RuntimeError) as error: - estimator = Estimator( + with pytest.raises(RuntimeError): + Estimator( image_name="some-image", role="some_image", train_instance_count=1, @@ -2407,9 +2407,9 @@ def test_estimator_local_mode(sagemaker_session): ) -def test_estimator_local_mode(sagemaker_local_session): +def test_estimator_local_mode_ok(sagemaker_local_session): # When using instance local with a session which is not LocalSession we should error out - estimator = Estimator( + Estimator( image_name="some-image", role="some_image", train_instance_count=1, From b6cc198b6248a18513e401e61cf5ed0d17b1effa Mon Sep 17 00:00:00 2001 From: Pedro Larroy Date: Thu, 19 Mar 2020 15:02:25 -0700 Subject: [PATCH 06/11] fix test --- src/sagemaker/estimator.py | 2 +- tests/unit/test_estimator.py | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/sagemaker/estimator.py b/src/sagemaker/estimator.py index 92b8b03d97..1da3767448 100644 --- a/src/sagemaker/estimator.py +++ b/src/sagemaker/estimator.py @@ -218,7 +218,7 @@ def __init__( if self.train_instance_type == "local_gpu" and self.train_instance_count > 1: raise RuntimeError("Distributed Training in Local GPU is not supported") self.sagemaker_session = sagemaker_session or LocalSession() - if not isinstance(self.sagemaker_session, LocalSession): + if not isinstance(self.sagemaker_session, sagemaker.local.LocalSession): raise RuntimeError( "instance_type local or local_gpu is only supported with an" "instance of LocalSession" diff --git a/tests/unit/test_estimator.py b/tests/unit/test_estimator.py index bd67b16ded..b56efb53c6 100644 --- a/tests/unit/test_estimator.py +++ b/tests/unit/test_estimator.py @@ -30,6 +30,7 @@ from sagemaker.session import s3_input, ShuffleConfig from sagemaker.transformer import Transformer from botocore.exceptions import ClientError +import sagemaker.local MODEL_DATA = "s3://bucket/model.tar.gz" MODEL_IMAGE = "mi" @@ -2259,7 +2260,7 @@ def test_distributed_gpu_local_mode(LocalSession): @patch("sagemaker.estimator.LocalSession") def test_local_mode_file_output_path(local_session_class): - local_session = Mock() + local_session = Mock(spec=sagemaker.local.LocalSession) local_session.local_mode = True local_session_class.return_value = local_session From d1deccd8846ff3b4f3ee3928eeb0a652a64ed8c0 Mon Sep 17 00:00:00 2001 From: Pedro Larroy Date: Thu, 19 Mar 2020 16:51:26 -0700 Subject: [PATCH 07/11] fix test --- tests/integ/test_airflow_config.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integ/test_airflow_config.py b/tests/integ/test_airflow_config.py index 8b1e112c49..95e97791be 100644 --- a/tests/integ/test_airflow_config.py +++ b/tests/integ/test_airflow_config.py @@ -443,7 +443,7 @@ def test_rcf_airflow_config_uploads_data_source_to_s3(sagemaker_session, cpu_ins @pytest.mark.canary_quick def test_chainer_airflow_config_uploads_data_source_to_s3( - sagemaker_session, cpu_instance_type, chainer_full_version + sagemaker_local_session, cpu_instance_type, chainer_full_version ): with timeout(seconds=AIRFLOW_CONFIG_TIMEOUT_IN_SECONDS): script_path = os.path.join(DATA_DIR, "chainer_mnist", "mnist.py") From f16f78268bf883cdc133baef9611fb97e2a9e685 Mon Sep 17 00:00:00 2001 From: Pedro Larroy Date: Thu, 19 Mar 2020 19:22:36 -0700 Subject: [PATCH 08/11] fix test --- tests/integ/test_airflow_config.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/integ/test_airflow_config.py b/tests/integ/test_airflow_config.py index 95e97791be..25439f67af 100644 --- a/tests/integ/test_airflow_config.py +++ b/tests/integ/test_airflow_config.py @@ -456,7 +456,7 @@ def test_chainer_airflow_config_uploads_data_source_to_s3( train_instance_type="local", framework_version=chainer_full_version, py_version=PYTHON_VERSION, - sagemaker_session=sagemaker_session, + sagemaker_session=sagemaker_local_session, hyperparameters={"epochs": 1}, use_mpi=True, num_processes=2, @@ -474,7 +474,7 @@ def test_chainer_airflow_config_uploads_data_source_to_s3( ) _assert_that_s3_url_contains_data( - sagemaker_session, + sagemaker_local_session, training_config["HyperParameters"]["sagemaker_submit_directory"].strip('"'), ) From 5a5bac988c636d81f6c221a4b11aab027967292e Mon Sep 17 00:00:00 2001 From: Pedro Larroy Date: Thu, 19 Mar 2020 20:53:13 -0700 Subject: [PATCH 09/11] fix test --- tests/unit/test_estimator.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/unit/test_estimator.py b/tests/unit/test_estimator.py index b56efb53c6..15b60548d0 100644 --- a/tests/unit/test_estimator.py +++ b/tests/unit/test_estimator.py @@ -561,6 +561,7 @@ def test_local_code_location(): boto_region_name=REGION, config=config, local_mode=True, + spec=sagemaker.local.LocalSession ) t = DummyFramework( entry_point=SCRIPT_PATH, From 43cd5a28f8abb8180baf45729a1e36e9bcdfa67c Mon Sep 17 00:00:00 2001 From: Pedro Larroy Date: Fri, 20 Mar 2020 02:10:11 -0700 Subject: [PATCH 10/11] fix lint --- tests/unit/test_estimator.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit/test_estimator.py b/tests/unit/test_estimator.py index 15b60548d0..7275c09b5c 100644 --- a/tests/unit/test_estimator.py +++ b/tests/unit/test_estimator.py @@ -561,7 +561,7 @@ def test_local_code_location(): boto_region_name=REGION, config=config, local_mode=True, - spec=sagemaker.local.LocalSession + spec=sagemaker.local.LocalSession, ) t = DummyFramework( entry_point=SCRIPT_PATH, From 38f0fc4a6338ccf56884f52d513c9924690a4659 Mon Sep 17 00:00:00 2001 From: Pedro Larroy Date: Fri, 20 Mar 2020 13:20:07 -0700 Subject: [PATCH 11/11] fix unit test --- tests/unit/test_estimator.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit/test_estimator.py b/tests/unit/test_estimator.py index 7275c09b5c..3069860b1e 100644 --- a/tests/unit/test_estimator.py +++ b/tests/unit/test_estimator.py @@ -2233,7 +2233,7 @@ def test_deploy_with_no_model_name(sagemaker_session): @patch("sagemaker.estimator.LocalSession") @patch("sagemaker.estimator.Session") def test_local_mode(session_class, local_session_class): - local_session = Mock() + local_session = Mock(spec=sagemaker.local.LocalSession) local_session.local_mode = True session = Mock()