Skip to content

Conversation

@fredricz-20070104
Copy link
Collaborator

@fredricz-20070104 fredricz-20070104 commented Nov 27, 2025

Summary by CodeRabbit

  • New Features

    • Added a tuned runtime/performance configuration for a specific model variant: iteration performance stats, CUDA-graph and batching tweaks, MoE and speculative decoding options, overlap-scheduler toggle, autotuner, KV-cache tuning, and chunked prefill.
  • Tests

    • Added an integration test validating the new configuration end-to-end (hardware requirement noted) and added it to the QA test list.

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

Add test case for RCCA 5527655.

try to use b200 to load this one to speed up a little bit

add test case for rcca 5527655

Signed-off-by: FredricZ-2007 <226039983+fredricz-20070104@users.noreply.github.com>
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 27, 2025

📝 Walkthrough

Walkthrough

Adds a YAML runtime configuration for DeepSeek-R1-FP4 and a matching integration test that launches the local serve binary, waits for readiness, performs an OpenAI chat completion against the server, and registers the test in the QA list.

Changes

Cohort / File(s) Change Summary
New test config
tests/integration/defs/examples/serve/test_configs/DeepSeek-R1-FP4.yml
Adds a YAML config enabling runtime/perf flags (iter perf stats, cuda_graph_config, moe_config, speculative_config, autotuner, kv_cache_config, chunked prefill, disable_overlap_scheduler, etc.).
Integration test
tests/integration/defs/examples/serve/test_serve.py
Adds test_extra_llm_api_options_for_deepseek_r1_fp4 (with skip_pre_hopper, skip_post_blackwell_ultra, pytest.mark.skip_less_device(8)) that validates config/model paths, launches trtllm-serve, waits for readiness, runs an OpenAI chat completion against the local server, then cleans up.
Test list entry
tests/integration/test_lists/qa/llm_function_core.txt
Registers the new test: examples/serve/test_serve.py::test_extra_llm_api_options_for_deepseek_r1_fp4 TIMEOUT (120) alongside existing entries.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Test as Integration Test
  participant FS as Files (YAML, Test List)
  participant Serve as trtllm-serve
  participant Client as Local OpenAI Client

  Note over Test,FS: Setup
  Test->>FS: read DeepSeek-R1-FP4.yml & model path
  Test->>Serve: start process (popen) with config
  Serve-->>Test: process started (pid)

  alt server becomes ready
    Serve-->>Test: readiness probe success
    Note over Test,Client: Run inference
    Test->>Client: send chat completion request to local endpoint
    Client-->>Test: completion response
    Test->>Serve: terminate process
    Serve-->>Test: exit
  else timeout / failure
    Serve-->>Test: no readiness (timeout)
    Test->>Serve: terminate process
  end
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Check YAML keys/values for typos and semantic validity vs. runtime expectations (moe_config, cuda_graph_config, kv_cache_config, etc.).
  • Review new test decorators and timeout to ensure they match CI/hardware constraints.
  • Verify server start/stop handling and readiness timeout in test_serve.py.

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Description check ⚠️ Warning The PR description is minimal and lacks the required sections from the template such as detailed description, test coverage explanation, and PR checklist confirmation. Expand the description to include: (1) detailed explanation of what the test validates and why it's needed for RCCA 5527655, (2) explicit listing of test coverage provided by the new test, and (3) confirmation of PR checklist items.
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 identifies the referenced RCCA issue and describes the main change (adding a test case), which aligns with the actual additions of a new test function and configuration for DeepSeek-R1-FP4.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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: 1

📜 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 5425d96 and 1f39185.

