Skip to content

[Mypy Fix] Mypy fix for "vllm/model_executor/models/[tT]" - #53466

Merged
hmellor merged 4 commits into
vllm-project:mainfrom
ZHIHANCHEN03:fix/mypy-models-t
Aug 26, 2026
Merged

hmellor merged 4 commits into
vllm-project:mainfrom
ZHIHANCHEN03:fix/mypy-models-t

Conversation

@ZHIHANCHEN03

Copy link
Copy Markdown
Contributor

Purpose

Mypy fix for vllm/model_executor/models/[tT], continuing the split in #48490 after
[aA][bB] (#48977), [cC][dD] (#52003) and [eE][fF] (#53381).

10 errors across telechat2.py and terratorch.py:

  • terratorch.py — annotate loaded_buffers as list[str].
  • telechat2.py — narrow layer.self_attn / layer.mlp to LlamaAttention and
    LlamaMLP before touching their submodules. Module.__getattr__ is typed
    Tensor | Module, so the attribute chain could not be checked (8 of the 10 errors).
    The layer types are fixed here: TeleChat2Model does not pass layer_type, so
    LlamaModel uses LlamaDecoderLayer, whose attn_layer_type defaults to
    LlamaAttention and whose mlp is always LlamaMLP.
  • telechat2.py — drop the bias with register_parameter("bias", None) instead of
    assigning None to a Parameter-typed attribute. This is the idiom linear.py
    itself uses for bias-less layers (e.g. linear.py L373/L518/L1611), and it is
    exactly what nn.Module.__setattr__ already did for the previous assignment.
  • telechat2.pyattribute_map is a ClassVar on PretrainedConfig; the
    instance-level shadow is intentional, so it is marked type: ignore[misc] rather
    than restructured.

No behavior change.

Note on the EXCLUDE entry

EXCLUDE entries are prefix patterns (re.compile(f"^{'|'.join(EXCLUDE)}.*")), so
models/[tT] also covered the models/transformers package, which still has errors.
Simply dropping the entry fails the full-repo run. The entry is therefore narrowed to
models/transformers, which enforces the three [tT] files and leaves that package
excluded. models/transformers is the only subdirectory under models/, so earlier
letter groups did not hit this.

Duplicate-work check

Searched open PRs for mypy in the title and for model_executor/models mypy work.
The only open mypy PRs are #45296 (kimi / [kK]), #51043 (tests/ directories) and
#30960 (mypy → ty migration); none touch [tT].

AI assistance disclosure: Claude Code assisted with the audit, implementation and
review. I reviewed and understand all changed lines and validated the behavior locally.

Test Plan

# the four versions CI runs, on the whole repo
pre-commit run mypy-3.10 --all-files --hook-stage manual
pre-commit run mypy-3.11 --all-files --hook-stage manual
pre-commit run mypy-3.12 --all-files --hook-stage manual
pre-commit run mypy-3.13 --all-files --hook-stage manual

pre-commit run --files tools/pre_commit/mypy.py \
    vllm/model_executor/models/telechat2.py \
    vllm/model_executor/models/teleflm.py \
    vllm/model_executor/models/terratorch.py

Test Result

Run mypy for Python 3.10.................................................Passed
Run mypy for Python 3.11.................................................Passed
Run mypy for Python 3.12.................................................Passed
Run mypy for Python 3.13.................................................Passed
All applicable pre-commit hooks passed.

Before the change the same three files reported 10 errors; the naive version of this
patch (dropping the [tT] entry outright) fails the full-repo run with
Found 145 errors in 12 files, which is what motivated narrowing the entry instead.

I also verified that register_parameter("bias", None) is exactly equivalent to the
previous bias = None assignment — same bias attribute, same _parameters entry,
same state_dict() and named_parameters() — since nn.Module.__setattr__ routes a
None assigned over a registered parameter through register_parameter.

Model-level tests for TeleChat2/TeleFLM/Terratorch were not run locally: they need real
weights, and this was developed on an Apple Silicon CPU build. terratorch.py also
imports the optional terratorch package, which is not installed locally; mypy checks it
under --follow-imports silent, unaffected.

Continues the split in vllm-project#48490, after [aA][bB] (vllm-project#48977), [cC][dD] (vllm-project#52003)
and [eE][fF] (vllm-project#53381). Fixes the 10 mypy errors under
`vllm/model_executor/models/[tT]` so the group can be enforced.

- `terratorch.py`: annotate `loaded_buffers` as `list[str]`.
- `telechat2.py`: narrow `layer.self_attn` / `layer.mlp` to `LlamaAttention`
  and `LlamaMLP` before touching their submodules. `Module.__getattr__` is
  typed `Tensor | Module`, so the attribute chain could not be checked. The
  layer types are fixed: `TeleChat2Model` does not pass `layer_type`, so
  `LlamaModel` uses `LlamaDecoderLayer`, whose `attn_layer_type` defaults to
  `LlamaAttention` and whose `mlp` is always `LlamaMLP`.
- `telechat2.py`: drop the bias with `register_parameter("bias", None)`
  instead of assigning `None` to a `Parameter`-typed attribute. This is the
  idiom `linear.py` itself uses for bias-less layers and is exactly what
  `nn.Module.__setattr__` did for the previous assignment.
- `telechat2.py`: `attribute_map` is a `ClassVar` on `PretrainedConfig`;
  the instance-level shadow is intentional, so it is marked
  `type: ignore[misc]` rather than restructured.

`EXCLUDE` entries are prefix patterns, so `models/[tT]` also covered the
`models/transformers` package, which still has errors. The entry is
narrowed to `models/transformers` rather than dropped, which enforces the
three `[tT]` files while leaving that package excluded.

No behavior change.

Signed-off-by: Jensen Chen <a1043904820@gmail.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@ZHIHANCHEN03
ZHIHANCHEN03 requested a review from hmellor as a code owner August 23, 2026 16:41

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@ZHIHANCHEN03

Copy link
Copy Markdown
Contributor Author

@yewentao256 continuing the [aA][bB] / [cC][dD] / [eE][fF] split from #48490 with [tT]. Could you take a look when you get a chance?

One thing worth flagging: EXCLUDE entries are prefix patterns (re.compile(f"^{'|'.join(EXCLUDE)}.*")), so models/[tT] also covered the models/transformers package, which still has errors — dropping the entry outright fails the full-repo run with Found 145 errors in 12 files. I narrowed the entry to models/transformers instead, which enforces the three [tT] files and leaves that package excluded. Happy to take a different approach if you would prefer one.

Verified locally with pre-commit run mypy-3.10/3.11/3.12/3.13 --all-files --hook-stage manual, all Passed.

@yewentao256 yewentao256 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the work!

Comment thread tools/pre_commit/mypy.py Outdated
Comment on lines +128 to +129
# `[tT]` is otherwise clean; the `transformers` package is still excluded.
r"vllm/model_executor/models/transformers",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# `[tT]` is otherwise clean; the `transformers` package is still excluded.
r"vllm/model_executor/models/transformers",
r"vllm/model_executor/models/transformers",

Comment not needed

Comment thread vllm/model_executor/models/telechat2.py Outdated
Comment on lines +49 to +51
# `attribute_map` is a ClassVar on `PretrainedConfig`; shadowing it on the
# instance is intentional here so LlamaModel can read TeleChat2's fields.
vllm_config.model_config.hf_config.attribute_map = { # type: ignore[misc]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# `attribute_map` is a ClassVar on `PretrainedConfig`; shadowing it on the
# instance is intentional here so LlamaModel can read TeleChat2's fields.
vllm_config.model_config.hf_config.attribute_map = { # type: ignore[misc]
vllm_config.model_config.hf_config.attribute_map = { # type: ignore[misc]

comment not needed

Signed-off-by: Jensen Chen <a1043904820@gmail.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@ZHIHANCHEN03

Copy link
Copy Markdown
Contributor Author

Thanks for the review! Both comments dropped in 8e3e891.

Re-verified after the change: pre-commit run mypy-3.10/3.11/3.12/3.13 --all-files --hook-stage manual all Passed, and the full hook set passes on the changed files.

@yewentao256 yewentao256 added the ready ONLY add when PR is ready to merge/full CI is needed label Aug 24, 2026
@github-actions

Copy link
Copy Markdown

@ZHIHANCHEN03, CI is now available for this PR.

  • /ci run starts upstream CI; /amd-ci run starts AMD CI only.
  • /ci retry retries failed jobs in the CI build for the current PR head. If the current head has no CI build, it starts a new CI build for the current head containing only jobs that failed in the latest earlier CI build for this PR.
  • /amd-ci retry retries failed jobs in AMD CI for the current PR head. Use /amd-ci run when the current head has no AMD CI build.
  • /ci cancel cancels scheduled or running CI builds for this PR branch; /amd-ci cancel does the same for AMD CI only.

@ZHIHANCHEN03

Copy link
Copy Markdown
Contributor Author

/ci retry

@github-actions

Copy link
Copy Markdown

✅ No earlier CI build exists for this PR. Use /ci run first.

@ZHIHANCHEN03

Copy link
Copy Markdown
Contributor Author

/ci run

@github-actions

Copy link
Copy Markdown

✅ Triggered Buildkite CI #85436 for commit 8e3e891f7334.

@hmellor

hmellor commented Aug 26, 2026

Copy link
Copy Markdown
Member

/ci run

@hmellor
hmellor enabled auto-merge (squash) August 26, 2026 12:26
@github-actions

Copy link
Copy Markdown

✅ Triggered Buildkite CI #85649 for commit fa00c3962464.

@hmellor
hmellor merged commit c47ca4a into vllm-project:main Aug 26, 2026
90 checks passed
am-cohere pushed a commit to am-cohere/vllm that referenced this pull request Sep 1, 2026
…ct#53466)

Signed-off-by: Jensen Chen <a1043904820@gmail.com>
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
mikeshawcode pushed a commit to mikeshawcode/vllm that referenced this pull request Sep 1, 2026
…ct#53466)

Signed-off-by: Jensen Chen <a1043904820@gmail.com>
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Signed-off-by: mikeshawcode <michaelwshaw2@gmail.com>
mylibrar pushed a commit to tanyuqian/vllm that referenced this pull request Sep 3, 2026
…ct#53466)

Signed-off-by: Jensen Chen <a1043904820@gmail.com>
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready ONLY add when PR is ready to merge/full CI is needed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants