[Bugfix] Fix ImportError in get_chat_template for builtin chat-template names#4690
Open
waynehacking8 wants to merge 2 commits into
Open
[Bugfix] Fix ImportError in get_chat_template for builtin chat-template names#4690waynehacking8 wants to merge 2 commits into
waynehacking8 wants to merge 2 commits into
Conversation
…te names (InternLM#4533) `get_chat_template` (lmdeploy/cli/utils.py) imported `DEPRECATED_CHAT_TEMPLATE_NAMES` and `REMOVED_CHAT_TEMPLATE_NAMES` from `lmdeploy.model`, but those lists were removed in InternLM#4252 ([AsyncEngine Refactor 2/N] Remove deprecates from chat template). That refactor cleaned up model.py, cli.py, async_engine.py and the tests, but left the import (and its two guard blocks) dangling in cli/utils.py. As a result, passing any builtin chat-template name that is not a file path (e.g. `--chat-template internlm2`) to `lmdeploy serve api_server` or `lmdeploy chat` crashed with: ImportError: cannot import name 'DEPRECATED_CHAT_TEMPLATE_NAMES' from 'lmdeploy.model' before reaching the registry validation. Omitting `--chat-template`, or passing a `.json` file path, returns early, which is why it went unnoticed. Complete the refactor: drop the dead import and the removed-/deprecated-name guards, keep the `MODELS` registry check, and remove the now-unused module logger. Add regression tests in tests/test_lmdeploy/test_utils.py covering a builtin name, an unregistered name, and the None case. Co-authored-by: Claude <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a crash in the CLI --chat-template <name> path when <name> is a builtin template name by removing stale imports/guards in get_chat_template that referenced constants deleted in a prior refactor (#4252). It also adds regression tests to ensure builtin names resolve and unknown names are rejected.
Changes:
- Remove the dangling import of
DEPRECATED_CHAT_TEMPLATE_NAMES/REMOVED_CHAT_TEMPLATE_NAMESand associated guard blocks fromlmdeploy/cli/utils.py:get_chat_template. - Keep builtin template validation via the
MODELSregistry and passmodel_paththrough toChatTemplateConfig. - Add regression tests covering builtin-name resolution, unregistered-name rejection, and
Nonepassthrough.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
lmdeploy/cli/utils.py |
Removes dead imports/logic that caused ImportError for builtin chat-template names; keeps registry validation. |
tests/test_lmdeploy/test_utils.py |
Adds regression tests to prevent reintroducing the builtin-name crash and to validate behavior for unknown/None inputs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
67
to
71
| @@ -73,7 +69,7 @@ def get_chat_template(chat_template: str, model_path: str = None): | |||
|
|
|||
| Args: | |||
| chat_template(str): it could be a builtin chat template name, or a chat template json file | |||
Contributor
Author
There was a problem hiding this comment.
Good catch — done in c900106. Both chat_template and model_path are now annotated str | None (min supported Python is 3.10, so the union syntax is fine), and the docstring arg types updated to match.
Comment on lines
+108
to
+114
| from lmdeploy.cli.utils import get_chat_template | ||
| raised = False | ||
| try: | ||
| get_chat_template('not-a-registered-template') | ||
| except AssertionError: | ||
| raised = True | ||
| assert raised, 'an unregistered chat template name should raise AssertionError' |
Contributor
Author
There was a problem hiding this comment.
Done in c900106 — replaced the manual try/except + boolean flag with pytest.raises(AssertionError) (added import pytest).
- get_chat_template: annotate chat_template/model_path as `str | None` (both accept None in CLI flows and tests) and update docstring arg types. - test_get_chat_template_unregistered_name: replace manual try/except + boolean flag with pytest.raises(AssertionError) for clarity.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
lmdeploy serve api_server ... --chat-template <name>andlmdeploy chat ... --chat-template <name>crash withImportErrorwhenever<name>is a builtin chat-template name (anything that is not a.jsonfile path):Fixes #4533.
Root cause
get_chat_template(lmdeploy/cli/utils.py) importsDEPRECATED_CHAT_TEMPLATE_NAMESandREMOVED_CHAT_TEMPLATE_NAMESfromlmdeploy.model. Those lists were removed in #4252 ([AsyncEngine Refactor 2/N] Remove deprecates from chat template), which cleaned upmodel.py,cli.py,async_engine.pyand the tests but left the import — and its two guard blocks — dangling incli/utils.py. The two names now exist nowhere in the repo, so the import raises before reaching the registry validation.The crash only fires on the
elsebranch (a builtin name that is not a file), which is why it went unnoticed: passing no--chat-template, or a.jsonfile path, both return earlier. It is reachable from bothlmdeploy/cli/serve.py(serve api_server) andlmdeploy/cli/chat.py(chat).Modification
Complete the #4252 refactor in
get_chat_template: drop the dead import and the removed-/deprecated-name guard blocks, keep the existingMODELSregistry check, and remove the now-unused module-levellogger. Unknown names are still rejected — by the registryassert, which lists the builtin templates.BC-breaking (Optional)
No. Builtin names that previously crashed now work; unknown names still raise (an
AssertionErrorfrom the registry check instead of anImportError).Use cases (Optional)
Checklist
tests/test_lmdeploy/test_utils.py(GPU-free, no model load):test_get_chat_template_builtin_name—get_chat_template('llama2')returns aChatTemplateConfig. Fails withImportErroron currentmain, passes with this fix.test_get_chat_template_unregistered_name— an unregistered name raisesAssertionError(registry check), notImportError.test_get_chat_template_none—Nonein →Noneout.cli/utils.pychange makestest_get_chat_template_builtin_namefail atcli/utils.py:85: ImportError; with the fix all three pass.pre-commit(ruff-check / docformatter / copyright) is clean.🤖 Generated with Claude Code