From 1d6a1a73b05ade8fa1a35e61054847931e1be287 Mon Sep 17 00:00:00 2001 From: lavaraja padala Date: Sun, 2 Mar 2025 16:47:56 +0000 Subject: [PATCH 1/9] Update serving.py - Added a new function 'configure_logging()' to dynamically set log levels - Utilize TS_LOGLEVEL environment variable to control logging verbosity - Support log levels: OFF, FATAL, ERROR, WARN, INFO, DEBUG, TRACE - Modify log4j2.xml file using sed command based on TS_LOGLEVEL - Handle potential errors during log configuration gracefully - Call configure_logging() before starting TorchServe - Aim to reduce excessive logging and associated CloudWatch costs - Maintain default logging if TS_LOGLEVEL is not set or invalid --- .../serving.py | 33 +++++++++++++++++-- 1 file changed, 31 insertions(+), 2 deletions(-) diff --git a/src/sagemaker_pytorch_serving_container/serving.py b/src/sagemaker_pytorch_serving_container/serving.py index 5e70c961..11924edf 100644 --- a/src/sagemaker_pytorch_serving_container/serving.py +++ b/src/sagemaker_pytorch_serving_container/serving.py @@ -12,6 +12,8 @@ # language governing permissions and limitations under the License. from __future__ import absolute_import +import os +import subprocess from subprocess import CalledProcessError from retrying import retry @@ -20,11 +22,35 @@ HANDLER_SERVICE = handler_service.__file__ +def configure_logging(): + log_levels = { + '0': 'off', + '10': 'fatal', + '20': 'error', + '30': 'warn', + '40': 'info', + '50': 'debug', + '60': 'trace' + } + + ts_loglevel = os.environ.get('TS_LOGLEVEL') + + if ts_loglevel is not None: + if ts_loglevel in log_levels: + try: + log_level = log_levels[ts_loglevel] + subprocess.run(['sed', '-i', f's/info/{log_level}/g', 'etc/log4j2.xml'], check=True) + print(f"Logging level set to {log_level}") + except subprocess.CalledProcessError: + print("Error configuring the logging", file=sys.stderr) + else: + print(f"Invalid TS_LOGLEVEL value: {ts_loglevel}. No changes made to logging configuration.", file=sys.stderr) + else: + print("TS_LOGLEVEL not set. Using default logging configuration.") def _retry_if_error(exception): return isinstance(exception, CalledProcessError) - @retry(stop_max_delay=1000 * 30, retry_on_exception=_retry_if_error) def _start_torchserve(): @@ -33,6 +59,9 @@ def _start_torchserve(): # retry starting mms until it's ready torchserve.start_torchserve(handler_service=HANDLER_SERVICE) - def main(): + configure_logging() _start_torchserve() + +if __name__ == '__main__': + main() From 058008f83d091b39549cb073908890b32ae752f9 Mon Sep 17 00:00:00 2001 From: lavaraja padala Date: Sun, 2 Mar 2025 16:54:14 +0000 Subject: [PATCH 2/9] Update serving.py Added missing import. --- src/sagemaker_pytorch_serving_container/serving.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sagemaker_pytorch_serving_container/serving.py b/src/sagemaker_pytorch_serving_container/serving.py index 11924edf..bfb6fa7d 100644 --- a/src/sagemaker_pytorch_serving_container/serving.py +++ b/src/sagemaker_pytorch_serving_container/serving.py @@ -12,7 +12,7 @@ # language governing permissions and limitations under the License. from __future__ import absolute_import -import os +import os,sys import subprocess from subprocess import CalledProcessError From 66ac023b5b284a5aa69a10fc1861c5c290b373ac Mon Sep 17 00:00:00 2001 From: lavaraja padala Date: Sun, 2 Mar 2025 17:00:26 +0000 Subject: [PATCH 3/9] Update serving.py updating the log4j2 path. --- src/sagemaker_pytorch_serving_container/serving.py | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/src/sagemaker_pytorch_serving_container/serving.py b/src/sagemaker_pytorch_serving_container/serving.py index bfb6fa7d..97bd68bb 100644 --- a/src/sagemaker_pytorch_serving_container/serving.py +++ b/src/sagemaker_pytorch_serving_container/serving.py @@ -34,15 +34,23 @@ def configure_logging(): } ts_loglevel = os.environ.get('TS_LOGLEVEL') + log4j2_path = 'etc/log4j2.xml' + + print(f"Current working directory: {os.getcwd()}") + print(f"log4j2.xml path: {os.path.abspath(log4j2_path)}") + + if not os.path.exists(log4j2_path): + print(f"Error: {log4j2_path} does not exist", file=sys.stderr) + return if ts_loglevel is not None: if ts_loglevel in log_levels: try: log_level = log_levels[ts_loglevel] - subprocess.run(['sed', '-i', f's/info/{log_level}/g', 'etc/log4j2.xml'], check=True) + subprocess.run(['sed', '-i', f's/info/{log_level}/g', log4j2_path], check=True) print(f"Logging level set to {log_level}") - except subprocess.CalledProcessError: - print("Error configuring the logging", file=sys.stderr) + except subprocess.CalledProcessError as e: + print(f"Error configuring the logging: {e}", file=sys.stderr) else: print(f"Invalid TS_LOGLEVEL value: {ts_loglevel}. No changes made to logging configuration.", file=sys.stderr) else: From 67a4ea78b06841091b82d04461cea2e1b55ed946 Mon Sep 17 00:00:00 2001 From: lavaraja padala Date: Sun, 2 Mar 2025 17:05:37 +0000 Subject: [PATCH 4/9] Update serving.py using absolute path to prevent file not found errors. --- src/sagemaker_pytorch_serving_container/serving.py | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/src/sagemaker_pytorch_serving_container/serving.py b/src/sagemaker_pytorch_serving_container/serving.py index 97bd68bb..107d5c45 100644 --- a/src/sagemaker_pytorch_serving_container/serving.py +++ b/src/sagemaker_pytorch_serving_container/serving.py @@ -33,16 +33,21 @@ def configure_logging(): '60': 'trace' } - ts_loglevel = os.environ.get('TS_LOGLEVEL') - log4j2_path = 'etc/log4j2.xml' + # Get the directory of the current script + current_script_path = os.path.abspath(__file__) + current_dir = os.path.dirname(current_script_path) + # Construct the path to log4j2.xml relative to the script location + log4j2_path = os.path.join(os.path.dirname(current_dir), 'etc', 'log4j2.xml') - print(f"Current working directory: {os.getcwd()}") - print(f"log4j2.xml path: {os.path.abspath(log4j2_path)}") + print(f"Current script path: {current_script_path}") + print(f"log4j2.xml path: {log4j2_path}") if not os.path.exists(log4j2_path): print(f"Error: {log4j2_path} does not exist", file=sys.stderr) return + ts_loglevel = os.environ.get('TS_LOGLEVEL') + if ts_loglevel is not None: if ts_loglevel in log_levels: try: From 8f733c2c0dc43f3172cccc71a682d75e625c7089 Mon Sep 17 00:00:00 2001 From: lavaraja padala Date: Sun, 2 Mar 2025 17:18:49 +0000 Subject: [PATCH 5/9] Update serving.py --- src/sagemaker_pytorch_serving_container/serving.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/sagemaker_pytorch_serving_container/serving.py b/src/sagemaker_pytorch_serving_container/serving.py index 107d5c45..c0c75269 100644 --- a/src/sagemaker_pytorch_serving_container/serving.py +++ b/src/sagemaker_pytorch_serving_container/serving.py @@ -35,9 +35,9 @@ def configure_logging(): # Get the directory of the current script current_script_path = os.path.abspath(__file__) - current_dir = os.path.dirname(current_script_path) + # Construct the path to log4j2.xml relative to the script location - log4j2_path = os.path.join(os.path.dirname(current_dir), 'etc', 'log4j2.xml') + log4j2_path = os.path.join(os.path.dirname(current_script_path), 'etc', 'log4j2.xml') print(f"Current script path: {current_script_path}") print(f"log4j2.xml path: {log4j2_path}") From 45eaa802870e7241fe368be31382d93e05fc90c5 Mon Sep 17 00:00:00 2001 From: lavaraja padala Date: Wed, 5 Mar 2025 17:19:54 +0000 Subject: [PATCH 6/9] Update serving.py Fix for https://github.com/aws/sagemaker-pytorch-inference-toolkit/issues/83 --- src/sagemaker_pytorch_serving_container/serving.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/sagemaker_pytorch_serving_container/serving.py b/src/sagemaker_pytorch_serving_container/serving.py index c0c75269..2919b57f 100644 --- a/src/sagemaker_pytorch_serving_container/serving.py +++ b/src/sagemaker_pytorch_serving_container/serving.py @@ -22,6 +22,7 @@ HANDLER_SERVICE = handler_service.__file__ +## added logging function to configure log4j2 loglevel. def configure_logging(): log_levels = { '0': 'off', From 6fdc332f725d42a62cb34d2628338cdf9a4d8b47 Mon Sep 17 00:00:00 2001 From: lavaraja Date: Sun, 9 Mar 2025 11:53:35 +0000 Subject: [PATCH 7/9] unit test for logging added --- test/unit/test_log_config.py | 51 ++++++++++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+) create mode 100644 test/unit/test_log_config.py diff --git a/test/unit/test_log_config.py b/test/unit/test_log_config.py new file mode 100644 index 00000000..3255a2aa --- /dev/null +++ b/test/unit/test_log_config.py @@ -0,0 +1,51 @@ +import unittest +from unittest.mock import patch, MagicMock +import os +from sagemaker_pytorch_serving_container.serving import configure_log_level + +import unittest +from unittest.mock import patch, MagicMock +import os +from your_module import configure_log_level # replace 'your_module' with the actual module name + +class TestLogConfig(unittest.TestCase): + + @patch('os.environ.get') + @patch('subprocess.run') + def test_valid_log_level(self, mock_run, mock_env_get): + mock_env_get.return_value = '20' + mock_run.return_value = MagicMock(returncode=0) + + result = configure_log_level() + + self.assertEqual(result, "Logging level set to error") + mock_run.assert_called_once() + + @patch('os.environ.get') + def test_invalid_log_level(self, mock_env_get): + mock_env_get.return_value = '70' + + result = configure_log_level() + + self.assertEqual(result, "Invalid TS_LOGLEVEL value: 70. No changes made to logging configuration.") + + @patch('os.environ.get') + def test_no_log_level_set(self, mock_env_get): + mock_env_get.return_value = None + + result = configure_log_level() + + self.assertEqual(result, "TS_LOGLEVEL not set. Using default logging configuration.") + + @patch('os.environ.get') + @patch('subprocess.run') + def test_subprocess_error(self, mock_run, mock_env_get): + mock_env_get.return_value = '20' + mock_run.side_effect = subprocess.CalledProcessError(1, 'sed') + + result = configure_log_level() + + self.assertTrue(result.startswith("Error configuring the logging")) + +if __name__ == '__main__': + unittest.main() \ No newline at end of file From 91fc23d08fd3a5c39a3b082141727e9adf48881b Mon Sep 17 00:00:00 2001 From: lavaraja Date: Sun, 9 Mar 2025 12:31:06 +0000 Subject: [PATCH 8/9] modified: test_log_config.py --- test/unit/test_log_config.py | 61 +++++++++++++++++++++++------------- 1 file changed, 39 insertions(+), 22 deletions(-) diff --git a/test/unit/test_log_config.py b/test/unit/test_log_config.py index 3255a2aa..2b07d46d 100644 --- a/test/unit/test_log_config.py +++ b/test/unit/test_log_config.py @@ -1,51 +1,68 @@ import unittest from unittest.mock import patch, MagicMock -import os -from sagemaker_pytorch_serving_container.serving import configure_log_level +import os , subprocess +import sys +import io -import unittest -from unittest.mock import patch, MagicMock -import os -from your_module import configure_log_level # replace 'your_module' with the actual module name +# Add the src directory to the Python path +sys.path.insert(0, os.path.abspath(os.path.join(os.path.dirname(__file__), '..', '..', 'src'))) -class TestLogConfig(unittest.TestCase): +from sagemaker_pytorch_serving_container.serving import configure_logging +class TestLogConfig(unittest.TestCase): + @patch('os.path.exists') @patch('os.environ.get') @patch('subprocess.run') - def test_valid_log_level(self, mock_run, mock_env_get): + def test_valid_log_level(self, mock_run, mock_env_get, mock_exists): + mock_exists.return_value = True mock_env_get.return_value = '20' mock_run.return_value = MagicMock(returncode=0) - result = configure_log_level() + with patch('sys.stdout', new=io.StringIO()) as fake_out: + configure_logging() + self.assertIn("Logging level set to error", fake_out.getvalue()) - self.assertEqual(result, "Logging level set to error") mock_run.assert_called_once() + @patch('os.path.exists') @patch('os.environ.get') - def test_invalid_log_level(self, mock_env_get): + def test_invalid_log_level(self, mock_env_get, mock_exists): + mock_exists.return_value = True mock_env_get.return_value = '70' - result = configure_log_level() - - self.assertEqual(result, "Invalid TS_LOGLEVEL value: 70. No changes made to logging configuration.") + with patch('sys.stderr', new=io.StringIO()) as fake_err: + configure_logging() + self.assertIn("Invalid TS_LOGLEVEL value: 70", fake_err.getvalue()) + @patch('os.path.exists') @patch('os.environ.get') - def test_no_log_level_set(self, mock_env_get): + def test_no_log_level_set(self, mock_env_get, mock_exists): + mock_exists.return_value = True mock_env_get.return_value = None - result = configure_log_level() - - self.assertEqual(result, "TS_LOGLEVEL not set. Using default logging configuration.") + with patch('sys.stdout', new=io.StringIO()) as fake_out: + configure_logging() + self.assertIn("TS_LOGLEVEL not set", fake_out.getvalue()) + @patch('os.path.exists') @patch('os.environ.get') @patch('subprocess.run') - def test_subprocess_error(self, mock_run, mock_env_get): + def test_subprocess_error(self, mock_run, mock_env_get, mock_exists): + mock_exists.return_value = True mock_env_get.return_value = '20' mock_run.side_effect = subprocess.CalledProcessError(1, 'sed') - result = configure_log_level() + with patch('sys.stderr', new=io.StringIO()) as fake_err: + configure_logging() + self.assertIn("Error configuring the logging", fake_err.getvalue()) + + @patch('os.path.exists') + def test_log4j2_file_not_found(self, mock_exists): + mock_exists.return_value = False - self.assertTrue(result.startswith("Error configuring the logging")) + with patch('sys.stderr', new=io.StringIO()) as fake_err: + configure_logging() + self.assertIn("does not exist", fake_err.getvalue()) if __name__ == '__main__': - unittest.main() \ No newline at end of file + unittest.main() From 45d3fb85b62d5c36c84e8ef1ef106c09da31f10b Mon Sep 17 00:00:00 2001 From: lavaraja Date: Sun, 9 Mar 2025 18:05:13 +0000 Subject: [PATCH 9/9] Changed the env varible name from TS_LOGLEVEL to TS_LOG_LEVEL --- src/sagemaker_pytorch_serving_container/serving.py | 12 ++++++------ test/unit/test_log_config.py | 6 +++--- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/sagemaker_pytorch_serving_container/serving.py b/src/sagemaker_pytorch_serving_container/serving.py index 2919b57f..608fbf5d 100644 --- a/src/sagemaker_pytorch_serving_container/serving.py +++ b/src/sagemaker_pytorch_serving_container/serving.py @@ -47,20 +47,20 @@ def configure_logging(): print(f"Error: {log4j2_path} does not exist", file=sys.stderr) return - ts_loglevel = os.environ.get('TS_LOGLEVEL') + ts_log_level = os.environ.get('TS_LOG_LEVEL') - if ts_loglevel is not None: - if ts_loglevel in log_levels: + if ts_log_level is not None: + if ts_log_level in log_levels: try: - log_level = log_levels[ts_loglevel] + log_level = log_levels[ts_log_level] subprocess.run(['sed', '-i', f's/info/{log_level}/g', log4j2_path], check=True) print(f"Logging level set to {log_level}") except subprocess.CalledProcessError as e: print(f"Error configuring the logging: {e}", file=sys.stderr) else: - print(f"Invalid TS_LOGLEVEL value: {ts_loglevel}. No changes made to logging configuration.", file=sys.stderr) + print(f"Invalid TS_LOG_LEVEL value: {ts_log_level}. No changes made to logging configuration.", file=sys.stderr) else: - print("TS_LOGLEVEL not set. Using default logging configuration.") + print("TS_LOG_LEVEL not set. Using default logging configuration.") def _retry_if_error(exception): return isinstance(exception, CalledProcessError) diff --git a/test/unit/test_log_config.py b/test/unit/test_log_config.py index 2b07d46d..096b7f1e 100644 --- a/test/unit/test_log_config.py +++ b/test/unit/test_log_config.py @@ -1,6 +1,6 @@ import unittest from unittest.mock import patch, MagicMock -import os , subprocess +import os, subprocess import sys import io @@ -32,7 +32,7 @@ def test_invalid_log_level(self, mock_env_get, mock_exists): with patch('sys.stderr', new=io.StringIO()) as fake_err: configure_logging() - self.assertIn("Invalid TS_LOGLEVEL value: 70", fake_err.getvalue()) + self.assertIn("Invalid TS_LOG_LEVEL value: 70", fake_err.getvalue()) @patch('os.path.exists') @patch('os.environ.get') @@ -42,7 +42,7 @@ def test_no_log_level_set(self, mock_env_get, mock_exists): with patch('sys.stdout', new=io.StringIO()) as fake_out: configure_logging() - self.assertIn("TS_LOGLEVEL not set", fake_out.getvalue()) + self.assertIn("TS_LOG_LEVEL not set", fake_out.getvalue()) @patch('os.path.exists') @patch('os.environ.get')