-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
test: TC for Metric P0 nv_load_time per model #7697
base: main
Are you sure you want to change the base?
Conversation
…-server/server into ibhosale_metrics_google
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! Make sure the CI passes before merging.
#### Load Time Per-Model | ||
The *Model Load Duration* reflects the time to load a model from storage into GPU/CPU in seconds. | ||
``` | ||
# HELP nv_model_load_duration_secs Model load time in seconds |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need a sample output for a gauge metric?
qa/L0_metrics/test.sh
Outdated
# Test 3 for explicit mode UNLOAD | ||
python3 -m pytest --junitxml="general_metrics_test.test_metrics_load_time_explicit_unload.report.xml" $CLIENT_PY::TestGeneralMetrics::test_metrics_load_time_explicit_unload >> $CLIENT_LOG 2>&1 | ||
kill_server | ||
set -e | ||
|
||
# Test 4 for explicit mode LOAD and UNLOAD with multiple versions | ||
set +e | ||
CLIENT_PY="./general_metrics_test.py" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove
print(f"Model '{model_name}' loaded successfully.") | ||
else: | ||
except AssertionError: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want the test to pass if failed to load the model? If not, you should remove try...except...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that's expected behaviour.
Models should load and unload. Else test should fail as subsequent metrics will be incorrect
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If load or unload failure will result test to fail anyway, why not let it fail at the HTTP response code check instead of metrics check? This way people can easiler identify the root cause of job failure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How come the core PR was merged way before this one finished? We currently have no ongoing tests for the merged feature on our nightly pipelines in core, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was approved in parallel. A couple of days appart.
I was unable to get a CI passing due to other build issues.
And then @yinggeh added more comments after it was approved. Hence the delay.
Yes I will get this in ASAP after the trtllm Code freeze
What does the PR do?
Test Case of model load time metrics
Checklist
<commit_type>: <Title>
Commit Type:
Check the conventional commit type
box here and add the label to the github PR.
Related PRs:
Core : triton-inference-server/core#397
Where should the reviewer start?
qa/L0_metrics/general_metrics_test.py
Test plan:
Added tests for
Background
Improve metrics in Triton