Use pydantic validation in speculative.py config #27137
Conversation
Navya1707
requested review from
ProExpertProg,
WoosukKwon,
hmellor,
houseroad,
mgoin,
robertgshaw2-redhat,
simon-mo,
tlrmchlsmth,
yewentao256 and
youkaichao
as code owners
|
👋 Hi! Thank you for contributing to the vLLM project. 💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in #pr-reviews, coordinate on features in #feat- channels, or join special interest groups in #sig- channels. Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run You ask your reviewers to trigger select CI tests on top of Once the PR is approved and ready to go, your PR reviewer(s) can run CI to test the changes comprehensively before merging. To run CI, PR reviewers can either: Add If you have any questions, please reach out to us on Slack at https://slack.vllm.ai. 🚀 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces pydantic validation to the speculative.py configuration file, enhancing data integrity and preventing potential runtime errors. The changes involve adding Field with validation constraints (e.g., ge=1 for values greater than or equal to 1) to several configuration parameters, and replacing assert statements with ValueError exceptions for better error handling. Additionally, the __post_init__ method is replaced with a model_validator to ensure proper configuration and validation after object initialization.
| if self.prompt_lookup_max is None: | ||
| raise ValueError( | ||
| "prompt_lookup_max must be provided when using the ngram method." | ||
| ) |
There was a problem hiding this comment.
This ValueError is raised when self.prompt_lookup_max is None but it should be provided when using the ngram method. However, the check self.prompt_lookup_min is None on line 270 already implies that self.prompt_lookup_min is None at this point. Therefore, the error message should also mention that prompt_lookup_min is missing as well.
Consider rephrasing the error message to indicate that both prompt_lookup_max and prompt_lookup_min must be provided when using the ngram method.
raise ValueError(
"Both prompt_lookup_max and prompt_lookup_min must be provided when using the ngram method."
)| if self.prompt_lookup_min is None: | ||
| raise ValueError( | ||
| "prompt_lookup_min must be provided when using the ngram method." | ||
| ) |
There was a problem hiding this comment.
Similar to the previous comment, this ValueError is raised when self.prompt_lookup_min is None but it should be provided when using the ngram method. However, the check self.prompt_lookup_max is None on line 276 already implies that self.prompt_lookup_max is None at this point. Therefore, the error message should also mention that prompt_lookup_max is missing as well.
Consider rephrasing the error message to indicate that both prompt_lookup_max and prompt_lookup_min must be provided when using the ngram method.
raise ValueError(
"Both prompt_lookup_max and prompt_lookup_min must be provided when using the ngram method."
)| if not isinstance( | ||
| self.draft_model_config.hf_config, | ||
| (EAGLEConfig, SpeculatorsConfig), | ||
| ): | ||
| pass | ||
| else: | ||
| eagle_config = EAGLEConfig( | ||
| self.draft_model_config.hf_config, | ||
| method=self.method, | ||
| model_type="eagle", | ||
| ) | ||
| self.draft_model_config.hf_config = eagle_config |
There was a problem hiding this comment.
The code block is checking if self.draft_model_config.hf_config is an instance of EAGLEConfig or SpeculatorsConfig. If it's not, it proceeds to create an EAGLEConfig. However, if it is an instance of either of those classes, the code block does nothing. This seems like an incomplete implementation, as there might be a need to handle these cases differently or perform some other action. If no action is required, the if not isinstance(...) check is unnecessary and can be removed.
Consider removing the if not isinstance(...) check and directly create the eagle_config without the conditional check, or add a comment explaining why no action is needed when the condition is false.
eagle_config = EAGLEConfig(
self.draft_model_config.hf_config,
method=self.method,
model_type="eagle",
)
self.draft_model_config.hf_config = eagle_config![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| enforce_eager: bool | None = None | ||
| """Override the default enforce_eager from model_config""" | ||
| # General speculative decoding control | ||
| num_speculative_tokens: SkipValidation[int] = None # type: ignore | ||
| num_speculative_tokens: int = Field(default=None, validate_default=True, ge=1) | ||
| """The number of speculative tokens, if provided. It will default to the |
There was a problem hiding this comment.
num_speculative_tokens is now declared as int with Field(default=None, ge=1) but without | None. Pydantic treats this field as required integer and will raise a validation error before _configure runs when the value is omitted or left as None, even though later logic (e.g. inferring from n_predict in _configure and _verify_args) still relies on the field being optional. This prevents building a SpeculativeConfig that relies on the draft model to supply the token count, a configuration that previously worked. Consider making the type int | None so that validation only triggers when a value is provided.
Useful? React with 👍 / 👎.
1 participant
Part of #26366