Skip to content

[Bugfix][DeepSeek V4] Name the config field a checkpoint is missing - #57818

Closed
aoshen02 wants to merge 1 commit into
vllm-project:mainfrom
aoshen02:fix/deepseek-v4-config-required-fields
Closed

aoshen02 wants to merge 1 commit into
vllm-project:mainfrom
aoshen02:fix/deepseek-v4-config-required-fields

Conversation

@aoshen02

@aoshen02 aoshen02 commented Sep 20, 2026 •

Copy link
Copy Markdown
Contributor

Summary

vllm/models/deepseek_v4/** reads six fields straight off the config object
while building layers, with no default anywhere in the stack:

self.n_groups = config.o_groups                       # attention.py:212
self.topk_tokens = config.index_topk                  # attention.py:928
self.hc_mult = config.hc_mult                         # amd/model.py:724 (+20 more)
self.swiglu_limit = config.swiglu_limit               # amd/model.py:535
is_hash_moe = ... < config.num_hash_layers            # amd/model.py:553

A checkpoint whose config.json omits one of them fails deep inside layer
construction with:

AttributeError: 'DeepseekV4Config' object has no attribute 'o_groups'

which names the attribute but not the file it belongs in. This PR raises from
the config classes instead, naming config.json and the full required set.

Why not defaults

Because a default would be a guess at the model's shape. index_topk sets the
sparse selection size, compress_ratios the per-layer cache type, o_groups
the o-proj grouping — a wrong value builds a model that does not match the
weights and fails later, or worse, silently produces bad output. An explicit
error is the correct behaviour for a checkpoint that does not say.

Cost

The hook is __getattr__, which Python only calls when normal lookup fails, so
a complete checkpoint pays nothing and behaves exactly as before. Unrelated
missing attributes keep raising a plain AttributeError(name), so copy,
to_dict and the transformers machinery are unaffected — covered by a test.

Applies to both DeepseekV4Config and DeepseekV41Config (V4.1 flattens
text_config onto the top level and has the same six fields missing when the
checkpoint omits them).

Not a duplicate

gh pr list --search "DeepseekV4Config" returns only #44031, which normalizes
compress_ratios for transformers >= 4.57 inside the model files — a different
problem (a field that is present but shaped differently). No open PR or issue
covers the missing-field failure mode.

Test plan

pytest tests/transformers_utils/test_deepseek_v4_config_fields.py -q
# 16 passed

CPU-only. Covers each required field on both config classes, unrelated
attributes still raising plainly, and a complete config surviving deepcopy
and to_dict.

pre-commit run --files vllm/transformers_utils/configs/deepseek_v4.py \
  vllm/transformers_utils/configs/deepseek_v41.py \
  tests/transformers_utils/test_deepseek_v4_config_fields.py
# passed

No model-quality impact: no change for any checkpoint that loads today.

AI assistance

Developed with AI assistance (Claude). The missing-field list came from reading
every bare config.<field> access in the DeepSeek-V4/V4.1 model packages, and
was confirmed by constructing both config classes and checking each attribute. I
reviewed every changed line, ran the tests above, and I am the accountable
submitter.

🤖 Generated with Claude Code

Review rounds

A later adversarial round, told about the deleted list and asked for new
defects, went after the __getattr__ itself: copy/deepcopy, pickle protocols
0-5, cloudpickle and ForkingPickler (these configs cross the process boundary
to workers), probes on a pre-__init__ instance, hasattr fallbacks,
attribute_map and get_text_config() delegation, and dataclasses field
introspection. It executed all of them against transformers 5.12.1: both
DeepseekV4Config and DeepseekV41Config pass, and no caller matches the old
error text. No new defects.

@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.

@aoshen02

Copy link
Copy Markdown
Contributor Author

/ci run

@github-actions

Copy link
Copy Markdown

❌ This PR is 1 commit behind upstream main. Your branch must contain every commit currently on upstream main. No new CI build was started. Merge or rebase onto the latest main, then rerun /ci run. To test this branch at your own risk, use /ci run --allow-stale.

@mergify mergify Bot added deepseek Related to DeepSeek models DSv4 DSv4.1 Related to DeepSeek-V4.1 models bug Something isn't working labels Sep 20, 2026
@aoshen02
aoshen02 force-pushed the fix/deepseek-v4-config-required-fields branch from ec657d5 to 5148805 Compare September 20, 2026 17:40
@aoshen02

Copy link
Copy Markdown
Contributor Author

/ci run

@github-actions

Copy link
Copy Markdown

❌ This PR is 2 commits behind upstream main. Your branch must contain every commit currently on upstream main. No new CI build was started. Merge or rebase onto the latest main, then rerun /ci run. To test this branch at your own risk, use /ci run --allow-stale.

The V4 / V4.1 model packages read architecture fields straight off the
config while building layers, with no default in the read, so a checkpoint
whose config.json omits one fails deep inside layer construction with a
bare AttributeError naming only the attribute -- with no hint that the fix
is a config.json key.

Explain it from the config classes instead. The message names the field,
the model type and the file.

No list of "required" fields: the set differs by architecture, platform and
enabled feature, so a fixed list is both incomplete (head_dim,
index_n_heads, the Engram fields) and partly wrong (V4.1 reaches V4's
swiglu_limit read; num_hash_layers is supplied by both V4.1 MoE paths).
A list that advertises the wrong fields sends people to edit keys that do
not matter.

The hook only runs when normal attribute lookup fails, so a complete
checkpoint is unaffected and hasattr()/getattr(..., default) probes keep
deciding fallbacks exactly as before.

Signed-off-by: Ao Shen <aoshen@inferact.ai>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Signed-off-by: aoshen02 <aoshen@inferact.ai>
@aoshen02
aoshen02 force-pushed the fix/deepseek-v4-config-required-fields branch from 5148805 to 7f29eea Compare September 20, 2026 18:00
@aoshen02

Copy link
Copy Markdown
Contributor Author

/ci run

@github-actions

Copy link
Copy Markdown

❌ This PR is 2 commits behind upstream main. Your branch must contain every commit currently on upstream main. No new CI build was started. Merge or rebase onto the latest main, then rerun /ci run. To test this branch at your own risk, use /ci run --allow-stale.

@mergify

mergify Bot commented Sep 24, 2026

Copy link
Copy Markdown
Contributor

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @aoshen02.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@hmellor

hmellor commented Oct 9, 2026

Copy link
Copy Markdown
Member

Closing as we plan to delete this file in #60823

@hmellor hmellor closed this Oct 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working deepseek Related to DeepSeek models DSv4 DSv4.1 Related to DeepSeek-V4.1 models needs-rebase

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants