Skip to content

[None][fix] Clamp small non-zero sampling temperature - #15716

Open
chfeng-cs wants to merge 1 commit into
NVIDIA:mainfrom
chfeng-cs:fix/clamp-small-temperature
Open

[None][fix] Clamp small non-zero sampling temperature#15716
chfeng-cs wants to merge 1 commit into
NVIDIA:mainfrom
chfeng-cs:fix/clamp-small-temperature

Conversation

@chfeng-cs

@chfeng-cs chfeng-cs commented Jun 29, 2026

Copy link
Copy Markdown

Summary by CodeRabbit

  • Bug Fixes
    • Improved temperature validation to prevent very small nonzero values from causing unstable sampling behavior.
    • Very small temperatures are now automatically adjusted up to a safe minimum value.
    • Added coverage to verify the adjusted temperature is reflected correctly in sampling output.

Description

Fix #15715 .This PR clamps very small non-zero SamplingParams.temperature values to 1e-2 before they are passed to the sampling backend.

temperature=0 continues to represent greedy decoding, while positive values below the stability threshold are normalized to avoid excessive logit scaling from logits / temperature.

TensorRT-LLM currently validates that temperature >= 0, but values such as 1e-12 can still enter the sampling path and may lead to inf, nan, or unstable sampling behavior in edge cases.

This mirrors the numerical stability guard used by vLLM while preserving the existing greedy decoding semantics for temperature=0.

Changes

Added MIN_SAMPLING_TEMPERATURE = 1e-2.

Clamped 0 < temperature < MIN_SAMPLING_TEMPERATURE to MIN_SAMPLING_TEMPERATURE in SamplingParams._validate().

Added debug logging when a small non-zero temperature is clamped.

Added a unit test covering temperature=0, very small positive temperatures, the threshold value, and normal temperatures.

Test Coverage

pytest tests/unittest/llmapi/test_sampling_params.py -k temperature_clamps_small_nonzero_values

PR Checklist

Please review the following before submitting your PR:

  • PR description clearly explains what and why. If using CodeRabbit's summary, please make sure it makes sense.

  • PR Follows TRT-LLM CODING GUIDELINES to the best of your knowledge.

  • Test cases are provided for new code paths (see test instructions)

  • If PR introduces API changes, an appropriate PR label is added - either api-compatible or api-breaking. For api-breaking, include BREAKING in the PR title.

  • Any new dependencies have been scanned for license and vulnerabilities

  • CODEOWNERS updated if ownership changes

  • Documentation updated as needed

  • Update tava architecture diagram if there is a significant design change in PR.

  • The reviewers assigned automatically/manually are appropriate for the PR.

  • Please check this after reviewing the above items as appropriate for this PR.

@coderabbitai

coderabbitai Bot commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

Adds a MIN_SAMPLING_TEMPERATURE = 1e-2 constant to sampling_params.py and updates SamplingParams._validate() to clamp temperature values in the range (0, MIN_SAMPLING_TEMPERATURE) up to MIN_SAMPLING_TEMPERATURE with a debug log. A parametrized unit test verifies the clamping behavior.

Temperature Clamping

