Skip to content

fix: log message when sdk defaults not applied #4104

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
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
7 changes: 5 additions & 2 deletions src/sagemaker/config/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -119,12 +119,15 @@ 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

logger.debug(error)
if config_from_file:
validate_sagemaker_config(config_from_file)
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


Expand Down
168 changes: 167 additions & 1 deletion tests/unit/sagemaker/config/test_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,15 @@
import os
import pytest
import yaml
import logging
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

Expand Down Expand Up @@ -236,3 +242,163 @@ 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_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

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
)
del os.environ["SAGEMAKER_ADMIN_CONFIG_OVERRIDE"]
del os.environ["SAGEMAKER_USER_CONFIG_OVERRIDE"]
logger.propagate = False


def test_logging_when_overridden_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_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
caplog.set_level(logging.DEBUG, logger=logger.name)
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
)
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_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
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"]
logger.propagate = False


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
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"]
logger.propagate = False


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
)
logger.propagate = False