Skip to content

Conversation

@hchings
Copy link
Collaborator

@hchings hchings commented Nov 22, 2025

Summary by CodeRabbit

  • New Features

    • Enhanced logprobs support with sampling-aware handling for improved per-token probability tracking.
    • Logprobs now properly propagate through the request processing pipeline when enabled.
  • Documentation

    • Added example script demonstrating text generation with logprobs extraction, including per-token probabilities and sampled token information.

✏️ Tip: You can customize this high-level summary in your review settings.

Description

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)

  • 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

/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)]

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. Examples: "A10-PyTorch-1, xxx". 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.

--extra-stage "H100_PCIe-TensorRT-Post-Merge-1, xxx" (OPTIONAL) : Run the ordinary L0 pre-merge pipeline and specified test stages. Examples: --extra-stage "H100_PCIe-TensorRT-Post-Merge-1, xxx".

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

For guidance on mapping tests to stage names, see docs/source/reference/ci-overview.md
and the scripts/test_to_stage_mapping.py helper.

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.

@hchings hchings self-assigned this Nov 22, 2025
@hchings hchings requested a review from shuyixiong November 22, 2025 10:08
@hchings hchings force-pushed the sampled_logprob branch 2 times, most recently from ede907b to 5bf8c50 Compare November 25, 2025 00:24
@hchings hchings changed the title [None][fix] WAR for sampled logprob [None][fix] Change PyT to always include sampled logprob Nov 26, 2025
@hchings hchings marked this pull request as ready for review November 26, 2025 08:18
@hchings hchings requested review from a team as code owners November 26, 2025 08:18
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 26, 2025

📝 Walkthrough

Walkthrough

This PR adds a new example script demonstrating LLM text generation with logprob extraction and modifies the sampler to propagate sampled logprobs from sampling results back to requests for improved logprob handling with a sampling-aware fallback mechanism.

Changes

Cohort / File(s) Change Summary
Example Script
examples/llm-api/llm_inference_logprob.py
New file: Python script demonstrating LLM text generation with extraction of logprobs, per-token logprob ranks, and sampled token information using local TinyLlama or TensorRT-based models.
Sampler Logprobs Handling
tensorrt_llm/_torch/pyexecutor/sampler.py
Modified handle_logprobs to use sampling-aware path when py_sampled_logprobs are available, falling back to original top-K logprob handling otherwise. Added logic in _process_requests to collect and propagate sampled logprobs from batched sampling results back to per-request LlmRequest instances via per-request offsets.

Sequence Diagram

sequenceDiagram
    participant LLM as Text Generation
    participant Sampler as Sampler
    participant ReqMgmt as Request Management
    participant LogProb as Logprob Handler

    LLM->>Sampler: Generate tokens with sampling
    Sampler->>Sampler: Collect sampled logprobs<br/>(py_sampled_logprobs)
    Sampler->>ReqMgmt: _process_requests()
    ReqMgmt->>ReqMgmt: Map batched results to<br/>per-request offsets
    ReqMgmt->>ReqMgmt: Attach sampled_logprobs<br/>to LlmRequest instances
    ReqMgmt->>LogProb: handle_logprobs()
    rect rgb(200, 230, 250)
    Note over LogProb: Sampling-aware path
    LogProb->>LogProb: Use py_sampled_logprobs<br/>if available
    end
    rect rgb(240, 220, 200)
    Note over LogProb: Fallback path
    LogProb->>LogProb: Use py_topk_logprobs<br/>if sampled unavailable
    end
    LogProb->>LogProb: Build per-token<br/>logprob dicts
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~30 minutes

Areas requiring extra attention:

  • Correctness of per-request offset mapping logic in _process_requests when aligning batched sampling results to individual requests
  • Conditional branching logic in handle_logprobs and fallback correctness when py_sampled_logprobs are unavailable
  • Data structure compatibility between sampling result arrays and LlmRequest instance attributes

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Description check ⚠️ Warning The PR description is largely empty, containing only the template boilerplate without filling in required sections like Description and Test Coverage. Complete the Description section explaining the issue and solution, and add Test Coverage section detailing relevant tests that validate the changes.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the main change: modifying PyTorch behavior to always include sampled logprob.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Tip

