[#17158][fix] Reject NaN top_p, min_p and temperature in SamplingParams

[Yigtwxx]

Description

Fixes #17158.

SamplingParams._validate() bounded top_p, min_p and temperature with positive
range checks of the form value < low or value > high. Every comparison against NaN
evaluates to False, so SamplingParams(top_p=float("nan")) and its min_p /
temperature equivalents passed validation unchanged and were forwarded to the sampling
backend, which applies no guard of its own — sampler_strategy.py:325-337 divides the
logits by the raw temperature tensor and ops/flashinfer.py:216 hands it to the fused
softmax. The user got a request sampling from an undefined distribution rather than a
clear error at the API boundary.

The three checks are rewritten as negated range checks (not 0 <= top_p <= 1,
not 0 <= min_p <= 1, not temperature >= 0). This rejects NaN and leaves the accepted
set otherwise identical, with the existing error messages unchanged. It is also the form
the top_p_decay and top_p_min checks a few lines below already use — those two
consequently reject NaN today, which is what made the file inconsistent with itself.

Deliberately out of scope: temperature=float("inf") is still accepted. It flattens the
distribution rather than corrupting it, so rejecting it would be a behavior change to
input that currently works. #15715, which asks for very small non-zero temperatures to be
clamped, is likewise a separate discussion about valid input and is not addressed here.

No API signature changes; tests/unittest/api_stability is unaffected.

Test Coverage

tests/unittest/llmapi/test_sampling_params.py (CPU-only, no model weights):

• test_sampling_params_rejects_nan — parametrized over top_p, min_p, temperature;
  each must raise ValueError. This is the regression guard: all three cases fail
  against main, where NaN is accepted, and pass with this change.
• test_sampling_params_rejects_out_of_range — the previously covered rejections
  (-0.1, 1.1, temperature=-1.0) still raise, confirming the rewrite did not narrow
  the checks.
• test_sampling_params_accepts_in_range_values — boundary and typical values (0.0,
  0.9, 1.0, 0.5, temperature=0.0, temperature=1.0) are still accepted and round
  trip unchanged.

Results: 16 cases, all passing on this branch. Against main with only the test changes
applied, exactly the 3 NaN cases fail and the other 13 pass, so the accepted set is
otherwise unchanged.

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.

GitHub Bot Help

To see a list of available CI bot commands, please comment /bot help.

Dev Engineer Review

• SamplingParams._validate() now rejects NaN for top_p, min_p, and temperature.
• Existing error messages and valid range behavior remain unchanged.
• top_k validation is unchanged.
• No public API changes are introduced.
• The A10 test-list entry uses the correct test path and does not expand test scope beyond the CPU-only unit test.

Verdict: sufficient

QA Engineer Review

• Added coverage for:
  • NaN rejection for top_p, min_p, and temperature.
  • Existing out-of-range rejection.
  • Accepted boundary values.
  • Accepted representative in-range values.
• The test file is registered in tests/integration/test_lists/test-db/l0_a10.yml.
• CI coverage is sufficient for the added regression tests.

Verdict: sufficient

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 801ca6c9-bd22-4e89-9590-da97b31ebc44

📥 Commits

Reviewing files that changed from the base of the PR and between 73888bc and 5fdb20a.

📒 Files selected for processing (1)

• tests/integration/test_lists/test-db/l0_a10.yml

---

Walkthrough

Changes

Sampling parameter validation

Layer / File(s)	Summary
Range validation and test coverage tensorrt_llm/sampling_params.py, tests/unittest/llmapi/test_sampling_params.py, tests/integration/test_lists/test-db/l0_a10.yml	top_p, min_p, and temperature now reject NaN values. Tests cover NaN, out-of-range, boundary, and representative valid values. The test runs in the A10 PyTorch pre-merge suite.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested reviewers: qijune, brnguyen2

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly identifies the fix and the three affected SamplingParams fields.
Description check	✅ Passed	The description explains the bug, solution, scope, tests, and impact, and includes the required sections.
Linked Issues check	✅ Passed	The changes directly satisfy issue #17158 by rejecting NaN while preserving valid values and positive infinity handling.
Out of Scope Changes check	✅ Passed	The code, tests, and CI registration are relevant to the NaN validation fix and contain no unrelated changes.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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

[coderabbitai]

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 `@tests/unittest/llmapi/test_sampling_params.py`:
- Around line 118-121: Add tests/unittest/llmapi/test_sampling_params.py to both
the CI test-db lists and the QA test lists, using the existing list conventions
and preserving the newly added SamplingParams coverage.

🪄 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: 613ef542-c6f1-435d-abf2-587812ac16e1

📥 Commits

Reviewing files that changed from the base of the PR and between a9544e0 and 73888bc.

📒 Files selected for processing (2)

• tensorrt_llm/sampling_params.py
• tests/unittest/llmapi/test_sampling_params.py

[BowenFu]

Verified the rewrite is exactly equivalent to the old checks for every non-NaN input, including the 0/1 boundaries and ±inf, so the only behavior change is that NaN now raises. test_sampling_params_accepts_in_range_values is the right guard against over-tightening.

Not blocking: tests/unittest/llmapi/test_sampling_params.py isn't in any test-db/ list, so these tests won't actually run in pre-merge L0. Worth registering.