Skip to content

Conversation

@mgoin
Copy link
Member

@mgoin mgoin commented Sep 17, 2025

Purpose

Add back missing create_dummy_prompt function to test_sequence.py removed in #25082 to fix Engine Test https://buildkite.com/vllm/ci/builds/31153/steps/canvas?sid=01995896-c6f7-469d-9582-e6a8570819ae

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.

Signed-off-by: mgoin <mgoin64@gmail.com>
@mgoin mgoin added the ci-failure Issue about an unexpected test failure in CI label Sep 17, 2025
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 re-introduces the create_dummy_prompt function to test_sequence.py to fix a CI failure. My review identifies a critical issue in the implementation where block_size can become negative, leading to potential crashes or incorrect behavior. I've provided a code suggestion to fix this by ensuring block_size is always a positive integer and correctly derived from the prompt's length.

Comment on lines +29 to +35
if not block_size:
block_size = prompt_length

if prompt_tokens is None:
# Create dummy prompt sequence with tokens 0...block_size-1
# and prompt "0 ... block_size".
prompt_tokens = list(range(prompt_length))
Copy link
Contributor

Choose a reason for hiding this comment

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

critical

The logic for determining block_size is flawed and can lead to a negative value, which is invalid for a Sequence. Specifically:

  1. If prompt_tokens is provided, prompt_length defaults to -1, causing block_size to be set to -1.
  2. A negative block_size can cause incorrect calculations for the number of blocks, or even crashes (e.g., division by zero if it were 0).
  3. The comment on line 33 is inconsistent with the code, as it refers to block_size for generating tokens while the code uses prompt_length.

I suggest refactoring this block to correctly determine prompt_tokens and block_size, ensuring block_size is always a positive integer.

    if prompt_tokens is None:
        prompt_tokens = list(range(max(0, prompt_length)))

    if block_size is None:
        if prompt_embeds is not None:
            block_size = len(prompt_embeds)
        else:
            block_size = len(prompt_tokens)

    if block_size <= 0:
        block_size = 1

@mgoin mgoin closed this Sep 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci-failure Issue about an unexpected test failure in CI

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

1 participant