Skip to content

[Fix] Disable top-k filtering by default - #5079

Merged
silencelamb merged 4 commits into
InternLM:mainfrom
JT-Ushio:fix/default-top-k-unrestricted
Nov 3, 2026
Merged

silencelamb merged 4 commits into
InternLM:mainfrom
JT-Ushio:fix/default-top-k-unrestricted

Conversation

@JT-Ushio

@JT-Ushio JT-Ushio commented Oct 30, 2026 •

Copy link
Copy Markdown
Contributor

Motivation

When neither the request nor the model’s generation_config.json specifies top_k, LMDeploy falls back to 50. Callers can easily specify only temperature and top_p and overlook this additional truncation, leading to unintended sampling differences across serving engines.

Other engines default to no top-k filtering when no value is supplied:

For example, with model defaults {"temperature": 1.0, "top_p": 0.95}, omitting top_k currently introduces a top-50 restriction in LMDeploy. This PR removes that implicit restriction.

Modification

  • Change GenerationConfig.top_k from 50 to 0, the existing value for disabling top-k filtering.
  • Document the default.
  • Load explicitly declared JSON values without dropping values that match Transformers defaults (including top_k=50 in Transformers 4.x). Absent fields still fall back to LMDeploy defaults.
  • Treat JSON null generation defaults as unset, preserving explicit values such as 0 and top_k=50.
  • Add regression coverage for fallback, auto/custom file sources, explicit and null sampling values, full Transformers config exports, and request overrides.

Compatibility

Sampled outputs may change when top_k is omitted from both the effective defaults and the request. Users can explicitly set top_k=50 to retain the previous behavior.

Model/server defaults and request overrides retain their existing precedence. Greedy decoding is unaffected.

Validation

  • CPU configuration and message tests with Transformers 4.57.6, 5.5.4, and 5.16.0: 56 passed, 4 tokenizer-download cases deselected for each version.
  • Coverage includes explicit JSON nulls for top_k, top_p, temperature, and min_p; zero values; mixed null/non-null defaults; and files exported via to_dict() and to_json_file(..., use_diff=False).
  • The 14 null-related regression cases fail against the preceding loader and pass with null defaults filtered out. Explicit top_k=50 and request override coverage remain passing.
  • All applicable pre-commit checks passed.

Copilot AI balanced review requested due to automatic review settings October 30, 2026 11:56

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The loader discards explicitly configured top_k=50 as a default value, causing requests that omit top_k to fall back to 0 and unintentionally disable top-k filtering.

1 open issue
What changed in this PR

This PR removes implicit top-50 truncation from LMDeploy’s generation defaults, making omitted top_k disable filtering.

Changes:

  • Change GenerationConfig.top_k from 50 to 0.
  • Document that 0 disables top-k filtering.
  • Add regression cases for fallback behavior and model/request overrides, including explicit 0.
File Description
tests/​test_lmdeploy/​serve/​test_generation_config.py Add parameterized tests for top-k fallback and override precedence.
lmdeploy/​messages.py Set the top-k default to 0 and document its meaning.

Review effort: Balanced

Comment thread lmdeploy/messages.py Resolved

@silencelamb silencelamb left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM. Explicit top_k values are preserved, and null generation defaults are correctly treated as unset. Both review concerns are resolved.

@RunningLeon RunningLeon left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM

@silencelamb
silencelamb merged commit 3228395 into InternLM:main Nov 3, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants