Remove incorrect tokenizer info test#33565
Conversation
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
There was a problem hiding this comment.
Code Review
This pull request removes an incorrect test for the /tokenizer_info endpoint. The test was asserting an incorrect structure for added_tokens_decoder. While removing the test is a valid fix, I've suggested replacing it with a corrected test that asserts the actual API behavior. This maintains test coverage for this feature.
I am having trouble creating individual review comments. Click here to see my feedback.
tests/entrypoints/openai/test_tokenization.py (304-321)
You've correctly identified that this test is incorrect given the serialization logic. While removing it fixes the immediate issue of a failing test, it also removes test coverage for the added_tokens_decoder field.
A better approach would be to correct the test to reflect the actual behavior of the API. Based on your description, the token_info is serialized to just the string content. The test should be updated to assert this.
This ensures we still have a test for this part of the API response, and it correctly documents the expected behavior.
@pytest.mark.asyncio
async def test_tokenizer_info_added_tokens_structure(
server: RemoteOpenAIServer,
):
"""Test added_tokens_decoder structure if present."""
response = requests.get(server.url_for("tokenizer_info"))
response.raise_for_status()
result = response.json()
added_tokens = result.get("added_tokens_decoder")
if added_tokens:
for token_id, token_info in added_tokens.items():
assert isinstance(token_id, str), "Token IDs should be strings"
assert isinstance(token_info, str), "Token info should be a string"
|
I'd ignore Gemini's suggestion. There is little value in testing that a JSON is a |
DarkLight1337
left a comment
There was a problem hiding this comment.
Thanks for the detailed explanation!
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com> Signed-off-by: Pai <416932041@qq.com>
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com> Signed-off-by: Pai <416932041@qq.com>
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com> Signed-off-by: felix01.yu <felix01.yu@vipshop.com>
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
#20575 added, what is now called,
/tokenizer_infoendpoint.It added the following serialisation method that is used before the tokenizer info is sent:
vllm/vllm/entrypoints/serve/tokenize/serving.py
Lines 186 to 195 in d95b4be
In this you can see that if an
objhas the attributecontentit will only return thecontentfrom that object. The values of the theadded_tokens_decoderfield areAddedTokenobjects, which do have the attributecontent.This means that, by design, the tokenizer info endpoint was only meant to return the string value of the token. Unfortunately, this is directly contradicted in the test that was added by the same PR
vllm/tests/entrypoints/openai/test_tokenization.py
Lines 304 to 321 in d95b4be
This was not noticed because the model used in the test does not have and
added_tokens_decoderfor Transformers v4.This PR opts to simply remove the test so that:
added_tokens_decoderare present and the test fails