Skip to content
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

Add support for tgi metricesfor vllm #1513

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mwaykole
Copy link
Member

@mwaykole mwaykole commented Jun 7, 2024

No description provided.

@mwaykole mwaykole requested a review from lugi0 June 7, 2024 16:22
@mwaykole mwaykole self-assigned this Jun 7, 2024
Copy link

sonarcloud bot commented Jun 7, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

Copy link
Contributor

github-actions bot commented Jun 7, 2024

Robot Results

✅ Passed ❌ Failed ⏭️ Skipped Total Pass %
477 0 0 477 100

@mwaykole
Copy link
Member Author

mwaykole commented Jun 8, 2024

Screenshot from 2024-06-08 17-51-31

@mwaykole mwaykole added the verified This PR has been tested with Jenkins label Jun 8, 2024
@mwaykole mwaykole requested a review from tarukumar June 10, 2024 11:56
Copy link
Contributor

@lugi0 lugi0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test currently does not validate TGI metrics' values, and I would suggest separating the testing of TGI and vLLM metrics in two different TCs for better visibility into their behaviour

@@ -89,7 +103,7 @@ Verify Vllm Metrics Are Present
Set Suite Variable ${token}
Metrics Should Exist In UserWorkloadMonitoring ${thanos_url} ${token} ${SEARCH_METRICS}

Verify Vllm Metrics Values Match Between UWM And Endpoint
Verify Vllm And tgi Metrics Values Match Between UWM And Endpoint
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not work - Line 98/112 fetches the metrics to test for by looking at those that start with vllm:, so TGI metrics are not being tested at all here.
You will need to either change the logic of the Get Vllm Metrics And Values in Helpers.py or develop an equivalent keyword for TGI metrics

Comment on lines -58 to 75
Verify User Can Deploy A Model With Vllm Via CLI
Verify User Can Deploy A Model With Vllm And tgi Via CLI
[Documentation] Deploy a model (gpt2) using the vllm runtime and confirm that it's running
[Tags] Tier1 Sanity Resources-GPU RHOAIENG-6264 VLLM
${rc} ${out}= Run And Return Rc And Output oc apply -f ${DL_POD_FILEPATH}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is not using tgi to deploy the model, it's using vLLM, please revert the name of this TC

Comment on lines +92 to +93
Verify Vllm And tgi Metrics Are Present
[Documentation] Confirm vLLM and tgi metrics are exposed in OpenShift metrics
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO you should create a separate test to validate that TGI metrics are present - as far as I understand you've already encountered the issue where only vLLM ones would get exposed, so it'd be better to separate the two and have clear reporting on which ones (if any) are failing to get exposed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ack

@mwaykole mwaykole added do not merge Do not merge this yet please and removed verified This PR has been tested with Jenkins labels Jun 11, 2024
@mwaykole mwaykole closed this Jun 18, 2024
@mwaykole mwaykole reopened this Jun 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not merge Do not merge this yet please
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants