[WIP] Use file metadata to determine whether profiler config should be reloaded.

config/profiler/run_profiler_integration_tests.sh

@@ -44,7 +44,7 @@ check_changed_files() {
 }
 
 # If we are not force running the tests, determine whether to run the tests based on the files changed in the branch compared to master.
-run_tests="true"
+run_tests="false"
 if [ $force_run_tests = "false" ]; then
   run_tests=$( check_changed_files )
 fi

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):

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

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,32 +407,51 @@ 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(5)  # allow time to pass so new modified time will be different
+    with open(step_profiler_config_parser_path, "w") as dst:
+        with open(new_step_profiler_config_parser_path, "r") as src:
+            json.dump(json.load(src), dst)
     profiler_config_parser.load_config()
+
+    # verify that the config was 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 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
     assert profiler_config_parser.config.detailed_profiling_config.num_steps == 5
 
 
 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 +465,27 @@ 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(5)  # allow time to pass so new modified time will be different
+    with open(time_profiler_config_parser_path, "w") as dst:
+        with open(new_time_profiler_config_parser_path, "r") as src:
+            json.dump(json.load(src), dst)
     profiler_config_parser.load_config()
+
+    # verify that the config was 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 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

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