Skip to content

Conversation

@derekhiggins
Copy link
Contributor

@derekhiggins derekhiggins commented Aug 13, 2025

o Introduces vLLM provider support to the record/replay testing framework
o Enabling both recording and replay of vLLM API interactions alongside existing Ollama support.

The changes enable testing of vLLM functionality. vLLM tests focus on
inference capabilities, while Ollama continues to exercise the full API surface
including vision features.

Related: #2888

--
see alternative using qwen here #3545

@meta-cla meta-cla bot added the CLA Signed This label is managed by the Meta Open Source bot. label Aug 13, 2025
@derekhiggins derekhiggins force-pushed the vllm-ci-2 branch 4 times, most recently from 21f2737 to 0f0e9ca Compare August 20, 2025 15:12
@derekhiggins
Copy link
Contributor Author

derekhiggins commented Aug 20, 2025

@ashwinb With this we'll need to run the record tests for 2 providers, but they can't be run in parrallel because

CONFLICT (content): Merge conflict in tests/integration/recordings/index.sqlite

it works if you run them sequentially

@ashwin to avoid conflicts what would you think about removing the index.sqlite file altogether?
From what I can see it is only used to get the path of the recording, and we can instead infer this from the request_hash
there are probably other places this file is going to cause conflicts so possibly its good to remove it anyways

@derekhiggins derekhiggins force-pushed the vllm-ci-2 branch 2 times, most recently from fbe1472 to 9e9687d Compare August 25, 2025 10:07
@r3v5
Copy link
Contributor

r3v5 commented Aug 25, 2025

@ashwinb With this we'll need to run the record tests for 2 providers, but they can't be run in parrallel because

CONFLICT (content): Merge conflict in tests/integration/recordings/index.sqlite

it works if you run them sequentially

@ashwin to avoid conflicts what would you think about removing the index.sqlite file altogether? From what I can see it is only used to get the path of the recording, and we can instead infer this from the request_hash there are probably other places this file is going to cause conflicts so possibly its good to remove it anyways

I also dealt with index.sqlite conflict when re-recorded tests responses for CI.

@derekhiggins derekhiggins force-pushed the vllm-ci-2 branch 4 times, most recently from 2ef7b4a to 79784ff Compare August 26, 2025 16:26
@derekhiggins
Copy link
Contributor Author

@ashwinb With this we'll need to run the record tests for 2 providers, but they can't be run in parrallel because

CONFLICT (content): Merge conflict in tests/integration/recordings/index.sqlite

it works if you run them sequentially

@ashwin to avoid conflicts what would you think about removing the index.sqlite file altogether? From what I can see it is only used to get the path of the recording, and we can instead infer this from the request_hash there are probably other places this file is going to cause conflicts so possibly its good to remove it anyways

index.sqlite has been removed here #3254

# Additional exclusions for vllm setup
if [[ "$TEST_SETUP" == "vllm" ]]; then
EXCLUDE_TESTS="${EXCLUDE_TESTS} or test_inference_store_tool_calls"
EXCLUDE_TESTS="${EXCLUDE_TESTS} or test_inference_store_tool_calls or test_text_chat_completion_structured_output"
Copy link
Collaborator

Choose a reason for hiding this comment

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

what about adding these to the skips in the test files directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem here is because of the model, Our skips in test files are all based on provider
I put the skips here so that it only skips them in CI, anybody running integration test with a more capable model will still be able to use them.

If we can get to the point that this job is running, I'll happy test other models to see if I can get ride of this line alltogether.

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes please, ci w/ a model that passes more tests.

having a gap between what ci test and what developers see in the test suite is going to lead to bugs and confusion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've opened an alternative PR that instead use qwen3 #3545
I'll can close which ever one we don't want to go with

@mattf
Copy link
Collaborator

mattf commented Sep 23, 2025

image image

what changes did you make in your last force-push?

@derekhiggins
Copy link
Contributor Author

derekhiggins commented Sep 23, 2025

what changes did you make in your last force-push?

tldr: I removed all of the trivial changes that the ollama record ci job produced

--
I used the ollama record CI jobs to create new recordings for ollama, this resulted in a PR with recordings with mostly trivial changes
this was 72da0de9b01e27f30abe0012875ad09f84b8bb29 101 files changed

I took this commit and removed the trivial changes (these arn't needed),
1a759f5 2 files changed

derekhiggins and others added 4 commits September 24, 2025 09:38
- Update earth question to be more specific with multiple choice format
  to prevent Llama-3.2-1B-Instruct from rambling about other planets
- Skip test_text_chat_completion_structured_output as it sometimes
  times out during CI execution again with Llama-3.2-1B-Instruct on vllm

Signed-off-by: Derek Higgins <[email protected]>
Add vLLM provider support to integration test CI workflows alongside
existing Ollama support. Configure provider-specific test execution
where vLLM runs only inference specific tests (excluding vision tests) while
Ollama continues to run the full test suite.

This enables comprehensive CI testing of both inference providers but
keeps the vLLM footprint small, this can be expanded later if it proves
to not be too disruptive.

Signed-off-by: Derek Higgins <[email protected]>
@derekhiggins
Copy link
Contributor Author

Closing this i favour of the qwen version (as per discussion in community meeting)
#3545

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

Labels

CLA Signed This label is managed by the Meta Open Source bot.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants