Add JSON exact match test for vLLM embeddings - #22180
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Greptile SummaryThis PR adds a test that verifies the HTTP request body sent to a vLLM embedding endpoint when using
Confidence Score: 3/5
|
| Filename | Overview |
|---|---|
| tests/logging_callback_tests/test_langfuse_e2e_test.py | Adds a new test for vLLM embedding request body validation. Test logic is sound, but it's misplaced in the Langfuse e2e test file since it doesn't test any Langfuse behavior. |
| tests/logging_callback_tests/langfuse_expected_request_body/embedding_with_vllm.json | Simple JSON fixture with expected vLLM embedding request body. Content correctly matches the output of HostedVLLMEmbeddingConfig.transform_embedding_request(). |
Sequence Diagram
sequenceDiagram
participant Test as test_langfuse_logging_vllm_embedding
participant LiteLLM as litellm.aembedding()
participant Embedding as embedding() in main.py
participant Handler as BaseLLMHTTPHandler
participant Mock as AsyncHTTPHandler (mocked .post)
Test->>LiteLLM: aembedding(model="hosted_vllm/BAAI/bge-small-en-v1.5", client=mock)
LiteLLM->>Embedding: embedding(..., aembedding=True)
Embedding->>Handler: base_llm_http_handler.embedding(client=mock)
Handler->>Handler: transform_embedding_request() strips "hosted_vllm/" prefix
Handler-->>LiteLLM: returns coroutine (aembedding=True)
LiteLLM->>Mock: await mock.post(json={"model": "BAAI/bge-small-en-v1.5", "input": [...]})
Mock-->>LiteLLM: httpx.Response(200, embedding data)
LiteLLM-->>Test: EmbeddingResponse
Test->>Test: assert mock.post.call_args.kwargs["json"] == expected fixture
Last reviewed commit: 1a3d4c8
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| async def test_langfuse_logging_vllm_embedding(self, mock_setup): | ||
| """ | ||
| Test that the request sent to the vllm embedding endpoint is correct. | ||
|
|
||
| Verifies the request body matches the expected JSON fixture, | ||
| including that the hosted_vllm/ prefix is stripped from the model name | ||
| and that no unexpected fields (e.g. encoding_format) are included. | ||
| """ | ||
| setup = mock_setup | ||
|
|
||
| vllm_response_data = { | ||
| "object": "list", | ||
| "data": [{"object": "embedding", "index": 0, "embedding": [0.1, 0.2, 0.3]}], | ||
| "model": "BAAI/bge-small-en-v1.5", | ||
| "usage": {"prompt_tokens": 10, "total_tokens": 10}, | ||
| } | ||
| mock_vllm_response = httpx.Response( | ||
| status_code=200, | ||
| json=vllm_response_data, | ||
| ) | ||
|
|
||
| mock_async_client = AsyncHTTPHandler() | ||
| mock_async_client.post = AsyncMock(return_value=mock_vllm_response) | ||
|
|
||
| with patch("httpx.Client.post", setup["mock_post"]): | ||
| await litellm.aembedding( | ||
| model="hosted_vllm/BAAI/bge-small-en-v1.5", | ||
| input=["Hello from litellm!"], | ||
| api_base="http://my-fake-vllm.com/v1", | ||
| metadata={"trace_id": setup["trace_id"]}, | ||
| client=mock_async_client, | ||
| ) | ||
|
|
||
| # Verify the request sent to vllm matches the expected JSON fixture | ||
| assert mock_async_client.post.call_count == 1 | ||
| actual_vllm_request = mock_async_client.post.call_args.kwargs["json"] | ||
|
|
||
| pwd = os.path.dirname(os.path.realpath(__file__)) | ||
| expected_body_path = os.path.join( | ||
| pwd, "langfuse_expected_request_body", "embedding_with_vllm.json" | ||
| ) | ||
| with open(expected_body_path, "r") as f: | ||
| expected_vllm_request = json.load(f) | ||
|
|
||
| assert actual_vllm_request == expected_vllm_request, ( | ||
| f"vllm request body mismatch:\n" | ||
| f"actual: {json.dumps(actual_vllm_request, indent=2)}\n" | ||
| f"expected: {json.dumps(expected_vllm_request, indent=2)}" | ||
| ) |
There was a problem hiding this comment.
Test placed in wrong test file
This test verifies the HTTP request body sent to the vLLM embedding endpoint (model name stripping, absence of encoding_format), but it doesn't verify any Langfuse logging behavior. Placing it in test_langfuse_e2e_test.py is misleading — it uses the mock_setup fixture only for its mock_post to stub out Langfuse's httpx.Client.post, but the actual assertions are entirely about the vLLM request body.
Consider moving this test to a vLLM-specific test file (e.g., under tests/llm_translation/ or a dedicated vLLM embedding test). This would also decouple it from the Langfuse mock_setup fixture, which sets up Langfuse-specific state (litellm.success_callback = ["langfuse"], trace IDs) that this test doesn't need.
Additionally, the PR checklist requires at least one test in the tests/litellm/ directory, which is not satisfied.
Context Used: Rule from dashboard - What: Ensure that any PR claiming to fix an issue includes evidence that the issue is resolved, such... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
2/5 only because location is wrong
Add JSON exact match test for vLLM embeddings
1 participant
Relevant issues
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
tests/litellm/directory, Adding at least 1 test is a hard requirement - see detailsmake test-unit@greptileaiand received a Confidence Score of at least 4/5 before requesting a maintainer reviewCI (LiteLLM team)
Branch creation CI run
Link:
CI run for the last commit
Link:
Merge / cherry-pick CI run
Links:
Type
🆕 New Feature
🐛 Bug Fix
🧹 Refactoring
📖 Documentation
🚄 Infrastructure
✅ Test
Changes