[BugFix] Pass config_format via try_get_generation_config #25912
Conversation
acisseJZhong
requested review from
simon-mo,
WoosukKwon,
youkaichao,
robertgshaw2-redhat,
mgoin,
tlrmchlsmth,
houseroad,
hmellor,
yewentao256 and
ProExpertProg
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. 🚀 |
|
@acisseJZhong has exported this pull request. If you are a Meta employee, you can view the originating diff in D83405333. |
![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 correctly adds support for specifying config_format when loading a model's generation configuration. The changes propagate the config_format from ModelConfig down to the configuration loading utilities. While the implementation is mostly correct, I've found one issue where the model revision is not consistently passed when loading a generation configuration from a separate source, which could lead to loading an incorrect version of the config. My review includes a suggestion to fix this.
| config = try_get_generation_config( | ||
| self.generation_config, | ||
| trust_remote_code=self.trust_remote_code, | ||
| config_format=self.config_format, | ||
| ) |
There was a problem hiding this comment.
The revision parameter is not being passed to try_get_generation_config in this else block. If self.generation_config refers to a model on the Hugging Face Hub, this will cause it to always use the default revision (e.g., main branch), ignoring the revision specified for the main model. This could lead to loading an incorrect or incompatible generation configuration. You should pass self.revision here for consistency with the if block.
| config = try_get_generation_config( | |
| self.generation_config, | |
| trust_remote_code=self.trust_remote_code, | |
| config_format=self.config_format, | |
| ) | |
| config = try_get_generation_config( | |
| self.generation_config, | |
| trust_remote_code=self.trust_remote_code, | |
| revision=self.revision, | |
| config_format=self.config_format, | |
| ) |
acisseJZhong
changed the title
Summary: test llama4x with shardist config parser Test Plan: buck test //vllm/fb/tests\:test_llama4x_hf -- --print-passing-details https://www.internalfb.com/intern/testinfra/testconsole/testrun/1970325152656551/ buck test 'fbcode//mode/opt' fbcode//vllm/fb/plugins/tests:test_shardist_loader -- --exact 'vllm/fb/plugins/tests:test_shardist_loader - test_shardist_model_loading (vllm.fb.plugins.tests.test_shardist_loader.TestShardistLoader)' --run-disabled https://www.internalfb.com/intern/testinfra/testconsole/testrun/844425379106729/ Differential Revision: D83405333
acisseJZhong
force-pushed
the
export-D83405333
branch
from
2574fba to
5b869d9
Compare
|
@acisseJZhong has exported this pull request. If you are a Meta employee, you can view the originating diff in D83405333. |
github-actions
bot
added
the
ready
22quinn
changed the title
Signed-off-by: yewentao256 <zhyanwentao@126.com>
Fixed a bug when using customized config parser: we need to pass in
self.config_format.