[#17158][fix] Reject NaN top_p, min_p and temperature in SamplingParams - #17159
Conversation
Walkthrough
ChangesSampling parameter validation
Estimated code review effort: 1 (Trivial) | ~5 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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
📒 Files selected for processing (2)
tensorrt_llm/sampling_params.pytests/unittest/llmapi/test_sampling_params.py
BowenFu
left a comment
BowenFu
left a comment
There was a problem hiding this comment.
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.
|
/bot run |
|
Could a maintainer trigger CI on this one with I posted Everything else looks settled: the review feedback about registering the test module in the test lists was addressed in The change is CPU-only and touches |
|
/bot run |
|
PR_Github #63699 [ run ] triggered by Bot. Commit: |
|
PR_Github #63699 [ run ] completed with state
|
|
Thanks for triggering CI @zhaoyangwang-nvidia. I can't open the failure report or the Jenkins job (both are on the internal network), so could you share which stage/test Meanwhile, some evidence that suggests the failure isn't specific to this PR: in the 2026-08-03 → 2026-08-05 window, 13 of 13 completed One thing worth noting for anyone skimming: in the On this PR's side, the change is CPU-only: three comparisons in The branch is currently 58 commits behind |
…ngParams The range checks for these three parameters were written in positive form, as `value < low or value > high`. Every comparison against NaN is False, so NaN passed validation and reached the sampling backend, which applies no guard of its own. Rewrite the three checks as negated range checks, which rejects NaN while leaving all other values unchanged. This is the form the neighbouring top_p_decay and top_p_min checks already use. Signed-off-by: Yigtwxx <yigiterdogan023@gmail.com>
…re-merge list The module was absent from every test list, so none of its cases -- including the NaN regression tests added in this branch -- ran in CI. Add it to the llmapi group of the CPU pre-merge stage, alongside the other CPU-only llmapi unit tests such as test_reasoning_parser.py. The tests need no GPU: they only construct SamplingParams and assert on validation. Signed-off-by: Yigtwxx <yigiterdogan023@gmail.com>
5fdb20a to
30897c3
Compare
|
Rebased onto current The conflict was only in the test-list file, and resolving it turned out to be a real change rather than a mechanical one:
Verified after the rebase: the new tests still pass locally (16 cases in the file), and Ready for |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/integration/test_lists/test-db/l0_cpu.yml (1)
1-1: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd the required NVIDIA copyright header.
The supplied new file starts with
version: 0.0.1and has no NVIDIA copyright header. Add the repository-standard header before the YAML content.🤖 Prompt for 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. In `@tests/integration/test_lists/test-db/l0_cpu.yml` at line 1, Add the repository-standard NVIDIA copyright header at the beginning of the YAML file, before the existing version declaration; leave the version and remaining YAML content unchanged.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@tests/integration/test_lists/test-db/l0_cpu.yml`:
- Line 1: Add the repository-standard NVIDIA copyright header at the beginning
of the YAML file, before the existing version declaration; leave the version and
remaining YAML content unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 70fbfe0a-265b-4ed5-acc1-568f33593077
📒 Files selected for processing (2)
tensorrt_llm/sampling_params.pytests/integration/test_lists/test-db/l0_cpu.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- tensorrt_llm/sampling_params.py
|
@coderabbitai Re: the outside-diff finding on
Happy to be corrected if a maintainer wants headers rolled out across |
|
@zhaoyangwang-nvidia — quick ping since you triggered the last run: the branch has been rebased onto current Could you re-trigger And if this run also comes back red, could you paste which stage/test failed? The report and Jenkins links are internal-only, so I can't see them from outside. No other blockers on my side: four approvals, review feedback addressed, and the CodeRabbit copyright-header finding answered above. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
file='tests/integration/test_lists/test-db/l0_cpu.yml'
echo '--- changed files and status against base ---'
git diff --name-status "$(git merge-base HEAD origin/main)" HEAD -- "$file" || true
echo '--- diff for the test-list file ---'
git diff --unified=3 "$(git merge-base HEAD origin/main)" HEAD -- "$file" || true
echo '--- first line of each test-db YAML file ---'
fd --type f --extension yml . tests/integration/test_lists/test-db \
| sort \
| while IFS= read -r candidate; do
printf '%s\t%s\n' "$(head -n 1 "$candidate")" "$candidate"
done
echo '--- YAML license/header hook references ---'
rg -n -i -C2 'insert-license|license' .pre-commit-config.yaml || trueLength of output: 6350
The existing test-list convention does not require a copyright header. All other listed YAML files except I withdraw the finding. ✏️ Learnings added
You are interacting with an AI system. |
|
Correction to my earlier comment (#17159 (comment)): the "13 of 13 completed pipelines failed" figure in it is wrong. Retracting it, since I'd rather it not feed into anyone's triage. I redid the count over a much larger sample — 70 PRs, every
So That removes the "this isn't specific to my PR" inference, so the ask stands on its own merits rather than on that claim: @zhaoyangwang-nvidia — could you re-trigger |
|
/bot run |
|
PR_Github #63869 [ run ] triggered by Bot. Commit: |
|
PR_Github #63869 [ run ] completed with state
|
|
/bot run |
|
PR_Github #63908 [ run ] triggered by Bot. Commit: |
|
PR_Github #63908 [ run ] completed with state |
7 participants
Description
Fixes #17158.
SamplingParams._validate()boundedtop_p,min_pandtemperaturewith positiverange checks of the form
value < low or value > high. Every comparison against NaNevaluates to
False, soSamplingParams(top_p=float("nan"))and itsmin_p/temperatureequivalents passed validation unchanged and were forwarded to the samplingbackend, which applies no guard of its own —
sampler_strategy.py:325-337divides thelogits by the raw temperature tensor and
ops/flashinfer.py:216hands it to the fusedsoftmax. 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 acceptedset otherwise identical, with the existing error messages unchanged. It is also the form
the
top_p_decayandtop_p_minchecks a few lines below already use — those twoconsequently reject NaN today, which is what made the file inconsistent with itself.
Deliberately out of scope:
temperature=float("inf")is still accepted. It flattens thedistribution 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_stabilityis unaffected.Test Coverage
tests/unittest/llmapi/test_sampling_params.py(CPU-only, no model weights):test_sampling_params_rejects_nan— parametrized overtop_p,min_p,temperature;each must raise
ValueError. This is the regression guard: all three cases failagainst
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 narrowthe 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 roundtrip unchanged.
Results: 16 cases, all passing on this branch. Against
mainwith only the test changesapplied, 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-compatibleorapi-breaking. Forapi-breaking, includeBREAKINGin 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 rejectsNaNfortop_p,min_p, andtemperature.top_kvalidation is unchanged.Verdict: sufficient
QA Engineer Review
NaNrejection fortop_p,min_p, andtemperature.tests/integration/test_lists/test-db/l0_cpu.yml.Verdict: sufficient
Summary
NaNfortop_p,min_p, andtemperatureinSamplingParams._validate().temperatureand small-temperature clamping remain out of scope.Dev Engineer Review
NaNbecause comparisons withNaNevaluate to false.top_kvalidation remains unchanged.QA Engineer Review
NaNrejection fortop_p,min_p, andtemperature.tests/integration/test_lists/test-db/l0_cpu.yml.