Layer / File(s) Summary
Constant and clamping logic
tensorrt_llm/sampling_params.py
Introduces MIN_SAMPLING_TEMPERATURE = 1e-2 and adds a clamping branch in _validate() for 0 < temperature < MIN_SAMPLING_TEMPERATURE, emitting a debug log.
Parametrized clamping test
tests/unittest/llmapi/test_sampling_params.py
Adds test_temperature_clamps_small_nonzero_values with parametrized inputs verifying temperature and _get_sampling_config().temperature are clamped; also updates imports to include MIN_SAMPLING_TEMPERATURE.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~5 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title is concise, specific, and accurately describes the main fix.
Description check ✅ Passed The description includes the required sections and clearly explains the problem, fix, and test coverage.
Linked Issues check ✅ Passed The changes implement the linked bug fix by clamping tiny non-zero temperatures while preserving temperature=0 behavior.
Out of Scope Changes check ✅ Passed The diff is limited to the temperature clamp and its unit test, with no unrelated changes.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@tensorrt_llm/sampling_params.py`:
- Around line 376-381: Update the public documentation for SamplingParams to
reflect the new temperature normalization behavior. In the SamplingParams class
docstring (and any temperature field description used by the user-facing API),
add that values greater than 0 but below MIN_SAMPLING_TEMPERATURE are clamped to
MIN_SAMPLING_TEMPERATURE, while 0 still means greedy decoding. Keep the wording
aligned with the existing temperature description in sampling_params.py so the
contract matches the behavior in the temperature handling logic.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 68c9e8a0-1271-425b-884b-35a8a0e8f402

📥 Commits

Reviewing files that changed from the base of the PR and between fde8e0a and 770deb1.

📒 Files selected for processing (2)
  • tensorrt_llm/sampling_params.py
  • tests/unittest/llmapi/test_sampling_params.py

Comment thread tensorrt_llm/sampling_params.py Outdated
@chfeng-cs
chfeng-cs force-pushed the fix/clamp-small-temperature branch 2 times, most recently from 7f35e15 to cac012d Compare June 29, 2026 11:27
Signed-off-by: Ethan Feng <ethan.fengch@gmail.com>
@chfeng-cs
chfeng-cs force-pushed the fix/clamp-small-temperature branch from cac012d to 60468cc Compare July 16, 2026 03:27
@chfeng-cs
chfeng-cs requested a review from a team as a code owner July 16, 2026 03:27
@chfeng-cs

Copy link
Copy Markdown
Author

@YihuiLu512 @lori-ren, could you please review #15716 before the merge decision and consider consolidating the work here, since this was the earlier implementation for #15715?

@zhaoyangwang-nvidia

Copy link
Copy Markdown
Collaborator

Thanks for the fix! Before we settle on the approach, a few questions:

The sampler already handles this case internally: GREEDY_TEMPERATURE_THRESHOLD = 1e-4 in _torch/pyexecutor/sampler/ops/vanilla.py treats any request with T <= 1e-4 as greedy (the temperature is never used as a divisor), so on the PyTorch sampler path a value like 1e-12 safely degrades to greedy rather than overflowing.

Given that, I have some concerns about clamping up to MIN_SAMPLING_TEMPERATURE = 1e-2:

  • It works against user intent. Someone passing T = 1e-12 clearly wants near-deterministic output. Clamping to 1e-2 silently turns that into genuine sampling, whereas snapping small values down to greedy (like the existing sampler-side threshold, and IIRC also what vLLM does with its epsilon check) preserves the intent and avoids the division entirely.
  • It silently changes valid requests. T = 5e-3 is numerically fine to compute, but would be rewritten to 1e-2 — a visibly different distribution — with only a debug-level log.
  • Two disconnected thresholds. After this change we'd have 1e-2 at the API layer and 1e-4 inside the sampler, unaware of each other; the sampler-side one becomes dead code for API users.

Could you share which path actually exhibited the inf/nan in #15715 (TRTLLMSampler? a path that bypasses the greedy threshold?) That would help decide whether the right fix is mapping tiny temperatures to greedy at the API level (reusing/aligning with GREEDY_TEMPERATURE_THRESHOLD), or adding the same guard to the affected path, rather than introducing a new clamp value.

@chfeng-cs

chfeng-cs commented Jul 16, 2026

Copy link
Copy Markdown
Author

@zhaoyangwang-nvidia Thanks for raising these questions. I appreciate the concerns you raised. I’ll carefully review the current sampling paths, including the exact coverage of the existing threshold and which paths may still be affected, and then follow up with a detailed answer.

@zhaoyangwang-nvidia

Copy link
Copy Markdown
Collaborator

@zhaoyangwang-nvidia Thanks for raising these questions. I appreciate the concerns you raised. I’ll carefully review the current sampling paths, including the exact coverage of the existing threshold and which paths may still be affected, and then follow up with a detailed answer.

Thanks for the context and for double-checking the current sampling paths.

Just to add, we are also actively refactoring the Torch sampler recently, so some of the related logic may continue to evolve. If you find any bugs, corner cases, or have suggestions while reviewing the sampling behavior, please feel free to raise them or discuss with me directly. I’d be happy to go through them together.

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.

[Bug]: Clamp very small non-zero temperature values to avoid numerical instability

2 participants