Add CLI argument for server metrics url #37
Conversation
genai-perf/genai_perf/parser.py
Outdated
| def is_valid_url(url): | ||
| """ | ||
| Checks if a URL is valid. It must use 'http' or 'https', have a valid | ||
| netloc, optional port, and contain '/metrics' in the path. |
There was a problem hiding this comment.
can we add this to the docstring:
<scheme>://<netloc>/<path>;<params>?<query>#<fragment>
so we know what the pattern is for the url?
genai-perf/genai_perf/parser.py
Outdated
|
|
||
| return True | ||
| except Exception: | ||
| return False |
There was a problem hiding this comment.
Do we just want to return false here?
There was a problem hiding this comment.
This function returns if url is valid or not and the _check_server_metrics_url will display error message based on the return value.
There was a problem hiding this comment.
I will move the exception to this function.
genai-perf/genai_perf/parser.py
Outdated
| and args.server_metrics_url | ||
| and not is_valid_url(args.server_metrics_url) | ||
| ): | ||
| parser.error("The --server-metrics-url option contains an invalid URL format.") |
There was a problem hiding this comment.
I would move this exception into is_valid_url.
And update this method to check service kind and existence of the metrics url. Then return the result of _is_valid_url.
This way, as we add more checks later, we can make the call to the url validation method and it will bubble up the exception if one is thrown.
genai-perf/genai_perf/parser.py
Outdated
| @@ -604,6 +647,14 @@ def _add_endpoint_args(parser): | |||
| help="URL of the endpoint to target for benchmarking.", | |||
| ) | |||
|
|
|||
| endpoint_group.add_argument( | |||
| "--server-metrics-url", | |||
There was a problem hiding this comment.
Lets keep these alphabetized based on the option name.
| if self._server_metrics_url: | ||
| try: | ||
| response = requests.get(self._server_metrics_url, timeout=5) | ||
| return response.status_code == 200 |
There was a problem hiding this comment.
| return response.status_code == 200 | |
| return response.status_code == response.codes.ok |
genai-perf/genai_perf/parser.py
Outdated
| """ | ||
|
|
||
| # Check if the URL is valid and contains the expected path | ||
| if args.service_kind == 'triton' and args.server_metrics_url and not is_valid_url(args.server_metrics_url): |
There was a problem hiding this comment.
Could we return the exact error with the URL?
genai-perf/genai_perf/parser.py
Outdated
| def is_valid_url(url): | ||
| """ | ||
| Checks if a URL is valid. It must use 'http' or 'https', have a valid | ||
| netloc, optional port, and contain '/metrics' in the path. |
There was a problem hiding this comment.
This comment seems redundant and like it'll get out of sync with the code below. It's already out of sync, since it mentions that it must have an optional port, which I don't think means anything and there is no port-related check here.
There was a problem hiding this comment.
Yeah, I will update the comment. I had a check for port earlier but removed it after reading about netloc.
genai-perf/genai_perf/parser.py
Outdated
| type=str, | ||
| default=None, | ||
| required=False, | ||
| help="URL of the server metrics endpoint. Required for 'openai' service kind. Defaults to the default URL if 'service_kind' is 'triton'.", |
There was a problem hiding this comment.
This doesn't seem to be required. It'd be good to avoid adding required args.
There was a problem hiding this comment.
I will modify the comment.
lkomali
force-pushed
the
lkomali-tpa-192
branch
from
f85bac3 to
8251542
Compare
lkomali
force-pushed
the
lkomali-tpa-275
branch
from
cfc252a to
ed3b315
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| benchmark_duration=self.TEST_BENCHMARK_DURATION, | ||
| ) | ||
|
|
||
| assert gc.goodput == None # before computing |
Check notice
Code scanning / CodeQL
Testing equality to None
| metric=test_llm_metrics_1, | ||
| benchmark_duration=self.TEST_BENCHMARK_DURATION, | ||
| ) | ||
| assert gc_1.goodput == None # before computing |
Check notice
Code scanning / CodeQL
Testing equality to None
| metric=test_llm_metrics_2, | ||
| benchmark_duration=self.TEST_BENCHMARK_DURATION, | ||
| ) | ||
| assert gc_2.goodput == None # before computing |
Check notice
Code scanning / CodeQL
Testing equality to None
lkomali
force-pushed
the
lkomali-tpa-275
branch
from
f28fc1d to
e67f9ca
Compare
This PR adds a CLI argument to get server metrics url when the server is running on a machine different from the one genai-perf is running.