📝 Customizable high-level summaries are now available in beta!

You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.

  • Provide your own instructions using the high_level_summary_instructions setting.
  • Format the summary however you like (bullet lists, tables, multi-section layouts, contributor stats, etc.).
  • Use high_level_summary_in_walkthrough to move the summary from the description to the walkthrough section.

Example instruction:

"Divide the high-level summary into five sections:

  1. 📝 Description — Summarize the main change in 50–60 words, explaining what was done.
  2. 📓 References — List relevant issues, discussions, documentation, or related PRs.
  3. 📦 Dependencies & Requirements — Mention any new/updated dependencies, environment variable changes, or configuration updates.
  4. 📊 Contributor Summary — Include a Markdown table showing contributions:
    | Contributor | Lines Added | Lines Removed | Files Changed |
  5. ✔️ Additional Notes — Add any extra reviewer context.
    Keep each section concise (under 200 words) and use bullet or numbered lists for clarity."

Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

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

765-773: Unused loop variable and missing strict= parameter.

The step variable from enumerate is unused in the loop body. Replace with _ for clarity. The static analysis hint is valid here.

                 token_log_probs = []
-                for step, (topk_token, topk_logprob) in enumerate(zip(topk_log_probs_indices, topk_log_probs_vals)):
+                for _, (topk_token, topk_logprob) in enumerate(zip(topk_log_probs_indices, topk_log_probs_vals, strict=True)):
                     step_dict = {
                         token: Logprob(logprob=logprob, rank=rank + 1)
                         for rank, (token, logprob) in enumerate(
-                            zip(topk_token.tolist(), topk_logprob.tolist())
+                            zip(topk_token.tolist(), topk_logprob.tolist(), strict=True)
                         )
                     }
                     token_log_probs.append(step_dict)

1842-1871: Performance: O(n²) complexity from repeated .index() lookups and cumsum calculations.

The loop uses logprobs_req_indices.index(req_id) (O(n)) inside an O(n) loop, resulting in O(n²) complexity. Additionally, sum(req_num_steps[...].tolist()) recomputes partial sums on each iteration.

Consider precomputing the mappings before the loop:

         if return_log_probs:
             sampled_tokens_cuda = batched_sampling_result.batch_next_tokens_cuda_int
             logprobs_req_set = set(logprobs_req_indices)
             sampled_logprobs_list = []
+            
+            # Precompute req_id -> (logprobs_idx, start_offset) mapping
+            req_id_to_offset = {}
+            cumulative_offset = 0
+            for logprobs_idx, req_id in enumerate(logprobs_req_indices):
+                req_id_to_offset[req_id] = (logprobs_idx, cumulative_offset)
+                cumulative_offset += req_num_steps[req_id].item()
 
             for req_id in range(len(requests)):
                 if req_id in logprobs_req_set:
-                    logprobs_idx = logprobs_req_indices.index(req_id)
-
-                    if logprobs_idx == 0:
-                        start_offset = 0
-                    else:
-                        start_offset = sum(req_num_steps[logprobs_req_indices[:logprobs_idx]].tolist())
+                    logprobs_idx, start_offset = req_id_to_offset[req_id]
 
                     num_steps_this_req = req_num_steps[req_id].item()
                     end_offset = start_offset + num_steps_this_req
examples/llm-api/llm_inference_logprob.py (2)

47-51: Minor style issues: f-string without placeholders and missing strict=.

The f-string on line 48 has no placeholders, and the zip() on line 50 could benefit from strict=True to catch length mismatches.

         if output.outputs[0].logprobs:
-            print(f"\nLogprobs for each generated token:")
+            print("\nLogprobs for each generated token:")
             for i, (token_id, token_logprobs) in enumerate(
-                zip(output.outputs[0].token_ids, output.outputs[0].logprobs)
+                zip(output.outputs[0].token_ids, output.outputs[0].logprobs, strict=True)
             ):

54-56: TODO comment and assertion in example code.

