Skip to content

Conversation

@WoosukKwon
Copy link
Collaborator

No description provided.

Signed-off-by: Woosuk Kwon <woosuk.kwon@berkeley.edu>
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request reverts a previous change, moving the prep_prompts and check_answers test utility functions back into tests/utils.py.

I've reviewed the reverted code and found two high-severity issues:

  1. In prep_prompts, the use of random.seed() affects the global random state, which can lead to flaky tests. I've suggested using an isolated random.Random instance to ensure deterministic behavior without side effects.
  2. In check_answers, the logic for parsing model outputs is brittle and can fail with leading whitespace. I've proposed a more robust parsing method.

These changes will improve the reliability and maintainability of the test suite.

Comment on lines +1169 to +1177
random.seed(1)
for _ in range(batch_size):
idx = random.randint(30, 90)
indices.append(idx)
prompt = "```python\n# We set a number of variables, " + \
f"x{idx} will be important later\n"
ln = random.randint(*ln_range)
for k in range(30, ln):
v = random.randint(10, 99)
Copy link
Contributor

Choose a reason for hiding this comment

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

high

Using random.seed() has a global effect and can interfere with other tests that rely on the random number generator. It's better to use an isolated random.Random instance for deterministic generation within this function.

Additionally, the prompt string construction can be simplified into a single f-string for better readability.

Suggested change
random.seed(1)
for _ in range(batch_size):
idx = random.randint(30, 90)
indices.append(idx)
prompt = "```python\n# We set a number of variables, " + \
f"x{idx} will be important later\n"
ln = random.randint(*ln_range)
for k in range(30, ln):
v = random.randint(10, 99)
rng = random.Random(1)
for _ in range(batch_size):
idx = rng.randint(30, 90)
indices.append(idx)
prompt = f"```python\n# We set a number of variables, x{idx} will be important later\n"
ln = rng.randint(*ln_range)
for k in range(30, ln):
v = rng.randint(10, 99)

answer: list[int],
outputs: list[str],
accept_rate: float = 0.7):
answer2 = [int(text[0:2].strip()) for text in outputs]
Copy link
Contributor

Choose a reason for hiding this comment

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

high

The current logic for parsing the model's output is brittle. If the output has leading whitespace, for example " 55" instead of "55", text[0:2] would be " 5", which incorrectly parses to 5.

A more robust approach is to strip whitespace from the whole string first, then split it and take the first part. This correctly handles leading/trailing whitespace.

Note that this can still raise an IndexError for empty outputs or a ValueError if the output is not a number, but this might be an acceptable risk for this test's context.

Suggested change
answer2 = [int(text[0:2].strip()) for text in outputs]
answer2 = [int(text.strip().split()[0]) for text in outputs]

@WoosukKwon WoosukKwon merged commit eb68c2d into main Sep 17, 2025
11 of 15 checks passed
@WoosukKwon WoosukKwon deleted the woosuk/fix-test-bug branch September 17, 2025 18:03
debroy-rh pushed a commit to debroy-rh/vllm that referenced this pull request Sep 19, 2025
Signed-off-by: Woosuk Kwon <woosuk.kwon@berkeley.edu>
FeiDaLI pushed a commit to FeiDaLI/vllm that referenced this pull request Sep 25, 2025
Signed-off-by: Woosuk Kwon <woosuk.kwon@berkeley.edu>
charlifu pushed a commit to ROCm/vllm that referenced this pull request Sep 25, 2025
Signed-off-by: Woosuk Kwon <woosuk.kwon@berkeley.edu>
Signed-off-by: charlifu <charlifu@amd.com>
xuebwang-amd pushed a commit to xuebwang-amd/vllm that referenced this pull request Oct 10, 2025
Signed-off-by: Woosuk Kwon <woosuk.kwon@berkeley.edu>
Signed-off-by: xuebwang-amd <xuebwang@amd.com>
choprahetarth pushed a commit to Tandemn-Labs/vllm that referenced this pull request Oct 11, 2025
Signed-off-by: Woosuk Kwon <woosuk.kwon@berkeley.edu>
xuebwang-amd pushed a commit to xuebwang-amd/vllm that referenced this pull request Oct 24, 2025
Signed-off-by: Woosuk Kwon <woosuk.kwon@berkeley.edu>
Signed-off-by: xuebwang-amd <xuebwang@amd.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants