Skip to content

Conversation

Linda-Stadter
Copy link
Collaborator

@Linda-Stadter Linda-Stadter commented Jul 24, 2025

Fix for integration tests when using nanobind

Description

This change fixes the remaining integration tests that were failing when using nanobind as the binding library. This draft PR shows the tests passing in the last CI run.

Problems were:

  • Exceptions (PeftTaskNotCachedException and LoraCacheFullException) were not correctly thrown
  • init function of OutputConfig did not accept none types correctly -> correct behavior is to have optional boolean arguments that are set to a default false value when undefined
  • _return_log_probs was set to an int value instead of boolean

Test Coverage

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.

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.

Summary by CodeRabbit

  • New Features

    • Improved handling of backend selection in configuration by supporting case-insensitive string conversion to enum values.
    • Enhanced Python API to allow optional boolean parameters for output configuration, enabling more flexible request construction.
  • Bug Fixes

    • Ensured backend configuration strings are properly converted to the expected enum type in Python bindings.
    • Guaranteed that log probability return flags are always interpreted as boolean values.
  • Refactor

    • Centralized and streamlined exception handling in Python bindings for more robust error reporting.

@Linda-Stadter Linda-Stadter requested a review from a team as a code owner July 24, 2025 08:59
Copy link
Contributor

coderabbitai bot commented Jul 24, 2025

📝 Walkthrough

Walkthrough

The changes adjust exception handling in C++ Python bindings, update how optional booleans are handled in constructor bindings, modify enum conversion from strings in pybind, and ensure correct type conversion for parameters in Python API layers. These updates affect both the C++ nanobind/pybind bindings and their corresponding Python interfaces.

Changes

Files/Paths Change Summary
cpp/tensorrt_llm/nanobind/batch_manager/bindings.cpp Stores registered exception objects in static variables and updates exception translation to use them.
cpp/tensorrt_llm/nanobind/executor/request.cpp Changes OutputConfig constructor binding to accept optional booleans, using custom __init__ lambda.
cpp/tensorrt_llm/pybind/executor/executorConfig.cpp Replaces enum constructor binding with a static from_string method; removes implicit string conversion.
tensorrt_llm/llmapi/llm_args.py Imports enum binding and updates backend conversion in _to_pybind to use the new from_string method.
tensorrt_llm/serve/openai_protocol.py Converts logprobs to boolean explicitly in to_sampling_params methods.

Sequence Diagram(s)

sequenceDiagram
    participant Python
    participant PyBind
    participant Cpp

    Python->>PyBind: Call CacheTransceiverConfig._to_pybind()
    PyBind->>PyBind: _CacheTransceiverBackendType.from_string(self.backend)
    PyBind->>Cpp: Pass enum value to C++ config constructor
Loading
sequenceDiagram
    participant Python
    participant Nanobind
    participant Cpp

    Python->>Nanobind: Instantiate OutputConfig with optional booleans
    Nanobind->>Nanobind: __init__ lambda: value_or(false) for each param
    Nanobind->>Cpp: Call OutputConfig C++ constructor with resolved booleans
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Suggested reviewers

  • DomBrown
  • litaotju

Note

⚡️ Unit Test Generation is now available in beta!

Learn more here, or try it out under "Finishing Touches" below.


📜 Recent review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a63a1ac and b4338e8.

