fix: fall back to model generation config - #4929
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The fallback logic introduces redundant config fetching and lacks unit tests for the new config.json fallback and “no config” cases.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a fallback for Hugging Face generation configuration loading so get_hf_gen_cfg() can derive a GenerationConfig from the model’s config.json when generation_config.json is missing, while keeping the previous {} behavior when neither config is available.
Changes:
- Update
get_hf_gen_cfg()to fall back toGenerationConfig.from_model_config()whenGenerationConfig.from_pretrained()fails due to missinggeneration_config.json. - Preserve stop/eos handling expectations downstream by continuing to return a dict for
update_from_hf_gen_cfg(). - Minor comment clarification in
update_from_hf_gen_cfg().
File summaries
| File | Description |
|---|---|
| lmdeploy/utils.py | Implements fallback logic for loading generation config from the model config when generation_config.json is absent. |
| lmdeploy/messages.py | Clarifies comment wording around reading EOS token IDs from the model’s generation config. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| model_cfg, _ = PretrainedConfig.get_config_dict(path) | ||
| if not model_cfg: | ||
| return {} | ||
| from lmdeploy.hf_configs import config_from_pretrained | ||
| model_cfg = config_from_pretrained(path, trust_remote_code=trust_remote_code) | ||
| cfg = GenerationConfig.from_model_config(model_cfg) |
There was a problem hiding this comment.
You are right that this loads the model config twice. However, the suggested simplification changes behavior for local paths: config_from_pretrained() returns a default PreTrainedConfig for an empty directory instead of raising, so it would no longer return {} when both files are absent. The explicit pre-check preserves that existing contract; this path runs only once at startup and only when generation_config.json is missing.
|
Validation has been done, acc back to normal even without |
Summary
Validation
Assistance
Assisted with Codex + GPT-5.6-Sol xHigh, reviewed manually