Skip to content

[CI] Optimize entrypoints API server tests#23896

Closed
csahithi wants to merge 22 commits intovllm-project:mainfrom
csahithi:entrypoint-tests-optimize
Closed

[CI] Optimize entrypoints API server tests#23896
csahithi wants to merge 22 commits intovllm-project:mainfrom
csahithi:entrypoint-tests-optimize

Conversation

@csahithi
Copy link
Contributor

@csahithi csahithi commented Aug 29, 2025

Purpose

  • Reduce CI time for entrypoint tests by creating shared server for grouped tests
  • Removed v0 references in entrypoint tests
  • Replaced large models with smaller ones - hmellor/tiny-random-LlamaForCausalLM, microsoft/DialoGPT-small

Test Plan

Test Result


Essential Elements of an Effective PR Description Checklist
  • The purpose of the PR, such as "Fix some issue (link existing issues this PR will resolve)".
  • The test plan, such as providing test command.
  • The test results, such as pasting the results comparison before and after, or e2e results
  • (Optional) The necessary documentation update, such as updating supported_models.md and examples for a new model.
  • (Optional) Release notes update. If your change is user facing, please update the release notes draft in the Google Doc.

@robertgshaw2-redhat
Copy link
Collaborator

wow! great job!

@csahithi csahithi force-pushed the entrypoint-tests-optimize branch from 68a2e19 to 1b41751 Compare August 29, 2025 04:23
@mergify
Copy link

mergify bot commented Aug 29, 2025

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @csahithi.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@mergify mergify bot added the needs-rebase label Aug 29, 2025
@njhill njhill self-requested a review August 29, 2025 16:04
@csahithi csahithi marked this pull request as ready for review August 29, 2025 16:07
@njhill njhill changed the title Optimize entrypoints API server tests [CI] Optimize entrypoints API server tests Aug 29, 2025
Copy link
Member

@njhill njhill left a comment

Choose a reason for hiding this comment

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

Thanks @csahithi this is great!!

Replaced large models with smaller ones - hmellor/tiny-random-LlamaForCausalLM, microsoft/DialoGPT-small

Is the reason for the latter that the former doesn't have a chat template?
If so we can just ask @hmellor to add the llama 3.2 chat template and replace them all with that.

Oh sorry I see that it does already have a chat template. Then I'm curious what's the reason for using microsoft/DialoGPT-small too?

I know you have ideas for possible further streamlining but in the interests of incremental improvement could we get this merged first?

Could you fix the merge conflicts and we can see what the new CI timings are like after that too.

@hmellor
Copy link
Member

hmellor commented Aug 30, 2025

If anything needs changing about hmellor/tiny-random-LlamaForCausalLM to make it more useful for our tests do let me know, vLLM testing is what I made it for and it's easy to update!

@csahithi csahithi force-pushed the entrypoint-tests-optimize branch from 4732592 to e799966 Compare September 2, 2025 23:25
@mergify mergify bot removed the needs-rebase label Sep 2, 2025
@csahithi csahithi force-pushed the entrypoint-tests-optimize branch 2 times, most recently from aac1623 to fba4775 Compare September 5, 2025 13:33
@njhill
Copy link
Member

njhill commented Sep 5, 2025

CI failures look related

@mergify
Copy link

mergify bot commented Sep 7, 2025

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @csahithi.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@mergify mergify bot added the needs-rebase label Sep 7, 2025
@csahithi
Copy link
Contributor Author

csahithi commented Oct 1, 2025

@csahithi re the fix to test_chat_echo.py, do you know why this wasn't failing before this PR? (with "top_logprobs": -1)

I'm not sure what the cause is, seems like it started failing after the rebase
I see this change top_logprobs: -1 has been added with this PR #25031 recently

Signed-off-by: Sahithi Chigurupati <chigurupati.sahithi@gmail.com>
@csahithi csahithi force-pushed the entrypoint-tests-optimize branch from 137c6e3 to 525293f Compare October 1, 2025 17:21
@njhill
Copy link
Member

njhill commented Oct 1, 2025

@csahithi re the fix to test_chat_echo.py, do you know why this wasn't failing before this PR? (with "top_logprobs": -1)

I'm not sure what the cause is, seems like it started failing after the rebase I see this change top_logprobs: -1 has been added with this PR #25031 recently

It looks like this is because that test used to run the server with --max-logprobs arg set, as it was specifically intended to test the -1 case. So we may want to have this one keep the module-scoped server.

Not sure if there are others which had test-specific args that we should look out for?

@njhill
Copy link
Member

njhill commented Oct 1, 2025

Interestingly it seems that top_logprobs: -1 actually results in 0 top logprobs returned, seems like a bug; the test doesn't actually check for this (setting it to 50 does result in top 50 logprobs returned).

We can separately follow up on that but for now it would be good to have this test at least work as it did before, i.e. exercise the -1 case even though it's not properly checking the top_logprob count of the output. We can still change it to use hmellor/tiny-random-LlamaForCausalLM.

I've opened #26194 for this issue.

csahithi and others added 5 commits October 2, 2025 05:23
Signed-off-by: Sahithi Chigurupati <chigurupati.sahithi@gmail.com>
Signed-off-by: Nick Hill <nhill@redhat.com>
Signed-off-by: Nick Hill <nhill@redhat.com>
@mergify
Copy link

mergify bot commented Oct 6, 2025

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @csahithi.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@mergify mergify bot added the needs-rebase label Oct 6, 2025
@chaunceyjiang
Copy link
Collaborator

It looks like this is because that test used to run the server with --max-logprobs arg set, as it was specifically intended to test the -1 case. So we may want to have this one keep the module-scoped server.

Not sure if there are others which had test-specific args that we should look out for?

Yes, top_logprobs: -1 depends on the --max-logprobs parameter.

Interestingly it seems that top_logprobs: -1 actually results in 0 top logprobs returned, seems like a bug; the test doesn't actually check for this (setting it to 50 does result in top 50 logprobs returned).

I've submitted a PR to fix this issue, sorry, the original PR didn't check the length of top_logprobs.

@csahithi @njhill #26470

@hmellor
Copy link
Member

hmellor commented Oct 9, 2025

These conflicts are caused by our migration to ruff. Please see https://vllm-dev.slack.com/archives/C07R5Q1Q2BB/p1759663228844749 which contains detailed instructions to make updating your branch as painless as possible.

Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
@hmellor
Copy link
Member

hmellor commented Nov 4, 2025

I've got the merge past ruff, but it wasn't a clean process because the process we came up with doesn't handle deleted files well so I had to do it manually.

@mergify mergify bot removed the needs-rebase label Nov 4, 2025
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
@njhill
Copy link
Member

njhill commented Nov 4, 2025

Thanks @hmellor!

Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
@mergify
Copy link

mergify bot commented Nov 7, 2025

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @csahithi.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@mergify mergify bot added the needs-rebase label Nov 7, 2025
@DarkLight1337
Copy link
Member

Any update on this?

@hmellor
Copy link
Member

hmellor commented Dec 23, 2025

Closing as it's now too stale. #31228 does a small amount of organising but not on the same scale as this PR did.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci/build needs-rebase ready ONLY add when PR is ready to merge/full CI is needed tool-calling

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

7 participants