Skip to content

[https://nvbugs/6550099][fix] Raise no-top-k equivalence tolerance - #17998

Merged
lori-ren merged 3 commits into
NVIDIA:mainfrom
lori-ren:fix/advanced-sampling-top-p-mismatch
Aug 24, 2026
Merged

[https://nvbugs/6550099][fix] Raise no-top-k equivalence tolerance#17998
lori-ren merged 3 commits into
NVIDIA:mainfrom
lori-ren:fix/advanced-sampling-top-p-mismatch

Conversation

@lori-ren

@lori-ren lori-ren commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Dev Engineer Review

  • Updated test_no_topk_matches_full to account for FP32 variation at the nucleus cutoff.
  • Raised the no-top-k equivalence tolerance from 1e-5 to 1e-4.
  • Removed the matching waiver from tests/integration/test_lists/waives.txt.
  • The change is scoped to the affected test. No API or public entity changes were made.
  • The waiver path matches the modified test. No duplicate or format issues are evident.

QA Engineer Review

  • Modified test: test_no_topk_matches_full.
  • The test is not listed in test-db/ or qa/.
  • The waiver entry was removed.
  • The full test file and 30 consecutive runs of the affected case passed on an A100.
  • Verdict: needs follow-up because CBTS coverage data is unavailable.

Description

Root cause: the assertion's tolerance was tighter than the precision of the operator it compares. FULL and NO_TOPK are bit-identical up to the top-p renorm, so nothing in the sampling path differs. At the nucleus cutoff fp32 precision decides whether one token is kept, and that token is worth up to 7.8e-5 of mass in input batch — about 8x the assertion's atol=1e-5. The test asked for more than fp32 offers, so it failed whenever that boundary token moved.

Fixed by raising atol to 1e-4.
Validated on A100: full file passes, 30 consecutive runs of the case pass. The waiver is removed in the fix PR.

Test Coverage

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.

…ove top-p renorm fp32 precision

Signed-off-by: Lori Ren <lorir@nvidia.com>
@lori-ren
lori-ren requested a review from a team as a code owner August 20, 2026 04:00
@coderabbitai

coderabbitai Bot commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

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: a796089e-bbd5-4c4b-8fcb-b131b08b0fa2

📥 Commits

Reviewing files that changed from the base of the PR and between 334413e and f6e1319.

📒 Files selected for processing (1)
  • tests/unittest/_torch/speculative/hw_agnostic/test_advanced_sampling_mode.py

Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review.


Walkthrough

The advanced sampling test now documents nucleus-cutoff nondeterminism, compares per-row probability-mass differences with a 1e-3 limit, and removes the waiver for the affected case.

Changes

Advanced sampling validation

Layer / File(s) Summary
Sampling comparison tolerance
tests/unittest/_torch/speculative/hw_agnostic/test_advanced_sampling_mode.py, tests/integration/test_lists/waives.txt
The test documents boundary variation and uses a maximum per-row L1 difference below 1e-3. The waiver for test_no_topk_matches_full[0.9] is removed.

Estimated code review effort: 1 (Trivial) | ~5 minutes

Merge Risk: ⚪ Minimal · up to f6e13

This narrowly raises a test tolerance and removes the related waiver; no actionable merge-blocking risk remains after normal checks and review.

Suggested reviewers: bowenfu, cascade812, mikeiovine

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title follows the required NVBugs and type format and clearly states the tolerance fix.
Description check ✅ Passed The description explains the root cause, solution, and validation results, but places test coverage details under Description instead of Test Coverage.
Docstring Coverage ✅ Passed Docstring check was indeterminate for this PR — some files could not be analyzed in time. Not blocking.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

@lori-ren

Copy link
Copy Markdown
Contributor Author

/bot run --stage-list DGX_H100-PyTorch-4

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #67652 [ run ] triggered by Bot. Commit: ac29200 Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #67652 [ run ] completed with state FAILURE. Commit: ac29200
/LLM/main/L0_MergeRequest_PR pipeline #55138 (Partly Tested) completed with status: 'FAILURE'

CI Report

⚠️ Action Required:

  • Please check the failed tests and fix your PR
  • If you cannot view the failures, ask the CI triggerer to share details
  • Once fixed, request an NVIDIA team member to trigger CI again

CI Agent Failure Analysis

Link to invocation

@lori-ren

Copy link
Copy Markdown
Contributor Author

/bot run --stage-list DGX_H100-PyTorch-4

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #67739 [ run ] triggered by Bot. Commit: ac29200 Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #67739 [ run ] completed with state FAILURE. Commit: ac29200
/LLM/main/L0_MergeRequest_PR pipeline #55218 (Partly Tested) completed with status: 'FAILURE'

CI Report

⚠️ Action Required:

  • Please check the failed tests and fix your PR
  • If you cannot view the failures, ask the CI triggerer to share details
  • Once fixed, request an NVIDIA team member to trigger CI again

CI Agent Failure Analysis

Link to invocation

@lori-ren

Copy link
Copy Markdown
Contributor Author

/bot run --extra-stage "DGX_H100-PyTorch-4"

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #67763 [ run ] triggered by Bot. Commit: ac29200 Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #67763 [ run ] completed with state FAILURE. Commit: ac29200
/LLM/main/L0_MergeRequest_PR pipeline #55242 completed with status: 'FAILURE'

CI Report

⚠️ Action Required:

  • Please check the failed tests and fix your PR
  • If you cannot view the failures, ask the CI triggerer to share details
  • Once fixed, request an NVIDIA team member to trigger CI again

CI Agent Failure Analysis

Link to invocation

@lori-ren

Copy link
Copy Markdown
Contributor Author

/bot run --disable-fail-fast

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #67786 [ run ] triggered by Bot. Commit: ac29200 Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #67786 [ run ] completed with state FAILURE. Commit: ac29200
/LLM/main/L0_MergeRequest_PR pipeline #55264 completed with status: 'FAILURE'

CI Report

⚠️ Action Required:

  • Please check the failed tests and fix your PR
  • If you cannot view the failures, ask the CI triggerer to share details
  • Once fixed, request an NVIDIA team member to trigger CI again

CI Agent Failure Analysis

Link to invocation

@lori-ren

Copy link
Copy Markdown
Contributor Author

/bot run --stage-list "DGX_H100-PyTorch-4"

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #67885 [ run ] triggered by Bot. Commit: ac29200 Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #67885 [ run ] completed with state FAILURE. Commit: ac29200
/LLM/main/L0_MergeRequest_PR pipeline #55352 (Partly Tested) completed with status: 'FAILURE'

CI Report

⚠️ Action Required:

  • Please check the failed tests and fix your PR
  • If you cannot view the failures, ask the CI triggerer to share details
  • Once fixed, request an NVIDIA team member to trigger CI again

CI Agent Failure Analysis

Link to invocation

@lori-ren

Copy link
Copy Markdown
Contributor Author

/bot run --extra-stage "DGX_H100-PyTorch-4"

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #68057 [ run ] triggered by Bot. Commit: ac29200 Link to invocation

@lori-ren

Copy link
Copy Markdown
Contributor Author

/bot run --stage-list "DGX_H100-PyTorch-4"

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #68057 [ run ] completed with state SUCCESS. Commit: ac29200
/LLM/main/L0_MergeRequest_PR pipeline #55508 completed with status: 'FAILURE'

CI Report

⚠️ Action Required:

  • Please check the failed tests and fix your PR
  • If you cannot view the failures, ask the CI triggerer to share details
  • Once fixed, request an NVIDIA team member to trigger CI again

CI Agent Failure Analysis

Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #68079 [ run ] triggered by Bot. Commit: ac29200 Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #68079 [ run ] completed with state FAILURE. Commit: ac29200
/LLM/main/L0_MergeRequest_PR pipeline #55527 (Partly Tested) completed with status: 'FAILURE'

CI Report

⚠️ Action Required:

  • Please check the failed tests and fix your PR
  • If you cannot view the failures, ask the CI triggerer to share details
  • Once fixed, request an NVIDIA team member to trigger CI again

CI Agent Failure Analysis

Link to invocation

@lori-ren

Copy link
Copy Markdown
Contributor Author

/bot run --stage-list "DGX_H100-PyTorch-4" --disable-fail-fast

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #68137 [ run ] triggered by Bot. Commit: ac29200 Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #68137 [ run ] completed with state FAILURE. Commit: ac29200
/LLM/main/L0_MergeRequest_PR pipeline #55585 (Partly Tested) completed with status: 'FAILURE'

CI Report

⚠️ Action Required:

  • Please check the failed tests and fix your PR
  • If you cannot view the failures, ask the CI triggerer to share details
  • Once fixed, request an NVIDIA team member to trigger CI again

CI Agent Failure Analysis

Link to invocation

@coderabbitai

coderabbitai Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

@lori-ren

Copy link
Copy Markdown
Contributor Author

/bot run --stage-list "DGX_H100-PyTorch-4" --disable-fail-fast

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #68203 [ run ] triggered by Bot. Commit: 334413e Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #68203 [ run ] completed with state SUCCESS. Commit: 334413e
/LLM/main/L0_MergeRequest_PR pipeline #55649 (Partly Tested) completed with status: 'SUCCESS'

CI Report

Link to invocation

@zhaoyangwang-nvidia zhaoyangwang-nvidia 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.

Approve with comments.

Comment thread tests/unittest/_torch/speculative/hw_agnostic/test_advanced_sampling_mode.py Outdated
Comment thread tests/unittest/_torch/speculative/hw_agnostic/test_advanced_sampling_mode.py Outdated
… total mass, not atol

Signed-off-by: Lori Ren <lorir@nvidia.com>
@lori-ren

Copy link
Copy Markdown
Contributor Author

/bot run --stage-list "DGX_H100-PyTorch-4"

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #68672 [ run ] triggered by Bot. Commit: f6e1319 Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #68672 [ run ] completed with state FAILURE. Commit: f6e1319
/LLM/main/L0_MergeRequest_PR pipeline #56077 (Partly Tested) completed with status: 'FAILURE'

CI Report

⚠️ Action Required:

  • Please check the failed tests and fix your PR
  • If you cannot view the failures, ask the CI triggerer to share details
  • Once fixed, request an NVIDIA team member to trigger CI again

CI Agent Failure Analysis

Link to invocation

@lori-ren

Copy link
Copy Markdown
Contributor Author

/bot run --stage-list "DGX_H100-PyTorch-4" --disable-fail-fast

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #68687 [ run ] triggered by Bot. Commit: f6e1319 Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #68687 [ run ] completed with state SUCCESS. Commit: f6e1319
/LLM/main/L0_MergeRequest_PR pipeline #56091 (Partly Tested) completed with status: 'SUCCESS'

CI Report

Link to invocation

@lori-ren

Copy link
Copy Markdown
Contributor Author

/bot run --disable-fail-fast

@lori-ren
lori-ren enabled auto-merge (squash) August 24, 2026 06:59
@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #68722 [ run ] triggered by Bot. Commit: f6e1319 Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #68722 [ run ] completed with state SUCCESS. Commit: f6e1319
/LLM/main/L0_MergeRequest_PR pipeline #56124 completed with status: 'SUCCESS'

CI Report

Link to invocation

@lori-ren
lori-ren merged commit df43752 into NVIDIA:main Aug 24, 2026
11 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.

3 participants