Disable dynamic trace file (#7106) (#7113)

yinggeh · web-flow · commit a7ce16af65b5 · 2024-04-15T08:43:26.000-07:00
diff --git a/qa/L0_trace/test.sh b/qa/L0_trace/test.sh
@@ -60,6 +60,7 @@ SERVER=/opt/tritonserver/bin/tritonserver
 source ../common/util.sh
 
 rm -f *.log
+rm -f *.log.*
 rm -fr $MODELSDIR && mkdir -p $MODELSDIR
 
 # set up simple and global_simple model using MODELBASE
@@ -232,27 +233,18 @@ set +e
 
 # Add trace setting for 'simple' via trace API, first use the same trace file
 update_trace_setting "simple" '{"trace_file":"global_trace.log"}'
-assert_curl_success "Failed to modify trace settings for 'simple' model"
+assert_curl_failure "trace_file updated through network protocol expects an error"
 
 # Check if the current setting is returned (not specified setting from global)
-if [ `grep -c "\"trace_level\":\[\"TIMESTAMPS\"\]" ./curl.out` != "1" ]; then
-    RET=1
-fi
-if [ `grep -c "\"trace_rate\":\"6\"" ./curl.out` != "1" ]; then
-    RET=1
-fi
-if [ `grep -c "\"log_frequency\":\"0\"" ./curl.out` != "1" ]; then
-    RET=1
-fi
-if [ `grep -c "\"trace_file\":\"global_trace.log\"" ./curl.out` != "1" ]; then
+if [ `grep -c "\"error\":\"trace file location can not be updated through network protocol\"" ./curl.out` != "1" ]; then
     RET=1
 fi
 if [ `grep -c "\"trace_mode\":\"triton\"" ./curl.out` != "1" ]; then
     RET=1
 fi
 
 # Use a different name
-update_trace_setting "simple" '{"trace_file":"simple_trace.log","log_frequency":"2"}'
+update_trace_setting "simple" '{"log_frequency":"2"}'
 assert_curl_success "Failed to modify trace settings for 'simple' model"
 
 # Check if the current setting is returned (not specified setting from global)
@@ -268,7 +260,7 @@ fi
 if [ `grep -c "\"log_frequency\":\"2\"" ./curl.out` != "1" ]; then
     RET=1
 fi
-if [ `grep -c "\"trace_file\":\"simple_trace.log\"" ./curl.out` != "1" ]; then
+if [ `grep -c "\"trace_file\":\"global_trace.log\"" ./curl.out` != "1" ]; then
     RET=1
 fi
 if [ `grep -c "\"trace_mode\":\"triton\"" ./curl.out` != "1" ]; then
@@ -284,35 +276,35 @@ wait $SERVER_PID
 
 set +e
 
-if [ -f ./global_trace.log ]; then
-    echo -e "\n***\n*** Test Failed, unexpected generation of global_trace.log\n***"
+if [ -f ./simple_trace.log ]; then
+    echo -e "\n***\n*** Test Failed, unexpected generation of simple_trace.log\n***"
     RET=1
 fi
 
-$TRACE_SUMMARY -t simple_trace.log.0 > summary_simple_trace.log.0
+$TRACE_SUMMARY -t global_trace.log.0 > summary_global_trace.log.0
 
-if [ `grep -c "COMPUTE_INPUT_END" summary_simple_trace.log.0` != "2" ]; then
-    cat summary_simple_trace.log.0
+if [ `grep -c "COMPUTE_INPUT_END" summary_global_trace.log.0` != "2" ]; then
+    cat summary_global_trace.log.0
     echo -e "\n***\n*** Test Failed\n***"
     RET=1
 fi
 
-if [ `grep -c ^simple summary_simple_trace.log.0` != "2" ]; then
-    cat summary_simple_trace.log.0
+if [ `grep -c ^simple summary_global_trace.log.0` != "2" ]; then
+    cat summary_global_trace.log.0
     echo -e "\n***\n*** Test Failed\n***"
     RET=1
 fi
 
-$TRACE_SUMMARY -t simple_trace.log.1 > summary_simple_trace.log.1
+$TRACE_SUMMARY -t global_trace.log.1 > summary_global_trace.log.1
 
-if [ `grep -c "COMPUTE_INPUT_END" summary_simple_trace.log.1` != "1" ]; then
-    cat summary_simple_trace.log.1
+if [ `grep -c "COMPUTE_INPUT_END" summary_global_trace.log.1` != "1" ]; then
+    cat summary_global_trace.log.1
     echo -e "\n***\n*** Test Failed\n***"
     RET=1
 fi
 
-if [ `grep -c ^simple summary_simple_trace.log.1` != "1" ]; then
-    cat summary_simple_trace.log.1
+if [ `grep -c ^simple summary_global_trace.log.1` != "1" ]; then
+    cat summary_global_trace.log.1
     echo -e "\n***\n*** Test Failed\n***"
     RET=1
 fi
@@ -332,10 +324,10 @@ fi
 set +e
 
 # Add model setting and update it
-update_trace_setting "simple" '{"trace_file":"update_trace.log","trace_rate":"1"}'
+update_trace_setting "simple" '{"trace_rate":"1"}'
 assert_curl_success "Failed to modify trace settings for 'simple' model"
 
-update_trace_setting "simple" '{"trace_file":"update_trace.log","trace_level":["OFF"]}'
+update_trace_setting "simple" '{"trace_level":["OFF"]}'
 assert_curl_success "Failed to modify trace settings for 'simple' model"
 
 # Check if the current setting is returned
@@ -351,7 +343,7 @@ fi
 if [ `grep -c "\"log_frequency\":\"0\"" ./curl.out` != "1" ]; then
     RET=1
 fi
-if [ `grep -c "\"trace_file\":\"update_trace.log\"" ./curl.out` != "1" ]; then
+if [ `grep -c "\"trace_file\":\"global_trace.log\"" ./curl.out` != "1" ]; then
     RET=1
 fi
 if [ `grep -c "\"trace_mode\":\"triton\"" ./curl.out` != "1" ]; then
@@ -365,7 +357,7 @@ rm -f ./curl.out
 set +e
 
 # Clear trace setting by explicitly asking removal for every field except 'trace_rate'
-update_trace_setting "simple" '{"trace_file":null,"trace_level":null}'
+update_trace_setting "simple" '{"trace_level":null}'
 assert_curl_success "Failed to modify trace settings for 'simple' model"
 
 # Check if the current setting (global) is returned
@@ -510,8 +502,8 @@ if [ `grep -c "\"trace_mode\":\"triton\"" ./curl.out` != "1" ]; then
 fi
 
 # Check if the indexed file has been generated when trace count reaches 0
-if [ -f ./global_trace.log.0 ]; then
-    echo -e "\n***\n*** Test Failed, expect generation of global_trace.log.0 before stopping server\n***"
+if [ ! -f ./global_count.log.0 ]; then
+    echo -e "\n***\n*** Test Failed, expect generation of global_count.log.0 before stopping server\n***"
     RET=1
 fi
 
diff --git a/qa/L0_trace/trace_endpoint_test.py b/qa/L0_trace/trace_endpoint_test.py
@@ -38,6 +38,7 @@
 import tritonclient.grpc as grpcclient
 import tritonclient.http as httpclient
 from google.protobuf import json_format
+from tritonclient.utils import InferenceServerException
 
 
 # Similar set up as dynamic batcher tests
@@ -49,7 +50,6 @@ def tearDown(self):
         # tearDown() is properly executed and not affecting start state of
         # other test cases
         clear_settings = {
-            "trace_file": None,
             "trace_level": None,
             "trace_rate": None,
             "trace_count": None,
@@ -157,23 +157,26 @@ def test_http_update_settings(self):
         self.check_server_initial_state()
 
         expected_first_model_settings = {
-            "trace_file": "model.log",
+            "trace_file": "global_unittest.log",
             "trace_level": ["TIMESTAMPS"],
             "trace_rate": "1",
             "trace_count": "-1",
             "log_frequency": "0",
             "trace_mode": "triton",
         }
+        expected_first_model_response = {
+            "error": "trace file location can not be updated through network protocol"
+        }
         expected_second_model_settings = {
-            "trace_file": "model.log",
+            "trace_file": "global_unittest.log",
             "trace_level": ["TIMESTAMPS", "TENSORS"],
             "trace_rate": "1",
             "trace_count": "-1",
             "log_frequency": "0",
             "trace_mode": "triton",
         }
         expected_global_settings = {
-            "trace_file": "another.log",
+            "trace_file": "global_unittest.log",
             "trace_level": ["TIMESTAMPS", "TENSORS"],
             "trace_rate": "1",
             "trace_count": "-1",
@@ -183,17 +186,20 @@ def test_http_update_settings(self):
 
         model_update_settings = {"trace_file": "model.log"}
         global_update_settings = {
-            "trace_file": "another.log",
             "trace_level": ["TIMESTAMPS", "TENSORS"],
         }
 
         triton_client = httpclient.InferenceServerClient("localhost:8000")
-        self.assertEqual(
-            expected_first_model_settings,
+        with self.assertRaisesRegex(
+            InferenceServerException, expected_first_model_response["error"]
+        ) as e:
             triton_client.update_trace_settings(
                 model_name="simple", settings=model_update_settings
-            ),
-            "Unexpected updated model trace settings",
+            )
+        self.assertEqual(
+            expected_first_model_settings,
+            triton_client.get_trace_settings(model_name="simple"),
+            "Unexpected model trace settings after global update",
         )
         # Note that 'trace_level' may be mismatch due to the order of
         # the levels listed, currently we assume the order is the same
@@ -230,7 +236,7 @@ def test_grpc_update_settings(self):
             json.dumps(
                 {
                     "settings": {
-                        "trace_file": {"value": ["model.log"]},
+                        "trace_file": {"value": ["global_unittest.log"]},
                         "trace_level": {"value": ["TIMESTAMPS"]},
                         "trace_rate": {"value": ["1"]},
                         "trace_count": {"value": ["-1"]},
@@ -247,7 +253,7 @@ def test_grpc_update_settings(self):
             json.dumps(
                 {
                     "settings": {
-                        "trace_file": {"value": ["model.log"]},
+                        "trace_file": {"value": ["global_unittest.log"]},
                         "trace_level": {"value": ["TIMESTAMPS", "TENSORS"]},
                         "trace_rate": {"value": ["1"]},
                         "trace_count": {"value": ["-1"]},
@@ -264,7 +270,7 @@ def test_grpc_update_settings(self):
             json.dumps(
                 {
                     "settings": {
-                        "trace_file": {"value": ["another.log"]},
+                        "trace_file": {"value": ["global_unittest.log"]},
                         "trace_level": {"value": ["TIMESTAMPS", "TENSORS"]},
                         "trace_rate": {"value": ["1"]},
                         "trace_count": {"value": ["-1"]},
@@ -278,18 +284,10 @@ def test_grpc_update_settings(self):
 
         model_update_settings = {"trace_file": "model.log"}
         global_update_settings = {
-            "trace_file": "another.log",
             "trace_level": ["TIMESTAMPS", "TENSORS"],
         }
 
         triton_client = grpcclient.InferenceServerClient("localhost:8001")
-        self.assertEqual(
-            expected_first_model_settings,
-            triton_client.update_trace_settings(
-                model_name="simple", settings=model_update_settings
-            ),
-            "Unexpected updated model trace settings",
-        )
         # Note that 'trace_level' may be mismatch due to the order of
         # the levels listed, currently we assume the order is the same
         # for simplicity. But the order shouldn't be enforced and this checking
diff --git a/src/grpc/grpc_server.cc b/src/grpc/grpc_server.cc
@@ -1210,18 +1210,11 @@ CommonHandler::RegisterTrace()
         static std::string setting_name = "trace_file";
         auto it = request.settings().find(setting_name);
         if (it != request.settings().end()) {
-          if (it->second.value().size() == 0) {
-            new_setting.clear_filepath_ = true;
-          } else if (it->second.value().size() == 1) {
-            filepath = it->second.value()[0];
-            new_setting.filepath_ = &filepath;
-          } else {
-            err = TRITONSERVER_ErrorNew(
-                TRITONSERVER_ERROR_INVALID_ARG,
-                (std::string("expect only 1 value for '") + setting_name + "'")
-                    .c_str());
-            GOTO_IF_ERR(err, earlyexit);
-          }
+          err = TRITONSERVER_ErrorNew(
+              TRITONSERVER_ERROR_UNSUPPORTED,
+              "trace file location can not be updated through network "
+              "protocol");
+          GOTO_IF_ERR(err, earlyexit);
         }
       }
       {
diff --git a/src/http_server.cc b/src/http_server.cc
@@ -1856,12 +1856,11 @@ HTTPAPIServer::HandleTrace(evhtp_request_t* req, const std::string& model_name)
 
     triton::common::TritonJson::Value setting_json;
     if (request.Find("trace_file", &setting_json)) {
-      if (setting_json.IsNull()) {
-        new_setting.clear_filepath_ = true;
-      } else {
-        RETURN_AND_RESPOND_IF_ERR(req, setting_json.AsString(&filepath));
-        new_setting.filepath_ = &filepath;
-      }
+      RETURN_AND_RESPOND_IF_ERR(
+          req, TRITONSERVER_ErrorNew(
+                   TRITONSERVER_ERROR_UNSUPPORTED,
+                   "trace file location can not be updated through network "
+                   "protocol"));
     }
     if (request.Find("trace_level", &setting_json)) {
       if (setting_json.IsNull()) {
diff --git a/src/tracer.cc b/src/tracer.cc
@@ -162,10 +162,7 @@ TraceManager::UpdateTraceSettingInternal(
                current_setting->log_frequency_specified_) ||
               (new_setting.log_frequency_ != nullptr)));
   const bool filepath_specified =
-      (new_setting.clear_filepath_ ? false
-                                   : (((current_setting != nullptr) &&
-                                       current_setting->filepath_specified_) ||
-                                      (new_setting.filepath_ != nullptr)));
+      (((current_setting != nullptr) && current_setting->filepath_specified_));
 
   if (level_specified) {
     level = (new_setting.level_ != nullptr) ? *new_setting.level_
@@ -185,9 +182,7 @@ TraceManager::UpdateTraceSettingInternal(
                         : current_setting->log_frequency_;
   }
   if (filepath_specified) {
-    filepath = (new_setting.filepath_ != nullptr)
-                   ? *new_setting.filepath_
-                   : current_setting->file_->FileName();
+    filepath = current_setting->file_->FileName();
   }
 
   // Some special case when updating model setting
diff --git a/src/tracer.h b/src/tracer.h
@@ -106,8 +106,7 @@ class TraceManager {
     NewSetting()
         : clear_level_(false), level_(nullptr), clear_rate_(false),
           rate_(nullptr), clear_count_(false), count_(nullptr),
-          clear_log_frequency_(false), log_frequency_(nullptr),
-          clear_filepath_(false), filepath_(nullptr), mode_(nullptr),
+          clear_log_frequency_(false), log_frequency_(nullptr), mode_(nullptr),
           config_map_(nullptr)
     {
     }
@@ -123,9 +122,6 @@ class TraceManager {
     bool clear_log_frequency_;
     const uint32_t* log_frequency_;
 
-    bool clear_filepath_;
-    const std::string* filepath_;
-
     const InferenceTraceMode* mode_;
 
     const TraceConfigMap* config_map_;
