Skip to content

[https://nvbugs/5708901][perf] avoid logits copies when computing logprobs - #16958

Merged
ixlmar merged 10 commits into
NVIDIA:mainfrom
ixlmar:perf/logprobs-fewer-copies
Jul 31, 2026
Merged

[https://nvbugs/5708901][perf] avoid logits copies when computing logprobs#16958
ixlmar merged 10 commits into
NVIDIA:mainfrom
ixlmar:perf/logprobs-fewer-copies

Conversation

@ixlmar

@ixlmar ixlmar commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator

Description

Logprobs handling improvements in TorchSampler:

  • Avoid snapshotting logits for later computing log probs, but either

    • track logit slice indices (for computing raw logprobs); or

    • save one log_softmax() call by directly computing logprobs by offsetting logits by log(max(SoftMax(logits))) (for processed logprobs)

  • Simplify indexing in processed logprobs calculation (replace _Packed... with torch.repeat_interleaved + .masked_select)

  • Fast-path indexing in processed logprobs calculation if all requests require processed logprobs

  • Document several limitations/assumptions and potential future optimizations in code comments

Test Coverage

Relying on existing tests

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

  • Improved TorchSampler logprobs computation to reduce work and avoid logits snapshots:
    • Split “raw” vs “processed” logprobs flows and track logits/logit-slice indices to support efficient scatter/gather.
    • Support processed-logprobs without re-snapshotting logits by computing log-softmax with an offset derived from the maximum softmax-log value.
    • Simplified processed-logprobs indexing, added a fast path when all requests require processed logprobs, and filtered raw/processed computation to the correct request subsets (including excluding beam-search requests from the shared logprobs_cuda path).
    • Updated _CachingRequestGrouper.group_requests_by_strategy_key to return need_raw_logprobs, and reshaped _BatchedSamplingResult to carry separate processed/raw indices/logit indices.
  • Refactored sampler plumbing:
    • Updated _process_logprobs to accept logits_cuda and new_tokens_cuda explicitly and adjusted the logprobs_cuda/store layout assumptions.
    • Updated _process_requests call site and invocation ordering around _unbatch_sampling_results.
  • Updated fusion helpers in tensorrt_llm/_torch/pyexecutor/sampler/ops/vanilla.py:
    • Removed Fusions.gather_scatter / _gather_scatter_impl.
    • Replaced allocating Fusions.gather_log_softmax(...) with an output-buffered Fusions.gather_log_softmax_with_output(...), backed by _gather_log_softmax_impl(out=...) that writes in-place (preserving torch._dynamo.mark_dynamic behavior).
  • Added/updated documentation in code comments regarding assumptions/limitations and potential future optimizations (as described in the PR objectives).

QA Engineer Review

  • Modified test:
    • tests/unittest/_torch/sampler/test_torch_sampler.py::test_unbatch_sampling_results
      • Updated _BatchedSamplingResult construction/side-effect assertions to use req_indices and next_tokens_cuda_int.
  • Added test:
    • tests/unittest/_torch/sampler/test_logits_logprobs.py::TestLogsprobsInBatchedSampling::test_logprobs
      • Deterministic validation across batched/mixed configurations for raw vs LogprobMode.PROCESSED, including beam-search variants when supported by constraints; verifies sampled logprobs/top-k behavior matches recomputed expectations and checks logits aren’t mutated (with poisoned buffer regions to catch stale-state reliance).
  • CI test-list coverage:
    • tests/integration/test_lists/test-db/l0_a10.yml: unittest/_torch/sampler/test_torch_sampler.py
    • tests/integration/test_lists/test-db/l0_a30.yml: unittest/_torch/sampler/test_logits_logprobs.py
  • Verdict: sufficient

@ixlmar
ixlmar force-pushed the perf/logprobs-fewer-copies branch from 26337c0 to 93c4651 Compare July 28, 2026 16:16
@ixlmar
ixlmar requested a review from stnie July 28, 2026 16:16
Signed-off-by: ixlmar <206748156+ixlmar@users.noreply.github.com>
@ixlmar
ixlmar force-pushed the perf/logprobs-fewer-copies branch from 93c4651 to bf4109d Compare July 28, 2026 16:16
@ixlmar

ixlmar commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator Author

/bot run

@ixlmar ixlmar changed the title perf: avoid logits copies when computing logprobs [https://nvbugs/5708901][perf] avoid logits copies when computing logprobs Jul 28, 2026
@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #62231 [ run ] triggered by Bot. Commit: bf4109d Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #62231 [ run ] completed with state FAILURE. Commit: bf4109d
/LLM/main/L0_MergeRequest_PR pipeline #50399 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

@ixlmar

ixlmar commented Jul 29, 2026

Copy link
Copy Markdown
Collaborator Author

/bot run --disable-fail-fast

@ixlmar
ixlmar requested a review from Funatiq July 29, 2026 06:33
@ixlmar
ixlmar marked this pull request as ready for review July 29, 2026 06:34
@ixlmar
ixlmar requested a review from a team as a code owner July 29, 2026 06:34
@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #62454 [ run ] triggered by Bot. Commit: bf4109d Link to invocation

@coderabbitai

coderabbitai Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

The sampler separates raw and processed logprob requirements, excludes beam-search requests from shared buffers, uses output-buffered log-softmax fusion, and adds comprehensive mixed-batch validation.

Changes

Sampler logprob refactor

Layer / File(s) Summary
Request grouping contract
tensorrt_llm/_torch/pyexecutor/sampler/sampler.py
Raw-logprob requirements are returned separately from grouped request values while processed-logprob requirements remain in grouped metadata.
Batched logprob preparation
tensorrt_llm/_torch/pyexecutor/sampler/sampler.py
Sampling filters beam-search requests, computes processed logprobs into a dedicated buffer, prepares raw-logprob indices, and records separate result metadata.
Logprob processing and fusion output
tensorrt_llm/_torch/pyexecutor/sampler/ops/vanilla.py, tensorrt_llm/_torch/pyexecutor/sampler/sampler.py
Logprob processing reconstructs and scatters raw and processed values, while the fusion API writes gathered log-softmax results into an output tensor.
Batched logprob validation
tests/unittest/_torch/sampler/test_logits_logprobs.py, tests/unittest/_torch/sampler/test_torch_sampler.py
Tests cover mixed sampling configurations, beam-search filtering, returned logprob values, ranks, top-k sets, and renamed batched result fields.

Estimated code review effort: 4 (Complex) | ~60 minutes

Possibly related PRs

Suggested reviewers: brnguyen2, allisonlim-nv, funatiq

Sequence Diagram(s)

sequenceDiagram
  participant Grouper as _CachingRequestGrouper
  participant Sampler as _sample_batched_by_strategy
  participant Processor as _process_logprobs
  participant Fusion as Fusions
  Grouper->>Sampler: grouped requests and raw-logprob mask
  Sampler->>Sampler: filter beam requests and compute logprob buffers
  Sampler->>Processor: batched result with raw and processed indices
  Processor->>Fusion: gather_log_softmax_with_output
  Fusion->>Processor: write raw logprobs into output buffer
  Processor->>Processor: gather and scatter final logprob outputs
Loading
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title is concise and matches the main change: reducing logits copies during logprobs computation.
Description check ✅ Passed The description covers what changed, why, test coverage, and includes the checklist, though test details are minimal.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.

@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: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
tests/unittest/_torch/sampler/test_torch_sampler.py (1)

2510-2518: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Test coverage summary (tests/ review).**

  1. Changed test functions: only test_unbatch_sampling_results was touched, and solely to follow the _BatchedSamplingResult field renames (req_indices, next_tokens_cuda_int) plus one added side-effect assertion. No test functions were added or removed.
  2. Test-list files: no entries under tests/integration/test_lists/test-db/ or tests/integration/test_lists/qa/ were modified; since no new test functions were introduced, no list updates are strictly required — but if the logprob paths get dedicated cases, please register them in the appropriate test-db/ list.
  3. Coverage verdict: insufficient for this PR's behavior change. The rewritten logprob pipeline (separate raw vs processed buffers, per-group partial PROCESSED subsets, joint raw+processed indexing, gather_log_softmax_with_output) has no direct assertions here; the only touched test exercises unbatching. Recommended additions: a TestBatchedSampling-style case parameterized over logprobs_mode (RAW / PROCESSED / mixed within one strategy group) validating sampled logprob values, ranks and top-k output, including permuted request order and multi-step (draft) rows.

I'm happy to draft those cases if useful.

As per path instructions, "Always produce a test coverage summary, even if no issues are found" and the summary must state which tests changed, whether they are listed under tests/integration/test_lists/, and a coverage verdict.

🤖 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/_torch/sampler/test_torch_sampler.py` around lines 2510 -
2518, Extend the tests around TestBatchedSampling to cover the rewritten logprob
pipeline, rather than only updating test_unbatch_sampling_results for renamed
fields. Parameterize cases for RAW, PROCESSED, and mixed logprobs_mode within
one strategy group, asserting sampled logprob values, ranks, and top-k output
with permuted request order and multi-step draft rows. Register any new
integration test cases in the appropriate tests/integration/test_lists/ test-db
or qa list.

Source: Path instructions

🧹 Nitpick comments (1)
tensorrt_llm/_torch/pyexecutor/sampler/sampler.py (1)

5072-5072: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

logprobs_inout_indices_cuda_size is assigned but unused in the 1-beam branch.

Line 5072 computes it and the 1-beam block never reads it (only the n-beam block at Line 5147 does). Drop the 1-beam assignment.

Also applies to: 5147-5147

🤖 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/_torch/pyexecutor/sampler/sampler.py` at line 5072, Remove the
unused logprobs_inout_indices_cuda_size assignment from the 1-beam branch, while
retaining the assignment in the n-beam branch where it is consumed.
🤖 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/_torch/pyexecutor/sampler/sampler.py`:
- Around line 4576-4619: Fix the mixed processed-logprobs branch around
proc_lp_step_mask_cuda to gather rows by the selected request indices rather
than constructing a mask from only proc_lp_steps. Apply the same group-aligned
row selection to logits, softmax, and tensor temperature data, preserving scalar
temperatures, and add coverage for mixed processed/non-processed groups with
permuted request ordering.
- Around line 4995-5013: The 1-beam logprob path must preserve alignment with
rows in logprobs_cuda, whose order follows logprobs_reqs_indices. Update the
filtering logic around logprobs_reqs_indices_1_beam and the subsequent 1-beam
torch.gather flow to carry explicit source row indices or reorder/slice
logprobs_cuda before gathering, ensuring mixed 1-beam and n-beam batches read
the correct request rows.

---

Outside diff comments:
In `@tests/unittest/_torch/sampler/test_torch_sampler.py`:
- Around line 2510-2518: Extend the tests around TestBatchedSampling to cover
the rewritten logprob pipeline, rather than only updating
test_unbatch_sampling_results for renamed fields. Parameterize cases for RAW,
PROCESSED, and mixed logprobs_mode within one strategy group, asserting sampled
logprob values, ranks, and top-k output with permuted request order and
multi-step draft rows. Register any new integration test cases in the
appropriate tests/integration/test_lists/ test-db or qa list.

---

Nitpick comments:
In `@tensorrt_llm/_torch/pyexecutor/sampler/sampler.py`:
- Line 5072: Remove the unused logprobs_inout_indices_cuda_size assignment from
the 1-beam branch, while retaining the assignment in the n-beam branch where it
is consumed.
🪄 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: a4cff641-f0dd-4c40-8715-a2d53e6ed8b7

📥 Commits

Reviewing files that changed from the base of the PR and between 1f1acea and bf4109d.

📒 Files selected for processing (3)
  • tensorrt_llm/_torch/pyexecutor/sampler/ops/vanilla.py
  • tensorrt_llm/_torch/pyexecutor/sampler/sampler.py
  • tests/unittest/_torch/sampler/test_torch_sampler.py

Comment thread tensorrt_llm/_torch/pyexecutor/sampler/sampler.py
Comment thread tensorrt_llm/_torch/pyexecutor/sampler/sampler.py
Signed-off-by: ixlmar <206748156+ixlmar@users.noreply.github.com>
@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #62454 [ run ] completed with state SUCCESS. Commit: bf4109d
/LLM/main/L0_MergeRequest_PR pipeline #50607 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

Signed-off-by: ixlmar <206748156+ixlmar@users.noreply.github.com>
@ixlmar

ixlmar commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator Author

Note: New tests take ~26 seconds on RTX 6000 PRO.

@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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
tensorrt_llm/_torch/pyexecutor/sampler/sampler.py (1)

4735-4764: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Keep RAW beam requests in raw_logprobs_reqs_indices
Filtering this list down to py_beam_width == 1 drops beam requests from _process_logprobs, so sampled_log_prob_indices is never refreshed for RAW-mode beam batches and can be copied out stale. Keep the full request list here and use the filtered tensor only for batch_raw_logprob_indices_cuda.

🤖 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/_torch/pyexecutor/sampler/sampler.py` around lines 4735 - 4764,
In the raw-logprob preparation block, preserve all entries in
reqs_indices_needing_raw_logprobs for _process_logprobs and
sampled_log_prob_indices updates, including beam requests. Keep filtering
py_beam_width == 1 only when building gather_raw_logprobs_req_indices_tensor and
batch_raw_logprob_indices_cuda, while assigning the unfiltered request indices
to reqs_indices_needing_raw_logprobs.
🧹 Nitpick comments (1)
tests/unittest/_torch/sampler/test_logits_logprobs.py (1)

916-944: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

The two FIXME/exclusion comments carve out exactly the paths changed in this PR.

  • Line 932-934 skips temperature + logprobs=2 whenever the sampler is beam-capable, because validate_request rejects num_logprobs > 1 for any request once max_beam_width > 1 — even single-beam ones.
  • Line 942-944 skips PROCESSED + beam-capable batches, which is precisely the branch containing the beam-filter defect flagged in tensorrt_llm/_torch/pyexecutor/sampler/sampler.py (Lines 4568-4602).

Want me to open a tracking issue for the validate_request over-restriction, and/or add a mixed PROCESSED + beam-search case once the sampler branch is fixed?

🤖 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/_torch/sampler/test_logits_logprobs.py` around lines 916 -
944, Update LOGPROBS_CASES and BASE_CASES so the tests no longer exclude
temperature beam-capable requests with logprobs=2 or beam-search requests using
processed logprobs. Enable coverage for these paths, including the mixed
PROCESSED plus beam-search case, and remove the obsolete FIXME-based filters.
🤖 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/_torch/pyexecutor/sampler/sampler.py`:
- Around line 4568-4602: Correct the beam-search filtering in the
`group_gather_processed_logprobs` setup: retain `py_beam_width == 1` requests
for gathering and exclude the beam-search requests, rather than clearing the
width-one rows. Use each request’s actual group index (`req_idx`) when updating
the mask, not its enumeration position in
`need_processed_logprobs_req_indices_list`, and keep the count aligned with the
resulting gathered rows.

---

Outside diff comments:
In `@tensorrt_llm/_torch/pyexecutor/sampler/sampler.py`:
- Around line 4735-4764: In the raw-logprob preparation block, preserve all
entries in reqs_indices_needing_raw_logprobs for _process_logprobs and
sampled_log_prob_indices updates, including beam requests. Keep filtering
py_beam_width == 1 only when building gather_raw_logprobs_req_indices_tensor and
batch_raw_logprob_indices_cuda, while assigning the unfiltered request indices
to reqs_indices_needing_raw_logprobs.

---

Nitpick comments:
In `@tests/unittest/_torch/sampler/test_logits_logprobs.py`:
- Around line 916-944: Update LOGPROBS_CASES and BASE_CASES so the tests no
longer exclude temperature beam-capable requests with logprobs=2 or beam-search
requests using processed logprobs. Enable coverage for these paths, including
the mixed PROCESSED plus beam-search case, and remove the obsolete FIXME-based
filters.
🪄 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: 0d5b8126-cfc1-4753-b43f-e3c72ca91631

📥 Commits

Reviewing files that changed from the base of the PR and between 96b2675 and 6afef36.

📒 Files selected for processing (2)
  • tensorrt_llm/_torch/pyexecutor/sampler/sampler.py
  • tests/unittest/_torch/sampler/test_logits_logprobs.py

Comment thread tensorrt_llm/_torch/pyexecutor/sampler/sampler.py

@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

♻️ Duplicate comments (1)
tensorrt_llm/_torch/pyexecutor/sampler/sampler.py (1)

3126-3154: 🎯 Functional Correctness | 🔴 Critical | 🏗️ Heavy lift

Processed-logprobs beam mask is inverted and indexed in the wrong space.

Two problems in the self._use_beam_search branch:

  1. Polarity: gather_processed_logprobs_group_req_indices_list collects the py_beam_width == 1 requests — the ones that should contribute rows — and then clears them in the mask, leaving the beam-search requests selected.
  2. Index space: grp_idx is a position within need_processed_logprobs_req_indices_list (positions among the mask's True entries), not a position within the group-length mask group_gather_processed_logprobs. It only coincides when the selected requests happen to form a prefix of the group.

Consequently num_gather_processed_logprobs_req_indices (count of 1-beam requests) disagrees with the rows the mask selects, so mixed beam/non-beam groups can route the wrong logit rows into batch_processed_logprobs_cuda.

🐛 Suggested fix: build the mask from group positions and keep 1-beam requests
-                    if self._use_beam_search:
-                        gather_processed_logprobs_group_req_indices_list = [
-                            grp_idx
-                            for grp_idx, req_idx in enumerate(
-                                need_processed_logprobs_req_indices_list
-                            )
-                            if requests[req_idx].py_beam_width == 1
-                        ]
-
-                        # repurpose group_need_processed_logprobs
-                        group_gather_processed_logprobs = group_need_processed_logprobs
-                        del group_need_processed_logprobs
-                        gather_processed_logprobs_group_req_indices = torch.tensor(
-                            gather_processed_logprobs_group_req_indices_list,
-                            dtype=need_processed_logprobs_req_indices.dtype,
-                        )
-                        group_gather_processed_logprobs[
-                            gather_processed_logprobs_group_req_indices
-                        ] = False
-                        num_gather_processed_logprobs_req_indices = len(
-                            gather_processed_logprobs_group_req_indices_list
-                        )
+                    if self._use_beam_search:
+                        # Positions within the group (not within the selected sublist) whose
+                        # request is beam-search and must therefore be excluded.
+                        drop_group_positions = [
+                            grp_pos
+                            for grp_pos, req_idx in enumerate(group_req_indices.tolist())
+                            if requests[req_idx].py_beam_width > 1
+                        ]
+                        group_gather_processed_logprobs = group_need_processed_logprobs.clone()
+                        del group_need_processed_logprobs
+                        if drop_group_positions:
+                            group_gather_processed_logprobs[
+                                torch.tensor(drop_group_positions, dtype=torch.long)
+                            ] = False
+                        num_gather_processed_logprobs_req_indices = cast(
+                            int, group_gather_processed_logprobs.sum().item()
+                        )

Please also cover a group mixing beam-search and single-beam PROCESSED-logprob requests with permuted ordering.

🤖 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/_torch/pyexecutor/sampler/sampler.py` around lines 3126 - 3154,
Fix the self._use_beam_search branch so group_gather_processed_logprobs selects
only py_beam_width == 1 requests. Build the mask using each request’s position
in the full group mask, not grp_idx within
need_processed_logprobs_req_indices_list; preserve those group positions as true
and clear the beam-search positions. Keep
num_gather_processed_logprobs_req_indices equal to the number of retained
single-beam requests, and cover mixed PROCESSED-logprob requests in permuted
order.
🧹 Nitpick comments (2)
tests/unittest/_torch/sampler/test_logits_logprobs.py (1)

899-899: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Nit: class-name typo and missing return annotations. TestLogsprobsInBatchedSamplingTestLogprobsInBatchedSampling (it leaks into every generated test ID), and _validate_logprobs / test_logprobs / the inner validate_logprob_and_rank closure lack -> None.

As per coding guidelines: "Annotate every function, use None for non-returning functions".

Also applies to: 1285-1291, 1408-1417

🤖 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/_torch/sampler/test_logits_logprobs.py` at line 899, Rename
TestLogsprobsInBatchedSampling to TestLogprobsInBatchedSampling, and add -> None
annotations to _validate_logprobs, test_logprobs, and the inner
validate_logprob_and_rank closure, including the corresponding functions in the
referenced sections.

Source: Coding guidelines

tensorrt_llm/_torch/pyexecutor/sampler/ops/vanilla.py (1)

321-340: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Document the out contract. A short Google-style docstring on gather_log_softmax_with_output stating the expected shape (indices_cuda.size(0) x vocab), that out must not alias inputs_cuda, and that dtype conversion is implicit would help future callers.

🤖 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/_torch/pyexecutor/sampler/ops/vanilla.py` around lines 321 -
340, Add a concise Google-style docstring to
Fusions.gather_log_softmax_with_output documenting that out has shape
indices_cuda.size(0) × vocab, must not alias inputs_cuda, and receives implicit
dtype conversion. Leave the implementation 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.

Inline comments:
In `@tests/unittest/_torch/sampler/test_logits_logprobs.py`:
- Around line 1140-1149: Update the sparsity assertion in the sequence-slot
setup so it requires num_seq_slots to be at most half of allowed_slots.size,
preserving rng.choice’s replace=False validity while enforcing the intended
sparse assignment.

---

Duplicate comments:
In `@tensorrt_llm/_torch/pyexecutor/sampler/sampler.py`:
- Around line 3126-3154: Fix the self._use_beam_search branch so
group_gather_processed_logprobs selects only py_beam_width == 1 requests. Build
the mask using each request’s position in the full group mask, not grp_idx
within need_processed_logprobs_req_indices_list; preserve those group positions
as true and clear the beam-search positions. Keep
num_gather_processed_logprobs_req_indices equal to the number of retained
single-beam requests, and cover mixed PROCESSED-logprob requests in permuted
order.

---

Nitpick comments:
In `@tensorrt_llm/_torch/pyexecutor/sampler/ops/vanilla.py`:
- Around line 321-340: Add a concise Google-style docstring to
Fusions.gather_log_softmax_with_output documenting that out has shape
indices_cuda.size(0) × vocab, must not alias inputs_cuda, and receives implicit
dtype conversion. Leave the implementation unchanged.

In `@tests/unittest/_torch/sampler/test_logits_logprobs.py`:
- Line 899: Rename TestLogsprobsInBatchedSampling to
TestLogprobsInBatchedSampling, and add -> None annotations to
_validate_logprobs, test_logprobs, and the inner validate_logprob_and_rank
closure, including the corresponding functions in the referenced sections.
🪄 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: 28f6e047-e50e-40bd-8765-bbf91af50a7e

📥 Commits

Reviewing files that changed from the base of the PR and between 6afef36 and 83d4221.

📒 Files selected for processing (4)
  • tensorrt_llm/_torch/pyexecutor/sampler/ops/vanilla.py
  • tensorrt_llm/_torch/pyexecutor/sampler/sampler.py
  • tests/unittest/_torch/sampler/test_logits_logprobs.py
  • tests/unittest/_torch/sampler/test_torch_sampler.py
🚧 Files skipped from review as they are similar to previous changes (1)
  • tests/unittest/_torch/sampler/test_torch_sampler.py

Comment thread tests/unittest/_torch/sampler/test_logits_logprobs.py
Signed-off-by: ixlmar <206748156+ixlmar@users.noreply.github.com>
@ixlmar

ixlmar commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator Author

/bot run

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #62841 [ run ] triggered by Bot. Commit: 5a0fcb9 Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #62841 [ run ] completed with state FAILURE. Commit: 5a0fcb9
/LLM/main/L0_MergeRequest_PR pipeline #50965 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

@ixlmar

ixlmar commented Jul 31, 2026

Copy link
Copy Markdown
Collaborator Author

/bot run --disable-fail-fast

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #62967 [ run ] triggered by Bot. Commit: 5a0fcb9 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 some little comments.

Comment thread tests/unittest/_torch/sampler/test_logits_logprobs.py Outdated
Comment thread tensorrt_llm/_torch/pyexecutor/sampler/sampler.py
Comment thread tests/unittest/_torch/sampler/test_logits_logprobs.py
Comment thread tensorrt_llm/_torch/pyexecutor/sampler/sampler.py Outdated
Signed-off-by: ixlmar <206748156+ixlmar@users.noreply.github.com>
@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #62967 [ run ] completed with state SUCCESS. Commit: 5a0fcb9
/LLM/main/L0_MergeRequest_PR pipeline #51080 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

Signed-off-by: ixlmar <206748156+ixlmar@users.noreply.github.com>
@ixlmar

ixlmar commented Jul 31, 2026

Copy link
Copy Markdown
Collaborator Author

/bot run --disable-fail-fast

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #62998 [ run ] triggered by Bot. Commit: 3c849ff Link to invocation

Signed-off-by: ixlmar <206748156+ixlmar@users.noreply.github.com>
@ixlmar

ixlmar commented Jul 31, 2026

Copy link
Copy Markdown
Collaborator Author

/bot run --disable-fail-fast

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #63025 [ run ] triggered by Bot. Commit: 7912282 Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #62998 [ run ] completed with state ABORTED. Commit: 3c849ff

Link to invocation

Signed-off-by: ixlmar <206748156+ixlmar@users.noreply.github.com>
@ixlmar

ixlmar commented Jul 31, 2026

Copy link
Copy Markdown
Collaborator Author

/bot run --disable-fail-fast

@ixlmar
ixlmar enabled auto-merge (squash) July 31, 2026 13:24
@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #63039 [ run ] triggered by Bot. Commit: 288a96e Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #63025 [ run ] completed with state ABORTED. Commit: 7912282

Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #63039 [ run ] completed with state ABORTED. Commit: 288a96e

Link to invocation

@stnie stnie 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.

Seems fine. Only a few small questions.
Do you have any measurements, how much this improves performance?

Comment thread tensorrt_llm/_torch/pyexecutor/sampler/sampler.py
Comment thread tests/unittest/_torch/sampler/test_logits_logprobs.py
Comment thread tests/unittest/_torch/sampler/test_logits_logprobs.py
@dpitman-nvda

Copy link
Copy Markdown
Collaborator

/bot help

@github-actions

Copy link
Copy Markdown

GitHub Bot Help

/bot [-h] ['run', 'kill', 'skip', 'reuse-pipeline'] ...

Provide a user friendly way for developers to interact with a Jenkins server.

Run /bot [-h|--help] to print this help message.

See details below for each supported subcommand.

Details

run [--reuse-test (optional)pipeline-id --disable-fail-fast --skip-test --stage-list "A10-PyTorch-1, xxx" --gpu-type "A30, H100_PCIe" --test-backend "pytorch, cpp" --add-multi-gpu-test --only-multi-gpu-test --disable-multi-gpu-test --post-merge --extra-stage "H100_PCIe-TensorRT-Post-Merge-1, xxx" --detailed-log --debug(experimental) --high-priority]

Launch build/test pipelines. All previously running jobs will be killed.

--reuse-test (optional)pipeline-id (OPTIONAL) : Allow the new pipeline to reuse build artifacts and skip successful test stages from a specified pipeline or the last pipeline if no pipeline-id is indicated. If the Git commit ID has changed, this option will be always ignored. The DEFAULT behavior of the bot is to reuse build artifacts and successful test results from the last pipeline.

--disable-reuse-test (OPTIONAL) : Explicitly prevent the pipeline from reusing build artifacts and skipping successful test stages from a previous pipeline. Ensure that all builds and tests are run regardless of previous successes.

--disable-fail-fast (OPTIONAL) : Disable fail fast on build/tests/infra failures.

--skip-test (OPTIONAL) : Skip all test stages, but still run build stages, package stages and sanity check stages. Note: Does NOT update GitHub check status.

--stage-list "A10-PyTorch-1, xxx" (OPTIONAL) : Only run the specified test stages. Supports wildcard * for pattern matching (e.g., "*PerfSanity*" matches all stages containing PerfSanity). Examples: "A10-PyTorch-1, xxx", "PerfSanity". The patterns "*", "*Post-Merge*", and "*PerfSanity*", including equivalent escaped or repeated-star forms and their use in comma-separated lists, require the ci: post-merge approved PR label. Note: Does NOT update GitHub check status.

--gpu-type "A30, H100_PCIe" (OPTIONAL) : Only run the test stages on the specified GPU types. Examples: "A30, H100_PCIe". Note: Does NOT update GitHub check status.

--test-backend "pytorch, cpp" (OPTIONAL) : Skip test stages which don't match the specified backends. Only support [pytorch, cpp, tensorrt, triton]. Examples: "pytorch, cpp" (does not run test stages with tensorrt or triton backend). Note: Does NOT update GitHub pipeline status.

--only-multi-gpu-test (OPTIONAL) : Only run the multi-GPU tests. Note: Does NOT update GitHub check status.

--disable-multi-gpu-test (OPTIONAL) : Disable the multi-GPU tests. Note: Does NOT update GitHub check status.

--add-multi-gpu-test (OPTIONAL) : Force run the multi-GPU tests in addition to running L0 pre-merge pipeline.

--post-merge (OPTIONAL) : Run the L0 post-merge pipeline instead of the ordinary L0 pre-merge pipeline. Requires the ci: post-merge approved PR label applied by an active member of NVIDIA/trt-llm-ci-approvers. The approval label remains in place when new commits are pushed.

--extra-stage "H100_PCIe-TensorRT-Post-Merge-1, xxx" (OPTIONAL) : Run the ordinary L0 pre-merge pipeline and specified test stages. Supports wildcard * for pattern matching. Examples: --extra-stage "H100_PCIe-TensorRT-Post-Merge-1, xxx", --extra-stage "Post-Merge". The patterns "*", "*Post-Merge*", and "*PerfSanity*", including equivalent escaped or repeated-star forms and their use in comma-separated lists, require the ci: post-merge approved PR label.

--detailed-log (OPTIONAL) : Enable flushing out all logs to the Jenkins console. This will significantly increase the log volume and may slow down the job.

--debug (OPTIONAL) : Experimental feature. Enable access to the CI container for debugging purpose. Note: Specify exactly one stage in the stage-list parameter to access the appropriate container environment. Note: Does NOT update GitHub check status.

--high-priority (OPTIONAL) : Run the pipeline with high priority. This option is restricted to authorized users only and will route the job to a high-priority queue.

kill

kill

Kill all running builds associated with pull request.

skip

skip --comment COMMENT

Skip testing for latest commit on pull request. --comment "Reason for skipping build/test" is required. IMPORTANT NOTE: This is dangerous since lack of user care and validation can cause top of tree to break.

reuse-pipeline

reuse-pipeline

Reuse a previous pipeline to validate current commit. This action will also kill all currently running builds associated with the pull request. IMPORTANT NOTE: This is dangerous since lack of user care and validation can cause top of tree to break.

@ixlmar

ixlmar commented Jul 31, 2026

Copy link
Copy Markdown
Collaborator Author

/bot run --disable-fail-fast

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #63057 [ run ] triggered by Bot. Commit: 288a96e Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #63057 [ run ] completed with state SUCCESS. Commit: 288a96e
/LLM/main/L0_MergeRequest_PR pipeline #51157 completed with status: 'SUCCESS'
Pipeline passed with automatic retried tests. Check the rerun report for details.

CI Report

Link to invocation

@ixlmar
ixlmar merged commit 7443b7f into NVIDIA:main Jul 31, 2026
8 checks passed
@ixlmar
ixlmar deleted the perf/logprobs-fewer-copies branch July 31, 2026 19:33
yuanjingx87 pushed a commit that referenced this pull request Aug 1, 2026
…probs (#16958)

Signed-off-by: ixlmar <206748156+ixlmar@users.noreply.github.com>
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.

5 participants