Consider moving this assertion to a proper test file as noted in the TODO. Assertions in example scripts may confuse users if they fail unexpectedly.

Do you want me to help create a proper unit test for this validation?

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1a93583 and ec59aaa.

📒 Files selected for processing (2)
  • examples/llm-api/llm_inference_logprob.py (1 hunks)
  • tensorrt_llm/_torch/pyexecutor/sampler.py (2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py

📄 CodeRabbit inference engine (CODING_GUIDELINES.md)

**/*.py: The code developed for TensorRT-LLM should conform to Python 3.8+
Indent Python code with 4 spaces; do not use tabs
Always maintain the namespace when importing in Python, even if only one class or function from a module is used (e.g., use from package.subpackage import foo and then foo.SomeClass() instead of from package.subpackage.foo import SomeClass)
Python filenames should use snake_case (e.g., some_file.py)
Python class names should use PascalCase (e.g., class SomeClass)
Python function and method names should use snake_case (e.g., def my_awesome_function():)
Python local variable names should use snake_case, with prefix k for variable names that start with a number (e.g., k_99th_percentile = ...)
Python global variables should use upper snake_case with prefix G (e.g., G_MY_GLOBAL = ...)
Python constants should use upper snake_case (e.g., MY_CONSTANT = ...)
Avoid shadowing variables declared in an outer scope in Python
Initialize all externally visible members of a Python class in the constructor
For Python interfaces that may be used outside a file, prefer docstrings over comments
Python comments should be reserved for code within a function, or interfaces that are local to a file
Use Google style docstrings for Python classes and functions, which can be parsed by Sphinx
Python attributes and variables can be documented inline with type and description (e.g., self.x = 5 followed by """<type>: Description of 'x'""" )
Avoid using reflection in Python when functionality can be easily achieved without reflection
When using try-except blocks in Python, limit the except clause to the smallest set of specific errors possible instead of catching all exceptions
When using try-except blocks in Python to handle multiple possible variable types (duck-typing), keep the body of the try as small as possible and use the else block to implement the logic

Files:

  • examples/llm-api/llm_inference_logprob.py
  • tensorrt_llm/_torch/pyexecutor/sampler.py
**/*.{cpp,h,cu,py}

📄 CodeRabbit inference engine (CODING_GUIDELINES.md)

All TensorRT-LLM Open Source Software code files should contain an NVIDIA copyright header that includes the current year at the top

Files:

  • examples/llm-api/llm_inference_logprob.py
  • tensorrt_llm/_torch/pyexecutor/sampler.py
🧠 Learnings (8)
📓 Common learnings
Learnt from: venkywonka
Repo: NVIDIA/TensorRT-LLM PR: 6029
File: .github/pull_request_template.md:45-53
Timestamp: 2025-08-27T17:50:13.264Z
Learning: For PR templates in TensorRT-LLM, avoid suggesting changes that would increase developer overhead, such as converting plain bullets to mandatory checkboxes. The team prefers guidance-style bullets that don't require explicit interaction to reduce friction in the PR creation process.
📚 Learning: 2025-08-01T15:14:45.673Z
Learnt from: yibinl-nvidia
Repo: NVIDIA/TensorRT-LLM PR: 6506
File: examples/models/core/mixtral/requirements.txt:3-3
Timestamp: 2025-08-01T15:14:45.673Z
Learning: In TensorRT-LLM, examples directory can have different dependency versions than the root requirements.txt file. Version conflicts between root and examples dependencies are acceptable because examples are designed to be standalone and self-contained.

Applied to files:

  • examples/llm-api/llm_inference_logprob.py
📚 Learning: 2025-07-28T17:06:08.621Z
Learnt from: moraxu
Repo: NVIDIA/TensorRT-LLM PR: 6303
File: tests/integration/test_lists/qa/examples_test_list.txt:494-494
Timestamp: 2025-07-28T17:06:08.621Z
Learning: In TensorRT-LLM testing, it's common to have both CLI flow tests (test_cli_flow.py) and PyTorch API tests (test_llm_api_pytorch.py) for the same model. These serve different purposes: CLI flow tests validate the traditional command-line workflow, while PyTorch API tests validate the newer LLM API backend. Both are legitimate and should coexist.

Applied to files:

  • examples/llm-api/llm_inference_logprob.py
📚 Learning: 2025-09-09T09:40:45.658Z
Learnt from: fredricz-20070104
Repo: NVIDIA/TensorRT-LLM PR: 7645
File: tests/integration/test_lists/qa/llm_function_core.txt:648-648
Timestamp: 2025-09-09T09:40:45.658Z
Learning: In TensorRT-LLM test lists, it's common and intentional for the same test to appear in multiple test list files when they serve different purposes (e.g., llm_function_core.txt for comprehensive core functionality testing and llm_function_core_sanity.txt for quick sanity checks). This duplication allows tests to be run in different testing contexts.

Applied to files:

  • examples/llm-api/llm_inference_logprob.py
📚 Learning: 2025-08-06T13:58:07.506Z
Learnt from: galagam
Repo: NVIDIA/TensorRT-LLM PR: 6487
File: tests/unittest/_torch/auto_deploy/unit/singlegpu/test_ad_trtllm_bench.py:1-12
Timestamp: 2025-08-06T13:58:07.506Z
Learning: In TensorRT-LLM, test files (files under tests/ directories) do not require NVIDIA copyright headers, unlike production source code files. Test files typically start directly with imports, docstrings, or code.

Applied to files:

  • examples/llm-api/llm_inference_logprob.py
📚 Learning: 2025-08-29T14:07:45.863Z
Learnt from: EmmaQiaoCh
Repo: NVIDIA/TensorRT-LLM PR: 7370
File: tests/unittest/trt/model_api/test_model_quantization.py:24-27
Timestamp: 2025-08-29T14:07:45.863Z
Learning: In TensorRT-LLM's CI infrastructure, pytest skip markers (pytest.mark.skip) are properly honored even when test files have __main__ blocks that call test functions directly. The testing system correctly skips tests without requiring modifications to the __main__ block execution pattern.

Applied to files:

  • examples/llm-api/llm_inference_logprob.py
📚 Learning: 2025-08-28T10:25:22.370Z
Learnt from: ixlmar
Repo: NVIDIA/TensorRT-LLM PR: 7294
File: tensorrt_llm/_torch/pyexecutor/sampler.py:887-891
Timestamp: 2025-08-28T10:25:22.370Z
Learning: In tensorrt_llm/_torch/pyexecutor/sampler.py, the draft_probs and target_probs tensors have shapes [1, steps] not [steps, vocab_size] as might be expected, making the .squeeze(0) operations appropriate for removing the batch dimension of size 1.

Applied to files:

  • tensorrt_llm/_torch/pyexecutor/sampler.py
📚 Learning: 2025-08-28T10:22:02.288Z
Learnt from: ixlmar
Repo: NVIDIA/TensorRT-LLM PR: 7294
File: tensorrt_llm/_torch/pyexecutor/sampler.py:1191-1197
Timestamp: 2025-08-28T10:22:02.288Z
Learning: In tensorrt_llm/_torch/pyexecutor/sampler.py, the object identity comparison `softmax_req_indices is not group_req_indices_cuda` on line ~1191 is intentional and used as an optimization to determine whether to reuse an existing indexer or create a new one, based on which code path was taken during tensor assignment.

Applied to files:

  • tensorrt_llm/_torch/pyexecutor/sampler.py
🧬 Code graph analysis (2)
examples/llm-api/llm_inference_logprob.py (3)
tests/unittest/_torch/modeling/test_modeling_out_of_tree.py (2)
  • prompts (38-44)
  • sampling_params (58-59)
tensorrt_llm/llmapi/llm.py (1)
  • prompt (86-87)
tensorrt_llm/_torch/pyexecutor/llm_request.py (1)
  • generation_logits (329-340)
tensorrt_llm/_torch/pyexecutor/sampler.py (2)
tests/unittest/_torch/auto_deploy/unit/singlegpu/shim/test_engine.py (1)
  • get_tokens (164-165)
tensorrt_llm/executor/result.py (1)
  • Logprob (47-50)
🪛 GitHub Actions: Release Checks
examples/llm-api/llm_inference_logprob.py

[error] 42-42: E501 Line too long (128 > 120).

🪛 Ruff (0.14.5)
examples/llm-api/llm_inference_logprob.py

48-48: f-string without any placeholders

Remove extraneous f prefix

(F541)


50-50: zip() without an explicit strict= parameter

Add explicit value for parameter strict=

(B905)

tensorrt_llm/_torch/pyexecutor/sampler.py

766-766: Loop control variable step not used within loop body

(B007)


766-766: zip() without an explicit strict= parameter

Add explicit value for parameter strict=

(B905)


770-770: zip() without an explicit strict= parameter

Add explicit value for parameter strict=

(B905)

🔇 Additional comments (1)
tensorrt_llm/_torch/pyexecutor/sampler.py (1)

750-759: Sampled logprob branch looks correct, but rank=1 may be semantically misleading.

The rank=1 hardcoded value implies this is the top-1 token, but for non-greedy sampling (temperature, top-p, top-k), the sampled token may not be the most probable. Consider renaming or documenting this as "sampled token marker" rather than true rank.

Also, verify that py_sampled_logprobs length always matches or exceeds count to avoid potential index errors.

Comment on lines 4 to 11
from tensorrt_llm._tensorrt_engine import LLM as TrtLLM


def main():
llm = LLM(
model="/scratch/llm-models/llama-models-v2/TinyLlama-1.1B-Chat-v1.0",
orchestrator_type="ray"
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

Unused import and hardcoded model path.

TrtLLM is imported but only used in commented-out code. Consider removing the unused import. The hardcoded path /scratch/llm-models/... is not portable and will fail for other users.

-from tensorrt_llm._tensorrt_engine import LLM as TrtLLM
-
 
 def main():
+    # NOTE: Update this path to your local model location or use a HuggingFace model ID
     llm = LLM(
-        model="/scratch/llm-models/llama-models-v2/TinyLlama-1.1B-Chat-v1.0",
+        model="TinyLlama/TinyLlama-1.1B-Chat-v1.0",
         orchestrator_type="ray"
     )
-
-    # llm = TrtLLM(
-    #     model="TinyLlama/TinyLlama-1.1B-Chat-v1.0",
-    # )

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In examples/llm-api/llm_inference_logprob.py around lines 4 to 11, remove the
unused import of TrtLLM (or alternatively use it if you intended to instantiate
the TensorRT LLM) and replace the hardcoded model path with a portable approach:
accept the model path via an environment variable or a CLI argument (with a
sensible default), validate the path before use, and instantiate the correct
class (LLM or TrtLLM) consistently so there are no unused imports or
non-portable hardcoded paths.

Comment on lines 42 to 45
# sanity check on sampled logits
num_logits = logits.shape[0]
sampled_logits = [logits[i, token_id].item() for i, token_id in enumerate(output.outputs[0].token_ids[:num_logits])]
print(f"Logits of sampled tokens: {sampled_logits}")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Line too long (pipeline failure).

Line 44 exceeds 120 characters. Break it for readability and to fix the CI failure.

             # sanity check on sampled logits
             num_logits = logits.shape[0]
-            sampled_logits = [logits[i, token_id].item() for i, token_id in enumerate(output.outputs[0].token_ids[:num_logits])]
+            token_ids = output.outputs[0].token_ids[:num_logits]
+            sampled_logits = [
+                logits[i, token_id].item()
+                for i, token_id in enumerate(token_ids)
+            ]
             print(f"Logits of sampled tokens: {sampled_logits}")
📝 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
# sanity check on sampled logits
num_logits = logits.shape[0]
sampled_logits = [logits[i, token_id].item() for i, token_id in enumerate(output.outputs[0].token_ids[:num_logits])]
print(f"Logits of sampled tokens: {sampled_logits}")
# sanity check on sampled logits
num_logits = logits.shape[0]
token_ids = output.outputs[0].token_ids[:num_logits]
sampled_logits = [
logits[i, token_id].item()
for i, token_id in enumerate(token_ids)
]
print(f"Logits of sampled tokens: {sampled_logits}")
🧰 Tools
🪛 GitHub Actions: Release Checks

[error] 42-42: E501 Line too long (128 > 120).

🤖 Prompt for AI Agents
In examples/llm-api/llm_inference_logprob.py around lines 42 to 45, the list
comprehension that builds sampled_logits produces a line longer than 120
characters; split the expression across multiple lines (for example assign the
token_ids slice to a local variable, iterate in a short for loop or use a
multiline list comprehension) so each source line is <=120 chars and behavior
remains identical (same order and values).

@hchings hchings requested review from a team as code owners November 27, 2025 05:42
@hchings hchings requested a review from mlefeb01 November 27, 2025 05:42
@hchings hchings force-pushed the sampled_logprob branch 2 times, most recently from 7ebff28 to cac6a23 Compare December 1, 2025 07:20
@LinPoly
Copy link
Collaborator

LinPoly commented Dec 1, 2025

@hchings Hi Erin, just want to check the logprobs status, seems like we've supported top-k logprobs in TorchSampler and this PR is for ensuring that we have the logprob of sampled token in the list, right?

@hchings
Copy link
Collaborator Author

hchings commented Dec 2, 2025

@hchings Hi Erin, just want to check the logprobs status, seems like we've supported top-k logprobs in TorchSampler and this PR is for ensuring that we have the logprob of sampled token in the list, right?

Yes, the behavior of this MR will be the below for both TRT and PyTorch backends:

  • When logprob=0, it returns the sampled token.
  • When logprob>=1 (TopK), if the sampled token is not already in TopK, it'll be attached at the end of the logprob results. Hence it returns K and potentially + 1 logprobs in total.

However, I saw Stefan added beam-search support in TorchSampler in his MR. My changes doesn't cover that path at the moment.

@hchings hchings requested review from dcaox and stnie December 2, 2025 00:38
@hchings hchings requested review from dcaox and stnie December 2, 2025 19:10
# Sampled token is in top-K, use its rank
rank = topk_tokens_list.index(sampled_token) + 1
else:
# TODO: fix rank
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rank calculation could be done, by calculating the rank in_process_requests where you have access to all logprobs. You can then pass it forward as request.py_sampled_rank similar to how you pass request.py_sampled_logprobs

current_offset:next_offset, :k_for_req
]

# context requests do not have multiple input beams, but they need multiple output beams
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the beam search part is obsolete and may be removed

start_offset = 0
else:
start_offset = sum(
req_num_steps[logprobs_req_indices[:logprobs_idx]].tolist()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe using req_num_steps[logprobs_req_indices[:logprobs_idx]].sum().item() might be more efficient. This may also allow you to remove the if-else condition

sampled_logprobs_cpu = sampled_logprobs_cuda.to(device="cpu", non_blocking=True)
sampled_logprobs_list.append((req_id, sampled_logprobs_cpu))

for req_id, sampled_logprobs in sampled_logprobs_list:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might integrate the second loop into the first one and drop the sampled_logprobs_list by directly assigning requests[req_id].py_sampled_logprobs = sampled_logprobs_cpu

):
step_dict = {}
topk_tokens_list = topk_tokens.tolist()
topk_logprobs_list = topk_logprobs.tolist()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you could merge the case request.py_num_logprobs == 0 with this one, by iterating only forrequest.py_num_logprobs steps. Essentially, this loop would then be empty

):
req = requests[req_id]
next_offset = current_offset + steps
# Store at least k=1 for all requests (including logprobs=0) to compute ranks
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you elaborate why it is necessary to use k>=1 for rank computation? I would have expected that it should work with 0 as well.

fix step

remove dependency on return_genereation_logits

align API across backends, add tests

test

fix group sampling strategy
@hchings
Copy link
Collaborator Author

hchings commented Dec 5, 2025

Closing in favor of #9675 which already taken care of the performance issue.
@stnie please help cherry-pick the tests and TRT side of changes from here. Thanks.

@hchings hchings closed this Dec 5, 2025
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