From 517c6f034a8a750244ddc71395811ebadfef3b4d Mon Sep 17 00:00:00 2001 From: lkomali Date: Wed, 14 Aug 2024 23:33:07 -0700 Subject: [PATCH 01/16] Add CLI argument for server-metrics-url --- genai-perf/genai_perf/parser.py | 57 +++++++++++++++++- genai-perf/genai_perf/wrapper.py | 2 + genai-perf/tests/test_cli.py | 72 ++++++++++++++++++++++ genai-perf/tests/test_json_exporter.py | 1 + genai-perf/tests/test_profile_handler.py | 76 ++++++++++++++++++++++++ 5 files changed, 206 insertions(+), 2 deletions(-) create mode 100755 genai-perf/tests/test_profile_handler.py diff --git a/genai-perf/genai_perf/parser.py b/genai-perf/genai_perf/parser.py index b1405ac6..0b134298 100644 --- a/genai-perf/genai_perf/parser.py +++ b/genai-perf/genai_perf/parser.py @@ -31,6 +31,7 @@ from enum import Enum, auto from pathlib import Path from typing import Tuple +from urllib.parse import urlparse import genai_perf.logging as logging import genai_perf.utils as utils @@ -269,6 +270,49 @@ def _check_goodput_args(args): return args +def _is_valid_url(url: str, parser: argparse.ArgumentParser) -> None: + """ + Validates a URL to ensure it meets the following criteria: + - The scheme must be 'http' or 'https'. + - The netloc (domain) must be present. + - The path must contain '/metrics'. + + Raises: + `parser.error()` if the URL is invalid. + + The URL structure is expected to follow: + :///;?# + """ + parsed_url = urlparse(url) + + if ( + parsed_url.scheme not in ["http", "https"] + or not parsed_url.netloc + or "/metrics" not in parsed_url.path + or parsed_url.port is None + ): + parser.error( + "The URL passed for --server-metrics-url is invalid. " + "It must use 'http' or 'https', have a valid domain, " + "and contain '/metrics' in the path. The expected structure is: " + ":///;?#" + ) + + +def _check_server_metrics_url( + parser: argparse.ArgumentParser, args: argparse.Namespace +) -> argparse.Namespace: + """ + Checks if the server metrics URL passed is valid + """ + + # Check if the URL is valid and contains the expected path + if args.service_kind == "triton" and args.server_metrics_url: + _is_valid_url(args.server_metrics_url, parser) + + return args + + def _set_artifact_paths(args: argparse.Namespace) -> argparse.Namespace: """ Set paths for all the artifacts. @@ -650,6 +694,14 @@ def _add_endpoint_args(parser): "you must specify an api via --endpoint-type.", ) + endpoint_group.add_argument( + "--server-metrics-url", + type=str, + default=None, + required=False, + help="The full URL to access the server metrics endpoint. This argument is required if the metrics are available on a different machine than localhost (where GenAI-Perf is running).", + ) + endpoint_group.add_argument( "--streaming", action="store_true", @@ -830,9 +882,9 @@ def profile_handler(args, extra_args): telemetry_data_collector = None if args.service_kind == "triton": - # TPA-275: pass server url as a CLI option in non-default case + server_metrics_url = args.server_metrics_url or DEFAULT_TRITON_METRICS_URL telemetry_data_collector = TritonTelemetryDataCollector( - server_metrics_url=DEFAULT_TRITON_METRICS_URL + server_metrics_url=server_metrics_url ) Profiler.run( @@ -888,6 +940,7 @@ def refine_args( args = _check_conditional_args(parser, args) args = _check_image_input_args(parser, args) args = _check_load_manager_args(args) + args = _check_server_metrics_url(parser, args) args = _set_artifact_paths(args) args = _check_goodput_args(args) elif args.subcommand == Subcommand.COMPARE.to_lowercase(): diff --git a/genai-perf/genai_perf/wrapper.py b/genai-perf/genai_perf/wrapper.py index d87e3485..95583ff1 100644 --- a/genai-perf/genai_perf/wrapper.py +++ b/genai-perf/genai_perf/wrapper.py @@ -102,6 +102,7 @@ def build_cmd(args: Namespace, extra_args: Optional[List[str]] = None) -> List[s "image_height_stddev", "image_format", "goodput", + "server_metrics_url", ] utils.remove_file(args.profile_export_file) @@ -169,3 +170,4 @@ def run( finally: if telemetry_data_collector is not None: telemetry_data_collector.stop() + print(telemetry_data_collector.metrics) diff --git a/genai-perf/tests/test_cli.py b/genai-perf/tests/test_cli.py index 71bb9ca3..ef4841d4 100644 --- a/genai-perf/tests/test_cli.py +++ b/genai-perf/tests/test_cli.py @@ -917,3 +917,75 @@ def test_get_extra_inputs_as_dict(self, extra_inputs_list, expected_dict): namespace.extra_inputs = extra_inputs_list actual_dict = parser.get_extra_inputs_as_dict(namespace) assert actual_dict == expected_dict + + TEST_TRITON_METRICS_URL = "http://custom-metrics-url:8002/metrics" + INVALID_URL = "invalid_url" + INVALID_URL_ERROR_MESSAGE = ( + "The URL passed for --server-metrics-url is invalid. " + "It must use 'http' or 'https', have a valid domain, " + "and contain '/metrics' in the path. The expected structure is: " + ":///;?#" + ) + + @pytest.mark.parametrize( + "args_list, expected_url, expected_error", + [ + # Test with a custom URL + ( + [ + "genai-perf", + "profile", + "--model", + "test_model", + "--service-kind", + "triton", + "--server-metrics-url", + TEST_TRITON_METRICS_URL, + ], + TEST_TRITON_METRICS_URL, + None, + ), + # Test with default URL + ( + [ + "genai-perf", + "profile", + "--model", + "test_model", + "--service-kind", + "triton", + ], + None, + None, + ), + # Test with invalid URL + ( + [ + "genai-perf", + "profile", + "--model", + "test_model", + "--service-kind", + "triton", + "--server-metrics-url", + INVALID_URL, + ], + None, + INVALID_URL_ERROR_MESSAGE, + ), + ], + ) + def test_server_metrics_url_for_triton( + self, args_list, expected_url, expected_error, monkeypatch, capsys + ): + monkeypatch.setattr("sys.argv", args_list) + + if expected_error: + with pytest.raises(SystemExit) as excinfo: + parser.parse_args() + captured = capsys.readouterr() + assert expected_error in captured.err + assert excinfo.value.code != 0 + else: + args, _ = parser.parse_args() + assert args.server_metrics_url == expected_url diff --git a/genai-perf/tests/test_json_exporter.py b/genai-perf/tests/test_json_exporter.py index 0367b435..2eead632 100644 --- a/genai-perf/tests/test_json_exporter.py +++ b/genai-perf/tests/test_json_exporter.py @@ -325,6 +325,7 @@ def test_generate_json_custom_export( "endpoint": null, "endpoint_type": null, "service_kind": "triton", + "server_metrics_url": null, "streaming": true, "u": null, "input_dataset": null, diff --git a/genai-perf/tests/test_profile_handler.py b/genai-perf/tests/test_profile_handler.py new file mode 100755 index 00000000..84be8cc9 --- /dev/null +++ b/genai-perf/tests/test_profile_handler.py @@ -0,0 +1,76 @@ +#!/usr/bin/env python3 +# +# Copyright 2024, NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# +# Redistribution and use in source and binary forms, with or without +# modification, are permitted provided that the following conditions +# are met: +# * Redistributions of source code must retain the above copyright +# notice, this list of conditions and the following disclaimer. +# * Redistributions in binary form must reproduce the above copyright +# notice, this list of conditions and the following disclaimer in the +# documentation and/or other materials provided with the distribution. +# * Neither the name of NVIDIA CORPORATION nor the names of its +# contributors may be used to endorse or promote products derived +# from this software without specific prior written permission. +# +# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS ``AS IS'' AND ANY +# EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE +# IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR +# PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR +# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, +# EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, +# PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR +# PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY +# OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT +# (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE +# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + +from unittest.mock import patch + +import pytest +from genai_perf.constants import DEFAULT_TRITON_METRICS_URL +from genai_perf.parser import profile_handler +from genai_perf.telemetry_data.triton_telemetry_data_collector import ( + TritonTelemetryDataCollector, +) + +TRITON_TEST_METRICS_URL = "http://tritonmetrics.com:8080/metrics" + + +# Parameterized fixture +@pytest.fixture( + params=[ + {"service_kind": "triton", "server_metrics_url": TRITON_TEST_METRICS_URL}, + {"service_kind": "triton", "server_metrics_url": None}, # Default URL case + ] +) +def mock_args(request): + params = request.param + + class MockArgs: + def __init__(self, service_kind, server_metrics_url): + self.service_kind = service_kind + self.server_metrics_url = server_metrics_url + + return MockArgs(params["service_kind"], params["server_metrics_url"]) + + +@patch("genai_perf.wrapper.Profiler.run") +def test_profile_handler_creates_telemetry_collector_triton( + mock_profiler_run, mock_args +): + profile_handler(mock_args, extra_args={}) + mock_profiler_run.assert_called_once() + + args, kwargs = mock_profiler_run.call_args + telemetry_collector = kwargs.get("telemetry_data_collector") + + expected_url = ( + TRITON_TEST_METRICS_URL + if mock_args.server_metrics_url == TRITON_TEST_METRICS_URL + else DEFAULT_TRITON_METRICS_URL + ) + + assert isinstance(telemetry_collector, TritonTelemetryDataCollector) + assert telemetry_collector.metrics_url == expected_url From bb50ce27f0a9cc2d131592b0f5c81f91a0761de4 Mon Sep 17 00:00:00 2001 From: lkomali Date: Wed, 14 Aug 2024 23:37:40 -0700 Subject: [PATCH 02/16] Remove print statement --- genai-perf/genai_perf/wrapper.py | 1 - genai-perf/tests/test_profile_handler.py | 0 2 files changed, 1 deletion(-) mode change 100755 => 100644 genai-perf/tests/test_profile_handler.py diff --git a/genai-perf/genai_perf/wrapper.py b/genai-perf/genai_perf/wrapper.py index 95583ff1..96e9dbdf 100644 --- a/genai-perf/genai_perf/wrapper.py +++ b/genai-perf/genai_perf/wrapper.py @@ -170,4 +170,3 @@ def run( finally: if telemetry_data_collector is not None: telemetry_data_collector.stop() - print(telemetry_data_collector.metrics) diff --git a/genai-perf/tests/test_profile_handler.py b/genai-perf/tests/test_profile_handler.py old mode 100755 new mode 100644 From fddf58f26ac70c3b55f05a51d97208e5f9ef8788 Mon Sep 17 00:00:00 2001 From: lkomali Date: Wed, 14 Aug 2024 23:40:53 -0700 Subject: [PATCH 03/16] Fix pre-commit error --- genai-perf/tests/test_profile_handler.py | 0 1 file changed, 0 insertions(+), 0 deletions(-) mode change 100644 => 100755 genai-perf/tests/test_profile_handler.py diff --git a/genai-perf/tests/test_profile_handler.py b/genai-perf/tests/test_profile_handler.py old mode 100644 new mode 100755 From d135c19908aab8eb31bbc908d687e3d40391a0c2 Mon Sep 17 00:00:00 2001 From: Harshini Komali <157742537+lkomali@users.noreply.github.com> Date: Tue, 27 Aug 2024 11:02:03 -0700 Subject: [PATCH 04/16] Graceful handling of exception thrown by genai-perf if metrics url is unreachable (#47) * Fix exception thrown by genai-perf if metrics url is unreachable * Fix comments * Fix comments * Fix comments * Add space in error message --- genai-perf/tests/test_telemetry_data_collector.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/genai-perf/tests/test_telemetry_data_collector.py b/genai-perf/tests/test_telemetry_data_collector.py index 781f790f..5ca264ed 100755 --- a/genai-perf/tests/test_telemetry_data_collector.py +++ b/genai-perf/tests/test_telemetry_data_collector.py @@ -42,6 +42,7 @@ class TestTelemetryDataCollector: TEST_SERVER_URL = "http://testserver:8080/metrics" + TRITON_METRICS_RESPONSE = """\ TRITON_METRICS_RESPONSE = """\ nv_gpu_power_usage{gpu="0",uuid="GPU-1234"} 123.45 nv_gpu_power_usage{gpu="1",uuid="GPU-5678"} 234.56 @@ -99,6 +100,7 @@ def test_fetch_metrics_success( mock_response = MagicMock() mock_response.status_code = 200 mock_response.text = self.TRITON_METRICS_RESPONSE + mock_response.text = self.TRITON_METRICS_RESPONSE mock_requests_get.return_value = mock_response result = collector._fetch_metrics() @@ -106,6 +108,7 @@ def test_fetch_metrics_success( mock_requests_get.assert_called_once_with(self.TEST_SERVER_URL) assert result == self.TRITON_METRICS_RESPONSE + assert result == self.TRITON_METRICS_RESPONSE @patch("requests.get") def test_fetch_metrics_failure( @@ -126,19 +129,23 @@ def test_collect_metrics( ) -> None: mock_fetch_metrics.return_value = self.TRITON_METRICS_RESPONSE + mock_fetch_metrics.return_value = self.TRITON_METRICS_RESPONSE with patch.object( collector, "_process_and_update_metrics", new_callable=MagicMock ) as mock_process_and_update_metrics: + collector._stop_event = MagicMock() collector._stop_event.is_set = MagicMock(side_effect=[False, True]) + collector._stop_event.is_set = MagicMock(side_effect=[False, True]) collector._collect_metrics() mock_fetch_metrics.assert_called_once() mock_process_and_update_metrics.assert_called_once_with( self.TRITON_METRICS_RESPONSE + self.TRITON_METRICS_RESPONSE ) mock_sleep.assert_called_once() From 91bb0dc2400564864d09f7025272169f0b2c028c Mon Sep 17 00:00:00 2001 From: lkomali Date: Wed, 14 Aug 2024 23:33:07 -0700 Subject: [PATCH 05/16] Add CLI argument for server-metrics-url --- genai-perf/genai_perf/wrapper.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/genai-perf/genai_perf/wrapper.py b/genai-perf/genai_perf/wrapper.py index 96e9dbdf..15fcd99b 100644 --- a/genai-perf/genai_perf/wrapper.py +++ b/genai-perf/genai_perf/wrapper.py @@ -101,6 +101,7 @@ def build_cmd(args: Namespace, extra_args: Optional[List[str]] = None) -> List[s "image_height_mean", "image_height_stddev", "image_format", + "server_metrics_url", "goodput", "server_metrics_url", ] @@ -170,3 +171,4 @@ def run( finally: if telemetry_data_collector is not None: telemetry_data_collector.stop() + print(telemetry_data_collector.metrics) From 96d9dff171721941908d4ee0176427e00534e491 Mon Sep 17 00:00:00 2001 From: lkomali Date: Tue, 27 Aug 2024 11:36:15 -0700 Subject: [PATCH 06/16] Fix pre-commit error and remove duplicates --- genai-perf/tests/test_telemetry_data_collector.py | 7 ------- 1 file changed, 7 deletions(-) diff --git a/genai-perf/tests/test_telemetry_data_collector.py b/genai-perf/tests/test_telemetry_data_collector.py index 5ca264ed..781f790f 100755 --- a/genai-perf/tests/test_telemetry_data_collector.py +++ b/genai-perf/tests/test_telemetry_data_collector.py @@ -42,7 +42,6 @@ class TestTelemetryDataCollector: TEST_SERVER_URL = "http://testserver:8080/metrics" - TRITON_METRICS_RESPONSE = """\ TRITON_METRICS_RESPONSE = """\ nv_gpu_power_usage{gpu="0",uuid="GPU-1234"} 123.45 nv_gpu_power_usage{gpu="1",uuid="GPU-5678"} 234.56 @@ -100,7 +99,6 @@ def test_fetch_metrics_success( mock_response = MagicMock() mock_response.status_code = 200 mock_response.text = self.TRITON_METRICS_RESPONSE - mock_response.text = self.TRITON_METRICS_RESPONSE mock_requests_get.return_value = mock_response result = collector._fetch_metrics() @@ -108,7 +106,6 @@ def test_fetch_metrics_success( mock_requests_get.assert_called_once_with(self.TEST_SERVER_URL) assert result == self.TRITON_METRICS_RESPONSE - assert result == self.TRITON_METRICS_RESPONSE @patch("requests.get") def test_fetch_metrics_failure( @@ -129,23 +126,19 @@ def test_collect_metrics( ) -> None: mock_fetch_metrics.return_value = self.TRITON_METRICS_RESPONSE - mock_fetch_metrics.return_value = self.TRITON_METRICS_RESPONSE with patch.object( collector, "_process_and_update_metrics", new_callable=MagicMock ) as mock_process_and_update_metrics: - collector._stop_event = MagicMock() collector._stop_event.is_set = MagicMock(side_effect=[False, True]) - collector._stop_event.is_set = MagicMock(side_effect=[False, True]) collector._collect_metrics() mock_fetch_metrics.assert_called_once() mock_process_and_update_metrics.assert_called_once_with( self.TRITON_METRICS_RESPONSE - self.TRITON_METRICS_RESPONSE ) mock_sleep.assert_called_once() From 37b6af0a613e53ddb9bec3b63f01fe1a06d61740 Mon Sep 17 00:00:00 2001 From: lkomali Date: Tue, 27 Aug 2024 12:17:09 -0700 Subject: [PATCH 07/16] Remove unnecessary print --- genai-perf/genai_perf/wrapper.py | 1 - 1 file changed, 1 deletion(-) diff --git a/genai-perf/genai_perf/wrapper.py b/genai-perf/genai_perf/wrapper.py index 15fcd99b..6a73582d 100644 --- a/genai-perf/genai_perf/wrapper.py +++ b/genai-perf/genai_perf/wrapper.py @@ -171,4 +171,3 @@ def run( finally: if telemetry_data_collector is not None: telemetry_data_collector.stop() - print(telemetry_data_collector.metrics) From 8cff8ead4948da65ee42ec2c4a8ddd2d9393d417 Mon Sep 17 00:00:00 2001 From: lkomali Date: Tue, 27 Aug 2024 14:22:09 -0700 Subject: [PATCH 08/16] Fix comments --- genai-perf/genai_perf/parser.py | 10 ++++++---- genai-perf/genai_perf/wrapper.py | 19 +++++++++---------- 2 files changed, 15 insertions(+), 14 deletions(-) diff --git a/genai-perf/genai_perf/parser.py b/genai-perf/genai_perf/parser.py index 0b134298..2c6671ed 100644 --- a/genai-perf/genai_perf/parser.py +++ b/genai-perf/genai_perf/parser.py @@ -270,7 +270,7 @@ def _check_goodput_args(args): return args -def _is_valid_url(url: str, parser: argparse.ArgumentParser) -> None: +def _is_valid_url(parser: argparse.ArgumentParser, url: str) -> None: """ Validates a URL to ensure it meets the following criteria: - The scheme must be 'http' or 'https'. @@ -293,7 +293,7 @@ def _is_valid_url(url: str, parser: argparse.ArgumentParser) -> None: ): parser.error( "The URL passed for --server-metrics-url is invalid. " - "It must use 'http' or 'https', have a valid domain, " + "It must use 'http' or 'https', have a valid domain and port, " "and contain '/metrics' in the path. The expected structure is: " ":///;?#" ) @@ -308,7 +308,7 @@ def _check_server_metrics_url( # Check if the URL is valid and contains the expected path if args.service_kind == "triton" and args.server_metrics_url: - _is_valid_url(args.server_metrics_url, parser) + _is_valid_url(parser, args.server_metrics_url) return args @@ -699,7 +699,9 @@ def _add_endpoint_args(parser): type=str, default=None, required=False, - help="The full URL to access the server metrics endpoint. This argument is required if the metrics are available on a different machine than localhost (where GenAI-Perf is running).", + help="The full URL to access the server metrics endpoint. " + "This argument is required if the metrics are available on " + "a different machine than localhost (where GenAI-Perf is running).", ) endpoint_group.add_argument( diff --git a/genai-perf/genai_perf/wrapper.py b/genai-perf/genai_perf/wrapper.py index 6a73582d..1d8e23de 100644 --- a/genai-perf/genai_perf/wrapper.py +++ b/genai-perf/genai_perf/wrapper.py @@ -74,6 +74,12 @@ def build_cmd(args: Namespace, extra_args: Optional[List[str]] = None) -> List[s "formatted_model_name", "func", "generate_plots", + "goodput", + "image_format", + "image_height_mean", + "image_height_stddev", + "image_width_mean", + "image_width_stddev", "input_dataset", "input_file", "input_format", @@ -81,29 +87,22 @@ def build_cmd(args: Namespace, extra_args: Optional[List[str]] = None) -> List[s "model_selection_strategy", "num_prompts", "output_format", - "output_tokens_mean_deterministic", "output_tokens_mean", + "output_tokens_mean_deterministic", "output_tokens_stddev", "prompt_source", "random_seed", "request_rate", + "server_metrics_url", # The 'streaming' passed in to this script is to determine if the # LLM response should be streaming. That is different than the # 'streaming' that PA takes, which means something else (and is # required for decoupled models into triton). "streaming", + "subcommand", "synthetic_input_tokens_mean", "synthetic_input_tokens_stddev", - "subcommand", "tokenizer", - "image_width_mean", - "image_width_stddev", - "image_height_mean", - "image_height_stddev", - "image_format", - "server_metrics_url", - "goodput", - "server_metrics_url", ] utils.remove_file(args.profile_export_file) From fefe4abe01870dfca04a427b931f7ad38de74324 Mon Sep 17 00:00:00 2001 From: lkomali Date: Tue, 27 Aug 2024 14:31:58 -0700 Subject: [PATCH 09/16] Fix pytest error --- genai-perf/tests/test_cli.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/genai-perf/tests/test_cli.py b/genai-perf/tests/test_cli.py index ef4841d4..dedbd1ac 100644 --- a/genai-perf/tests/test_cli.py +++ b/genai-perf/tests/test_cli.py @@ -922,7 +922,7 @@ def test_get_extra_inputs_as_dict(self, extra_inputs_list, expected_dict): INVALID_URL = "invalid_url" INVALID_URL_ERROR_MESSAGE = ( "The URL passed for --server-metrics-url is invalid. " - "It must use 'http' or 'https', have a valid domain, " + "It must use 'http' or 'https', have a valid domain and port, " "and contain '/metrics' in the path. The expected structure is: " ":///;?#" ) From f461c83551b32bf1d3147f6f93948f9a294e8f5f Mon Sep 17 00:00:00 2001 From: lkomali Date: Tue, 27 Aug 2024 14:56:56 -0700 Subject: [PATCH 10/16] Fix comments --- genai-perf/tests/test_cli.py | 60 +++++++++++++++++------------------- 1 file changed, 29 insertions(+), 31 deletions(-) diff --git a/genai-perf/tests/test_cli.py b/genai-perf/tests/test_cli.py index dedbd1ac..f182020c 100644 --- a/genai-perf/tests/test_cli.py +++ b/genai-perf/tests/test_cli.py @@ -918,19 +918,10 @@ def test_get_extra_inputs_as_dict(self, extra_inputs_list, expected_dict): actual_dict = parser.get_extra_inputs_as_dict(namespace) assert actual_dict == expected_dict - TEST_TRITON_METRICS_URL = "http://custom-metrics-url:8002/metrics" - INVALID_URL = "invalid_url" - INVALID_URL_ERROR_MESSAGE = ( - "The URL passed for --server-metrics-url is invalid. " - "It must use 'http' or 'https', have a valid domain and port, " - "and contain '/metrics' in the path. The expected structure is: " - ":///;?#" - ) - @pytest.mark.parametrize( - "args_list, expected_url, expected_error", + "args_list, expected_url", [ - # Test with a custom URL + # server-metrics-url is specified ( [ "genai-perf", @@ -940,12 +931,11 @@ def test_get_extra_inputs_as_dict(self, extra_inputs_list, expected_dict): "--service-kind", "triton", "--server-metrics-url", - TEST_TRITON_METRICS_URL, + "http://test-metrics-url:8002/metrics", ], - TEST_TRITON_METRICS_URL, - None, + "http://test-metrics-url:8002/metrics", ), - # Test with default URL + # server-metrics-url is not specified ( [ "genai-perf", @@ -956,9 +946,18 @@ def test_get_extra_inputs_as_dict(self, extra_inputs_list, expected_dict): "triton", ], None, - None, ), - # Test with invalid URL + ], + ) + def test_server_metrics_url_valid_args(self, args_list, expected_url, monkeypatch): + monkeypatch.setattr("sys.argv", args_list) + args, _ = parser.parse_args() + assert args.server_metrics_url == expected_url + + @pytest.mark.parametrize( + "args_list, expected_error", + [ + # Test with an invalid URL ( [ "genai-perf", @@ -968,24 +967,23 @@ def test_get_extra_inputs_as_dict(self, extra_inputs_list, expected_dict): "--service-kind", "triton", "--server-metrics-url", - INVALID_URL, + "invalid_url", ], - None, - INVALID_URL_ERROR_MESSAGE, + "The URL passed for --server-metrics-url is invalid. " + "It must use 'http' or 'https', have a valid domain and port, " + "and contain '/metrics' in the path. The expected structure is: " + ":///;?#", ), ], ) - def test_server_metrics_url_for_triton( - self, args_list, expected_url, expected_error, monkeypatch, capsys + def test_server_metrics_url_invalid_args( + self, args_list, expected_error, monkeypatch, capsys ): monkeypatch.setattr("sys.argv", args_list) - if expected_error: - with pytest.raises(SystemExit) as excinfo: - parser.parse_args() - captured = capsys.readouterr() - assert expected_error in captured.err - assert excinfo.value.code != 0 - else: - args, _ = parser.parse_args() - assert args.server_metrics_url == expected_url + with pytest.raises(SystemExit) as excinfo: + parser.parse_args() + + captured = capsys.readouterr() + assert expected_error in captured.err + assert excinfo.value.code != 0 From 05ff8fb655d426ca7fe8e9889fc9d9d92ae72393 Mon Sep 17 00:00:00 2001 From: lkomali Date: Tue, 27 Aug 2024 14:59:30 -0700 Subject: [PATCH 11/16] Refactor test function names --- genai-perf/tests/test_cli.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/genai-perf/tests/test_cli.py b/genai-perf/tests/test_cli.py index f182020c..6127b0ce 100644 --- a/genai-perf/tests/test_cli.py +++ b/genai-perf/tests/test_cli.py @@ -949,7 +949,7 @@ def test_get_extra_inputs_as_dict(self, extra_inputs_list, expected_dict): ), ], ) - def test_server_metrics_url_valid_args(self, args_list, expected_url, monkeypatch): + def test_server_metrics_url_valid(self, args_list, expected_url, monkeypatch): monkeypatch.setattr("sys.argv", args_list) args, _ = parser.parse_args() assert args.server_metrics_url == expected_url @@ -976,7 +976,7 @@ def test_server_metrics_url_valid_args(self, args_list, expected_url, monkeypatc ), ], ) - def test_server_metrics_url_invalid_args( + def test_server_metrics_url_invalid( self, args_list, expected_error, monkeypatch, capsys ): monkeypatch.setattr("sys.argv", args_list) From b922910eab249c61b458742a9d8c57345b94c012 Mon Sep 17 00:00:00 2001 From: lkomali Date: Tue, 27 Aug 2024 15:19:04 -0700 Subject: [PATCH 12/16] Fix comments --- genai-perf/tests/test_cli.py | 52 ++++++++++++------------------------ 1 file changed, 17 insertions(+), 35 deletions(-) diff --git a/genai-perf/tests/test_cli.py b/genai-perf/tests/test_cli.py index 6127b0ce..df1a0418 100644 --- a/genai-perf/tests/test_cli.py +++ b/genai-perf/tests/test_cli.py @@ -652,6 +652,22 @@ def test_unrecognized_arg(self, monkeypatch, capsys): ], "The --generate-plots option is not currently supported with the image_retrieval endpoint type", ), + ( + [ + "genai-perf", + "profile", + "--model", + "test_model", + "--service-kind", + "triton", + "--server-metrics-url", + "invalid_url", + ], + "The URL passed for --server-metrics-url is invalid. " + "It must use 'http' or 'https', have a valid domain and port, " + "and contain '/metrics' in the path. The expected structure is: " + ":///;?#", + ), ], ) def test_conditional_errors(self, args, expected_output, monkeypatch, capsys): @@ -949,41 +965,7 @@ def test_get_extra_inputs_as_dict(self, extra_inputs_list, expected_dict): ), ], ) - def test_server_metrics_url_valid(self, args_list, expected_url, monkeypatch): + def test_server_metrics_url_arg_valid(self, args_list, expected_url, monkeypatch): monkeypatch.setattr("sys.argv", args_list) args, _ = parser.parse_args() assert args.server_metrics_url == expected_url - - @pytest.mark.parametrize( - "args_list, expected_error", - [ - # Test with an invalid URL - ( - [ - "genai-perf", - "profile", - "--model", - "test_model", - "--service-kind", - "triton", - "--server-metrics-url", - "invalid_url", - ], - "The URL passed for --server-metrics-url is invalid. " - "It must use 'http' or 'https', have a valid domain and port, " - "and contain '/metrics' in the path. The expected structure is: " - ":///;?#", - ), - ], - ) - def test_server_metrics_url_invalid( - self, args_list, expected_error, monkeypatch, capsys - ): - monkeypatch.setattr("sys.argv", args_list) - - with pytest.raises(SystemExit) as excinfo: - parser.parse_args() - - captured = capsys.readouterr() - assert expected_error in captured.err - assert excinfo.value.code != 0 From 4daf3778730c3c48920debea79d15ad4bde6a5e1 Mon Sep 17 00:00:00 2001 From: lkomali Date: Tue, 27 Aug 2024 15:21:54 -0700 Subject: [PATCH 13/16] Fix comment in test_profile_handler --- genai-perf/tests/test_profile_handler.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/genai-perf/tests/test_profile_handler.py b/genai-perf/tests/test_profile_handler.py index 84be8cc9..014396b1 100755 --- a/genai-perf/tests/test_profile_handler.py +++ b/genai-perf/tests/test_profile_handler.py @@ -42,7 +42,7 @@ @pytest.fixture( params=[ {"service_kind": "triton", "server_metrics_url": TRITON_TEST_METRICS_URL}, - {"service_kind": "triton", "server_metrics_url": None}, # Default URL case + {"service_kind": "triton", "server_metrics_url": None}, # server_metrics_url is not specified ] ) def mock_args(request): From aee3a929ceef4035f58c07b5fb6c4320b5e54518 Mon Sep 17 00:00:00 2001 From: lkomali Date: Tue, 27 Aug 2024 15:43:55 -0700 Subject: [PATCH 14/16] Cleanup test_profile_handler.py test --- genai-perf/tests/test_profile_handler.py | 60 +++++++++++------------- 1 file changed, 27 insertions(+), 33 deletions(-) diff --git a/genai-perf/tests/test_profile_handler.py b/genai-perf/tests/test_profile_handler.py index 014396b1..4ace0e18 100755 --- a/genai-perf/tests/test_profile_handler.py +++ b/genai-perf/tests/test_profile_handler.py @@ -35,42 +35,36 @@ TritonTelemetryDataCollector, ) -TRITON_TEST_METRICS_URL = "http://tritonmetrics.com:8080/metrics" +class TestProfileHandler: + test_triton_metrics_url = "http://tritonmetrics.com:8080/metrics" -# Parameterized fixture -@pytest.fixture( - params=[ - {"service_kind": "triton", "server_metrics_url": TRITON_TEST_METRICS_URL}, - {"service_kind": "triton", "server_metrics_url": None}, # server_metrics_url is not specified - ] -) -def mock_args(request): - params = request.param - - class MockArgs: - def __init__(self, service_kind, server_metrics_url): - self.service_kind = service_kind - self.server_metrics_url = server_metrics_url - - return MockArgs(params["service_kind"], params["server_metrics_url"]) - + @pytest.mark.parametrize( + "server_metrics_url, expected_url", + [ + (test_triton_metrics_url, test_triton_metrics_url), + (None, DEFAULT_TRITON_METRICS_URL), + ], + ) + @patch("genai_perf.wrapper.Profiler.run") + def test_profile_handler_creates_telemetry_collector( + self, mock_profiler_run, server_metrics_url, expected_url + ): + class MockArgs: + def __init__(self, service_kind, server_metrics_url): + self.service_kind = service_kind + self.server_metrics_url = server_metrics_url -@patch("genai_perf.wrapper.Profiler.run") -def test_profile_handler_creates_telemetry_collector_triton( - mock_profiler_run, mock_args -): - profile_handler(mock_args, extra_args={}) - mock_profiler_run.assert_called_once() + mock_args = MockArgs( + service_kind="triton", server_metrics_url=server_metrics_url + ) + profile_handler(mock_args, extra_args={}) + mock_profiler_run.assert_called_once() - args, kwargs = mock_profiler_run.call_args - telemetry_collector = kwargs.get("telemetry_data_collector") + args, kwargs = mock_profiler_run.call_args - expected_url = ( - TRITON_TEST_METRICS_URL - if mock_args.server_metrics_url == TRITON_TEST_METRICS_URL - else DEFAULT_TRITON_METRICS_URL - ) + assert "telemetry_data_collector" in kwargs - assert isinstance(telemetry_collector, TritonTelemetryDataCollector) - assert telemetry_collector.metrics_url == expected_url + telemetry_data_collector = kwargs["telemetry_data_collector"] + assert isinstance(telemetry_data_collector, TritonTelemetryDataCollector) + assert telemetry_data_collector.metrics_url == expected_url From f3efd80e9b18d12b584a87a3b70df791f373db7b Mon Sep 17 00:00:00 2001 From: lkomali Date: Tue, 27 Aug 2024 15:51:06 -0700 Subject: [PATCH 15/16] Move MockArgs outside TestProfileHandler --- genai-perf/tests/test_profile_handler.py | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/genai-perf/tests/test_profile_handler.py b/genai-perf/tests/test_profile_handler.py index 4ace0e18..c5477a4e 100755 --- a/genai-perf/tests/test_profile_handler.py +++ b/genai-perf/tests/test_profile_handler.py @@ -36,6 +36,12 @@ ) +class MockArgs: + def __init__(self, service_kind, server_metrics_url): + self.service_kind = service_kind + self.server_metrics_url = server_metrics_url + + class TestProfileHandler: test_triton_metrics_url = "http://tritonmetrics.com:8080/metrics" @@ -50,11 +56,6 @@ class TestProfileHandler: def test_profile_handler_creates_telemetry_collector( self, mock_profiler_run, server_metrics_url, expected_url ): - class MockArgs: - def __init__(self, service_kind, server_metrics_url): - self.service_kind = service_kind - self.server_metrics_url = server_metrics_url - mock_args = MockArgs( service_kind="triton", server_metrics_url=server_metrics_url ) From 27d505589304e86f2328b99ff72ee1423674dcd5 Mon Sep 17 00:00:00 2001 From: lkomali Date: Tue, 27 Aug 2024 15:54:49 -0700 Subject: [PATCH 16/16] Move test triton metrics url to a variable --- genai-perf/tests/test_cli.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/genai-perf/tests/test_cli.py b/genai-perf/tests/test_cli.py index df1a0418..0a095d73 100644 --- a/genai-perf/tests/test_cli.py +++ b/genai-perf/tests/test_cli.py @@ -934,6 +934,8 @@ def test_get_extra_inputs_as_dict(self, extra_inputs_list, expected_dict): actual_dict = parser.get_extra_inputs_as_dict(namespace) assert actual_dict == expected_dict + test_triton_metrics_url = "http://tritonmetrics.com:8002/metrics" + @pytest.mark.parametrize( "args_list, expected_url", [ @@ -947,9 +949,9 @@ def test_get_extra_inputs_as_dict(self, extra_inputs_list, expected_dict): "--service-kind", "triton", "--server-metrics-url", - "http://test-metrics-url:8002/metrics", + test_triton_metrics_url, ], - "http://test-metrics-url:8002/metrics", + test_triton_metrics_url, ), # server-metrics-url is not specified (