Typecasting str profiling parameters to bool (#58)

* Typecasting str profiling parameters to bool

smdebug/profiler/profiler_config.py

@@ -2,6 +2,7 @@
 import time
 
 # First Party
+from smdebug.core.logger import get_logger
 from smdebug.profiler.profiler_constants import PROFILER_NUM_STEPS_DEFAULT
 
 
@@ -35,15 +36,31 @@ def __init__(self, profile_range):
         self.end_step = None
 
         # time range
-        self.start_time = profile_range.get("StartTime")
-        self.duration = profile_range.get("Duration")
+        self.start_time_in_sec = profile_range.get("StartTimeInSecSinceEpoch")
+        self.duration_in_sec = profile_range.get("DurationInSeconds")
         self.end_time = None
 
+        # convert to the correct type
+        try:
+            if self.start_step is not None:
+                self.start_step = int(self.start_step)
+            if self.num_steps is not None:
+                self.num_steps = int(self.num_steps)
+            if self.start_time_in_sec is not None:
+                self.start_time_in_sec = float(self.start_time_in_sec)
+            if self.duration_in_sec is not None:
+                self.duration_in_sec = float(self.duration_in_sec)
+        except ValueError as e:
+            get_logger("smdebug-profiler").info(
+                f"{e} encountered in DetailedProfilingConfig. Disabling Detailed profiling."
+            )
+            self.start_step = self.num_steps = self.start_time_in_sec = self.duration_in_sec = None
+
     def has_step_range(self):
         return self.start_step or self.num_steps
 
     def has_time_range(self):
-        return self.start_time or self.duration
+        return self.start_time_in_sec or self.duration_in_sec
 
     def can_start_detailed_profiling(self, current_step, current_time=time.time()):
         """Determine whether the values from the config are valid for detailed profiling.
@@ -57,14 +74,14 @@ def can_start_detailed_profiling(self, current_step, current_time=time.time()):
                 self.end_step = self.start_step + self.num_steps
             return self.start_step <= current_step < self.end_step
         elif self.has_time_range():
-            if not self.start_time:
-                self.start_time = current_time
-            if self.duration:
+            if not self.start_time_in_sec:
+                self.start_time_in_sec = current_time
+            if self.duration_in_sec:
                 if not self.end_time:
-                    self.end_time = self.start_time + self.duration
-                return self.start_time <= current_time < self.end_time
+                    self.end_time = self.start_time_in_sec + self.duration_in_sec
+                return self.start_time_in_sec <= current_time < self.end_time
             else:
-                if self.start_time <= current_time:
+                if self.start_time_in_sec <= current_time:
                     if not self.end_step:
                         self.end_step = current_step + 1
                     return current_step < self.end_step

smdebug/profiler/profiler_config_parser.py

@@ -13,6 +13,7 @@
     FILE_OPEN_FAIL_THRESHOLD_DEFAULT,
     MAX_FILE_SIZE_DEFAULT,
 )
+from smdebug.profiler.utils import str2bool
 
 
 class LastProfilingStatus(Enum):
@@ -63,7 +64,15 @@ def load_config(self):
                         self.last_status = LastProfilingStatus.INVALID_CONFIG
                     self.profiling_enabled = False
                     return
-            if config.get("ProfilerEnabled", True):
+            try:
+                profiler_enabled = str2bool(config.get("ProfilerEnabled", True))
+            except ValueError as e:
+                get_logger("smdebug-profiler").info(
+                    f"{e} in ProfilingParameters. Enabling profiling with default "
+                    f"parameter values."
+                )
+                profiler_enabled = True
+            if profiler_enabled is True:
                 if self.last_status != LastProfilingStatus.PROFILER_ENABLED:
                     get_logger("smdebug-profiler").info(f"Using config at {config_path}.")
                     self.last_status = LastProfilingStatus.PROFILER_ENABLED
@@ -83,14 +92,24 @@ def load_config(self):
             self.profiling_enabled = False
             return
 
-        local_path = config.get("LocalPath", BASE_FOLDER_DEFAULT)
-        file_max_size = int(config.get("RotateMaxFileSizeInBytes", MAX_FILE_SIZE_DEFAULT))
-        file_close_interval = int(
-            config.get("RotateFileCloseIntervalInSeconds", CLOSE_FILE_INTERVAL_DEFAULT)
-        )
-        file_open_fail_threshold = int(
-            config.get("FileOpenFailThreshold", FILE_OPEN_FAIL_THRESHOLD_DEFAULT)
-        )
+        try:
+            local_path = config.get("LocalPath", BASE_FOLDER_DEFAULT)
+            file_max_size = int(config.get("RotateMaxFileSizeInBytes", MAX_FILE_SIZE_DEFAULT))
+            file_close_interval = float(
+                config.get("RotateFileCloseIntervalInSeconds", CLOSE_FILE_INTERVAL_DEFAULT)
+            )
+            file_open_fail_threshold = int(
+                config.get("FileOpenFailThreshold", FILE_OPEN_FAIL_THRESHOLD_DEFAULT)
+            )
+        except ValueError as e:
+            get_logger("smdebug-profiler").info(
+                f"{e} in ProfilingParameters. Enabling profiling with default " f"parameter values."
+            )
+            local_path = BASE_FOLDER_DEFAULT
+            file_max_size = MAX_FILE_SIZE_DEFAULT
+            file_close_interval = CLOSE_FILE_INTERVAL_DEFAULT
+            file_open_fail_threshold = FILE_OPEN_FAIL_THRESHOLD_DEFAULT
+
         profile_range = config.get("DetailedProfilingConfig", {})
 
         self.config = ProfilerConfig(

smdebug/profiler/utils.py

@@ -5,6 +5,7 @@
 # Standard Library
 import re
 from datetime import datetime
+from distutils.util import strtobool
 from enum import Enum
 
 # First Party
@@ -147,6 +148,13 @@ def validate_system_profiler_file(filename) -> bool:
     return True
 
 
+def str2bool(v):
+    if isinstance(v, bool):
+        return v
+    else:
+        return bool(strtobool(v))
+
+
 def us_since_epoch_to_human_readable_time(us_since_epoch):
     dt = datetime.fromtimestamp(us_since_epoch / 1e6)
     return dt.strftime("%Y-%m-%dT%H:%M:%S:%f")

tests/core/json_configs/invalid_string_data_profiler_config_parser.json

@@ -0,0 +1,15 @@
+{
+    "ProfilingParameters": {
+        "ProfilerEnabled": "None",
+        "LocalPath": "/tmp/test",
+        "RotateMaxFileSizeInBytes": "None",
+        "RotateFileCloseIntervalInSeconds": "None",
+        "FileOpenFailThreshold": "None",
+        "DetailedProfilingConfig": {
+            "StartStep": "None",
+            "NumSteps": "None",
+            "StartTimeInSecSinceEpoch": "None",
+            "DurationInSeconds": "None"
+        }
+    }
+}

tests/core/json_configs/string_data_profiler_config_parser.json

@@ -0,0 +1,15 @@
+{
+    "ProfilingParameters": {
+        "ProfilerEnabled": "True",
+        "LocalPath": "/tmp/test",
+        "RotateMaxFileSizeInBytes": "300",
+        "RotateFileCloseIntervalInSeconds": "0.5",
+        "FileOpenFailThreshold": "2",
+        "DetailedProfilingConfig": {
+            "StartStep": "2",
+            "NumSteps": "1",
+            "StartTimeInSecSinceEpoch": "1594429959.418771",
+            "DurationInSeconds": "0.1"
+        }
+    }
+}

tests/core/json_configs/test_tf2_profiler_config_parser_by_time.json

@@ -3,7 +3,7 @@
         "ProfilerEnabled": true,
         "LocalPath": "/tmp/test",
         "DetailedProfilingConfig": {
-            "Duration": 0.1
+            "DurationInSeconds": 0.1
         }
     }
 }

tests/profiler/core/test_profiler_config_parser.py

@@ -42,6 +42,20 @@ def user_disabled_profiler_config_parser(config_folder, monkeypatch):
     return ProfilerConfigParser()
 
 
+@pytest.fixture
+def string_data_profiler_config_parser(config_folder, monkeypatch):
+    config_path = os.path.join(config_folder, "string_data_profiler_config_parser.json")
+    monkeypatch.setenv("SMPROFILER_CONFIG_PATH", config_path)
+    return ProfilerConfigParser()
+
+
+@pytest.fixture
+def invalid_string_data_profiler_config_parser(config_folder, monkeypatch):
+    config_path = os.path.join(config_folder, "invalid_string_data_profiler_config_parser.json")
+    monkeypatch.setenv("SMPROFILER_CONFIG_PATH", config_path)
+    return ProfilerConfigParser()
+
+
 @pytest.mark.parametrize("test_case", detailed_profiling_test_cases)
 def test_profiling_ranges(detailed_profiler_config_path, test_case):
     detailed_profiling_parameters, expected_detailed_profiling_enabled, expected_can_detailed_profile, expected_values = (
@@ -54,9 +68,9 @@ def test_profiling_ranges(detailed_profiler_config_path, test_case):
     if num_steps:
         detailed_profiler_config.update(NumSteps=num_steps)
     if start_time:
-        detailed_profiler_config.update(StartTime=start_time)
+        detailed_profiler_config.update(StartTimeInSecSinceEpoch=start_time)
     if duration:
-        detailed_profiler_config.update(Duration=duration)
+        detailed_profiler_config.update(DurationInSeconds=duration)
 
     full_config = {
         "ProfilingParameters": {
@@ -84,7 +98,7 @@ def test_profiling_ranges(detailed_profiler_config_path, test_case):
         )
         assert profile_range.start_step == expected_start_step
         assert profile_range.end_step == expected_end_step
-        assert profile_range.start_time == expected_start_time
+        assert profile_range.start_time_in_sec == expected_start_time
         assert profile_range.end_time == expected_end_time
 
 
@@ -117,7 +131,66 @@ def test_default_values(simple_profiler_config_parser):
         [
             profile_range.start_step,
             profile_range.num_steps,
-            profile_range.start_time,
-            profile_range.duration,
+            profile_range.start_time_in_sec,
+            profile_range.duration_in_sec,
         ]
     )
+
+
+def test_string_data_in_config(string_data_profiler_config_parser):
+    """
+    This test is meant to test that the profiler config parser can handle string data
+    and typecast to appropriate types before use.
+    """
+    assert string_data_profiler_config_parser.profiling_enabled
+
+    assert isinstance(
+        string_data_profiler_config_parser.config.trace_file.rotation_policy.file_max_size, int
+    )
+    assert isinstance(
+        string_data_profiler_config_parser.config.trace_file.rotation_policy.file_close_interval,
+        float,
+    )
+    assert isinstance(
+        string_data_profiler_config_parser.config.trace_file.file_open_fail_threshold, int
+    )
+
+    assert isinstance(string_data_profiler_config_parser.config.profile_range.start_step, int)
+    assert isinstance(string_data_profiler_config_parser.config.profile_range.num_steps, int)
+    assert isinstance(
+        string_data_profiler_config_parser.config.profile_range.start_time_in_sec, float
+    )
+    assert isinstance(
+        string_data_profiler_config_parser.config.profile_range.duration_in_sec, float
+    )
+
+
+def test_invalid_string_data_in_config(invalid_string_data_profiler_config_parser):
+    """
+    This test is meant to test that the profiler config parser can handle invalid string data
+    and fallback gracefully.
+    """
+    # Profiler is enabled even if data is invalid
+    assert invalid_string_data_profiler_config_parser.profiling_enabled
+
+    # Fallback to default values for profiling parameters
+    assert (
+        invalid_string_data_profiler_config_parser.config.trace_file.rotation_policy.file_max_size
+        == MAX_FILE_SIZE_DEFAULT
+    )
+    assert (
+        invalid_string_data_profiler_config_parser.config.trace_file.rotation_policy.file_close_interval
+        == CLOSE_FILE_INTERVAL_DEFAULT
+    )
+    assert (
+        invalid_string_data_profiler_config_parser.config.trace_file.file_open_fail_threshold
+        == FILE_OPEN_FAIL_THRESHOLD_DEFAULT
+    )
+
+    # Disable detailed profiling config if any of the fields are invalid
+    assert not invalid_string_data_profiler_config_parser.detailed_profiling_enabled
+
+    assert not invalid_string_data_profiler_config_parser.config.profile_range.start_step
+    assert not invalid_string_data_profiler_config_parser.config.profile_range.num_steps
+    assert not invalid_string_data_profiler_config_parser.config.profile_range.start_time_in_sec
+    assert not invalid_string_data_profiler_config_parser.config.profile_range.duration_in_sec