Improve documentation of ModelConfig.try_get_generation_config to prevent future confusion
#21526
Conversation
Signed-off-by: Harry Mellor <[email protected]>
Signed-off-by: Harry Mellor <[email protected]>
hmellor
requested review from
WoosukKwon,
robertgshaw2-redhat,
njhill,
ywang96,
comaniac,
alexm-redhat,
simon-mo,
youkaichao,
mgoin,
tlrmchlsmth,
houseroad and
zhuohan123
as code owners
![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 refactors the generation config loading by renaming try_get_generation_config to maybe_get_generation_config and improving its logic. The changes are mostly correct and improve clarity. However, I've found a critical issue where a call to the old function name was missed within vllm/config.py, which will lead to a runtime error.
Signed-off-by: Harry Mellor <[email protected]>
|
/gemini review |
hmellor
changed the title
There was a problem hiding this comment.
Code Review
The code changes introduce the ability to load multiple signed images to a registry using a bulk load feature. However, the refactoring of get_diff_sampling_param has introduced a regression. The logic to apply self.override_generation_config has become unreachable due to early return statements. This breaks the user-facing feature of overriding generation configs via --override-generation-config.
Signed-off-by: Harry Mellor <[email protected]>
Signed-off-by: Harry Mellor <[email protected]>
|
👋 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 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 🚀 |
Signed-off-by: Harry Mellor <[email protected]>
|
This pull request has merge conflicts that must be resolved before it can be |
Signed-off-by: Harry Mellor <[email protected]>
Signed-off-by: Harry Mellor <[email protected]>
Signed-off-by: Harry Mellor <[email protected]>
github-actions
bot
added
the
ready
Signed-off-by: Harry Mellor <[email protected]>
Signed-off-by: Harry Mellor <[email protected]>
hmellor
changed the title
ModelConfig.try_get_generation_config to prevent future confusion
…revent future confusion (vllm-project#21526) Signed-off-by: Harry Mellor <[email protected]>
…revent future confusion (vllm-project#21526) Signed-off-by: Harry Mellor <[email protected]> Signed-off-by: Jinzhen Lin <[email protected]>
…revent future confusion (vllm-project#21526) Signed-off-by: Harry Mellor <[email protected]> Signed-off-by: Noam Gat <[email protected]>
…revent future confusion (vllm-project#21526) Signed-off-by: Harry Mellor <[email protected]> Signed-off-by: Paul Pak <[email protected]>
…revent future confusion (vllm-project#21526) Signed-off-by: Harry Mellor <[email protected]> Signed-off-by: Diego-Castan <[email protected]>
…revent future confusion (vllm-project#21526) Signed-off-by: Harry Mellor <[email protected]>
…revent future confusion (vllm-project#21526) Signed-off-by: Harry Mellor <[email protected]>
try_get_generation_configexplaining why we still might get the generation config from the model when the generation config is set to vLLMget_diff_sampling_paramto make it more clear too