From 8d61a362803756d3cb0683c08e7bd8ba51b5b329 Mon Sep 17 00:00:00 2001 From: Erick Benitez-Ramos Date: Wed, 30 Aug 2023 15:33:41 -0700 Subject: [PATCH 1/5] fix: log message when sdk defaults not applied --- src/sagemaker/config/config.py | 3 +- tests/unit/sagemaker/config/test_config.py | 96 +++++++++++++++++++++- 2 files changed, 97 insertions(+), 2 deletions(-) diff --git a/src/sagemaker/config/config.py b/src/sagemaker/config/config.py index ce994f2520..9a9af61928 100644 --- a/src/sagemaker/config/config.py +++ b/src/sagemaker/config/config.py @@ -124,7 +124,8 @@ def load_sagemaker_config(additional_config_paths: List[str] = None, s3_resource merge_dicts(merged_config, config_from_file) logger.info("Fetched defaults config from location: %s", file_path) else: - logger.debug("Fetched defaults config from location: %s, but it was empty", file_path) + logger.info("Not applying SDK defaults from location: %s", file_path) + return merged_config diff --git a/tests/unit/sagemaker/config/test_config.py b/tests/unit/sagemaker/config/test_config.py index e07606000c..553353a51d 100644 --- a/tests/unit/sagemaker/config/test_config.py +++ b/tests/unit/sagemaker/config/test_config.py @@ -17,7 +17,12 @@ import yaml from mock import Mock, MagicMock -from sagemaker.config.config import load_sagemaker_config +from sagemaker.config.config import ( + load_sagemaker_config, + logger, + _DEFAULT_ADMIN_CONFIG_FILE_PATH, + _DEFAULT_USER_CONFIG_FILE_PATH, +) from jsonschema import exceptions from yaml.constructor import ConstructorError @@ -236,3 +241,92 @@ def test_merge_of_s3_default_config_file_and_regular_config_file( s3_resource=s3_resource_mock, ) del os.environ["SAGEMAKER_ADMIN_CONFIG_OVERRIDE"] + + +def test_logging_when_overriden_admin_and_user_configs_are_found(get_data_dir, caplog): + # Should log info message stating defaults were fetched since both exist + logger.propagate = True + os.environ["SAGEMAKER_ADMIN_CONFIG_OVERRIDE"] = get_data_dir + os.environ["SAGEMAKER_USER_CONFIG_OVERRIDE"] = get_data_dir + load_sagemaker_config() + assert "Fetched defaults config from location: {}".format(get_data_dir) in caplog.text + assert ( + "Not applying SDK defaults from location: {}".format(_DEFAULT_ADMIN_CONFIG_FILE_PATH) + not in caplog.text + ) + assert ( + "Not applying SDK defaults from location: {}".format(_DEFAULT_USER_CONFIG_FILE_PATH) + not in caplog.text + ) + + +def test_logging_when_default_admin_and_user_config_not_found(caplog): + # Should log info message stating sdk defaults were not applied + # for admin and user config since both are missing from default location + logger.propagate = True + load_sagemaker_config() + assert ( + "Not applying SDK defaults from location: {}".format(_DEFAULT_ADMIN_CONFIG_FILE_PATH) + in caplog.text + ) + assert ( + "Not applying SDK defaults from location: {}".format(_DEFAULT_USER_CONFIG_FILE_PATH) + in caplog.text + ) + + +def test_logging_when_default_admin_and_overriden_user_config_not_found(get_data_dir, caplog): + # Should only log info message stating sdk defaults were not applied from default admin config. + # Failing to load overriden user config should throw exception. + logger.propagate = True + fake_config_file_path = os.path.join(get_data_dir, "config-not-found.yaml") + os.environ["SAGEMAKER_USER_CONFIG_OVERRIDE"] = fake_config_file_path + with pytest.raises(ValueError): + load_sagemaker_config() + assert ( + "Not applying SDK defaults from location: {}".format(_DEFAULT_ADMIN_CONFIG_FILE_PATH) + in caplog.text + ) + assert ( + "Not applying SDK defaults from location: {}".format(_DEFAULT_USER_CONFIG_FILE_PATH) + not in caplog.text + ) + del os.environ["SAGEMAKER_USER_CONFIG_OVERRIDE"] + + +def test_logging_when_overriden_admin_and_overridden_user_config_not_found(get_data_dir, caplog): + # Should not log any info messages since both config paths are overridden. + # Should throw an exception on failure since both will fail to load. + logger.propagate = True + fake_config_file_path = os.path.join(get_data_dir, "config-not-found.yaml") + os.environ["SAGEMAKER_USER_CONFIG_OVERRIDE"] = fake_config_file_path + os.environ["SAGEMAKER_ADMIN_CONFIG_OVERRIDE"] = fake_config_file_path + with pytest.raises(ValueError): + load_sagemaker_config() + assert ( + "Not applying SDK defaults from location: {}".format(_DEFAULT_ADMIN_CONFIG_FILE_PATH) + not in caplog.text + ) + assert ( + "Not applying SDK defaults from location: {}".format(_DEFAULT_USER_CONFIG_FILE_PATH) + not in caplog.text + ) + del os.environ["SAGEMAKER_USER_CONFIG_OVERRIDE"] + del os.environ["SAGEMAKER_ADMIN_CONFIG_OVERRIDE"] + + +def test_logging_with_additional_configs_and_none_are_found(caplog): + # Should log info message stating sdk defaults were not applied + # for admin and user config since both are missing from default location. + # Should throw exception when config in additional_config_path is missing + logger.propagate = True + with pytest.raises(ValueError): + load_sagemaker_config(additional_config_paths=["fake-path"]) + assert ( + "Not applying SDK defaults from location: {}".format(_DEFAULT_ADMIN_CONFIG_FILE_PATH) + in caplog.text + ) + assert ( + "Not applying SDK defaults from location: {}".format(_DEFAULT_USER_CONFIG_FILE_PATH) + in caplog.text + ) From a90c3d724d95ed304f37e22ec5c32ab92d280eb1 Mon Sep 17 00:00:00 2001 From: Erick Benitez-Ramos Date: Thu, 31 Aug 2023 10:31:36 -0700 Subject: [PATCH 2/5] change: add debug level message and additional unit test for sdk default --- src/sagemaker/config/config.py | 4 +- tests/unit/sagemaker/config/test_config.py | 49 +++++++++++++++++++++- 2 files changed, 51 insertions(+), 2 deletions(-) diff --git a/src/sagemaker/config/config.py b/src/sagemaker/config/config.py index 9a9af61928..43116ba0cc 100644 --- a/src/sagemaker/config/config.py +++ b/src/sagemaker/config/config.py @@ -110,7 +110,7 @@ def load_sagemaker_config(additional_config_paths: List[str] = None, s3_resource else: try: config_from_file = _load_config_from_file(file_path) - except ValueError: + except ValueError as error: if file_path not in ( _DEFAULT_ADMIN_CONFIG_FILE_PATH, _DEFAULT_USER_CONFIG_FILE_PATH, @@ -119,6 +119,8 @@ def load_sagemaker_config(additional_config_paths: List[str] = None, s3_resource # If there are no files in the Default config file locations, don't throw # Exceptions. raise + else: + logger.debug(error) if config_from_file: validate_sagemaker_config(config_from_file) merge_dicts(merged_config, config_from_file) diff --git a/tests/unit/sagemaker/config/test_config.py b/tests/unit/sagemaker/config/test_config.py index 553353a51d..9a21fed6ba 100644 --- a/tests/unit/sagemaker/config/test_config.py +++ b/tests/unit/sagemaker/config/test_config.py @@ -15,6 +15,7 @@ import os import pytest import yaml +import logging from mock import Mock, MagicMock from sagemaker.config.config import ( @@ -246,6 +247,7 @@ def test_merge_of_s3_default_config_file_and_regular_config_file( def test_logging_when_overriden_admin_and_user_configs_are_found(get_data_dir, caplog): # Should log info message stating defaults were fetched since both exist logger.propagate = True + os.environ["SAGEMAKER_ADMIN_CONFIG_OVERRIDE"] = get_data_dir os.environ["SAGEMAKER_USER_CONFIG_OVERRIDE"] = get_data_dir load_sagemaker_config() @@ -258,12 +260,51 @@ def test_logging_when_overriden_admin_and_user_configs_are_found(get_data_dir, c "Not applying SDK defaults from location: {}".format(_DEFAULT_USER_CONFIG_FILE_PATH) not in caplog.text ) + del os.environ["SAGEMAKER_ADMIN_CONFIG_OVERRIDE"] + del os.environ["SAGEMAKER_USER_CONFIG_OVERRIDE"] + logger.propagate = False + +def test_logging_when_overriden_admin_is_found_and_default_user_config_not_found(get_data_dir, caplog): + logger.propagate = True + caplog.set_level(logging.DEBUG, logger=logger.name) + os.environ["SAGEMAKER_ADMIN_CONFIG_OVERRIDE"] = get_data_dir + load_sagemaker_config() + assert "Fetched defaults config from location: {}".format(get_data_dir) in caplog.text + assert ( + "Not applying SDK defaults from location: {}".format( + _DEFAULT_USER_CONFIG_FILE_PATH) + in caplog.text + ) + assert "Unable to load the config file from the location: {}".format( + _DEFAULT_USER_CONFIG_FILE_PATH + ) in caplog.text + del os.environ["SAGEMAKER_ADMIN_CONFIG_OVERRIDE"] + logger.propagate = False + + +def test_logging_when_default_admin_not_found_and_overriden_user_config_is_found(get_data_dir, caplog): + logger.propagate = True + caplog.set_level(logging.DEBUG, logger=logger.name) + os.environ["SAGEMAKER_USER_CONFIG_OVERRIDE"] = get_data_dir + load_sagemaker_config() + assert "Fetched defaults config from location: {}".format(get_data_dir) in caplog.text + assert ( + "Not applying SDK defaults from location: {}".format( + _DEFAULT_ADMIN_CONFIG_FILE_PATH) + in caplog.text + ) + assert "Unable to load the config file from the location: {}".format( + _DEFAULT_ADMIN_CONFIG_FILE_PATH + ) in caplog.text + del os.environ["SAGEMAKER_USER_CONFIG_OVERRIDE"] + logger.propagate = False -def test_logging_when_default_admin_and_user_config_not_found(caplog): +def test_logging_when_default_admin_and_default_user_config_not_found(caplog): # Should log info message stating sdk defaults were not applied # for admin and user config since both are missing from default location logger.propagate = True + caplog.set_level(logging.DEBUG, logger=logger.name) load_sagemaker_config() assert ( "Not applying SDK defaults from location: {}".format(_DEFAULT_ADMIN_CONFIG_FILE_PATH) @@ -273,6 +314,9 @@ def test_logging_when_default_admin_and_user_config_not_found(caplog): "Not applying SDK defaults from location: {}".format(_DEFAULT_USER_CONFIG_FILE_PATH) in caplog.text ) + assert "Unable to load the config file from the location: {}".format(_DEFAULT_ADMIN_CONFIG_FILE_PATH) in caplog.text + assert "Unable to load the config file from the location: {}".format(_DEFAULT_USER_CONFIG_FILE_PATH) in caplog.text + logger.propagate = False def test_logging_when_default_admin_and_overriden_user_config_not_found(get_data_dir, caplog): @@ -292,6 +336,7 @@ def test_logging_when_default_admin_and_overriden_user_config_not_found(get_data not in caplog.text ) del os.environ["SAGEMAKER_USER_CONFIG_OVERRIDE"] + logger.propagate = False def test_logging_when_overriden_admin_and_overridden_user_config_not_found(get_data_dir, caplog): @@ -313,6 +358,7 @@ def test_logging_when_overriden_admin_and_overridden_user_config_not_found(get_d ) del os.environ["SAGEMAKER_USER_CONFIG_OVERRIDE"] del os.environ["SAGEMAKER_ADMIN_CONFIG_OVERRIDE"] + logger.propagate = False def test_logging_with_additional_configs_and_none_are_found(caplog): @@ -330,3 +376,4 @@ def test_logging_with_additional_configs_and_none_are_found(caplog): "Not applying SDK defaults from location: {}".format(_DEFAULT_USER_CONFIG_FILE_PATH) in caplog.text ) + logger.propagate = False From 7f2ec305a3d29ebef0fd26443489d0b5958eca93 Mon Sep 17 00:00:00 2001 From: Erick Benitez-Ramos Date: Thu, 31 Aug 2023 10:44:48 -0700 Subject: [PATCH 3/5] chore: black-format --- tests/unit/sagemaker/config/test_config.py | 47 +++++++++++++++------- 1 file changed, 33 insertions(+), 14 deletions(-) diff --git a/tests/unit/sagemaker/config/test_config.py b/tests/unit/sagemaker/config/test_config.py index 9a21fed6ba..1e41b3c32d 100644 --- a/tests/unit/sagemaker/config/test_config.py +++ b/tests/unit/sagemaker/config/test_config.py @@ -264,38 +264,47 @@ def test_logging_when_overriden_admin_and_user_configs_are_found(get_data_dir, c del os.environ["SAGEMAKER_USER_CONFIG_OVERRIDE"] logger.propagate = False -def test_logging_when_overriden_admin_is_found_and_default_user_config_not_found(get_data_dir, caplog): + +def test_logging_when_overriden_admin_is_found_and_default_user_config_not_found( + get_data_dir, caplog +): logger.propagate = True caplog.set_level(logging.DEBUG, logger=logger.name) os.environ["SAGEMAKER_ADMIN_CONFIG_OVERRIDE"] = get_data_dir load_sagemaker_config() assert "Fetched defaults config from location: {}".format(get_data_dir) in caplog.text assert ( - "Not applying SDK defaults from location: {}".format( - _DEFAULT_USER_CONFIG_FILE_PATH) + "Not applying SDK defaults from location: {}".format(_DEFAULT_USER_CONFIG_FILE_PATH) + in caplog.text + ) + assert ( + "Unable to load the config file from the location: {}".format( + _DEFAULT_USER_CONFIG_FILE_PATH + ) in caplog.text ) - assert "Unable to load the config file from the location: {}".format( - _DEFAULT_USER_CONFIG_FILE_PATH - ) in caplog.text del os.environ["SAGEMAKER_ADMIN_CONFIG_OVERRIDE"] logger.propagate = False -def test_logging_when_default_admin_not_found_and_overriden_user_config_is_found(get_data_dir, caplog): +def test_logging_when_default_admin_not_found_and_overriden_user_config_is_found( + get_data_dir, caplog +): logger.propagate = True caplog.set_level(logging.DEBUG, logger=logger.name) os.environ["SAGEMAKER_USER_CONFIG_OVERRIDE"] = get_data_dir load_sagemaker_config() assert "Fetched defaults config from location: {}".format(get_data_dir) in caplog.text assert ( - "Not applying SDK defaults from location: {}".format( - _DEFAULT_ADMIN_CONFIG_FILE_PATH) + "Not applying SDK defaults from location: {}".format(_DEFAULT_ADMIN_CONFIG_FILE_PATH) + in caplog.text + ) + assert ( + "Unable to load the config file from the location: {}".format( + _DEFAULT_ADMIN_CONFIG_FILE_PATH + ) in caplog.text ) - assert "Unable to load the config file from the location: {}".format( - _DEFAULT_ADMIN_CONFIG_FILE_PATH - ) in caplog.text del os.environ["SAGEMAKER_USER_CONFIG_OVERRIDE"] logger.propagate = False @@ -314,8 +323,18 @@ def test_logging_when_default_admin_and_default_user_config_not_found(caplog): "Not applying SDK defaults from location: {}".format(_DEFAULT_USER_CONFIG_FILE_PATH) in caplog.text ) - assert "Unable to load the config file from the location: {}".format(_DEFAULT_ADMIN_CONFIG_FILE_PATH) in caplog.text - assert "Unable to load the config file from the location: {}".format(_DEFAULT_USER_CONFIG_FILE_PATH) in caplog.text + assert ( + "Unable to load the config file from the location: {}".format( + _DEFAULT_ADMIN_CONFIG_FILE_PATH + ) + in caplog.text + ) + assert ( + "Unable to load the config file from the location: {}".format( + _DEFAULT_USER_CONFIG_FILE_PATH + ) + in caplog.text + ) logger.propagate = False From 8f91fced73b2b6f9a5cd03a4dc4745e91e25a711 Mon Sep 17 00:00:00 2001 From: Erick Benitez-Ramos Date: Thu, 31 Aug 2023 10:48:15 -0700 Subject: [PATCH 4/5] chore: pylint --- src/sagemaker/config/config.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/sagemaker/config/config.py b/src/sagemaker/config/config.py index 43116ba0cc..3beb9dfe21 100644 --- a/src/sagemaker/config/config.py +++ b/src/sagemaker/config/config.py @@ -119,8 +119,8 @@ def load_sagemaker_config(additional_config_paths: List[str] = None, s3_resource # If there are no files in the Default config file locations, don't throw # Exceptions. raise - else: - logger.debug(error) + + logger.debug(error) if config_from_file: validate_sagemaker_config(config_from_file) merge_dicts(merged_config, config_from_file) From d99ea1b0ccc855bb3930b66b88edb9736fe9fd5c Mon Sep 17 00:00:00 2001 From: Erick Benitez-Ramos Date: Fri, 1 Sep 2023 09:56:07 -0700 Subject: [PATCH 5/5] fix: typos --- tests/unit/sagemaker/config/test_config.py | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/tests/unit/sagemaker/config/test_config.py b/tests/unit/sagemaker/config/test_config.py index 1e41b3c32d..54d236eaab 100644 --- a/tests/unit/sagemaker/config/test_config.py +++ b/tests/unit/sagemaker/config/test_config.py @@ -244,7 +244,9 @@ def test_merge_of_s3_default_config_file_and_regular_config_file( del os.environ["SAGEMAKER_ADMIN_CONFIG_OVERRIDE"] -def test_logging_when_overriden_admin_and_user_configs_are_found(get_data_dir, caplog): +def test_logging_when_overridden_admin_is_found_and_overridden_user_config_is_found( + get_data_dir, caplog +): # Should log info message stating defaults were fetched since both exist logger.propagate = True @@ -265,7 +267,7 @@ def test_logging_when_overriden_admin_and_user_configs_are_found(get_data_dir, c logger.propagate = False -def test_logging_when_overriden_admin_is_found_and_default_user_config_not_found( +def test_logging_when_overridden_admin_is_found_and_default_user_config_not_found( get_data_dir, caplog ): logger.propagate = True @@ -309,7 +311,7 @@ def test_logging_when_default_admin_not_found_and_overriden_user_config_is_found logger.propagate = False -def test_logging_when_default_admin_and_default_user_config_not_found(caplog): +def test_logging_when_default_admin_not_found_and_default_user_config_not_found(caplog): # Should log info message stating sdk defaults were not applied # for admin and user config since both are missing from default location logger.propagate = True @@ -338,7 +340,9 @@ def test_logging_when_default_admin_and_default_user_config_not_found(caplog): logger.propagate = False -def test_logging_when_default_admin_and_overriden_user_config_not_found(get_data_dir, caplog): +def test_logging_when_default_admin_not_found_and_overriden_user_config_not_found( + get_data_dir, caplog +): # Should only log info message stating sdk defaults were not applied from default admin config. # Failing to load overriden user config should throw exception. logger.propagate = True @@ -358,7 +362,9 @@ def test_logging_when_default_admin_and_overriden_user_config_not_found(get_data logger.propagate = False -def test_logging_when_overriden_admin_and_overridden_user_config_not_found(get_data_dir, caplog): +def test_logging_when_overriden_admin_not_found_and_overridden_user_config_not_found( + get_data_dir, caplog +): # Should not log any info messages since both config paths are overridden. # Should throw an exception on failure since both will fail to load. logger.propagate = True