📒 Files selected for processing (3)
  • tests/integration/defs/examples/serve/test_configs/DeepSeek-R1-FP4.yml (1 hunks)
  • tests/integration/defs/examples/serve/test_serve.py (2 hunks)
  • tests/integration/test_lists/qa/llm_function_core.txt (1 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:

  • tests/integration/defs/examples/serve/test_serve.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:

  • tests/integration/defs/examples/serve/test_serve.py
🧠 Learnings (8)
📚 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:

  • tests/integration/defs/examples/serve/test_serve.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:

  • tests/integration/defs/examples/serve/test_serve.py
  • tests/integration/test_lists/qa/llm_function_core.txt
📚 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:

  • tests/integration/defs/examples/serve/test_serve.py
  • tests/integration/test_lists/qa/llm_function_core.txt
📚 Learning: 2025-09-17T02:48:52.732Z
Learnt from: tongyuantongyu
Repo: NVIDIA/TensorRT-LLM PR: 7781
File: tests/integration/test_lists/waives.txt:313-313
Timestamp: 2025-09-17T02:48:52.732Z
Learning: In TensorRT-LLM, `tests/integration/test_lists/waives.txt` is specifically for waiving/skipping tests, while other test list files like those in `test-db/` and `qa/` directories are for different test execution contexts (pre-merge, post-merge, QA tests). The same test appearing in both waives.txt and execution list files is intentional - the test is part of test suites but will be skipped due to the waiver.

Applied to files:

  • tests/integration/test_lists/qa/llm_function_core.txt
📚 Learning: 2025-08-13T11:07:11.772Z
Learnt from: Funatiq
Repo: NVIDIA/TensorRT-LLM PR: 6754
File: tests/integration/test_lists/test-db/l0_a30.yml:41-47
Timestamp: 2025-08-13T11:07:11.772Z
Learning: In TensorRT-LLM test configuration files like tests/integration/test_lists/test-db/l0_a30.yml, TIMEOUT values are specified in minutes, not seconds.

Applied to files:

  • tests/integration/test_lists/qa/llm_function_core.txt
📚 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:

  • tests/integration/test_lists/qa/llm_function_core.txt
📚 Learning: 2025-08-26T09:37:10.463Z
Learnt from: jiaganc
Repo: NVIDIA/TensorRT-LLM PR: 7031
File: tensorrt_llm/bench/dataclasses/configuration.py:90-104
Timestamp: 2025-08-26T09:37:10.463Z
Learning: In TensorRT-LLM, the `get_pytorch_perf_config()` method returns `self.pytorch_config` which can contain default `cuda_graph_config` values, so `llm_args` may already have this config before the extra options processing.

Applied to files:

  • tests/integration/defs/examples/serve/test_configs/DeepSeek-R1-FP4.yml
📚 Learning: 2025-08-26T09:37:10.463Z
Learnt from: jiaganc
Repo: NVIDIA/TensorRT-LLM PR: 7031
File: tensorrt_llm/bench/dataclasses/configuration.py:90-104
Timestamp: 2025-08-26T09:37:10.463Z
Learning: In TensorRT-LLM's bench configuration, the `get_pytorch_perf_config()` method returns `self.pytorch_config` which is a Dict[str, Any] that can contain default values including `cuda_graph_config`, making the fallback `llm_args["cuda_graph_config"]` safe to use.

Applied to files:

  • tests/integration/defs/examples/serve/test_configs/DeepSeek-R1-FP4.yml
🧬 Code graph analysis (1)
tests/integration/defs/examples/serve/test_serve.py (2)
tests/integration/defs/conftest.py (2)
  • llm_models_root (80-94)
  • serve_test_root (2636-2640)
tests/integration/defs/trt_test_alternative.py (2)
  • print_info (300-306)
  • popen (199-218)
🪛 Ruff (0.14.6)
tests/integration/defs/examples/serve/test_serve.py

159-159: Possible binding to all interfaces

(S104)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Pre-commit Check
🔇 Additional comments (3)
tests/integration/test_lists/qa/llm_function_core.txt (1)

758-758: New DeepSeek-R1-FP4 serve QA entry looks consistent

The test list entry name matches the new test function and the TIMEOUT (120) aligns with other long-running examples/serve tests in this file. No issues from the list perspective.

tests/integration/defs/examples/serve/test_configs/DeepSeek-R1-FP4.yml (1)

1-18: DeepSeek-R1-FP4 extra LLM API options YAML is well-structured

Config keys and nesting (cuda_graph_config, moe_config, speculative_config, kv_cache_config, etc.) look consistent with existing perf/extra_llm_api_options usage, and values line up with the new serve test’s CLI arguments. No changes needed.

tests/integration/defs/examples/serve/test_serve.py (1)

3-6: Import updates for new skip marker are appropriate

Adding pytest and importing skip_pre_hopper / skip_post_blackwell_ultra is consistent with the new decorators on the DeepSeek-R1-FP4 test; no issues here.

Signed-off-by: FredricZ-2007 <226039983+fredricz-20070104@users.noreply.github.com>
Signed-off-by: FredricZ-2007 <226039983+fredricz-20070104@users.noreply.github.com>
Signed-off-by: FredricZ-2007 <226039983+fredricz-20070104@users.noreply.github.com>
Signed-off-by: FredricZ-2007 <226039983+fredricz-20070104@users.noreply.github.com>
check_openai_chat_completion(model_name=model_name)


@skip_pre_hopper
Copy link
Collaborator

Choose a reason for hiding this comment

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

fp4 does not support on hopper, use skip_pre_blackwell?

def test_extra_llm_api_options_for_deepseek_r1_fp4(serve_test_root):
test_configs_root = f"{serve_test_root}/test_configs"

# moe backend = CUTLASS which only supports fp8 blockscale on Hopper
Copy link
Collaborator

Choose a reason for hiding this comment

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

this comment is kind of misleading

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.

2 participants