Skip to content

Conversation

qthequartermasterman
Copy link
Contributor

@qthequartermasterman qthequartermasterman commented Sep 16, 2025

Purpose

In most places in the tests, instantiations of EngineCoreRequest use kwargs instead of positional arguments. There are too many arguments to keep track of positionally. Additionally after recent refactors simplifying the arguments passed in for multimodal(see #23779), one instance was not correspondingly updated. The tests were still passing as a happy accident despite values were getting passed into wrong arguments.

This change originally came up in #24278 (comment), by @DarkLight1337, but because we later refactored that PR to not require having to modify those instantiations at all, I broke this out into its own PR for easier review.

Test Plan

No new tests are needed.

Test Result

All of the tests in tests/detokenizer/test_min_tokens.py and tests/tokenization/test_detoknize.py are passing in my local environment. Pending CI, this PR should be good to go.


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.

…tra kwargs

Signed-off-by: Andrew Sansom <andrew@protopia.ai>
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 refactors the instantiation of EngineCoreRequest in test files to use keyword arguments, which significantly improves code readability and maintainability. This change also correctly fixes a bug where positional arguments were being passed in the wrong order. The updated code is much clearer and less prone to errors. The changes look good and I have no further suggestions.

@DarkLight1337 DarkLight1337 enabled auto-merge (squash) September 16, 2025 19:01
@github-actions github-actions bot added the ready ONLY add when PR is ready to merge/full CI is needed label Sep 16, 2025
@DarkLight1337 DarkLight1337 merged commit 02d4b85 into vllm-project:main Sep 16, 2025
27 of 29 checks passed
FeiDaLI pushed a commit to FeiDaLI/vllm that referenced this pull request Sep 25, 2025
…nd fix extra kwargs (vllm-project#24987)

Signed-off-by: Andrew Sansom <andrew@protopia.ai>
charlifu pushed a commit to ROCm/vllm that referenced this pull request Sep 25, 2025
…nd fix extra kwargs (vllm-project#24987)

Signed-off-by: Andrew Sansom <andrew@protopia.ai>
Signed-off-by: charlifu <charlifu@amd.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready ONLY add when PR is ready to merge/full CI is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants