[None][fix] clamp very small non-zero sampling temperature#16222
Open
lonexreb wants to merge 2 commits into
Open
[None][fix] clamp very small non-zero sampling temperature#16222lonexreb wants to merge 2 commits into
lonexreb wants to merge 2 commits into
Conversation
Positive temperatures below a numerically safe floor (1e-2) overflow logits / temperature to inf/nan in fp16/bf16 and produce undefined sampling behavior. Clamp them up to MIN_SAMPLING_TEMPERATURE while leaving temperature == 0 (greedy decoding) unchanged. Fixes NVIDIA#15715 Signed-off-by: lonexreb <reach2shubhankar@gmail.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
📝 WalkthroughWalkthroughAdds a ChangesSampling temperature safety
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/unittest/llmapi/test_sampling_params.py (2)
20-41: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd type annotations to the new test functions.
The Python guidelines require function annotations; use
-> Noneand annotate parametrized temperature arguments asfloat.Proposed fix
-def test_temperature_none_unchanged(): +def test_temperature_none_unchanged() -> None: -def test_temperature_zero_kept_for_greedy(): +def test_temperature_zero_kept_for_greedy() -> None: -def test_tiny_positive_temperature_clamped(tiny): +def test_tiny_positive_temperature_clamped(tiny: float) -> None: -def test_normal_temperature_unchanged(temp): +def test_normal_temperature_unchanged(temp: float) -> None: -def test_negative_temperature_rejected(): +def test_negative_temperature_rejected() -> None:🤖 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/unittest/llmapi/test_sampling_params.py` around lines 20 - 41, Add type annotations to all new test functions in the sampling parameter tests: use -> None for each test function and annotate parametrized arguments such as tiny and temp as float.Source: Coding guidelines
1-41: 📐 Maintainability & Code Quality | 🔵 TrivialNo QA test-list update is needed.
This is a narrow CPU-only unit-test change under
tests/unittest/llmapi; integration and performance QA lists do not need an entry.🤖 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/unittest/llmapi/test_sampling_params.py` around lines 1 - 41, Keep the change limited to the CPU-only unit tests in test_temperature_none_unchanged, test_temperature_zero_kept_for_greedy, test_tiny_positive_temperature_clamped, test_normal_temperature_unchanged, and test_negative_temperature_rejected; do not modify integration or performance QA test lists.Source: Path instructions
🤖 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 327-332: Validate that temperature is finite in the SamplingParams
temperature validation logic before the negative-value and minimum-floor checks.
Reject NaN and positive or negative infinity with a ValueError, and add tests
covering these non-finite values alongside the existing sign and clamping tests.
In `@tests/unittest/llmapi/test_sampling_params.py`:
- Around line 29-31: Update test_tiny_positive_temperature_clamped to assert the
contractual floor directly as 1e-2 instead of using MIN_SAMPLING_TEMPERATURE,
while retaining the existing tiny positive temperature parameterization.
---
Nitpick comments:
In `@tests/unittest/llmapi/test_sampling_params.py`:
- Around line 20-41: Add type annotations to all new test functions in the
sampling parameter tests: use -> None for each test function and annotate
parametrized arguments such as tiny and temp as float.
- Around line 1-41: Keep the change limited to the CPU-only unit tests in
test_temperature_none_unchanged, test_temperature_zero_kept_for_greedy,
test_tiny_positive_temperature_clamped, test_normal_temperature_unchanged, and
test_negative_temperature_rejected; do not modify integration or performance QA
test lists.
🪄 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: c57bc6e4-0cd6-40b4-ac35-91f3ff4094c0
📒 Files selected for processing (2)
tensorrt_llm/sampling_params.pytests/unittest/llmapi/test_sampling_params.py
Comment on lines
327
to
+332
| if self.temperature is not None and self.temperature < 0: | ||
| raise ValueError(f"require temperature >= 0, got temperature={self.temperature}") | ||
| # Clamp very small non-zero temperatures up to a numerically safe floor; | ||
| # keep 0.0 as-is so greedy decoding is unaffected. | ||
| if self.temperature is not None and 0 < self.temperature < MIN_SAMPLING_TEMPERATURE: | ||
| self.temperature = MIN_SAMPLING_TEMPERATURE |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject non-finite temperatures before applying the floor.
float("nan") makes both comparisons false, so it is forwarded to the backend; positive infinity also passes through. Validate finiteness and add NaN/Infinity tests before the sign/clamp checks.
Proposed fix
+import math
+
...
+ if self.temperature is not None and not math.isfinite(self.temperature):
+ raise ValueError(f"temperature must be finite, got temperature={self.temperature}")
if self.temperature is not None and self.temperature < 0:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if self.temperature is not None and self.temperature < 0: | |
| raise ValueError(f"require temperature >= 0, got temperature={self.temperature}") | |
| # Clamp very small non-zero temperatures up to a numerically safe floor; | |
| # keep 0.0 as-is so greedy decoding is unaffected. | |
| if self.temperature is not None and 0 < self.temperature < MIN_SAMPLING_TEMPERATURE: | |
| self.temperature = MIN_SAMPLING_TEMPERATURE | |
| if self.temperature is not None and not math.isfinite(self.temperature): | |
| raise ValueError(f"temperature must be finite, got temperature={self.temperature}") | |
| if self.temperature is not None and self.temperature < 0: | |
| raise ValueError(f"require temperature >= 0, got temperature={self.temperature}") | |
| # Clamp very small non-zero temperatures up to a numerically safe floor; | |
| # keep 0.0 as-is so greedy decoding is unaffected. | |
| if self.temperature is not None and 0 < self.temperature < MIN_SAMPLING_TEMPERATURE: | |
| self.temperature = MIN_SAMPLING_TEMPERATURE |
🤖 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 `@tensorrt_llm/sampling_params.py` around lines 327 - 332, Validate that
temperature is finite in the SamplingParams temperature validation logic before
the negative-value and minimum-floor checks. Reject NaN and positive or negative
infinity with a ValueError, and add tests covering these non-finite values
alongside the existing sign and clamping tests.
Comment on lines
+29
to
+31
| @pytest.mark.parametrize("tiny", [1e-12, 1e-6, MIN_SAMPLING_TEMPERATURE / 2]) | ||
| def test_tiny_positive_temperature_clamped(tiny): | ||
| assert SamplingParams(temperature=tiny).temperature == MIN_SAMPLING_TEMPERATURE |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the contractual floor independently of the implementation constant.
Using MIN_SAMPLING_TEMPERATURE as the expected value means an incorrect constant could still pass the tests. The requirement is specifically 1e-2.
Proposed fix
- assert SamplingParams(temperature=tiny).temperature == MIN_SAMPLING_TEMPERATURE
+ assert SamplingParams(temperature=tiny).temperature == 1e-2📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @pytest.mark.parametrize("tiny", [1e-12, 1e-6, MIN_SAMPLING_TEMPERATURE / 2]) | |
| def test_tiny_positive_temperature_clamped(tiny): | |
| assert SamplingParams(temperature=tiny).temperature == MIN_SAMPLING_TEMPERATURE | |
| `@pytest.mark.parametrize`("tiny", [1e-12, 1e-6, MIN_SAMPLING_TEMPERATURE / 2]) | |
| def test_tiny_positive_temperature_clamped(tiny): | |
| assert SamplingParams(temperature=tiny).temperature == 1e-2 |
🤖 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/unittest/llmapi/test_sampling_params.py` around lines 29 - 31, Update
test_tiny_positive_temperature_clamped to assert the contractual floor directly
as 1e-2 instead of using MIN_SAMPLING_TEMPERATURE, while retaining the existing
tiny positive temperature parameterization.
- Reject NaN/+-Inf temperatures before the sign/clamp checks (they bypassed both comparisons and reached the backend). - Add -> None / float type annotations on the new tests. - Assert the contractual floor 1e-2 directly, plus NaN/Inf rejection tests. Signed-off-by: lonexreb <reach2shubhankar@gmail.com>
Author
|
Thanks for the review. Addressed in ef42922:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes #15715.
SamplingParamsonly validatedtemperature >= 0, so tiny positive values (e.g.1e-12) passed through unchanged. In the sampling backend,logits / temperaturewith such values overflows toinf/nanin fp16/bf16 (typical logits ~10 divided by 1e-6 already exceed fp16's max ~65504), producing undefined sampling behavior.Change
MIN_SAMPLING_TEMPERATURE = 1e-2and clamp any positive temperature below it inSamplingParams._validate().temperature == 0.0(greedy decoding) is explicitly left unchanged.Test
New CPU-only unit tests in
tests/unittest/llmapi/test_sampling_params.pycovering:Nonepassthrough, greedy0.0preserved, tiny values (1e-12,1e-6,5e-3) clamped to the floor, normal temperatures unchanged, and negative rejected.Summary by CodeRabbit
Bug Fixes
Tests