📒 Files selected for processing (5)
  • cpp/tensorrt_llm/nanobind/batch_manager/bindings.cpp (2 hunks)
  • cpp/tensorrt_llm/nanobind/executor/request.cpp (1 hunks)
  • cpp/tensorrt_llm/pybind/executor/executorConfig.cpp (2 hunks)
  • tensorrt_llm/llmapi/llm_args.py (2 hunks)
  • tensorrt_llm/serve/openai_protocol.py (2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{cpp,h,hpp,cc,cxx}

📄 CodeRabbit Inference Engine (CODING_GUIDELINES.md)

**/*.{cpp,h,hpp,cc,cxx}: Closing braces of namespaces should have a comment saying the namespace it closes (e.g., } // namespace foo)
Prefer const or constexpr variables over #defines whenever possible, as the latter are not visible to the compiler.
A variable that is not modified after its initialization should be declared as const.
Except 0 (only used in comparison for checking signness/existence/emptiness) and nullptr, true, false, all other literals should only be used for variable initialization.
Use the Allman indentation style for braces in C++ code.
Put the semicolon for an empty for or while loop in a new line.
The statement forming the body of a switch, while, do .. while or for statement shall be a compound statement (use brace-delimited statements).
If and else should always be followed by brace-delimited statements, even if empty or a single statement.
C++ filenames should use camel case with first letter lowercase (e.g., thisIsAFilename.cpp) and must be case-insensitive unique within a compilation target.
All types (including class names) should use camel case with uppercase first letter (e.g., FooBarClass).
Local variables, methods, and namespaces should use camel case with first letter lowercase (e.g., localFooBar).
Non-magic-number global variables that are non-static and not defined in anonymous namespace should use camel case prefixed by a lower case 'g' (e.g., gDontUseGlobalFoos).
Non-magic-number global variables that are static or defined in an anonymous namespace should use camel case prefixed by a lower case 's' (e.g., sMutableStaticGlobal).
Locally visible static variables should use camel case with lowercase prefix 's' as the first letter (e.g., static std::once_flag sFlag;).
Class member variables should use camel case prefixed with an 'm' (e.g., mNbFooValues). Public member variables do not require the 'm' prefix but it is encouraged for clarity.
Enumerations, global constants, static constants at class-scope, and function-scope magic-n...

Files:

  • cpp/tensorrt_llm/nanobind/batch_manager/bindings.cpp
  • cpp/tensorrt_llm/nanobind/executor/request.cpp
  • cpp/tensorrt_llm/pybind/executor/executorConfig.cpp
**/*.{cpp,h,hpp,cc,cxx,cu,py}

📄 CodeRabbit Inference Engine (CODING_GUIDELINES.md)

All TensorRT-LLM Open Source Software code should contain an NVIDIA copyright header that includes the current year. This includes .cpp, .h, .cu, .py, and any other source files which are compiled or interpreted.

Files:

  • cpp/tensorrt_llm/nanobind/batch_manager/bindings.cpp
  • cpp/tensorrt_llm/nanobind/executor/request.cpp
  • tensorrt_llm/serve/openai_protocol.py
  • tensorrt_llm/llmapi/llm_args.py
  • cpp/tensorrt_llm/pybind/executor/executorConfig.cpp
**/*.py

📄 CodeRabbit Inference Engine (CODING_GUIDELINES.md)

**/*.py: Python code 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.
Python filenames should use snake_case (e.g., some_file.py).
Python classes should use PascalCase (e.g., class SomeClass).
Python functions and methods should use snake_case (e.g., def my_awesome_function():).
Python local variables should use snake_case. Prefix k for variable names that start with a number (e.g., k_99th_percentile = ...).
Python global variables should use upper snake_case and 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 interfaces that may be used outside a Python file, prefer docstrings over comments.
Comments in Python 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.
Attributes and variables in Python can be documented inline; attribute docstrings will be rendered under the docstring for the class.
Avoid using reflection in Python when functionality can be easily achieved without it.
When using try-except blocks in Python, limit the except to the smallest set of errors possible.
When using try-except blocks to handle multiple possible variable types in Python, keep the body of the try as small as possible, using the else block to implement the logic.

Files:

  • tensorrt_llm/serve/openai_protocol.py
  • tensorrt_llm/llmapi/llm_args.py
🧠 Learnings (2)
cpp/tensorrt_llm/nanobind/batch_manager/bindings.cpp (2)

Learnt from: amitz-nv
PR: #5616
File: tensorrt_llm/executor/worker.py:375-384
Timestamp: 2025-07-17T09:01:27.402Z
Learning: In tensorrt_llm/executor/worker.py, the LoRA adapter cache optimization logic that checks is_adapter_in_cpu_cache() and conditionally passes None for weights/config has a known race condition issue that cannot be solved with simple error handling or verification checks. This is a known limitation that requires a more comprehensive solution.

Learnt from: CR
PR: NVIDIA/TensorRT-LLM#0
File: CODING_GUIDELINES.md:0-0
Timestamp: 2025-07-24T08:22:03.570Z
Learning: Applies to **/*.{cpp,h,hpp,cc,cxx} : Exceptions must not be thrown across library boundaries.

tensorrt_llm/llmapi/llm_args.py (1)

Learnt from: amitz-nv
PR: #5616
File: tensorrt_llm/executor/worker.py:375-384
Timestamp: 2025-07-17T09:01:27.402Z
Learning: In tensorrt_llm/executor/worker.py, the LoRA adapter cache optimization logic that checks is_adapter_in_cpu_cache() and conditionally passes None for weights/config has a known race condition issue that cannot be solved with simple error handling or verification checks. This is a known limitation that requires a more comprehensive solution.

🔇 Additional comments (8)
cpp/tensorrt_llm/nanobind/batch_manager/bindings.cpp (2)

60-61: LGTM: Proper exception object storage for nanobind integration.

The static storage approach ensures the registered exception objects remain valid throughout the module's lifetime, which is essential for the exception translator to function correctly.


74-78: LGTM: Correct usage of stored exception objects.

The exception translator correctly uses the pre-stored exception objects via .ptr() to get the underlying PyObject* for PyErr_SetString. This approach is more efficient than dynamically retrieving exception types.

cpp/tensorrt_llm/pybind/executor/executorConfig.cpp (1)

427-439: LGTM: Well-implemented enum string conversion method.

The from_string static method provides a clean, explicit approach for converting strings to enum values with proper case-insensitive matching and descriptive error handling. This is preferable to implicit conversions and aligns with the PR's goal of improving type safety.

tensorrt_llm/serve/openai_protocol.py (2)

255-255: LGTM: Explicit boolean conversion improves type safety.

The explicit bool(self.logprobs) conversion ensures _return_log_probs is always a boolean type, preventing potential type confusion in downstream code. This change aligns with the PR's goal of fixing parameter type handling issues.


546-546: LGTM: Consistent boolean conversion applied.

The same explicit boolean conversion is correctly applied in ChatCompletionRequest, maintaining consistency with the CompletionRequest implementation.

tensorrt_llm/llmapi/llm_args.py (2)

33-33: LGTM: Import addition for enum conversion.

The import of CacheTransceiverBackendType is correctly added and follows the established naming pattern with underscore prefix for pybind types.


875-875: LGTM: Proper enum conversion for nanobind compatibility.

The change from passing self.backend directly to using _CacheTransceiverBackendType.from_string(self.backend) correctly converts the string backend value to the expected pybind enum type. This aligns with the nanobind integration fixes where enum constructors were replaced with static from_string methods.

cpp/tensorrt_llm/nanobind/executor/request.cpp (1)

213-228: LGTM: Clean solution for handling optional boolean parameters in nanobind.

The custom __init__ lambda effectively addresses the issue where OutputConfig constructor couldn't properly accept None types from Python. The use of std::optional<bool> with value_or(false) provides a robust way to handle optional boolean parameters while maintaining backward compatibility.

The implementation correctly:

  • Accepts None values from Python via std::optional<bool> parameters
  • Converts None to false using value_or(false)
  • Preserves the original default behavior
  • Maintains consistency with the additional_model_outputs parameter handling
✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai generate unit tests to generate unit tests for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai or @coderabbitai title anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@Linda-Stadter Linda-Stadter self-assigned this Jul 24, 2025
@Linda-Stadter
Copy link
Collaborator Author

/bot run

@coderabbitai coderabbitai bot requested review from DomBrown and litaotju July 24, 2025 09:01
@tensorrt-cicd
Copy link
Collaborator

PR_Github #12831 [ run ] triggered by Bot

@Linda-Stadter Linda-Stadter requested review from ixlmar and dcampora and removed request for litaotju and juney-nvidia July 24, 2025 11:32
Copy link
Collaborator

@ixlmar ixlmar left a comment

Choose a reason for hiding this comment

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

LGTM

@tensorrt-cicd
Copy link
Collaborator

PR_Github #12831 [ run ] completed with state SUCCESS
/LLM/main/L0_MergeRequest_PR pipeline #9564 completed with status: 'SUCCESS'

@dcampora dcampora enabled auto-merge (squash) July 24, 2025 21:21
Copy link
Collaborator

@dcampora dcampora left a comment

Choose a reason for hiding this comment

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

LGTM

@dcampora dcampora merged commit 9a99e6d into NVIDIA:main Jul 25, 2025
3 checks passed
NVShreyas pushed a commit to NVShreyas/TensorRT-LLM that referenced this pull request Jul 28, 2025
Signed-off-by: Linda-Stadter <[email protected]>
Signed-off-by: Shreyas Misra <[email protected]>
Ransiki pushed a commit to Ransiki/TensorRT-LLM that referenced this pull request Jul 29, 2025
Signed-off-by: Linda-Stadter <[email protected]>
Signed-off-by: Ransiki Zhang <[email protected]>
lancelly pushed a commit to lancelly/TensorRT-LLM that referenced this pull request Aug 6, 2025
Signed-off-by: Linda-Stadter <[email protected]>
Signed-off-by: Lanyu Liao <[email protected]>
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