diff --git a/smdebug/profiler/profiler_config_parser.py b/smdebug/profiler/profiler_config_parser.py index 6f49e0efe..2101774b1 100644 --- a/smdebug/profiler/profiler_config_parser.py +++ b/smdebug/profiler/profiler_config_parser.py @@ -16,6 +16,7 @@ FILE_OPEN_FAIL_THRESHOLD_DEFAULT, MAX_FILE_SIZE_DEFAULT, ) +from smdebug.profiler.utils import get_last_modified_time class LastProfilingStatus(Enum): @@ -76,7 +77,7 @@ class ProfilerConfigParser: def __init__(self): """Initialize the parser to be disabled for profiling and detailed profiling. """ - self.last_json_config = None + self.last_modified_time = None self.config = None self.profiling_enabled = False self.logger = get_logger() @@ -126,14 +127,15 @@ def load_config(self): config_path = os.environ.get("SMPROFILER_CONFIG_PATH", CONFIG_PATH_DEFAULT) if os.path.isfile(config_path): + last_modified_time = get_last_modified_time(config_path) + if self.last_modified_time == last_modified_time: + return + self.last_modified_time = last_modified_time + with open(config_path) as json_data: try: full_config = json.loads(json_data.read().lower()) - if full_config == self.last_json_config: - return - - self.last_json_config = full_config self.config = None if full_config.get(ProfilingParametersField.DISABLE_PROFILER.value, False): diff --git a/smdebug/profiler/utils.py b/smdebug/profiler/utils.py index 436690f1b..fd4258d95 100644 --- a/smdebug/profiler/utils.py +++ b/smdebug/profiler/utils.py @@ -300,3 +300,11 @@ def stop_smdataparallel_profiler(smdataparallel, base_dir): ensure_dir(new_file_name) if os.path.exists(smdataparallel_temp_file): shutil.move(smdataparallel_temp_file, new_file_name) + + +def get_last_modified_time(filepath): + """ + Get the last time that the file at the given filepath was modified, in the form of a datetime object. + """ + last_modified_time = os.path.getmtime(filepath) + return datetime.fromtimestamp(last_modified_time) # get the last time the config was modified diff --git a/tests/profiler/core/test_profiler_config_parser.py b/tests/profiler/core/test_profiler_config_parser.py index cddea785b..244176144 100644 --- a/tests/profiler/core/test_profiler_config_parser.py +++ b/tests/profiler/core/test_profiler_config_parser.py @@ -1,6 +1,7 @@ # Standard Library import json import os +import shutil import time # Third Party @@ -14,6 +15,7 @@ python_profiling_test_cases, smdataparallel_profiling_test_cases, ) +from tests.profiler.utils import get_last_accessed_time # First Party from smdebug.profiler.profiler_config_parser import MetricsCategory, ProfilerConfigParser @@ -76,6 +78,11 @@ def case_insensitive_profiler_config_parser(config_folder, monkeypatch): return ProfilerConfigParser() +@pytest.fixture +def step_profiler_config_parser_path(config_folder): + return os.path.join(config_folder, "step_profiler_config_parser.json") + + @pytest.fixture def old_step_profiler_config_parser_path(config_folder): return os.path.join(config_folder, "old_step_profiler_config_parser.json") @@ -86,6 +93,11 @@ def new_step_profiler_config_parser_path(config_folder): return os.path.join(config_folder, "new_step_profiler_config_parser.json") +@pytest.fixture +def time_profiler_config_parser_path(config_folder): + return os.path.join(config_folder, "time_profiler_config_parser.json") + + @pytest.fixture def old_time_profiler_config_parser_path(config_folder): return os.path.join(config_folder, "old_time_profiler_config_parser.json") @@ -368,17 +380,23 @@ def test_case_insensitive_profiler_config_parser(case_insensitive_profiler_confi def test_update_step_profiler_config_parser( - monkeypatch, old_step_profiler_config_parser_path, new_step_profiler_config_parser_path + monkeypatch, + step_profiler_config_parser_path, + old_step_profiler_config_parser_path, + new_step_profiler_config_parser_path, ): """ - This test is meant to test two behaviors when profiler config parser dynamically reloads a config with step fields: - - Reloading the config when the JSON hasn't changed will not reload the step fields (this is important when the - JSON does not have specified step parameters, for example). - - Reloading the config when the JSON has changed will reload the step fields in the new JSON. + This test is meant to test two behaviors when profiler config parser dynamically reloads a config JSON with step + fields: + - Calling load_config when the JSON hasn't changed will not reload the JSON into memory and its old step fields. + (this is important when the JSON does not have specified step parameters, for example). + - Calling load_config when the JSON has changed will reload the JSON into memory with the new step fields. """ # sanity check that the parser first parses the range fields as is. - monkeypatch.setenv("SMPROFILER_CONFIG_PATH", old_step_profiler_config_parser_path) + monkeypatch.setenv("SMPROFILER_CONFIG_PATH", step_profiler_config_parser_path) + shutil.copy(old_step_profiler_config_parser_path, step_profiler_config_parser_path) profiler_config_parser = ProfilerConfigParser() + first_accessed_time = get_last_accessed_time(step_profiler_config_parser_path) assert profiler_config_parser.profiling_enabled assert profiler_config_parser.config.detailed_profiling_config.is_enabled() assert profiler_config_parser.config.detailed_profiling_config.start_step is None @@ -389,14 +407,21 @@ def test_update_step_profiler_config_parser( assert profiler_config_parser.config.detailed_profiling_config.start_step == 5 assert profiler_config_parser.config.detailed_profiling_config.num_steps == 2 - # check that reloading the config when it hasn't changed won't change the config fields. profiler_config_parser.load_config() + + # check that the config wasn't loaded into memory again. + last_accessed_time = get_last_accessed_time(step_profiler_config_parser_path) + assert first_accessed_time == last_accessed_time + + # check that reloading the config when it hasn't changed won't change the config fields. assert profiler_config_parser.config.detailed_profiling_config.start_step == 5 assert profiler_config_parser.config.detailed_profiling_config.num_steps == 2 - # check that reloading the config when it has changed will update the config fields. - monkeypatch.setenv("SMPROFILER_CONFIG_PATH", new_step_profiler_config_parser_path) + time.sleep(0.1) # allow time to pass so new modified time will be different + shutil.copy(new_step_profiler_config_parser_path, step_profiler_config_parser_path) profiler_config_parser.load_config() + + # check that reloading the config when it has changed will update the config fields. assert profiler_config_parser.profiling_enabled assert profiler_config_parser.config.detailed_profiling_config.is_enabled() assert profiler_config_parser.config.detailed_profiling_config.start_step == 10 @@ -404,17 +429,23 @@ def test_update_step_profiler_config_parser( def test_update_time_profiler_config_parser( - monkeypatch, old_time_profiler_config_parser_path, new_time_profiler_config_parser_path + monkeypatch, + time_profiler_config_parser_path, + old_time_profiler_config_parser_path, + new_time_profiler_config_parser_path, ): """ - This test is meant to test two behaviors when profiler config parser dynamically reloads a config with time fields: - - Reloading the config when the JSON hasn't changed will not reload the time fields (this is important when the - JSON does not have specified time parameters, for example). - - Reloading the config when the JSON has changed will reload the time fields in the new JSON. + This test is meant to test two behaviors when profiler config parser dynamically reloads a config JSON with time + fields: + - Calling load_config when the JSON hasn't changed will not reload the JSON into memory and its old time fields. + (this is important when the JSON does not have specified time parameters, for example). + - Calling load_config when the JSON has changed will reload the JSON into memory with the new time fields. """ # sanity check that the parser first parses the range fields as is. - monkeypatch.setenv("SMPROFILER_CONFIG_PATH", old_time_profiler_config_parser_path) + monkeypatch.setenv("SMPROFILER_CONFIG_PATH", time_profiler_config_parser_path) + shutil.copy(old_time_profiler_config_parser_path, time_profiler_config_parser_path) profiler_config_parser = ProfilerConfigParser() + first_accessed_time = get_last_accessed_time(time_profiler_config_parser_path) assert profiler_config_parser.profiling_enabled assert profiler_config_parser.config.detailed_profiling_config.is_enabled() assert profiler_config_parser.config.detailed_profiling_config.start_time_in_sec is None @@ -428,14 +459,21 @@ def test_update_time_profiler_config_parser( assert profiler_config_parser.config.detailed_profiling_config.start_time_in_sec == timestamp1 assert profiler_config_parser.config.detailed_profiling_config.duration_in_sec == 0.1 - # check that reloading the config when it hasn't changed won't change the config fields. profiler_config_parser.load_config() + + # check that the config wasn't loaded into memory again. + last_accessed_time = get_last_accessed_time(time_profiler_config_parser_path) + assert first_accessed_time == last_accessed_time + + # check that reloading the config when it hasn't changed won't change the config fields. assert profiler_config_parser.config.detailed_profiling_config.start_time_in_sec == timestamp1 assert profiler_config_parser.config.detailed_profiling_config.duration_in_sec == 0.1 - # check that reloading the config when it has changed will update the config fields. - monkeypatch.setenv("SMPROFILER_CONFIG_PATH", new_time_profiler_config_parser_path) + time.sleep(0.1) # allow time to pass so new modified time will be different + shutil.copy(new_time_profiler_config_parser_path, time_profiler_config_parser_path) profiler_config_parser.load_config() + + # check that reloading the config when it has changed will update the config fields. assert profiler_config_parser.profiling_enabled assert profiler_config_parser.config.detailed_profiling_config.is_enabled() assert profiler_config_parser.config.detailed_profiling_config.start_time_in_sec == 1700000000 @@ -454,6 +492,7 @@ def test_update_disabled_profiler_config_parser( assert not profiler_config_parser.profiling_enabled # check that reloading the config when it has changed will update the config fields. + time.sleep(0.1) # allow time to pass so new modified time will be different monkeypatch.setenv("SMPROFILER_CONFIG_PATH", new_step_profiler_config_parser_path) profiler_config_parser.load_config() assert profiler_config_parser.profiling_enabled diff --git a/tests/profiler/utils.py b/tests/profiler/utils.py new file mode 100644 index 000000000..6405e64be --- /dev/null +++ b/tests/profiler/utils.py @@ -0,0 +1,11 @@ +# Standard Library +import os +from datetime import datetime + + +def get_last_accessed_time(filepath): + """ + Get the last time that the file at the given filepath was accessed, in the form of a datetime object. + """ + last_accessed_time = os.path.getatime(filepath) + return datetime.fromtimestamp(last_accessed_time) # get the last time the config was accessed