Skip to content

[Misc] Use VLLMValidationError consistently in SamplingParams._verify_args#35664

Open
umut-polat wants to merge 1 commit intovllm-project:mainfrom
umut-polat:fix/sampling-params-validation-errors
Open

[Misc] Use VLLMValidationError consistently in SamplingParams._verify_args#35664
umut-polat wants to merge 1 commit intovllm-project:mainfrom
umut-polat:fix/sampling-params-validation-errors

Conversation

@umut-polat
Copy link
Copy Markdown
Contributor

some parameter checks in _verify_args raised plain ValueError or TypeError while others already used VLLMValidationError. the custom exception carries parameter and value which the api layer uses to build structured error responses for clients.

migrated all remaining ValueError/TypeError raises in _verify_args to VLLMValidationError with appropriate parameter and value kwargs:

  • n (type check + range)
  • presence_penalty
  • frequency_penalty
  • repetition_penalty
  • top_k (range + type check)
  • min_p
  • min_tokens (range + max_tokens comparison)
  • stop_token_ids
  • stop (empty string + detokenize)

no changes to error message text or validation logic.

Copy link
Copy Markdown
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 correctly refactors the validation logic in SamplingParams._verify_args to consistently use VLLMValidationError. This change improves structured error reporting for API clients. The implementation is accurate and aligns with the pull request's goal. For future work, you might consider extending this refactoring to other validation methods within the SamplingParams class (e.g., _verify_greedy_sampling, _validate_spec_decode) to achieve full consistency.

@mergify
Copy link
Copy Markdown

mergify bot commented Mar 1, 2026

Hi @umut-polat, the pre-commit checks have failed. Please run:

uv pip install pre-commit
pre-commit install
pre-commit run --all-files

Then, commit the changes and push to your branch.

For future commits, pre-commit will run automatically on changed files before each commit.

Tip

Is mypy or markdownlint failing?
mypy and markdownlint are run differently in CI. If the failure is related to either of these checks, please use the following commands to run them locally:
# For mypy (substitute "3.10" with the failing version if needed)
pre-commit run --hook-stage manual mypy-3.10
# For markdownlint
pre-commit run --hook-stage manual markdownlint

@mergify
Copy link
Copy Markdown

mergify bot commented Mar 12, 2026

Hi @umut-polat, the pre-commit checks have failed. Please run:

uv pip install pre-commit
pre-commit install
pre-commit run --all-files

Then, commit the changes and push to your branch.

For future commits, pre-commit will run automatically on changed files before each commit.

Tip

Is mypy failing?
mypy is run differently in CI. If the failure is related to this check, please use the following command to run it locally:
# For mypy (substitute "3.10" with the failing version if needed)
pre-commit run --hook-stage manual mypy-3.10

@umut-polat umut-polat force-pushed the fix/sampling-params-validation-errors branch from 31bae87 to 988884b Compare March 17, 2026 10:09
some parameter checks in _verify_args raised plain ValueError or
TypeError while others already used VLLMValidationError. the custom
exception carries parameter name and value which the api layer uses
to build structured error responses for clients.

migrated all remaining ValueError/TypeError raises in _verify_args
to VLLMValidationError with appropriate parameter and value kwargs.
no changes to error message text or validation logic.

Signed-off-by: Umut Polat <52835619+umut-polat@users.noreply.github.com>
Signed-off-by: umut-polat <52835619+umut-polat@users.noreply.github.com>
@umut-polat umut-polat force-pushed the fix/sampling-params-validation-errors branch from 988884b to cc67c0e Compare March 17, 2026 18:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant